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.
Comment | File | Size | Author |
---|---|---|---|
#18 | 2206347-18.patch | 23.75 KB | damiankloip |
#15 | interdiff-2206347-15.txt | 12.18 KB | damiankloip |
#15 | 2206347-15.patch | 16.84 KB | damiankloip |
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedHere is a start. Adds the event dispatcher to the module handler, invokes the events, adds a ModuleEvents file for the event constants.
Comment #3
damiankloip CreditAttribution: damiankloip commentedInstaller changes.
Comment #5
damiankloip CreditAttribution: damiankloip commentedStill working on this.
Comment #6
BerdirSee also #2155635: Allow plugin managers to opt in to cache clear during module install, the order of things in there is very very fragile.
Comment #7
catchI 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?
Comment #8
damiankloip CreditAttribution: damiankloip commentedQuick 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?
Comment #10
tim.plunkettWould it make sense for there to be a Entity-specific subscriber? How cheap/expensive are these?
We usually make these classes
final
Comment #11
sunAwesome, I really like this proposal.
Do we really need a separate event subscriber class?
I.e., why can't we make the
ModuleHandler
service/class itself implementEventSubscriberInterface
?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? :-)
Shouldn't this be moved to e.g.
CacheFactory
...?Let's use 0 by default.
Custom priorities for custom events should only be necessary if/when they actually are necessary. :)
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
Given that
ThemeHandler
needs pretty much the identical thing, I wonder whether we should rename this toExtensionEvent
?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
andThemeHandler
will consist of almost the identical code — i.e., they ought to be able to inherit from a genericExtensionHandler
, 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
) toExtensionEvent
would be very much in line with the extension system changes and awesome if that would be possible to do here :-)Comment #12
damiankloip CreditAttribution: damiankloip commentedThanks sun, looks like great feedback. I will rebase my local branch for this and try and make those changes today!
Comment #13
sunForgot 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 theExtension
:-)Comment #14
damiankloip CreditAttribution: damiankloip commentedThat 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.
Comment #15
damiankloip CreditAttribution: damiankloip commentedDid 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.Comment #16
tstoecklerThat one got in
Comment #18
damiankloip CreditAttribution: damiankloip commentedLet's start with a reroll. This will be broken.
Comment #19
dawehnerLet's just test it.
Comment #21
andypostThe issue @catch mentioned in #7
Comment #24
dawehnerI'd still like to see that.
@damian
Do you think you might want to give a reroll a try?
Comment #25
mgiffordUnassigning stale issue. Hopefully someone else will pursue this.
Comment #31
jibranComment #32
markhalliwellNow 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.
Comment #33
dawehnerI think we should remove the CachedModuleHandler bit from this issue into its own bit. This issue should be just about introducing the event.
Comment #35
andypostComment #36
andypost