User tests: Successful: Unsuccessful:
Pull Request for Issue # .
This PR makes some small improvements to current component dispatcher implement:
Introduce option property so that component dispatcher is not coupled with $this->app->scope anymore
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.
Status | New | ⇒ | Pending |
Category | ⇒ | Administration com_banners com_content com_postinstall Front End Libraries |
Labels |
Added:
?
|
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.
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....)
I have tested this item
Everything seems to be working as expected, banners and articles lists are being displayed correctly.
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.
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.
Anyway, I am done with this PR. To sum up:
It introduces a new property $option to Component Dispatcher. That help removing the usage of $this->app->scope in component dispatcher
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.
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.
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)
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.
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 :)
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
Let's remove the default namespace part from here and get this merged
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
.
Category | Administration com_banners com_content com_postinstall Front End Libraries | ⇒ | Administration com_content com_postinstall Libraries |
Yes that is why I just said we would remove it from here ;)
Category | Administration com_content com_postinstall Libraries | ⇒ | Administration com_postinstall Libraries |
Sorry for multiple commits. I could not resolve conflicts, so I had to edit files via web. Should be OK now.
Oh, No, Not OK yet. Look like I removed $this->option
There are also some tabs at the end of some lines.
OK. Guess we are good to go, did a quick test to make sure nothing broken. Thanks all :)
Status | Pending | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-05-08 11:43:52 |
Closed_By | ⇒ | wilsonge |
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