[#19834] - [4.0] Module dispatcher
- Fixed in Code Base
- 18 Oct 2018
- Medium
- Build: 4.0-dev
- # 19834
- Diff
- Digital-Peak:j4/module/services
- Pending continuous-integration/drone/pr this build is pending Details
- Success hound No violations found. Woof! Details
User tests: Successful: Unsuccessful:
Summary of Changes
Introduces a ModuleDispatcher as replacement for the single entry file for modules. The workflow is then similar to components with a Dispatcher and a service provider.
A module needs then a file services/provider.php which does register the ModuleInterface::class resource in the container:
public function register(Container $container)
{
$container->registerServiceProvider(new DispatcherFactory('\\Joomla\\Module\\MyModule'));
$container->registerServiceProvider(new Module);
}
and a dispatcher created by the DispatcherFactory. The dispatcher can override the function getLayoutData() to provide additional data for the layout. The rest stays the same.
More information can be found in #20217 and #19667.
Testing Instructions
Open the front end.
Expected result
The Menu module should work as before.
Actual result
All is working fine.
Documentation Changes Required
Needs to be documented as with the Dispatcher for components.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Administration com_admin com_associations com_banners com_cache com_categories com_checkin com_config com_contact com_content com_contenthistory com_cpanel com_fields com_finder com_installer com_joomlaupdate com_languages com_login com_media com_menus com_messages com_modules com_newsfeeds com_plugins com_postinstall com_redirect com_search com_tags com_templates com_users Front End |
| Labels |
Added:
?
|
||
| Category | Administration com_admin com_associations com_banners com_cache com_categories com_checkin com_config com_contact com_content com_contenthistory com_cpanel com_fields com_finder com_installer com_joomlaupdate com_languages com_login com_media com_menus com_messages com_modules com_newsfeeds com_plugins com_postinstall com_redirect com_search com_tags com_templates com_users Front End | ⇒ | Administration com_admin com_associations com_banners com_cache com_categories com_checkin com_config com_contact com_content com_contenthistory com_cpanel com_fields com_finder com_installer com_joomlaupdate com_languages com_login com_media com_menus com_messages com_modules com_newsfeeds com_plugins com_postinstall com_redirect com_search com_tags com_templates com_users Repository |
| Category | Administration com_admin com_associations com_banners com_cache com_categories com_checkin com_config com_contact com_content com_contenthistory com_cpanel com_fields com_finder com_installer com_joomlaupdate com_languages com_login com_media com_menus com_messages com_modules com_newsfeeds com_plugins com_postinstall com_redirect com_search com_tags com_templates com_users Repository | ⇒ | Administration com_admin com_associations com_banners com_cache com_categories com_checkin com_config com_contact com_content com_contenthistory com_cpanel com_fields com_finder com_installer com_joomlaupdate com_languages com_login com_media com_menus com_messages com_newsfeeds com_plugins com_postinstall com_redirect com_search com_tags com_templates com_users Repository Front End |
Yes, I had to rename the generic Dispatcher class to ComponentDispatcher as there is now a new ModuleDispatcher.
| Category | Administration com_admin com_associations com_banners com_cache com_categories com_checkin com_config com_contact com_content com_contenthistory com_cpanel com_fields com_finder com_installer com_joomlaupdate com_languages com_login com_media com_menus com_messages com_newsfeeds com_plugins com_postinstall com_redirect com_search com_tags com_templates com_users Repository Front End | ⇒ | Administration com_admin com_associations com_banners com_categories com_config com_contact com_content com_contenthistory com_cpanel com_fields com_finder com_installer com_languages com_login com_media com_menus com_messages com_newsfeeds com_plugins com_postinstall com_redirect com_search com_tags com_users Repository Front End |
| Category | Administration com_admin com_associations com_banners com_categories com_config com_contact com_content com_contenthistory com_cpanel com_fields com_finder com_installer com_languages com_login com_media com_menus com_messages com_newsfeeds com_plugins com_postinstall com_redirect com_search com_tags com_users Repository Front End | ⇒ | Administration com_admin com_associations com_categories com_contact com_content com_contenthistory com_cpanel com_fields com_finder com_installer com_languages com_login com_media com_messages com_newsfeeds com_plugins com_postinstall com_redirect com_search com_tags com_users Repository Front End com_config com_mailto com_modules |
Modules aren't really designed to be service providers. They're more of a consumer of services from the core libraries or components. So do we really need to support ALL the same types of features as components (i.e. the ability to define services), or can we keep this to only use of the dispatcher system?
Also, keep in mind #8276 since this PR directly affects that issue, and inherently how all modules are rendered today. There are a lot of variables that presently scope creep into modules that need to be accounted for.
I don't have a better idea just at this moment but this looks a bit over engineered to me. For me Modules are making output that's it. What is the avantage of having a module as a service?
At the moment we can just use the dispatcher. But for example com_ajax can then be extended to check for a service and no manual file and class loading. I don't know how you guys feel but I like when there is always one way of doing things. It will be then easier to document it too as devs will have the same concept across all extension types.
At JAB we came up with the conclusion that this can be a first step into the direction to be able to offer component outputs as modules as modules are often just another view for a component.
I do think the dispatcher is a step in the right direction (especially if we want to try merging modules and components down the road and be able to support a more HMVC based architecture), I just don't think that modules by design need to be considered service providers as well since by design they are data consumers.
just don't think that modules by design need to be considered service providers
In general I agree. So you want to remove the services/provider.php file and manually instantiate the dispatcher in the module helper? While doing all this service provider stuff, I really like the fact that an extension gets back control how it is loaded.
Well crap. I don't think we need to hardcode the dispatcher instantiation in the module helper, but I feel like having the service provider file is going to open the door to devs trying to register global services in modules and we don't quite want that to happen (or rather, it should be heavily discouraged because it's a bit more awkward to "boot" a module for this type of thing and IMO that's a sign you're trying to do too much in a module). So maybe we just have to leave it and hope for the best?
The DI container, which is passed to the module, is a child container of the global one. So all the dev can do is to register stuff in the child container. The global one is not accessible, except of JFactory::getContainer.
We should be OK then (I had to make sure we didn't do something crazy like have a public $container->getParent(), we don't, so we're good).
Was double checking the code too about the visibility of the parent variable.
| Category | Administration com_admin com_associations com_categories com_config com_contact com_content com_contenthistory com_cpanel com_fields com_finder com_installer com_languages com_login com_media com_messages com_newsfeeds com_plugins com_postinstall com_redirect com_search com_tags com_users Repository Front End com_mailto com_modules | ⇒ | Administration com_admin com_associations com_categories com_contact com_content com_contenthistory com_cpanel com_fields com_login com_media com_newsfeeds Repository Front End com_config com_mailto com_modules Libraries |
| Category | Administration com_admin com_associations com_categories com_config com_contact com_content com_contenthistory com_cpanel com_fields com_login com_media com_newsfeeds Repository Front End com_mailto com_modules Libraries | ⇒ | Repository Administration com_associations Front End com_content Libraries Modules |
| Category | Administration com_associations com_content Repository Front End Libraries Modules | ⇒ | Repository Libraries Modules Front End |
| Category | Repository Front End Libraries Modules | ⇒ | Administration com_admin com_associations com_banners com_cache com_checkin com_config com_content com_contenthistory com_cpanel com_csp com_fields com_finder com_installer com_joomlaupdate com_languages com_login com_media com_menus com_messages com_modules com_plugins com_postinstall com_redirect com_search com_tags com_templates com_users com_workflow |
I'v renamed the classes and could also get rid of the BasicModule class as the new ModuleDispatcherFactory can now create the new DefaultModule too. For that I had to rename the DispatcherFactory to ComponentDispatcherFactory, which made the pr again bigger. But everything is now clearly separated.
Dispatcher namespace looks now:
Any more feedback?
@laoneo Why is it called DefaultModuleDispatcher.php versus the ComponentDispatcher.php? I would expect a ModuleDispatcher.php?
Other than that, the consistency looks good. This PR is also so huge I think we should just start using it and see if we run into any issues :)
Ok, renamed it. Looks indeed more clear.
| Category | Administration com_admin com_associations com_banners com_cache com_checkin com_config com_content com_contenthistory com_cpanel com_csp com_fields com_finder com_installer com_joomlaupdate com_languages com_login com_media com_menus com_messages com_modules com_plugins com_postinstall com_redirect com_search com_tags com_templates com_users com_workflow | ⇒ | Administration com_admin com_associations com_banners com_cache com_categories com_checkin com_config com_content com_contenthistory com_cpanel com_csp com_fields com_finder com_installer com_joomlaupdate com_languages com_login com_media com_menus com_messages com_modules com_plugins com_postinstall com_redirect com_search com_tags com_templates com_users com_workflow |
just rebooted the tests.
Drone is failing atm all the time with a user system test. Rebooting doesn't help, the test needs to be fixed.
| Status | Pending | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-10-18 10:30:12 |
| Closed_By | ⇒ | wilsonge |
Thanks for being so patient on this one!
Time brought a couple of good ideas.
administrator/components/com_content/Dispatcher/Dispatcher.phpIs everything correct here?
Must file name start with the capital letter?