[pp-1] Explore adding an ImplementationList object for hook implmentations

Let's have one round of review, to see if people agree with the direction or have different ideas where this should go.

donquixote → changed the visibility of the branch 3519561-DeprecationCanaryTest to hidden.

donquixote → changed the visibility of the branch 3519561-hook-ImplementationList-remove-getHookListeners to hidden.

donquixote → changed the visibility of the branch 3519561-DeprecationCanaryTest to hidden.

I added the unit test for ImplementationList in this issue, to prepare for 🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active .

Took a pass at it I think this is a pretty nice consolation!

Qrite a few comments and I definitely need to take a deeper look, but I like the direction.

Ok I think this is ready!

I've read through this several times, both changes in isolation and the files as they will be merged.

This is a nice consolidation separating the concerns so module handler doesn't need to parse so much.
I checked through all of the places that ModuleHandler gets the hooks and confirmed they are replaced with ImplementationLists.

Early on I was concerned how this would work with Alter, but they are already ordered and they are retrieved before runtime ordering happens.

Tests look good, I didn't see anything missing a test.

Edit, actually this might need a CR since we're adding a new object and we're changing some methods.

Actually on second thought this might need a CR since we're adding a new object and we're changing some methods.

Do we write change records for protected methods and protected properties?
And do we write change records for new classes marked as internal?

Ok I asked in slack:

@godotislate shared this link https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... →

And @quietone shared this link https://www.drupal.org/about/core/policies/core-change-policies/change-r... →

They basically said it depends on contrib usage and the docs say changes to internal code so not need a cr.

The thing is this isn't just a change to internal code it's renaming and changing the return type of a method.

I think it's low risk for a few reasons:
1 I maintain the only module I'm aware of that decorate module handler.
2 this is required to simplify the removal of event dispatcher from hooks and that will necessarily be a large bc break.
3 I don't see any way to cleanly deprecate getflathooklisteners
4 the method itself is brand new

I'm going to mark this as ready, I'm happy to write a cr if someone else thinks it's necessary.

nicxvan → credited godotislate → .

nicxvan → credited quietone → .

Took a pass at credit, added @godotislate and @quietone for their assistance in slack.

Going for a simpler and more confident title.

Tagging this because it pseudo blocks 🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active

Let's actually change the names from module / Modules to extensions so we can use this for themes too if we make them available.

I'm going to create a separate branch with that change.

nicxvan → changed the visibility of the branch 11.x to hidden.

Can we get a review of the new MR? All it is is the same MR @donquixote wrote but all references to module replaced with extension to make this more generic so we can use this with other extensions in the future.

Commented on MR 13020.

Let's actually change the names from module / Modules to extensions so we can use this for themes too if we make them available.

I am not sure about this change.
The problems that we solve with this ImplementationList are (currently) very specific to module hooks.

For module hooks, every implementation needs a module associated with it, for these reasons:

• In ModuleHandler->invokeAllWith(), the $module is passed to the callback.
• In ModuleHandler->getCombinedListeners() (for ->alter()), the implementations are grouped by module before they are passed to hook_module_implements_alter().
• In ModuleHandler->getCombinedListeners() (for ->alter()), when we apply the ordering rules, these also get access to the modules.

Also, until now, we don't have services registered for themes.

For theme hooks, we might be able work with a simpler ImplementationList which does not track the theme names.

On the other hand: If, in the future, we do align theme hooks more with module hooks, we may want to refactor parts of ModuleHandler and HookCollectorPass, moving shared functionality into a shared class.

Personally I would suggest to keep ImplementationList focused on modules for now, and also use the term "module" in variable names.
The class is already marked as internal, and the properties are protected, so it will be easy to change in the future.

In the future when we reuse this for themes we can either rename the properties, or we can create a new class, depending on the needs.

Until that happens, the code will be easier to understand if it says "$module" rather than "$extension".
We name things for what they are, not for what they could be.

I touched on something similar in a MR comment, but I agree with #31. As proposed, the `extensions` property is a flat array AFAICT, and it would not be possible to differentiate between themes and modules in the future without refactoring. In which case, I think that suggests we should just use modules nomenclature for now and refactor whatever we need to accommodate themes later.

I touched on something similar in a MR comment, but I agree with #31. As proposed, the `extensions` property is a flat array AFAICT, and it would not be possible to differentiate between themes and modules in the future without refactoring.

I imagine that even if we reuse this for theme hooks, we would have separate instances for modules and themes.
So we would never have a mix of modules and themes.
You would know from the context whether $list->extensions gives you only modules or only themes.

But, even if we want to reuse large parts of this ImplementationList class, we might still want to maintain two separate classes, even if they are similar, and even if the only difference is the name of these properties. DRY can be overrated.
I suspect there would be more differences, some methods might not be needed for themes.

Thank you for reviewing this, I have a couple of quick comments though:

Also, until now, we don't have services registered for themes.

OOP hooks in themes will necessarily include this.

For theme hooks, we might be able work with a simpler ImplementationList which does not track the theme names.

I don't think that is true, we have base themes and admin themes, so the same hook can before multiple themes and they all have to execute in the right order, but only for the active theme.

On the other hand: If, in the future, we do align theme hooks more with module hooks, we may want to refactor parts of ModuleHandler and HookCollectorPass, moving shared functionality into a shared class.

Yes, but that is not part of this MR at all.
Also ModuleHandler won't have much overlap if any at all and HookCollectorPass has less overlap than you might think.

The class is already marked as internal, and the properties are protected, so it will be easy to change in the future.

Just because it's internal doesn't mean we shouldn't try to plan ahead a bit.

There would be separate instances for themes and modules as @donquixote said in 33.

I think the theme version would only need: filterByExtensionNames

Maybe this isn't worth pursuing and we just have a much simplified ImplementationList for themes when the time comes.

nicxvan → changed the visibility of the branch 3519561-ExtensionImplementationList to hidden.

I think you guys convinced me, there were a couple of suggestions from @godotislate that I think are relevant here, if we can apply those and confirm it's still green I think we can move this back to RTBC.

Rebased since HEAD's been fixed.

Thanks! Back to rtbc the updates were fairly minor.

Left 3 suggestions. Was going to leave the status as they were quite subjective, but changing it because I believe list<string, string> is wrong.

Addressed all feedback, back to needs review!

One of the array_all updates broke the asserts so I undid them.
I don't think that @mstrelan considers that suggestion is enough to block this, but I'll leave that to @mstrelan to confirm, I made all other changes.

Yes it's not a blocker. I hadn't realised the callback for array_all requires both the value and the key. I think it would have to be written like this:

assert(array_all($listeners, fn ($value, $key) => is_callable($value)));
assert(array_all($modules, fn ($value, $key) => is_string($value)));

Yeah, which i don't think it's much better let's leave it as is.

I'm going to go ahead and mark this ready since the only change was a phpstan type and test variable name.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased, it was just the test conversion to attributes, nothing crucial changed, I'll keep an eye on tests.