Postponed until 8.1.x
As nice as ModuleHandler is compared to what we used to have (nothing) it is still DX hell. The method signatures are needlessly complex, some methods are well over 200 or 300 LoC long and would definitely please people who enjoy arrow-shaped code.
Additionally, we should consider implementing a similar manager for themes (ThemeHandler) as well as for profiles (ProfileHandler) and implement the managed items as proper objects with proper interfaces (probably using some sort of ArrayCollection/ParameterBag to provide nice DX e.g. when reading from .info.yml properties).
ThemeHandler, ProfileHandler and ModuleHandler
We should add both, a ThemeHandler and a ProfileHandler and probably consider setting up some level of inheritance including proper interfaces and an abstract base class.
- #2058217: Implement a ProfileHandler to manage profiles (@see ModuleHandler)
- #2029819: Implement a ThemeHandler to manage themes (@see ModuleHandler)
While we are at it we should probably provide proper classed objects for Modules, Themes and Profiles (#2024043: Add Module, Theme, Profile, and Extension value objects).
Fix ModuleHandler
-
ModuleHandler::install / ::uninstall are needlessly complex. Let's simplify both.
- Currently, both methods and their procedural wrappers (@deprecated) module_install()/uninstall() each expect an array of module names. This is one of the reasons why ModuleHandler::install()/uninstall() are as gigantic as they are today. Internally, the only thing that would justify an array of modules is the post-installation hook invocation at the end of these methods which expects an array as well. But that should really not stop us. @see #2024071: Change ModuleInstallerInterface::install()/uninstall() method signature to expect single values instead of arrays.
- Both methods also allow uninstallation/installation of modules without looking at dependent modules (uninstallation) or module dependencies (installation). Let's remove that and always include dependencies/dependent modules. The logic for building the properly sorted dependency lists for modules should be moved into public helper methods too (things like the module overview and confirmation form also need that). @see #2024001: Remove the ability to install/uninstall a module without it's dependencies/dependents
- Currently, both methods and their procedural wrappers (@deprecated) module_install()/uninstall() each expect an array of module names. This is one of the reasons why ModuleHandler::install()/uninstall() are as gigantic as they are today. Internally, the only thing that would justify an array of modules is the post-installation hook invocation at the end of these methods which expects an array as well. But that should really not stop us. @see #2024071: Change ModuleInstallerInterface::install()/uninstall() method signature to expect single values instead of arrays.
-
Remove theme-related code from ModuleHandler::alter().
The question is: Why do we invoke alter hooks in themes anyways? This should really not be allowed. Either themes are full modules or they are not. And currently they are not. However, can we introduce this API change for D8? Probably not, so what do we do instead? Do we need a ThemeHandler and an ExtensionHandler that wraps both? Anyways, it's definitely wrong to have that code in the module handler (apart from the fact that it's probably wrong to even support alter hooks in themes in the first place). The worst part about theme-level alter hooks is that whether or not they ever reach the theme depends on how early in the bootstrap they are invoked. So it's really trial- and error for a themer anyways as some of the alter hooks will simply not work in themes.
@see #2024217: Should themes be able to implement hooks, e.g. hook_system_info_alter()?
-
Add a public method for retrieving properly sorted lists of modules based on the dependency tree. Needs issue
We need this for the modules overview page and confirmation form when installing/uninstalling modules as well as the site installation batch process. Both places currently use custom code to fire module installation step by step and in the right order.
-
Clean up the entire thing and get rid of all those function calls.
-
Implement modules as classed objects according to #2024043: Add Module, Theme, Profile, and Extension value objects.
-
Refactor system.admin.inc and the site installation batch process to adjust to the aforementioned changes make use of the $module object properties wherever module information is being accessed once #2024043: Add Module, Theme, Profile, and Extension value objects is resolved. Needs issue
-
Refactor graph API according to #1915308: Make the graph component input an edge list and add topological sort
Related issues
#2024001: Remove the ability to install/uninstall a module without it's dependencies/dependents
#2024043: Add Module, Theme, Profile, and Extension value objects
#2024071: Change ModuleInstallerInterface::install()/uninstall() method signature to expect single values instead of arrays.
#1915308: Make the graph component input an edge list and add topological sort
#2024217: Should themes be able to implement hooks, e.g. hook_system_info_alter()?
#2021959: Refactor module handling responsibilities out of DrupalKernel
#1067408: Themes do not have an installation status
#2029819: Implement a ThemeHandler to manage themes (@see ModuleHandler)
#2058217: Implement a ProfileHandler to manage profiles (@see ModuleHandler)
Comment | File | Size | Author |
---|---|---|---|
#5 | ModuleHandlerReport.png | 20.51 KB | Mile23 |
Comments
Comment #0.0
fubhy CreditAttribution: fubhy commentedUpdating summary.
Comment #0.1
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #0.2
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #0.3
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #0.4
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #0.5
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #0.6
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #0.7
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #0.8
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #1
clemens.tolboomsubscribe and some slightly related issues
#1247476: ModuleHandler::parseDependency fails on major version ops (>27.x) and (<=28.x)
#1217648: drupal_depth_first_search (graph.inc) allows for circular graphs
Comment #1.0
clemens.tolboomUpdated issue summary.
Comment #2
clemens.tolboomI forgot
#1762204: Introduce Assetic compatibility layer for core's internal handling of assets
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedwoo, this looks fun.
also see #2021959: Refactor module handling responsibilities out of DrupalKernel.
Comment #3.0
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #3.1
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #4
ParisLiakos CreditAttribution: ParisLiakos commentedtagging
Comment #5
Mile23Just how complex is ModuleHandler? Let's ask PHPUnit's test coverage report:
How to generate a report like this: http://phpunit.de/manual/3.7/en/code-coverage-analysis.html
Before I found this thread, I added some tests to bring the C.R.A.P. figure down from 20294 to ~9300. The ideal is 1. :-)
I'll make an issue for that patch once some of the decisions in this issue are settled.
Comment #6
Mile23Given that modules, profiles, and themes all have a lot in common, should there be a hierarchy of interfaces for them? Something like this:
Optionally replace 'Handler' with 'Manager' or something.
This would allow for an ExtensionHandlerBase class which would cover the common needs, and would allow flexibility as the distinctions between these three things emerge in D8.
Please express interest one way or another and I'll start another issue. :-)
Related by the same pattern at a different level of design: #2024043: Add Module, Theme, Profile, and Extension value objects
Comment #7
fubhy CreditAttribution: fubhy commentedRe-scoping this issue to cover everything extension-system related: Profiles, Modules, Themes... Those things are quite related and we might want to put some shared interfaces in place. Also see #2024043: Add Module, Theme, Profile, and Extension value objects...
Going to update the issue summary accordingly tomorrow.
Comment #7.0
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #7.1
fubhy CreditAttribution: fubhy commentedAdding ThemeHandler/ProfileHandler stuff.
Comment #7.2
fubhy CreditAttribution: fubhy commentedAdding ProfileHandler issue.
Comment #8
xjmThanks @fubhy! There's definitely a lot of work to be done here. I think we should probably consider what's going to directly help with issues like:
#1067408: Themes do not have an installation status
#2040037: The requested page "/admin/appearance/settings/bartik" could not be found after installation
#1798732: Convert install_task, install_time and install_current_batch to use the state system
Deploying module and theme installation status (can't find the issues...)
The full rearchitecture (to add the three interfaces) sounds like a really good idea to me, but that seems more D9 at this point. Some of the child issues, however, are probably D8 material where they help resolve the instability in the installer, config, etc. around themes. Let's explore as much as possible what we can do safely in D8 during this phase of the release cycle.
I think #2029819: Implement a ThemeHandler to manage themes (@see ModuleHandler) in particular might be worth bringing up with a core maintainer to discuss whether it's critical enough to allow in D8.
Also, #5 is scary.
Comment #9
Mile23Just want to point to this: #2095037: Add vfsStream as a dependency in composer.json.
Allows for a mock filesystem in unit tests.
Comment #9.0
Mile23Updated issue summary.
Comment #10
xjmComment #11
filijonka CreditAttribution: filijonka commentedbased on https://www.drupal.org/core/beta-changes I'm setting this to 8.1.x and postponing it
Comment #12
rickvug CreditAttribution: rickvug commentedI guess there's also no chance of de-scoping to "simply" convert what we have now to OO?
Comment #18
almaudoh CreditAttribution: almaudoh commentedThis issue needs to be updated. We now have
ModuleHandler
,ModuleInstaller
,ThemeHandler
andThemeInstaller
.What is left from the original scope?
Comment #20
andypostComment #29
andypostI set active #2058217: Implement a ProfileHandler to manage profiles (@see ModuleHandler)
Totally not clear if
ProfileHandler
is needed at all except #2024043: Add Module, Theme, Profile, and Extension value objectsAlso unpostponed #2024001: Remove the ability to install/uninstall a module without it's dependencies/dependents
Comment #31
andypostDrivers list added as
extension.list.database_driver
in #3256642: Introduce database driver extensions and autoload database drivers' dependencies