HookCollectorPass registers event listeners but they do not follow the required calling convention

Why aren't you using the documented methods for invoking hooks?
What is the naming convention you're referring to?

Please check the problem/motivation section of 📌 OOP hooks using event dispatcher Needs review

I guess I need to clarify this a bit:

This is not a feature request: I do not wish to invoke a hook via the event dispatcher.

This is rather a bug report: If some preexisting custom / contrib module defines an event which happens to collide with one of the newly added drupal_hook.* pseudo-events, then that module will break when attempting to dispatch its own event (with a trace similar to the one in the issue summary). This might be BC problem.

The problem is that EventDispatcher expects a certain method signature for all registered listeners. OOP hook implementations explicitly do not implement that method signature.

Also should upstream decide to add sanity checks on the methods collected by RegisterListenersPass, then that will break OOP hooks in the future.

I have the impression that the close coupling of ModuleHandler to EventDispatcher is artificial. It looks like ModuleHandler only wants to reuse getListeners(). That means, it just uses EventDispatcher as a registry.

It would be easy to implement the registry part of EventDispatcher in ModuleHandler (or in a dedicated service) in order to decouple ModuleHandler entirely from upstream.

Yes, of course, you and I already wrote an event dispatcher in #1972300: Write a more scalable dispatcher → it's not like that's hard to do it was reverted in 📌 Replace ContainerAwareEventDispatcher with Symfony EventDispatcher Fixed we can unrevert it :D :D

we can unrevert it :D :D

That wouldn't resolve the bug, though. The hook registry needs to be separate from EventDispatcher.

The implementation from #1972300: Write a more scalable dispatcher → could certainly be reused for ModuleHandler.

Of course, of course, that's what I meant. https://www.drupal.org/project/drupal/issues/3485896 📌 Hook ordering across OOP, procedural and with extra types Active is first so people don't try to order via event listener priorities.

Truth to be told, I only kept the event dispatcher because it's easier to get changes in if they use Symfony. It's truth. I wish I was kidding but I am not.

First attempt, including changes from 📌 [pp-1] Explore adding an ImplementationList object for hook implmentations Active

Some things should be discussed further:
- Breaking changes in ModuleHandler constructor parameter signature.
- Whether the [$hook => ["$class::$method" => $module]] structure is a good idea for the parameter.

This should probably be postponed on the other issue since this is built on top of it.

Adding framework tag too.

I want to do a deeper review if the other issue first but one question on the mr.

As for changing the signature, high level process:

Add both types to the parameters that change.
In the constructor throw a deprecation message if it is the type being removed and replace the property with the new type.

A combination of https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... → and https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... →

It's green!
Now it is time for the BC police.
I am changing the signature of ModuleHandler, we need to see how we can make this BC friendly.

Actually one round of review feedback could be nice.

I discussed with @ghost of drupal past in slack about the ModuleHandler constructor BC support.

Technically it could be possible to do it, but it would require some advanced acrobatics to reconstruct the list.
In 11.0.x -> 11.1.x we already broke that signature once.

Our current consensus is to break it again, without BC support for custom or contrib code that extends ModuleHandler.

We only need to make sure that you can successfully run container rebuild (drush cr, or by other means?) after code update.

All review points have been addressed.

But we should do the following issues first to start at a good place:
📌 Enhance array docs in ModuleHandler Active
🐛 ModuleHandler::resetImplementations should reset $this->invokeMap Active

The ProceduralCall class is gone.
(TBD if we want to keep it for BC, or do that removal in a follow-up)

Actually I would be fine to do this in a follow-up.
When we do this, we also want to review the ordering attributes, where currently you would do #[ReOrderHook(ProceduralCall::class, 'foo', ..)]. See 📌 [pp-1] Create new attribute to make reordering procedural hooks easier. Postponed

How do we interact with the procedural calls if that class is gone?

How do we interact with the procedural calls if that class is gone?

I assume this is about "ProceduralCall" class.
That class is already marked as internal, so using it to target a hook would already be wrong.

A solution for this can be discussed in 📌 [pp-1] Create new attribute to make reordering procedural hooks easier. Postponed .

Using the proceduralcall class is not wrong. Internal is just a warning things could change.

We cannot remove it without providing an alternative way to target procedural hooks for ordering.

We cannot remove it without providing an alternative way to target procedural hooks for ordering.

In the other issue the argument was made that it is usually fine to just target all implementations of the respective module.

As a reminder, there were two cases where we really need to target just the procedural implementation:

• The target module has procedural _and_ OOP implementations for the same hook.
  This could be because the OOP implementations were added later, and provide different functionality, while the old procedural hook implementation has not been converted yet (because out of scope/budget).
• We want to support different versions of the target module.
  One version has a single procedural implementation for a given hook. The other version has multiple OOP implementations. We want to target the procedural implementation of the older version, and possible some (but not all) of the OOP implementations from the new version.

We would need to add a way to target a module for Reorder and Remove because both of those target single implementations.

Can't we move the proceduralcall removal to a followup? It looks like it's not necessary here and is scope creep.

This change feels far more important.

I added support for using ProceduralCall in order attributes.

We still would need tests for this.
But, tbh, ProceduralCall is marked as @internal, and I don't really want anybody to use it.

Can't we move the proceduralcall removal to a followup? It looks like it's not necessary here and is scope creep.

I was proposing the same earlier.
But it really hurts to see these "Drupal\Core\Extension\ProceduralCall::$function" identifiers, or to see $function passed as a method.
The class was necessary as long as we had to register hook implementations as event subscribers.

The complexity of an MR comes from the diff size, but also from the end result.
In this case, we get a cleaner end result by having a slightly larger diff size (but still less than some earlier MRs).
I don't want to engineer the new solution around this leftover.

In fact, if we want to reduce the size of this MR, we should get this in first:
📌 [pp-1] Explore adding an ImplementationList object for hook implmentations Active

This change feels far more important.

And in general, we should prioritize changes that feel less important, if that makes other changes easier.

I created an alternative MR where ProceduralCall is still used in hook implementation identifiers and in the parameters passed to ImplementationList::load().
Tbh, this feels out of place, I don't like it.