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.

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

  1. ModuleHandler::install / ::uninstall are needlessly complex. Let's simplify both.

  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 Module, Theme, Profile, and Extension value objects.

  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 Module, Theme, Profile, and Extension value objects 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 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)

CommentFileSizeAuthor
#5 ModuleHandlerReport.png20.51 KBMile23
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fubhy’s picture

Issue summary: View changes

Updating summary.

fubhy’s picture

Issue summary: View changes

Updated issue summary.

fubhy’s picture

Issue summary: View changes

Updated issue summary.

fubhy’s picture

Issue summary: View changes

Updated issue summary.

fubhy’s picture

Issue summary: View changes

Updated issue summary.

fubhy’s picture

Issue summary: View changes

Updated issue summary.

fubhy’s picture

Issue summary: View changes

Updated issue summary.

fubhy’s picture

Issue summary: View changes

Updated issue summary.

fubhy’s picture

Issue summary: View changes

Updated issue summary.

clemens.tolboom’s picture

clemens.tolboom’s picture

Issue summary: View changes

Updated issue summary.

clemens.tolboom’s picture

Anonymous’s picture

fubhy’s picture

Issue summary: View changes

Updated issue summary.

fubhy’s picture

Issue summary: View changes

Updated issue summary.

ParisLiakos’s picture

Issue tags: +Extension system

tagging

Mile23’s picture

FileSize
20.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.

Mile23’s picture

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 Module, Theme, Profile, and Extension value objects

fubhy’s picture

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 Module, Theme, Profile, and Extension value objects...

Going to update the issue summary accordingly tomorrow.

fubhy’s picture

Issue summary: View changes

Updated issue summary.

fubhy’s picture

Issue summary: View changes

Adding ThemeHandler/ProfileHandler stuff.

fubhy’s picture

Issue summary: View changes

Adding ProfileHandler issue.

xjm’s picture

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 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.

Mile23’s picture

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

Allows for a mock filesystem in unit tests.

Mile23’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Component: other » extension system
Issue summary: View changes
Issue tags: -Extension system
filijonka’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue summary: View changes

based on https://www.drupal.org/core/beta-changes I'm setting this to 8.1.x and postponing it

rickvug’s picture

I guess there's also no chance of de-scoping to "simply" convert what we have now to OO?

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

almaudoh’s picture

This issue needs to be updated. We now have ModuleHandler, ModuleInstaller, ThemeHandler and ThemeInstaller.

What is left from the original scope?

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture