Introduce ImplementationList objects per hook, to simplify ModuleHandler

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

I added the unit test for ImplementationList in this issue, to prepare for #3506930: Separate hooks from events.

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?

Going for a simpler and more confident title.

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.

Rebased since HEAD's been fixed.

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.

See https://phpstan.org/r/6bf89217-0a74-48af-91fa-bddf512036f8

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) => is_callable($value)));
assert(array_all($modules, fn ($value) => is_string($value)));

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.

I haven't caught up with the discussion here and even less so with #3506930: Separate hooks from events but I have some concerns about the container size from what I saw in the theme OOP issue, all the extra objects and services being serialized separtely aren't going to help. I'll try to do some tests on a real project with this.

I have some concerns about the container size from what I saw in the theme OOP issue, all the extra objects and services being serialized separtely aren't going to help.

Regarding theme hooks, yes, this would mean more services.

For the module hooks discussed in this issue, we already do have one service registered per class with hooks, and then some overhead in the event dispatcher. definition.

With this issue we don't get any changes to the container, because here the ImplementationList objects are created at runtime.

With the "separate hooks from events", we get one additional service per hook (if it has at least one implementation).
At the same time, we lose the additional overhead for the event dispatcher definition.
For the "container size" as in number and size of definitions, this could go either way.
For number and cost of instantiated services in an average request, I would hope that number to go down significantly.

I'll try to do some tests on a real project with this.

This would be nice to see!

Took quite a lot of time to update my distribution to 11.x, lots of patches to rebase. But yeah, I see, there's no meaningful difference between 11.x, in both cases the serialized string is around 1.3MB (which is... not great). I need to have a closer look at this and the other issue, I can see that the things I'm seeing are already being discussed there,

I agree with #50. I started testing my ideas a bit in the other issue and I understand a bit better how these currently work, in that they aren't really services but rely on having DI and get the resolved hook services injected. This would need to change with my approach so we might need to revert and change considerable aspects of this again.

@berdir

I agree with #14.

Are you sure this is the comment number you want to reference? That comment does not look like something to agree or disagree with..
(I will delete this comment text after it has been clarified)

I have no idea how I came up with #14, I meant comment #50. edited my comment. You don't have to delete your comment, I can live with that mistake being out in the open ;)