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.

While we are at it we should probably provide proper classed objects for Modules, Themes and Profiles (#2024043: Add ModuleInterface, ThemeInterface, ProfileInterface, ExtensionInterface and corresponding classes).

Fix ModuleHandler

  1. 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 ModuleHandlerInterface::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
  2. 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()?
  3. 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.
  4. Clean up the entire thing and get rid of all those function calls.
  5. Implement modules as classed objects according to #2024043: Add ModuleInterface, ThemeInterface, ProfileInterface, ExtensionInterface and corresponding classes.
  6. 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 ModuleInterface, ThemeInterface, ProfileInterface, ExtensionInterface and corresponding classes is resolved. Needs issue
  7. Refactor graph API according to #1915308: Make the graph component input an edge list and add topological sort

#2024001: Remove the ability to install/uninstall a module without it's dependencies/dependents
#2024043: Add ModuleInterface, ThemeInterface, ProfileInterface, ExtensionInterface and corresponding classes
#2024071: Change ModuleHandlerInterface::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 need an installation status
#2029819: Implement a ThemeHandler to manage themes (@see ModuleHandler)
#2058217: Implement a ProfileHandler to manage themes (@see ModuleHandler)

Files: 
CommentFileSizeAuthor
#5 ModuleHandlerReport.png20.51 KBMile23

Comments

Issue summary:View changes

Updating summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue tags:+Extension system

tagging

StatusFileSize
new20.51 KB

Just how complex is ModuleHandler? Let's ask PHPUnit's test coverage report:

ModuleHandlerReport.png

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.

Given that modules, profiles, and themes all have a lot in common, should there be a hierarchy of interfaces for them? Something like this:

ExtensionHandlerInterface
-- ModuleHandlerInterface
-- ProfileHandlerInterface
-- ThemeHandlerInterface

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 ModuleInterface, ThemeInterface, ProfileInterface, ExtensionInterface and corresponding classes

Title:[META] Clean up and simplify ModuleHandler to improve DX and maintainability[META] Improve the extension system (modules, profiles, themes)

Re-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 ModuleInterface, ThemeInterface, ProfileInterface, ExtensionInterface and corresponding classes...

Going to update the issue summary accordingly tomorrow.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Adding ThemeHandler/ProfileHandler stuff.

Issue summary:View changes

Adding ProfileHandler issue.

Thanks @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 need 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.

Just want to point to this: #2095037: Add vfsStream as a dependency in composer.json.

Allows for a mock filesystem in unit tests.

Issue summary:View changes

Updated issue summary.

Component:other» extension system
Issue summary:View changes
Issue tags:-Extension system