? Pending

User tests: Successful: Unsuccessful:

avatar joomdonation
joomdonation
26 Apr 2017

Pull Request for Issue # .

Summary of Changes

This PR makes some small improvements to current component dispatcher implement:

  1. Introduce option property so that component dispatcher is not coupled with $this->app->scope anymore

  2. Use standard namespace for component if $namespace is empty in specific component dispatcher class. Honestly, I don't know if we want to allow magic calculation like this, so looking for feedback.

Testing Instructions

  1. Install latest Joomla 4.0-dev branch
  2. Apply patch
  3. Login to administrator area of the site, just access to the two namespaced components com_content, com_banners, make sure it is loading as expected (no need for further testings, just make sure banners list and articles list is being displayed)
avatar joomdonation joomdonation - open - 26 Apr 2017
avatar joomdonation joomdonation - change - 26 Apr 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 26 Apr 2017
Category Administration com_banners com_content com_postinstall Front End Libraries
avatar joomdonation joomdonation - change - 26 Apr 2017
Labels Added: ?
cfc4032 26 Apr 2017 avatar joomdonation CS
avatar joomdonation
joomdonation - comment - 26 Apr 2017

OK. So I modified code to detect $option from class name for now as suggested for the time being. If it is needed, we can add it in the future. For now, it should be OK

avatar laoneo
laoneo - comment - 26 Apr 2017

We skipped in London HMVC in favor of reusable layouts and stateless models. In your use case, I think using the Modules model directly for the save operation would be enough. Perhaps I do miss something, but for me this is a path in the wrong direction. It complicates the dispatcher.

avatar joomdonation
joomdonation - comment - 26 Apr 2017

Look at this block of code https://github.com/joomla/joomla-cms/blob/4.0-dev/components/com_config/controller/modules/save.php#L62-L89 and you will see why calling controller save method is much easier than calling model directly

If you use Model directly, you will have to do other task like check permission, validate data (basically, what's done in Form controller save method and if Modules controller save method (I haven't checked yet), has extra code, you would have to do the same...

Calling the controller save method, you don't have to worry about how it is handled, just check the result (true/false...)

I even would go further with modify dispatch method to return data returned by controller method, but guess we will have to see how we want to handle case like that (another real use-case is allow third party calling users controller to save user account base on given use input data....)

avatar rjcf18
rjcf18 - comment - 27 Apr 2017

I have tested this item successfully on 3817c96

Everything seems to be working as expected, banners and articles lists are being displayed correctly.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15590.

avatar rjcf18 rjcf18 - test_item - 27 Apr 2017 - Tested successfully
avatar laoneo
laoneo - comment - 27 Apr 2017

But then use the controller directly like $controller = new ModulesController(). The job of the component dispatcher is to dispatch a component and not to act as a getInstance() replacement of the Controller class. How the dispatcher is now, it is clear what his job is. Adding more complexity for no gain is for me the wrong path. We need to make things more clear and less complex in J4, not the opposite.

avatar joomdonation
joomdonation - comment - 27 Apr 2017

If you do so, you would have to creating MVCFactory manually and pass it to controller constructor, load language manually. Guess defeat the purpose of having Component Dispatcher.

avatar joomdonation
joomdonation - comment - 27 Apr 2017

Anyway, I am done with this PR. To sum up:

  1. It introduces a new property $option to Component Dispatcher. That help removing the usage of $this->app->scope in component dispatcher

  2. Allow using default namespace for component if $namespace property is not initialized in specific component dispatcher

I will leave it to @wilsonge for making decision.

avatar wilsonge
wilsonge - comment - 8 May 2017

I don't think we want the magic namespace thing. The reality is it's going to be 3PD's who'd want the 'magic stuff' and they don't have a base namespace of Joomla\Component\. So let's remove that from here.

Scope's not working out for us either in a bunch of cases (components calling components, modules calling components and a bunch of other cases). So we have two options. We either change the scope everywhere (for example when you change scope in places). We parse it from the class name of the dispatcher or we inject it. I'm currently in favour of just injecting it like @joomdonation is doing. It seems simple enough.

avatar joomdonation
joomdonation - comment - 8 May 2017

So you prefer injecting $option instead of parse it from dispatcher classname (how it is at the moment after Allon feedback) ?

Also, while namespacing extensions, an issue raise. We need to find an easy way to get namespace of an extension so that we can call namespace helper class. Right now, we have to keep a blank class like this https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_banners/helpers/banners.php#L17 (or registerAlias).

As of right now, $namespace is stored as a property of Dispatcher class. So to get namespace, we need to create an instance of dispatcher, which is not like as we have to pass application object to constructor...

Not sure if I am clear? Do you have any idea to solve this concern? I need to have an really easy way to get namespace of a given component (given com_content, I want to get Joomla\Component\Content as it's namespace)

avatar joomdonation
joomdonation - comment - 8 May 2017

Of course, with the current implement, we can still get it from database via ComponentHelper. But we almost don't need to store namespace in database if we can solve this issue.

avatar wilsonge
wilsonge - comment - 8 May 2017

I agree that's a problem. I've been rethinking autoloading in the last few days (again). I'll drop some notes on it in the J4 glip group.

Dispatcher class name is good enough. Anything except using the scope which ended up being far too restrictive :)

avatar joomdonation
joomdonation - comment - 8 May 2017

OK. There is another problem with creating model object directly from it's class name. But I will open a separate issue for that or discuss later

avatar wilsonge
wilsonge - comment - 8 May 2017

Let's remove the default namespace part from here and get this merged

avatar laoneo
laoneo - comment - 8 May 2017

The things is that this approach only works for core components. So I don't really see a benefit here for 3rd party. I even have the fear that extension developers do put their extensions under the Joomla vendor like Joomla\Component\Foo\Administrator.

avatar joomla-cms-bot joomla-cms-bot - change - 8 May 2017
Category Administration com_banners com_content com_postinstall Front End Libraries Administration com_content com_postinstall Libraries
avatar wilsonge
wilsonge - comment - 8 May 2017

Yes that is why I just said we would remove it from here ;)

avatar joomla-cms-bot joomla-cms-bot - change - 8 May 2017
Category Administration com_content com_postinstall Libraries Administration com_postinstall Libraries
avatar joomdonation
joomdonation - comment - 8 May 2017

Sorry for multiple commits. I could not resolve conflicts, so I had to edit files via web. Should be OK now.

avatar joomdonation
joomdonation - comment - 8 May 2017

Oh, No, Not OK yet. Look like I removed $this->option

avatar laoneo
laoneo - comment - 8 May 2017

There are also some tabs at the end of some lines.

avatar joomdonation
joomdonation - comment - 8 May 2017

OK. Guess we are good to go, did a quick test to make sure nothing broken. Thanks all :)

avatar wilsonge wilsonge - change - 8 May 2017
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2017-05-08 11:43:52
Closed_By wilsonge
avatar wilsonge wilsonge - close - 8 May 2017
avatar wilsonge wilsonge - merge - 8 May 2017

Add a Comment

Login with GitHub to post a comment