Updated: Comment #N

Problem/Motivation

Look at Drupal\Core\Extension\ModuleHandler class. It's a mess. There are horrors in there like removeCacheBins() that is not something the module handler should ever care about.

Proposed resolution

Inject the event dispatcher into the module handler and fire events for module related hooks.

Remaining tasks

Patch, decide on how to deal with existing hooks.

User interface changes

None

API changes

Addition of module events.

Maybe not deprecation of hooks? Not sure.

CommentFileSizeAuthor
#1 2206347.patch9.53 KBdamiankloip
#3 2206347-3.patch10.24 KBdamiankloip
#3 interdiff-2206347-3.txt718 bytesdamiankloip
#8 2206347-8.patch16.78 KBdamiankloip
#8 interdiff-2206347-8.txt8.14 KBdamiankloip
#15 2206347-15.patch16.84 KBdamiankloip
#15 interdiff-2206347-15.txt12.18 KBdamiankloip
#18 2206347-18.patch23.75 KBdamiankloip

Comments

damiankloip’s picture

damiankloip commented
Status: Active » Needs review
StatusFileSize
new 2206347.patch9.53 KB

Here is a start. Adds the event dispatcher to the module handler, invokes the events, adds a ModuleEvents file for the event constants.

Status: Needs review » Needs work

The last submitted patch, 1: 2206347.patch, failed testing.

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review
StatusFileSize
new 2206347-3.patch10.24 KB
new interdiff-2206347-3.txt718 bytes

Installer changes.

Status: Needs review » Needs work

The last submitted patch, 3: 2206347-3.patch, failed testing.

damiankloip’s picture

damiankloip commented
Assigned: Unassigned » damiankloip

Still working on this.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

See also #2155635: Allow plugin managers to opt in to cache clear during module install, the order of things in there is very very fragile.

catch’s picture

catch
he/him
Primary language English
commented

I can't find the issue at the moment, but when ModuleHandler originally went in there was a major follow-up to split it into a CRUD vs. runtime stuff, we might want to do that first.

While it's ugly in there, I personally think it would be considerably worse to have low-level systems like cache know about the ModuleHandler, so not sure what exactly is going to be subscribing to these events?

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review
StatusFileSize
new 2206347-8.patch16.78 KB
new interdiff-2206347-8.txt8.14 KB

Quick implementation, but some thing along the lines of this.

Happy to look at the CRUD/runtime stuff. Do you happen to know if/where the issue for that is?

Status: Needs review » Needs work

The last submitted patch, 8: 2206347-8.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented 
  1. +++ b/core/lib/Drupal/Core/EventSubscriber/ModuleHandlerSubscriber.php
@@ -0,0 +1,90 @@
+    entity_info_cache_clear();
...
+  public function onModuleUninstall(ModuleEvent $event) {
...
+    entity_info_cache_clear();

    Would it make sense for there to be a Entity-specific subscriber? How cheap/expensive are these? 

  2. +++ b/core/lib/Drupal/Core/Extension/ModuleEvents.php
@@ -0,0 +1,57 @@
+class ModuleEvents {

    We usually make these classes final

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue tags: +API clean-up

Awesome, I really like this proposal.

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/ModuleHandlerSubscriber.php
@@ -0,0 +1,90 @@
+class ModuleHandlerSubscriber implements EventSubscriberInterface {

    Do we really need a separate event subscriber class?

    I.e., why can't we make the ModuleHandler service/class itself implement EventSubscriberInterface?

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/ModuleHandlerSubscriber.php
@@ -0,0 +1,90 @@
+    entity_info_cache_clear();

    This was replaced with a service in #2155635: Allow plugin managers to opt in to cache clear during module install

    Looks like that service just simply turns into a event subscriber now? :-) 

  3. +++ b/core/lib/Drupal/Core/EventSubscriber/ModuleHandlerSubscriber.php
@@ -0,0 +1,90 @@
+    // Remove any potential cache bins provided by the module.
+    $this->removeCacheBins($event->getModule());
...
+  protected function removeCacheBins($module) {

    Shouldn't this be moved to e.g. CacheFactory...?

  4. +++ b/core/lib/Drupal/Core/EventSubscriber/ModuleHandlerSubscriber.php
@@ -0,0 +1,90 @@
+    $events[ModuleEvents::INSTALL][] = array('onModuleInstall', 100);
+    $events[ModuleEvents::UNINSTALL][] = array('onModuleUninstall', 100);

    Let's use 0 by default.

    Custom priorities for custom events should only be necessary if/when they actually are necessary. :) 

  5. +++ b/core/lib/Drupal/Core/Extension/CachedModuleHandler.php
@@ -39,8 +40,9 @@ class CachedModuleHandler extends ModuleHandler implements CachedModuleHandlerIn
    * Constructs a new CachedModuleHandler object.

    Btw, this class is completely obsolete since #2213357: Use a proper kernel in early installer — the functionality can be merged into ModuleHandler now.

    Created #2231419: Merge obsolete CachedModuleHandler into ModuleHandler 

  6. +++ b/core/lib/Drupal/Core/Extension/ModuleEvent.php
@@ -0,0 +1,41 @@
+class ModuleEvent extends Event {

    Given that ThemeHandler needs pretty much the identical thing, I wonder whether we should rename this to ExtensionEvent?

    cf. #1067408: Themes do not have an installation status

    In addition, the ultimate goal of #2186491: [meta] D8 Extension System: Discovery/Listing/Info (in combination with #2228093: Modernize theme initialization) is that ModuleHandler and ThemeHandler will consist of almost the identical code — i.e., they ought to be able to inherit from a generic ExtensionHandler, but only to extend/override some mechanics that are internal to the extension type (such as validating that the default theme cannot be disabled/uninstalled).

    Therefore, a rename from ModuleEvent (+ ModuleEvents) to ExtensionEvent would be very much in line with the extension system changes and awesome if that would be possible to do here :-)

damiankloip’s picture

damiankloip commented

Thanks sun, looks like great feedback. I will rebase my local branch for this and try and make those changes today!

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Forgot one remark... I guess it's trivially self-explanatory when re-rolling this patch, but explicitly noting it nevertheless:

Since #340723: Make modules and installation profiles only require .info.yml files, we have proper Extension objects everywhere.

Thus, let's construct the event with that object (instead of just the name), so ExtensionEvent::getExtension() yields the Extension :-)

damiankloip’s picture

damiankloip commented

That seems like a good plan, yes :)

Alot of stuff has gone in since the last iteration of this patch which makes it look rather outdated like #2155635: Allow plugin managers to opt in to cache clear during module install.

damiankloip’s picture

damiankloip commented
Status: Needs work » Postponed
StatusFileSize
new 2206347-15.patch16.84 KB
new interdiff-2206347-15.txt12.18 KB

Did some work on this but then saw #2231419: Merge obsolete CachedModuleHandler into ModuleHandler, which will completely conflict with this. So not doing any more for now.

A couple of notes though:

I don't think we need to make the ModuleHandler an EventSubscriber itself. I think everything can move elsewhere. Also, it slightly goes against trying to clean up and make the class easier to understand.

Still need to make Extension events use the Extension.

I don't think one ExensionEvents class for all extension events is a good idea. Otherwise, the same events will get invoked for theme and modules. So all of these subscribers will likely need to do something like if ($event->getExtension()->getType() == 'module') { ... which will get tiresome.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Postponed » Needs review

That one got in

Status: Needs review » Needs work

The last submitted patch, 15: 2206347-15.patch, failed testing.

damiankloip’s picture

damiankloip commented
StatusFileSize
new 2206347-18.patch23.75 KB

Let's start with a reroll. This will be broken.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review

Let's just test it.

Status: Needs review » Needs work

The last submitted patch, 18: 2206347-18.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#2324055: Split up the module manager into runtime information and extension information.

The issue @catch mentioned in #7

Status: Needs work » Needs review

dawehner queued 18: 2206347-18.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 18: 2206347-18.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented

I'd still like to see that.

@damian
Do you think you might want to give a reroll a try?

mgifford’s picture

mgifford
he/him
Primary language English
commented
Assigned: damiankloip » Unassigned

Unassigning stale issue. Hopefully someone else will pursue this.

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

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should 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.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should 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.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should 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.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should 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.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Related issues: +#2350111: Introduce an event to decouple the module installer from the entity manager
markhalliwell’s picture

markhalliwell
Primary language English
commented

Now that #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList is in and #2659940: Extension System, Part III: ThemeExtensionList and ThemeEngineExtensionList on its way, I think it'd make more sense to broaden (creating a parent issue) for using events with all extension related events, not just modules.

dawehner’s picture

dawehner
Primary language German
commented

I think we should remove the CachedModuleHandler bit from this issue into its own bit. This issue should be just about introducing the event.

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

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should 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

andypost
he/him
Primary language Russian
commented
Related issues: +#3056604: Add/remove event listeners provided by a module on module install/uninstall
andypost’s picture

andypost
he/him
Primary language Russian
commented
Version: 8.6.x-dev » 8.8.x-dev

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.

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.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Postponed (maintainer needs more info)
Issue tags: +Needs issue summary update

Quite a lot has changed since this issue was opened, and even since the most recent comments here, could use a review of what if anything is remaining.

nicxvan’s picture

nicxvan commented

I think we can close this.

nicxvan’s picture

nicxvan commented
Status: Postponed (maintainer needs more info) » Closed (outdated)

Closing this as outdated.