Allow module services to specify hooks

1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -276,17 +276,28 @@ public function resetImplementations() {
   +    $function = $this->moduleList[$module]->hookPrefix . $hook;
   +    if (is_callable($function)) {
   +      return $function;
   +    }
   

   I wonder whether we can at least make them proper objects and just create them once and inject their dependencies using ContainerFactoryInterface for now.

2. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -310,9 +320,8 @@ public function invokeAll($hook, array $args = array()) {
   +      if (FALSE !== $callable = $this->implementsHook($module, $hook)) {
   

   Let's swap the check here for readability.

FWIW what #2043653: Allow modules to implement hooks by nominating methods in a module.implements.yml allows that this doesn't is:

• You can use any function name you like, and hence maintain coding standards
• Your object doesn't need to be named Hook
• Your object can received injected dependencies
• You can wire two hooks via one method (how many times have you seen hook_node_insert just call hook_node_update and vice-versa).

Ok, I agree that we should pool efforts and I concede that my approach relies on parsing yml files at runtime, which is bad.
It only parses them once and they are cached, as per HEAD, but still its got to be a performance hit compared to a simple function_exists({some_naming_pattern}).
I agree on your follow ups but I would like to see the ability for the Hook classes to implement ContainerInjectionInterface and thereby not rely on only static implementations - which is the same think @dawehner asked for - so the attached patch adds that and updates tests. Still fails on same things as patch at #2.
Pushed to platform sandbox as oo-hooks-2237831-larowlan

Idea is awesome!
Also it would be great to provide some performance stats for new approach.

Looks like hook object can't be cached because services could change between executions

Looks like hook object can't be cached because services could change between executions

Can you explain what you mean with that?

9: oo-hooks-2237831-larowlan.be4ed2f.patch queued for re-testing.

re-roll

Previous discussions mainly consisted of endless bikeshed and back and forth on "Ugh, OO." vs. "Yay, OO." which is unhelpful noise today.

Just saying, the previous discussions did contain a proposal of procedural with namespaces, so you would have

namespace Drupal\field;
function hook_group_permission() {..}

This was postponed to Drupal 9 mainly because it was considered too much work to change all hook implementations, and out of scope for Drupal 8. See #1428592-9: Proposal: PHP 5.3 Namespaces for module-provided functions and hook implementations..
(don't be too distracted by the "inventor" stuff in the issue summary)

I think the Hook::*() proposal here is technically quite similar, as long as we are using static methods which don't benefit from dependency injection.
I could see some benefits, mainly that procedural code really looks ugly.
But we should really identify and name the reasons that make this new proposal better.

We should also consider that modules often do their own version of invokeAll(), where they iterate over implementing modules and build the function name.

In #16 this is solved like this:

     $implementations = $this->getImplementations($hook);
     foreach ($implementations as $module) {
      if (FALSE !== $callable = $this->implementsHook($module, $hook)) {

So, you need getImplementations() AND implementsHook(), where previously we only had to call getImplementations() and function_exists().

Maybe a better solution could be if getImplementations() would return an array like

foreach ($this->getImplementations() as $module => $callback) {
  ..
}

And the function_exists() could happen in getImplementations() already?

Btw if we are going to change how getImplementations() works, we should fix this first:
#2263365: Second loop in module_implements() being repeated for no reason.

I think the Hook::*() proposal here is technically quite similar, as long as we are using static methods which don't benefit from dependency injection.

Spot on IMO. Just allowing hook implementations as static methods seems like a low-hanging fruit with big immediate DX benefits (get rid of procedural files completely in modules).
Hooks as methods on actual instantiated & stateful objects is a totally different can of worms (needs a factory and static persistence of already instantiated objects - also, brings the hook system closer to Symfony's events, and thus makes the coexistence of the two a bit more of a WTF)

If moving forward on just the former is less controversial, maybe it would be a good idea ?

I wonder if we should not at least try to benefit from interfaces?

namespace Drupal\group;

interface HookGroupPermissionInterface {
  static function group_permission();
}

namespace Drupal\system;

interface HookPermissionInterface {
  static function group_permission();
}

namespace Drupal\field;

class Hook implements HookPermissionInterface, HookGroupPermissionInterface {
  static function group_permission() {..}
  static function permission() {..}
}

Another suggestion:
Name the methods Hook::hook_group_permission() instead of just Hook::group_permission().
This allows to have other methods in the same class which are not interpreted as hooks.
And it allows for easier grep.

Seeing as we're in feature freeze and what not, I suggest that if we go through with this, we just implement the namespace solution, or we convert hook implementations to methods on a MODULENAME.hooks service. Adding an interface per hook is quite a big BC break and if we do that, it's just as easy to convert the whole hook to Symfony events anyway,

I suggest to implement this as a preparation:
#2299537: Let the hook cache store arbitrary callbacks instead of module names

(and as already said, fix #2263365: Second loop in module_implements() being repeated for no reason. before)

@Xano

Seeing as we're in feature freeze and what not

I think this counts as "fix stuff that is broken", even if not marked as a bug report.

we convert hook implementations to methods on a MODULENAME.hooks service

Aren't some hooks called before the container is ready?

Aren't some hooks called before the container is ready?

Not as far as I know. To invoke an hook you need the module handler, which is a service on the container.

I'd still like to see this happen, re-roll attached but I think it has a few issues.

Can we move it to 8.1.x now?

Added beta phase evaluation and it is quiet clear that we can move it to 8.1.x.

@jibran in my mind this issue can chose by "prioritized" because this point

• "usability and user experience improvements" If this is not only mean GUI...
• alternativ "accessibility improvements"

I'm sad to see that this don't come with 8.0. Currently we use functions like that as an alias and map them to a static method. (but this core patch look much better ;)

Do your guys need some specific help?

@Clemens Sahs the main problem right now at this point in release cycle is Disruption. We are trying to fix the critical issue right now and it will effect that momentum by breaking all the patches in issue queue.

@jibran ok this is a point, but allow me a question for my understanding.

In the case that:
- this issue has a working patch without crashing any tests
- work without any BC breaks
- and we are already in beta
It will be possible that this can merged with 8.0 or is that impossible? So we are working for 8.1 now? Because this are tagged as feature?

In the case it will be possible we(my company) can look to give some manpower for this?

Thank you for your fast feedback

best regards

@Clemens it mainly comes down to whether we'd aim to convert core to use hooks in classes. If we did, that's too disruptive for 8.0.x (but might be fine in 8.1.x if moving the hooks themselves isn't considered an API break).

On whether we could add support now, the only way this could get into 8.0.x is if 1. we don't convert core to use the new pattern yet 2. We think the API improvement is worth it.

I'm personally not sure about the second part of that.

I must admit, I do think introducing this would be a nice DX win. There is is going to be a lot of code like the following going on without it:

<?php
function MODULE_some_random_hook(&$input) {
  \Drupal::service('my_service')->doSomething($input);
}

Maybe that isn't so bad but feels pretty out of place / halfway between two approaches to me.

All of my contrib hook implementations for Drupal 8 are aliases to methods on services. I do this so I can unit-test the actual code. Moving the code to a namespace does not solve that problem. Instead, it only prevents collissions of functions that are considered hook implementations, but aren't. Both are real-world problems, and both require separate solutions. I will still use code like the example in #36 when hook implementations are moved to namespaces, simply because I will still not be able to use DI for the hooks. We need hooks as services to do that, or convert hook implementations to classes that have factory methods.

One of my patches here supported DI

My apologies. You are right. I only read the issue summary and the last few comments. The comments that discuss DI are somewhere in the middle of this issue.

Both are real-world problems, and both require separate solutions.

I'd say there are actually three problems:

1. Name clash. This is what this issue is mostly about, and imo it is important enough in itself that we need to do something.
2. Container-unawareness.
3. Not having an interface to implement. This means the IDE cannot easily verify or assist with the hook signature.

The last point is something I mentioned earlier, with a reply from Xano in #24:

Adding an interface per hook is quite a big BC break and if we do that, it's just as easy to convert the whole hook to Symfony events anyway,

Which makes me wonder how is this a BC break?
Assuming we make this optional both for the module that implements the hook, and the module that provides the hook.

This reminiscence of dark ages is really annoying. I don't want to start another discussion, but could someone explain (or point me to another thread) is there any reason why D8 core is still using those awful procedural hooks? After all they could have been at least replaced with Event Dispatchers, right?

Look at this function name my_module_language_fallback_candidates_locale_lookup_alter($candidates, $context) this is just awful and our new modern core still uses crap like that.

SiliconMind: Because Symfony's Event dispatcher is a bit slower than hooks, some use cases don't map cleanly (like alters), and because it's simply a lot of work to do and we didn't have enough people working on it.

I would not be surprised to see hooks gone in Drupal 9, and we can start the process in 8.1, etc. If you want to help with that it would be appreciated.

Here is a proposal to bring hooks back to OO land - keep the good parts, remove the bad parts, allow OO and proper DI:

• - Use tagged services 'hook_subscriber'
• - (optional) weight argument to that tag to override the module weight
• - 3a. Use magic naming: hookExampleAlter() - 'hook' . camelize('example_alter')
• - 3b. If we don't want magic naming, use annotations:

use Drupal\example\HookExampleAlterInterface;

class MyService implements HookExampleAlterInterface {
/**
 * {@inheritdoc}
 *
 * @hook example_alter()
 */
public function hookExampleAlter() {
}

The interface would be optional here.

• - Create a map at compile time (this is not worse than event subscribers):

  hook => [module1:@service1, module1:@service2, module2:@service3, ...]

and store it as argument to module_handler().

Then in module handler check the map for the hook, instantiate the services from the container and call the camelized method name (or store the method, too in the annotation case.)

Done.

I'm not a fan of "magic". When I'm telling folks about Drupal, I really dislike having to mention the black arts. 3a--, 3b++.

Maybe we want @implements instead of @hook.. depending where this discussion goes in phpDocumentor..
https://github.com/phpDocumentor/phpDocumentor2/issues/1689

@Fabianx Please, avoid this kind of magic :)
What about turning hooks into interfaces? For example:

// Basic parameterless, non-returnable alter hook
interface HookInterface {
  function alter();
}

interface CronHookInterface extends HookInterface {
}

class MyImplementationOfCronHook implements CronHookInterface {
}

And then when running cron, just invoke method alter() on all instances of classes that implement CronHookInterface.
No magic involved. Clean OOP structure. Easy to work with for developers.

@SiliconMind
We can add those optionally, but for obvious reasons we should go babysteps, not solve all possible problems.

@SiliconMind,
I think it is confusing if part of the hook name is reflected in the interface name, and another part of the hook name is reflected in the method name.

Also, if we implement interfaces, then the interface should dictate the method signature. So, no generic HookInterface::alter() with arbitrary signature.

I see two options (*if* we use interfaces):

1. The interface name reflects the hook name, but the method name is always the same. An interesting choice would be __invoke(). The signature depends on the hook. Yes, you can dictate a signature for __invoke().
   This means a class can only ever implement *one* hook. Which is ok for me. (Use composition!)
2. The interface name reflects the hook name. The method name also reflects the hook name. And there is a strict naming convention, so there is no creativity when choosing the method name.

All the hook definitions currently in xyz.api.php would be converted or mirrored to interfaces.

interface HookFieldPurgeFieldStorage {
  public function __invoke(FieldStorageConfig $field_storage);
}

or

interface HookFieldPurgeFieldStorage {
  public function hookFieldPurgeFieldStorage(FieldStorageConfig $field_storage);
}

I prefer the first option, it has less arbitrariness.

------------------

Obviously this all needs to be BC, so we still need to support the procedural hooks.

Imo the solution here is to have a generic callback dispatcher, which sends the initial call to a bunch of callbacks with matching signature, and assemble a return value. These callbacks can be procedural hook implementations, namespaced functions, static methods, or service methods.

This means the module_invoke() cache should no longer store simple module names. Instead, it needs to store something that allows to retrieve either of those callbacks.

This would also make it easier to insert arbitrary callbacks into the mix.
For BC reasons hook_module_implements_alter() would only work for the procedural hooks..

Individual modules could opt out of the procedural hook discovery.

---------------------

All this said, I wonder how close this all is to the event system..

#46

What about turning hooks into interfaces?

This has the same problem that anything else requiring registration of hook invocations has:

- we have several dynamic hooks in core, like hook_form_FORM_ID_alter(), can't/shouldn't/won't create an interface for every form ID.
- forcing anything invoking hooks to create the interface (or similar) would be a big API change, even without the dynamic hooks problem.

I like the look of #43 at first glance. One thing that's not mentioned is hook_module_implements_alter() - if at the end we end up with an array of callables, then that's probably not a big deal.

we have several dynamic hooks in core, like hook_form_FORM_ID_alter(), can't/shouldn't/won't create an interface for every form ID.

There would be only one interface, but still the implementation would only be called for the specific form id.
Instead of the magic naming, the implementation would have to be explicitly associated with the form id.

forcing anything invoking hooks to create the interface (or similar) would be a big API change, even without the dynamic hooks problem.

You are talking about custom code that explicitly invokes the hook, instead of using core APIs?

@donquixote the Event class makes the event system a lot less pleasant to work with than hooks, which is partly why I pushed back against attempts to do straight hook -> event conversions during the 8.x cycle.

1. When you have an alter hook, getting the actual thing you're altering as method argument is much clearer than the Event class or a subclass.

2. When you're not altering anything and just reacting like hook_cron(), the Event class is superfluous anyway.

Then separate to that, there's #2352351: Add before/after ordering to events / hook_module_implements_alter() - priorities don't cut it when you have multiple contrib and custom modules interacting.

What we're missing from hooks is dependency injection, the rest works pretty well.

For me that's the difference between this issue and #1972304: Add a HookEvent.

#50

No I mean right now, just invoking a hook is ModuleHandler::alter() or invokeAll()

By convention we also require a hook_my_module_whatever() my_module.api.php

If you then need to define an interface for that hook, so that implementing classes can be discovered, that's a third step which is not currently required.

Or to put it another way, #43 as it stands doesn't require people to do anything differently if their module invokes a hook - it just allows people to implement hooks (any hook at all) via a class vs. a procedural function.

#46

• - Interfaces are optional and will solve the DX needs of those using an IDE.
• - Annotations are not magic, but discovery.

D8 shipped with 'magic' names for hooks, so this is the best next step.

Besides 'kernel.exception' is a magic name, too. (http://symfony.com/doc/current/cookbook/event_dispatcher/event_listener....)

AND Symfony itself (D8 not however) does support magic camelized names for event_listeners, too:

kernel.exception => 'on' . camelize('kernel.exception') => onKernelException() (http://symfony.com/doc/current/cookbook/event_dispatcher/event_listener....)

Given they only do that, if the service has the explicit tag and/or a method name specified, but that is again worse DX to specify each hook in services.yml.

However Event Subscribers force to use always $event, which is worse DX for a hook that just needs to react to something, which means you either use a GenericEvent and loose DX or you create your own Event class, which is lots of effort and they don't even have interfaces for most things.

Also nothing would prevent one from creating a class for e.g. Node to have Hooks.php and you can then do:

$this->moduleHandler->invoke(Drupal\node\Hooks::NODE_LOAD, $node);

That internally Drupal\node\Hooks::NODE_LOAD is the string 'node_load' does not matter then.

That is not worse than:

$event = new NodeEvent();
$event->setNode($node);
$this->dispatcher>dispatch(Drupal\node\Events::NODE_LOAD, $event);
$node = $event->getNode();

So in theory - if we really wanted to - we could have getSubscribedHooks() method on the class same as event subscribers and a HookSubscriberInterface.

Which would give three possible methods of discovery:

• - a) Magic naming hook_example_alter => hookExampleAlter() -- same as we have in modules anyway.
• - b) Annotation @implements hook_example_alter
• - c) public static getSubscribedHooks() - return array(Drupal\example\Hooks::EXAMPLE_ALTER)

Or a combination thereof.

BUT

Given we want to support optional interfaces, we would force a magic naming of hookExampleAlter() anyway (because else you could not implement that interface) - so the names need to be fixed, so a) is workable and not less magic than we have now, but way less effort.

From this analysis the main remaining differences between events and hooks are:

• - a) Events use an $event data transfer object, hooks get direct data
• - b) For events the interface is on the event, for hooks it could be a defined interface per hook. If we allow interfaces, then there is a 1-1 relationship between hook name and method name.
• - c) Hooks are prioritized by module weight by default (though that would be overridable per class in this proposal), but can be altered via hook_module_implements_alter(). Events have priorities.

And thats it.

Overall events and hooks are more similar than one would think. event_listener in Symfony also use magic names (in the container, we use lots of magic in the container).

--

After this analysis I strongly again tend to directly use 'magic' names, but allow modules to provide as a convention:

• - Drupal\example\Hooks::EXAMPLE_LOAD
• - Drupal\example\Hooks::EXAMPLE_ALTER
• - Drupal\example\Hook\ExampleAlterInterface

And given all this similarities I think it might even be okay to map hooks to events and vice versa (but not doubled) - though that is another discussion:

- Hook => Event

// Normal hook

$event = new HookEvent();
$event->setArguments($args);
$this->dispacher->dispatch('drupal.' . Drupal\example\Hooks::EXAMPLE_LOAD);

// Code would use: getSubscribedEvents() { return array(array('drupal.' . Drupal\example\Hooks::EXAMPLE_LOAD, 'methodExampleLoad'));
// Code would use: $event->getArguments();
// Code would use: $event->addResult($return_value);

$results = array_merge($results, $event->getResults());

// Alter hook

$event = new HookAlterEvent();
$event->setArguments($args);
$this->dispacher->dispatch('drupal.' . Drupal\example\Hooks::EXAMPLE_ALTER);

// Code would use: $event->getArguments();
// Code would use: $event->setArgument(0, $new_value);

$i = 0;
foreach ($event->getArguments() as $argument) {
  if ($i > 3) {
    break;
  }
  $arguments[$i] = $argument; // This needs some work obviously; just the concept.
  $i++;
}

- Event => hook

$this->moduleHandler()->invoke('event_subscriber_EVENT_ID_FROM_CAMELIZE_TO_UNDERSCORE');

$this->moduleHandler()->invoke('event_subscriber_kernel_exception');

function my_module_event_subscriber_kernel_exception($event) {
}

So a 1-1 mapping is possible as just proven.

@catch (#52):
If a module only provides hook_xyz() in module.api.php, and does not provide an interface, then the modern way is simply not supported. So it would be completely optional, no BC break.

This said, maybe another module could provide an alternative interface integration..
Or any interface could be associated with the hook, like this:

interface HookFoo {
  /**
   * @implements hook_foo()
   */
  public function __invoke();
}

This would mean that all interface implementations will also be hook implementations.
More than one interface could implement the hook, and it could live outside the original module if necessary.

For the discovery part, we can still debate how far this would be automatic/magic, or if we want something explicit from the implementing module.
Maybe a module could somehow define in which of the discovery patterns it wants to participate.

@catch (#51):

@donquixote the Event class makes the event system a lot less pleasant to work with than hooks, which is partly why I pushed back against attempts to do straight hook -> event conversions during the 8.x cycle.

So whatever we do here might even be preferable to the Sf event system, because it allows arbitrary signatures!
I think the event system simply has the advantage that the engine does not need to use stuff like call_user_func_array() or $refl->invokeArgs(), but instead can call the respective method explicitly. But then the callee has to extract the relevant parts out of the event, which just moves the problem to a different place.

#48:

That a class can only implement just one hook is out of the question. As soon as we use hookExampleAlter(), we can directly specify a 1-1 magic mapping anyway.

#49:

Hook Module Implements Alter is supported like before.

The only flexibility the service based system is that a class can optionally specify a different weight than the module, which is very simple to implement.

#55:

So whatever we do here might even be preferable to the Sf event system, because it allows arbitrary signatures!

Yes, exactly. That would even be a DX win!

See #53 for examples mapping hooks and events to each other.

I think the event system simply has the advantage that the engine does not need to use stuff like call_user_func_array() or $refl->invokeArgs(), but instead can call the respective method explicitly. But then the callee has to extract the relevant parts out of the event, which just moves the problem to a different place.

Yes, however call_user_func_array() / $refl->invokeArgs() is well supported in PHP and even used by the Symfony ContainerBuilder itself, so I don't see that as a big deal.

@Fabianx (#53):

$this->moduleHandler->invoke(Drupal\node\Hooks::NODE_LOAD, $node);

One weakness here, compared to events for data transport, is that the IDE cannot verify if the call matches the expected signature.
On the other hand, the subscriber then only knows that it gets a generic event object, not a specific one.. (or does it?)

We could solve this by having dedicated adapters on top of module handler which implement a specific interface, so we can say
$this->hookNodeLoad->invokeAll($node);
and the IDE would know the expected signature.

And given all this similarities I think it might even be okay to map hooks to events and vice versa (but not doubled) - though that is another discussion:

Yes, adapters can be created to translate between different formats for data transport.

That a class can only implement just one hook is out of the question.

Why? What is wrong with having separate objects for separate purposes?

SiliconMind: We would still need some compile-time discovery/registration mechanism. "Find all classes that implement this interface" is a prohibitively expensive/complicated process. That's why tagged services exist. That it works for functions by name is more an accident of PHP than a deliberate feature.

We *could* scan just those services that are tagged properly, and then compile a list of those to the container. That's similar to what we do for tagged services generally. The caveat being that hooks would then potentially lose their module association, which I'd argue is good in general but could confuse some.

All that said, this is a stopgap only. In many if not post cases, our remaining hooks should not be OOP-ified as is. They should be replaced with well-thought-out events or tagged services of their own. Eg, I don't see why cron needs to be a hook anymore. It should be a CRON event, with event listeners. Because "cron is running" is an event, to which we respond. So rather than an OOP bridge for hook_cron, we should add an event for cron that fires alongside the hook and let people transition gracefully.

This problem space doesn't lend itself well to a shotgun approach. I'm not against a stopgap, but that stopgap should be only that, a stopgap for code organization until we get our act together and update the APIs directly. (And maintain BC layers for the duration of 8.x's lifecycle.)

-----

Hm, a bunch of additional comments while I was writing this. Posting anyway for context, as the stopgap vs. thoughtful transition question still remains.

@Crell (#60):

stopgap vs. thoughtful transition

If we agree on a system that is similar to Sf events, but allows a free signature, and that we consider a worthwhile and mature alternative to Sf events, then I think we are beyond stopgap.

Ok, the ugly part would be that it has to be BC with the existing hooks.
But maybe we can just see this as an ugly extension to a system that on its own is simple and elegant enough.

And of course individual hooks might still be poorly designed. But this can be dealt with later.

Just leaving this here:

Given a correct module.api.php we can automatically generate the interfaces for the hooks pointing to the old hook_ docs.

grep 'function hook_' system.api.php | cut -d')' -f1 | sed 's/$/);/; s/function //; s/\(hook_.*\)(\(.*\)/\/**\n * Implements \1().\n *\/\npublic function \1(\2\n/'

That should ease the transition to add optional interfaces for IDE users and would likely for well-documented projects just be one command.

./scripts/generate-hook-interfaces.sh modules/my_module/my_module.api.php

--

#58:

The event dispatcher also just dispatches a generic object.

While we use something like that in the Legacy service of service_container (https://github.com/LionsAd/service_container/blob/7.x-1.x/src/Legacy/Dru...) to avoid scrutinizer warnings, that method obviously does not scale - especially not for contrib.

Drupal\example\HookInvocations::hookExampleLoad($example);

would obviously be possible to optionally specify , but just map to call_user_func_array([\Drupal::moduleHandler(), 'invokeAll'], func_get_args()) anyway.

So automatic generation of that code is possible using all defined interfaces of that module, but here I am not sure it is worth the indirection.

#60:

- They would not loose their module meaning, but per the namespace we can discover and use the module of the class for the weight.

Obviously we can re-think that all for 9.0.0. But events have weaknesses, too.

From IRC on why services:

- And if you put all hooks in one class as dawehner pointed out => Load all dependencies always => bad
- If you put every hook in a separate class => DX :/ + DI injection again.
- Services are the most natural thing for it.

And Event Subscribers are services, too, so ... nothing lost compared to that.

For those that are interested I did create a contrib solution for this: https://www.drupal.org/project/hooks feedback welcome over there.

It has had a few implementations but currently supports both events with a HookEvent and an implicit style implementation that allows you to put a class in Drupal\$module\Hooks\HookName. The implicit approach uses a generic HookInterface with an alter method.

I quite like how easy the implicit approach is but speaking with a few people in IRC it seemed the consensus seemed to be against "magic" which is why I added the event based approach as well.

If a module only provides hook_xyz() in module.api.php, and does not provide an interface, then the modern way is simply not supported. So it would be completely optional, no BC break.

I think we should aim to keep the API for both invoking and implementing hooks consistent. If one alter hook works with the service and one doesn't that would quickly get confusing.

I quite like how easy the implicit approach is but speaking with a few people in IRC it seemed the consensus seemed to be against "magic" which is why I added the event based approach as well.

Personally I don't find method naming particularly 'magic' (particularly if the class implements an interface for the first round of discovery) when the 'non-magic' solution is annotations. The method naming also has advantages if you're looking at a backtrace or similar.

One advantage of the hook system as it stands is we do discovery on-demand when the hook is first invoked, which might be never on some sites for some hooks (form alters, crud hooks for unused entities etc.), so we need to be careful not to overdo things in terms of discovery performance degradation. It might be only on cold caches but it is going to block nearly every request.

Personally I don't find method naming particularly 'magic' (particularly if the class implements an interface for the first round of discovery) when the 'non-magic' solution is annotations. The method naming also has advantages if you're looking at a backtrace or similar.

Well the way it's currently implemented, it's the class name that is magic, the other non-magic approach includes registering an event subscribe for a "hooks.$hook_name" event.

One advantage of the hook system as it stands is we do discovery on-demand when the hook is first invoked

That's exactly how my implicit implementation works, see http://cgit.drupalcode.org/hooks/tree/src/ModuleHandler.php?h=8.x-1.x#n142 - I modelled it pretty much as closely as I could to the current module handler.

@benjy, you're my hero! I'm making this module required for all of my D8 projects ;) If only we could get invoke() and invokeAll() too... In worst case, we can write our own ModuleHandler service. It's better than patching core and good for building a proof of concept for a start.

#65: As long as the performance is similar to:

- module_load_all()
- function_exists()

I think we are fine.

Obviously we could check the classes on demand, too - but that would mean loading more classes (for reflection) on run-time for each hook.

On the other hand if in the future the modules don't need a .module file anymore, then you exchange one .class file with a .module file, but have the autoloader overhead obviously.

So I can see both things working:

- Store a list of hooks mapped to [module,service_id]s.
- Store a list of [module,class + service]s to check for the methods.

Or even a hybrid lazy loading:

- Cache the list of methods starting with hook* for the class candidates, but only do it on demand on the first hook invocation, but as that is during cold caches, likely even better to do it once during container compilation.

As I doubt $reflectionClass = new ReflectionClass($class_name); $reflectionClass->getMethods() is more costly than just checking individual methods via getMethod() - obviously it is more to store then - but that same problem exists with event subscribers.

So for me the measure is more if it is on comparable level to event subscribers.

I think the main reason why moduleHandler can't enumerate the hooks is, because it is impossible to distinguish a hook candidate and a normal method and there are a lot of methods starting with [module_name]_*.

Though we do that for the theme registry, so there is some precedent that we could have done it also with hooks.

Obviously function_exists() is pretty fast.

Note that https://www.drupal.org/node/2616814 is more or less a prerequisite to anything that happens here. We're pretty locked in until ModuleHandler actually handles modules 100%.

Re 67: https://www.drupal.org/node/2616814 is also of interest to you since the hooks module and/or any replacement of ModuleHandler can't change the behavior of *all* hooks (only the ones that go through ModuleHandler).

@cweagans I wasn't aware that that D8 core is not using it's own API properly. I'd expect that from contributed modules, but not from core. I guess there is still a lot to catch up.

@cweagans I think we could probably have a working patch with alters here to see what it looks like.

I would be hesitant to actually make the change until invocation is standardized in core though.

Yes, that is true.

getImplementations() returns module names and the function generates the hook name automatically out of that, so that won't work - but is a mis-use of the API - I think.

As I doubt $reflectionClass = new ReflectionClass($class_name); $reflectionClass->getMethods() is more costly than just checking individual methods via getMethod() - obviously it is more to store then - but that same problem exists with event subscribers.

Why do we need reflection, with the hooks module it currently just calls the "alter" method, the hook object implements an interface and the class picked up from the Hooks folder. Is there a reason to know what other methods it has? If it was for when we have info hooks as well, i'd just add another interface.

@cweagans I think we could probably have a working patch with alters here to see what it looks like.

I'm happy to turn the contrib module into a patch here if you want? I'm not sure which approach is preferred though still.

IF we are going to add a generic "put hook implementations inside classes" approach to core (and I still think most cases need more thought than that, this is a stopgap only), the approach needs to not be opt-in per-hook. It needs to allow for any arbitrary hook to work within a method on a service, if the implementer of the hook feels like doing so. That is, all we're doing is automating this (from a module I'm working on right now):

/**
 * Implements hook_entity_type_alter().
 */
function moderation_state_entity_type_alter(array &$entity_types) {
  \Drupal::service('moderation_state.entity_type')->entityTypeAlter($entity_types);
}

/**
 * Implements hook_entity_operation().
 */
function moderation_state_entity_operation(EntityInterface $entity) {
  return \Drupal::service('moderation_state.entity_type')->entityOperation($entity);
}

/**
 * Sets required flag based on enabled state.
 */
function moderation_state_entity_bundle_field_info_alter(&$fields, EntityTypeInterface $entity_type, $bundle) {
  return \Drupal::service('moderation_state.entity_type')->entityBundleFieldInfoAlter($fields, $entity_type, $bundle);
}

/**
 * Implements hook_entity_presave().
 */
function moderation_state_entity_presave(EntityInterface $entity) {
  return \Drupal::service('moderation_state.entity_operations')->entityPresave($entity);
}

Requiring the entity_type_alter hook, entity_operation hook, etc. to all define some extra bit before they work with whatever the new alternative is will be too high a barrier, lead to inconsistency, not work with dynamic hooks, and not actually improve live on this planet.

#73: The current proposal is #43. One hook per class feels excessive and is worse than event subscribers even.

#74: Crell, #43 is exactly tailoring that use case.

For that example you would do in my proposal (not hooks module):

moderation_state.services.yml

moderation_state.entity_type
  - class: Drupal\moderation_state\EntityState
  - tags:
    - { name: 'hook_subscriber' }

That registers the class for checking for hook* public functions.

class Drupal\moderation_state\EntityState {

/**
 * Implements hook_entity_type_alter().
 */
public function hookEntityTypeAlter(array &$entity_types) {
  // ...
}

/**
 * Implements hook_entity_operation().
 */
public function hookEntityOperation(EntityInterface $entity) {
  // ...
}

/**
 * Sets required flag based on enabled state.
 */
public function hookEntityBundleFieldInfoAlter(&$fields, EntityTypeInterface $entity_type, $bundle) {
  // ...
}

}

And optionally for e.g. best practice (if the module supports it) you would have interfaces available:

use Drupal\entity\Hook\HookEntityTypeAlterInterface;
use Drupal\entity\Hook\HookEntityOperationInterface;
use Drupal\entity\Hook\HookEntityBundleFieldInfoAlterInterface;

class Drupal\moderation_state\EntityState implements HookEntityTypeAlterInterface, HookEntityOperationInterface,  HookEntityBundleFieldInfoAlterInterface{

/**
 * {@inheritdoc}
 */
public function hookEntityTypeAlter(array &$entity_types) {
  // ...
}

/**
 * {@inheritdoc}
 */
public function hookEntityOperation(EntityInterface $entity) {
  // ...
}

/**
 * {@inheritdoc}
 */
public function hookEntityBundleFieldInfoAlter(&$fields, EntityTypeInterface $entity_type, $bundle) {
  // ...
}

}

So in essence you would skip the step of having to call \Drupal::service($x) -> $this->getContainer()->get($x) -> call().

But your class would be directly called and you would have nice interfaces (if and only if the module provides them).

Interfaces could be automatically generated with a script from module.api.php.

That proposal would continue to work with dynamic hooks like hook_form_FORMID_alter.

it would just be called:

hookFormMyFormAlter() what normally would be my_module_form_my_form_alter().

Obviously you can't add an interface for that.

But that is the new idea here.

FabianX: Yes, that's what I'm saying I'm on board with. :-) Although I'm not sure that the added interfaces are even necessary. Just the service tag.

@Crell
Its just nice to have those interfaces, its just more convenient as a module developer, but I agree, we would not have to require it on runtime.

I just realised that the current proposal here (using tagged services) is close to the issue summary of #2237831: Allow module services to specify hooks. However the title and patch/discussion in #2237831: Allow module services to specify hooks is using an event, not tagged services.

Fair question: Given the performance improvements we made to EventDispatcher, and that by the time use of "OOP hooks" becomes widespread I'd hope more people would be using PHP 7, is it worth making a straight OOP-hook bridge rather than bridging to events/adding events to replace hooks? Or rather, what's the balance point there?

@Crell: See e.g. #51 (catch). Also #55 and #57
Hooks allow a dedicated parameter signature per hook, whereas events always have the one event parameter.

@donquixote but you can derive from Event class and pass whatever data you want. Where's the limitation?

@SiliconMind: It is not a limitation.
It is simply a matter of taste, if you prefer separate parameters or if you prefer them wrapped in an event object.

Yes, this is about keeping the developer experience of explicit parameters (which includes no parameters at all sometimes).

For me personally I really, really do not like the Event class - it is both redundant when you don't need to pass anything around, and an extra layer of abstraction/indirection when you do - and from comments on here that opinion is not just mine.

Another idea I have:
#2649930: Annotated dependency injection?
Service dependency annotations + parameter currying.

Services are too much boilerplate. Just make them plugins. Namespace? Hook. Make hook a mandatory property of the annotation. Even id can be autogenerated -- modulename/hook. Of course, nothing stops you from having two classes implement the same hook if you provide your own id. Implementing a dynamic hook? That's not a problem, we made the hook you implement explicit. Easy. Everyone knows plugins. Implementation, discovery caching, implements alter -- all in the system already. Go.

hook_foo in modulename.api.php can be (as Fabian shown above) script-converted into an interface and added as a @see. I have no problems with HookFormFormIdAlterInterface, personally.

My personal problems with using plugins here is that unlike services, which are mostly for low level stuff, plugins are mostly used on a way more high level of the stack.
Also hooks change behaviour mostly these days and seeing that in one place, the services.yml file, is IMHO not a bad thing to get overview of a module.

@chx Service boilerplate is close to 0. All you have to do is write few lines in *.services.yml file and that's it. And as a benefit you get dependency injection that makes far better experience than using static \Drupal::service();

Let it be. But as this will cause an explosion in tagged services let's first figure out how we document those.

I remember talking about this with @chx couple of months ago. Any wasy i'm putting my self in the loop because iv'e start doing something very similar via plugins.

I start it in a contrib module -https://github.com/RoySegall/plugable_hooks - but i'll know more once i get into the loop of this issue.

RoySegall: Oh please oh please don't use Plugins for this. No. Just no. Too much overhead, too much added complexity. Tagged services are more than sufficient. We do not need plugin overhead here.

What Crell says.
The purpose of plugins is to expose a list of available implementations to a user, and to be able to obtain an implementation based on stored configuration (especially, a stored plugin id) - configuration that usually comes from a user-submitted form.
Neither of this is necessary or desirable for hook implementations.

... ignore ...

I was was not convinced, by far, that plugins are not the best solution here, but I didn't want to fight however #90 and #91 makes me believe more they are the right solution.

To counter #90, you are adding three lines into an existing doxygen, one of which is the closing }. A service definition at least requires repeating the classname and a much more complex tags syntax. To counter #91 storing configuration is just one option among many.

In general, tagged services are very hard to work (especially since the service collector has been invented) with because of the total lack of visibility into what handles them. Compare this to the trivial ease of opening the annotation class and looking at the doxygen which will contain the @see to the plugin manager, the base class and so forth -- everything laid bare, easy to find. How do you find what happens to services tagged with hook, on the other hand?

chx: So you're suggesting the solution to a documentation problem is to use a much more expensive system that involves considerable extra runtime cost and complexity?

To a first timer, there's no less black magic in "add this magic stuff to the docblock and weird things happen" than "add this magic stuff to a YAML file and weird things happen." But there's much more magic going on at runtime.

I would much rather just address the documentation problem. Don't use a sledge hammer when a ball-peen hammer will do. :-)

> So you're suggesting the solution to a documentation problem is to use a much more expensive system that involves considerable extra runtime cost and complexity?

Now, I am not even going to argue whether this is true or not. I filed the relevant documentation issue close to two years ago, haven't seen any significant progress for close to a year now. Unlike that issue, the coding issue is trivial to solve. I cook with whatever is at hand. I gave a presentation at NYC Camp about this, I filed an issue, I plead with the community, I can't do more. If that issue gets solved (I am not hopeful) then we can do more but until, I'd rather not see Drupal's fundamentals placed on something undocumentable.

We paid lip service to documentation in Drupal 8 on many important interfaces: #2344045: ContextInterface needs documentation #2641430-10: Typo/spelling error in LoggerChannelFactoryInterface and so on. Let's not make a bad problem even worse. And yes, undocumented and undocumentable code is worse than perhaps a bit complexer but documented code. There's no doubt to this.

I don't see the problem with plugins. it's cool, it's easy to set up - you just create a folder, add two 4 lines of annotation(with the plugin id, the information on the hook an the closing brackets) and boom you all set up.

The thing that this issue is critical for developers and in a random talk over the IRC I came here.

@chx, @RoySegall
Your arguments seem to me more like arguments for annotations (and possibly the ::create() method).

The plugin system just happens to be the only subsystem currently existing (afaik) that uses annotation discovery. But if we want annotations for hook implementations (which I have no strong opinion about, personally), we should do it without the overhead from plugins.

@FabianX already made some proposals for annotations above (#43, #53), just use find-in-page "annotation". Also see #91.

I currently have no strong opinion either way about annotations vs tagged services.
But I am convinced that we don't need plugins if all we want is annotations.

I used plugins because it was the easiest thing to implement and use it for notify my custom module service. We don't have to use plugin but I do believe we need plugins annotation in which we can add more information. The quickest one is: we can define in the annotation level if our definition should before other definition and in that way we can deprecate the one of the use of hook_implements_alter.

If we would go the same route as https://www.drupal.org/node/2023613 it would be even something else. So seriously, we agreed on some proposal, and no we reiterate on an undecidable decision. This is not a boolean but rather mostly opinionated decision.

I find tagged services really abominable and hard to trace what's happening.

Also, this really feels like a 9.x issue, not 8.1.x?

In any case, I agree annotations would be clearer if feasible. This also seems like the kind of debate that needs a whiteboard, more than an issue thread.

#2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList is in.

Please look at:
https://www.drupal.org/sandbox/olexyymailsgmailcom/hook_manager

- it should be rather fast;
- it uses caching and static;
- priority is supported;
- we have progress in naming convention problems;
- we can inject services to classes that implement hooks;

Please look at:
https://www.drupal.org/sandbox/olexyymailsgmailcom/hook_manager

- it should be rather fast;
- it uses caching and static;
- priority is supported;
- we have progress in naming convention problems;
- we can inject services to classes that implement hooks;

Moving this to a plan and active, it would be good to get it going again.

I've updated the issue summary based on Fabian's proposal in #43 and #75, short version is:

1. A module can provide a tagged service that implements hooks.

2. Hooks are implemented by adding a method on that class, this can be:

- (optionally) using an interface, for example Interface NodeLoad implements HookInterface

- for dynamic hooks and legacy/arbitrary hook callback support, an annotation like @hook node_load

- we could also support discovery by camel cased hook name, so public function nodeLoad() but that's not necessary.

The advantages over events:

1. We can allow dependency injection, interfaces etc. for every hook currently invoked in core, contrib and custom code, without doing a 1-1 hook -> event conversion.

2. Compared to events, we avoid the use of the often clunky Event class.

Moving to 'needs review'.

/**
 * {@inheritdoc}
 *
 * @hook example_alter()
 */

The API module does not support anything in the docblock in addition to a {@inheritdoc}. It's possible that's not going to change. See #1994890: Allow {@inheritdoc} and additional documentation.

If we're using interfaces we actually don't need the @hook foo annotation at all so just {@inheritdoc} should be OK for those cases. I'll update the issue summary...

#3056604: Add/remove event listeners provided by a module on module install/uninstall is another case where we're having to patch up incompatibilities between the module system and our forked event dispatcher.

I would still like to seriously look at doing the following:

1. In this issue, add support for OOP hooks.

2. Open follow-ups to add features like stopping propagation, as well as an explicit before/after (which would be better than both hook_module_implements_alter() and Symfony's magic number priority system).

3. For core-dispatched events, invoke a hook in the same place and deprecate the event

4. Start to invoke hooks for Symfony events that we expect Drupal modules to subscribe to, and encourage contrib/custom modules to implement the hook instead of using event listeners.

If we're able to fully deprecate the need for modules to subscribe to events, then in Drupal 10 we could shift back to using Symfony's actual event dispatcher instead of a forked one.

I just wanted to add a note that if something like this is added to Core, it would be good if it was done in such a way that the API module could recognize that a given class method is an implementation of a certain hook, so that on pages like
https://api.drupal.org/api/drupal/core%21core.api.php/function/hook_cron...
we could list the services/methods/whatever that implement the hook, as well as the functions that we are already listing.

Also hiding very old patches to avoid people like me being confused. Latest one is 5 years old and (from Slack) people said the current approach is totally different.

Re #118 we have one of those follow-ups already #2352351: Add before/after ordering to events

for #118

Looks like there's a contrib implementation for this kind of thing at Form Alters as Services.

@colan There's also the more broadly scoped and more widely used Hook Event Dispatcher. Granted, it defines hooks as events rather than just services so there's a bit more boilerplate code required, but I think it's worth looking at. There's also been some work and discussion on it to decorate the module handler service to make integrating it into Drupal core easier, though that still needs some work.

Also doing something with hooks defined by services in https://www.drupal.org/project/hux , without involving events/event subscribers.

E.g

services:
  my_module.hooks:
    class: Drupal\my_module\MyModuleHooks
    tags:
      - { name: hooks }

final class MyModuleHooks {

  #[Hook('entity_access')]
  public function myEntityAccess($entity, $operation, $account): AccessResult {}

The hux module looks interesting, as it makes use of PHP 8 attributes.

There's no patch atm

> - Modules can optionally for now provide MyHookInterface extends HookInterface when they invoke a hook

Should that say 'invent' rather than 'invoke'?

I skimmed the earlier discussion in this issue, which covered the idea: should we use the Symfony events system.

The discussion highlighted some concerns with doing that, for example comment #42 by @Crell:

SiliconMind: Because Symfony's Event dispatcher is a bit slower than hooks, some use cases don't map cleanly (like alters), and because it's simply a lot of work to do and we didn't have enough people working on it.

I would not be surprised to see hooks gone in Drupal 9, and we can start the process in 8.1, etc. If you want to help with that it would be appreciated.

That discussion took place back in 2014/15, about 7 to 8 years ago. Both Symfony and Drupal have come a long way in that time, and of course we now use newer and more efficient PHP versions. So, I wonder if it's time to reopen that discussion.

To me, one of the current DX problems with hooks is simply that we also have the Symfony Events system, which means we have two systems for doing basically the same thing: allowing code to react to something happening somewhere.

This is of course not ideal from a DX perspective, not only is it potentially confusing, but with some of the contrib space using Symfony Events, it means you may have to jump back and forward between hooks and events depending on what you're doing. Having these two systems also potentially increases the barrier to entry for new developers: instead of having just one system of events to understand, we have two, which is not an ideal place to be for something trying to learn Drupal and its ecosystems.

With that in mind, it seems counterproductive to modernise the Hook system into an OOP implementation, which would still leave us in a place of having two systems that basically do the same thing, and once done they are then arguably more similar and so it becomes more confusing. At least with hooks not being OOP right now there's a clear difference between Events and Hooks in code, but an OOP Hooks system would seriously blur those distinctions. Instead of just migrating to and improving the Symfony Events system, even if we have to build a wrapper around it or wait for upstream improvements.

So, I'd like us to get to a place where Drupal has one single events system, I think that's a much better place to be. I don't think it matters what the exact implantation is, as long as it's consistent: whether that is simply just Symfony Events, or a wrapper around it, or a modernised Hook system and we drop Symfony Events (probably not possible).

@AaronMcHale for the events conversion discussion, the issues outlined in #49-57 by my and Fabianx have not changed in the meantime:

Symfony's Event system still requires an event class, and this doesn't map well to either dynamic hooks or 'reaction-only' hooks. Unless we found a way to handle those with equivalent DX, then it'd be a step backwards in many ways.

There are two situations where events vs. hooks gets confusing:

1. There are some places where we rely on Symfony-dispatched events and that's the only way for contrib or custom code to interact.

2. Drupal has added its own events (particularly in the configuration system) where they were brand new for dependency injection and etc.

For #2, if we reworked the hooks system, we could convert those events back to hooks and deprecate the event.

For #1, we could potentially subscribe to those events in Drupal, and invoke hooks as a bridge to the hook system, discouraging subscribing to them directly.

Talking Symfony, one of the latest developments is using attributes as listeners.

https://symfony.com/doc/current/event_dispatcher.html#defining-event-lis...

I like that boilerplate on the subscriber side is reduced compared to EventSubscriberInterface. You need to define a class at a specific namespace, add public methods with appropriate attribute, and a service definition.

An example PHP8's attributes can be seen even at Attributes in PHP 8.

If we were to adopt PHP8 attributes, we wouldn't need to tie ourselves to a specific event/sub system: Pick a namespace for autodiscovery or require a service definition, then invent "hook" specific attributes for each "hook" type. The passed arguments can be invented on a per-hook basis, as we do now.

As I write this, it sounds more and more like (self plug, again) Hux. I encourage followers of this issue to take a look, and perhaps a proposal for bringing its concepts into core can be written up. One thing that is missing is hook-type specific attributes, and with that any thought around reworking how data is passed around. It currently takes in hook-type strings and same argument values.

I love the idea of being able to just attach an Attribute to a method and then the method is basically listening to that event.

It's very similar to a system I was using years ago when doing some work in Java, where to listen for an event, all you had to do was annotate a method in a class with the Annotation for that specific event, and you could also pass the priority in. It was the most friendly and elegant way I've used from a DX perspective, particularly when I was writing code that would react to a lot of events, it was also a very scalable approach. It was all IDE friendly as well, since the IDE knew where the Annotation was defined.

So, if we were able to adopt something like that, as outlined by @dpi in #140, then I think that would be an excellent step forward!

@dpi I absolutely support the hux direction:
- Discovery per class/service or per directory with method attributes.
- Keeping the parameter signature of the hook.

It feels very close and natural to our current procedural hooks.
At the same time, hux solves many of the shortcomings:

• It gets rid of procedural code.
• It allows dependency injection.
• It eliminates name clash paranoia.
• It allows more than one implementation per module.
• It allows one method to implement multiple hooks. (e.g. hook_node_insert() + hook_node_update() in one method).
• It allows to control the order of implementations, without having to use hook_module_implements_alter().

As for symfony events, I think for our purpose they are a distraction.
For a new use case we can always consider to introduce an event instead of a hook, or even something else.
But if we already have a hook, and want to keep supporting it, then method + attribute is the best way to go.

I think this would have been a harder sell before attributes were introduced, we would have had to use annotations.

Given previous comment, and a variety of external discussions, I'd like to propose closing this issue in favor of working towards a [yet to be created] issue for utilising classes + method attributes in a same-or-similar manner to Hux.

At its core, Hux is transparently registering services to the container, much like the proposed resolution of utilising services for this issue. Though there is not necessarily a requirement to define a service definition or dependencies explicitly with a YAML file. Thanks to @larowlan, a class autodiscovery implementation has turned out to be great.

I think it would be absolutely fine to open a new issue and then close this one as a duplicate, there's not really a competing proposal on here or anything, just a very old discussion and for me the hux API (not that I've used it yet, but based on what I've seen so far) covers all the original requirements that made us want to go in this general direction.

For what it is worth I have used both hux and hook_event_dispatcher in production projects.

I think both have their merits but hux is a more natural replacement for many of the current hooks because it does away with the overhead of the event object and instead the things one wants to interact with are directly passed as arguments to the method.
But from a DX perspective it would have to be much more clear of when to use which system, since there are subtle differences between events and hooks. Good documentation of which is more appropriate would be a benefit because now one essentially wants to use events just because they are OOP and can be used in services, but with hux all of a sudden the main selling point is gone and the argument for events becomes "be more like symfony". But if we would follow the suggestion by catch from #139 maybe events are for when modules interact directly with symfony and hux is for interacting with drupal components like entities etc.

@bircher

But from a DX perspective it would have to be much more clear of when to use which system, since there are subtle differences between events and hooks

I assume we are only talking about new cases where we would introduce either a new hook or a new event.
I see hux mostly as a tool for implementing existing hooks, without any additional effort on the party that publishes the hook. It is not really a "replacement", it just provides more options on how to implement them.

On the other hand, converting hooks into events requires extra effort on the party that publishes the hook/event. This is obvious from looking at the "Hook Event Dispatcher" module, which supports events only for _some_ hooks, with dedicated code for each of them, see https://git.drupalcode.org/project/hook_event_dispatcher/-/tree/3.x/modules.

The question of hooks vs events for new use cases already exists today.
In fact there are more options, such as tagged services with service collectors.

I think it is a valid question, but I think we would need to gather experience before we can recommend anything.
To implement an existing hook, I would always choose hux (or the equivalent that we would add in core).
To publish a new hook or event, I would try different options and see what works best.

I don't think this question should be a blocker for doing something like hux in core.

Responding to #146 and #147 I think the main question is whether, after core hooks are hux-ified, we would want to convert some core events (config and similar) to hooks or not. But as @donquixote that's not a blocker to the initial huxification of hooks but something that can be decided afterwards.

The main thing to decide is that we definitely want 'OOP hooks' instead of 'convert every hook to an event' and I am +1000 to OOP hooks and -1000 to 'convert every hook to an event' just in case it wasn't already clear ;)

Converting to hux-like implementations seems nice and easy, and it's not too different from other things done in core. You create a service class, implement some functions, and you're good to go. I'm on board.

@donquixote My understanding is that Hook Event Dispatcher now supports or is in the process of implementing a dynamic system that doesn't require manually implementing every procedural hook, as of Drupal core 9.4 and Hook Event Dispatcher 3.2.0.

@Ambient.Impact
I looked at that patch, and I still see the distinct event classes for different hooks.
I don't even know how you would ever get rid of these event classes.

Perhaps you could auto-generate them based on the hook signature, but this would be quite adventurous.
And in the end you need these classes as parameter types in the event subscribers. And with code generation, at the time you write the subscriber, the to-be-generated event class might not be generated yet, making it hard for the IDE to help you out.

@donquixote That issue just laid the groundwork from what I can tell, but that was a year ago and it's had a bunch of work since. Worth poking around the 4.x tree to see where they're at because it looks like they've got a bunch of stuff to generate events and also a plug-in factory, which I suppose could work better in some cases. This isn't necessarily a full endorsement of the event approach, since I agree it tends to have a lot more boilerplate needed and Hux looks more pleasant to get started with, but I've written so many event subscribers at this point that I can do in my sleep.

Just to be clear, I am a huge fan of hux. I think it is much better than the proposal in the issue summary (which of course predates possibilities with current php versions) and addresses all the concerns listed in comments.

Hux may still need to be profiled, but given that it replaces what is described in #36 and others I think it will probably be about the same as normal hooks when they call a method on a service.

Of course it is not a blocker to document the subtle differences between events and this hook2.0 system. And we may decide to move some hooks to events still or some events to hooks2.0 but how this is decided should be in its own documentation page.

I think the fact that hooks stayed around and have not all been replaced by events in a decade is probably a sign and it is not just due to the lack of funding. Hux on the other hand has a 1to1 upgrade path so it seems like a much easier transition.

Here we go, a new issue!
#3366083: [META] Hooks via attributes on service methods (hux style)

The Events system requires use of an Event object rather than named parameters, this creates additional boilerplate when defining events and additional levels of indirection when subscribing to events.

It's possible to use payload as an event object.

$node = Node::load(1);
$event_dispatcher->dispatch($node, 'entity_delete');

Multiple hook parameters can be wrapped into some generic event object. It could be even stdClass.

$data = [
  'build' => $build,
  'entity' => $entity,
  'display' => $display,
  'view_mode' => $view_mode,
];
$event_dispatcher->dispatch((object) $data, 'entity_view');

I would prefer dedicated event objects as they can have typenints. Also they serve as documentation. We do the same with hooks through module.api.php file.

'Payload as event object' has been possible since Symfony 4.3, which is after the issue summary was written. We should definitely compare Symfony 2023 with hux though instead of hux with Symfony as it was in 2014.. The old method signature was previously type hinted with Event.

The big remaining issue, which isn't in the issue summary but is discussed elsewhere, is that $event_dispatcher->dispatch($node, 'entity_delete'); wouldn't give us the dynamic hooks that we currently have (hook_entity_delete(), hook_node_delete()),
and similar ones like hook_form_FORM_ID_alter() etc.) so we'd still be looking at one-to-one conversions both for dispatching and implementation. This becomes even more so with the single $event argument - being able to send stdClass as essentially an array of arguments would mean less boilerplate in the conversions, but this would still need to be changed for every hook.

We'd also still have things like hook ordering vs. priorities to sort out too although hoping whatever we do can include some improvements over hook_module_implements_alter().

The big remaining issue, which isn't in the issue summary but is discussed elsewhere, is that $event_dispatcher->dispatch($node, 'entity_delete'); wouldn't give us the dynamic hooks that we currently have (hook_entity_delete(), hook_node_delete()),

One possible approach is dispatching two events. A module interesting in node events could subscribe only to endity_delete.node event.

$event_dispatcher->dispatch($entity, 'entity_delete');
$event_dispatcher->dispatch($entity, 'entity_delete.' . $entity->getEntityTypeId());

I think the arguments provided by @Chi in favor of events are very compelling.

For the priorities problem we have #2352351: Add before/after ordering to events, which seems doable at first sight.

As for the need to have an Event class (which is not a strict requirement anymore per #155), tbh I think that would be an improvement over our current way of documenting hooks and their signature in a module.api.php file.

$event_dispatcher->dispatch($entity, 'entity_delete');
$event_dispatcher->dispatch($entity, 'entity_delete.' . $entity->getEntityTypeId());

This is very similar to what we do in EntityStorageBase already:

  $this->moduleHandler()->invokeAll($this->entityTypeId . '_' . $hook, [$entity]);
    // Invoke the respective entity-level hook.
    $this->moduleHandler()->invokeAll('entity_' . $hook, [$entity]);

So combined with being able to use a payload rather than an Event object it'd be a lot closer to what we have now for at least single-argument event listeners - i.e. the listeners would be able to type hint on EntityInterface/NodeInterface etc. as appropriate.

However if we do that, then there's no Event class to document the event (dynamic name or not. What we currently do with the Events class constants has had issues in the past too (#3375453: [policy, no patch] Decide on policy regarding use of Events class with string constants for event names or class names instead isn't the issue I was thinking of but it's the issue I found).

@Chi @amateescu
I am sure a lot of the current hooks could be converted to events, but, assuming it is done as I imagine:

• It has to be done separately for each hook, including for contrib.
• It will cause BC break, if existing hook implementations no longer work.

Is this how you imagine it to be done?
Or do you propose a generic way to dispatch events directly from ModuleHandler->invokeAll(), with old implementations still working?

One main benefit of Hux is that it naturally integrates into the existing system, will immediately work for all hooks, and minimizes the BC impact.
To me, it is a huge benefit to be able to use contrib modules with multiple major versions of Drupal core, or to upgrade to a new core version with minimal effort.

And yes, the problems mentioned by @catch also still apply.

#155 is the first comment since about 2012 that made me seriously consider that events could be a viable alternative to hooks (compared to the original idea in this issue or hux), so trying to think through a bit more.

A big advantage of hux still is bc - because we'd still be dispatching hooks, the dispatching API doesn't change (or not unless we want it to) and we have central control over handling all the other additions and deprecations.

If we switch to events we lose at least some of that because eventually every hook invocation needs to eventually dispatch an event instead.

However maybe there's a bit of automation that's possible, although we'd need to deprecate different things at different times.

public function dispatchEventAndInvokeAllDeprecated(object $event, $event_name = NULL, $hook, $args = []) {}

This would:

1. Build a deprecation message with the event and hook names.
2. Call invokeAllDeprecated().
3. Dispatch the event
4. Merge the results. Maybe we need something else for merging since most hook invocations don't care about it.

We could then (eventually, once all of core is converted) deprecate ::invokeAll() and point people to ::dispatchEventAndinvokeAllDeprecated().

Then at some point in the next major release, we'd also deprecate ::dispatchEventAndInvokeAllDeprecated() and tell people to use event dispatcher directly. Although we'd probably also need an event-return-merging helper somewhere, not sure how many hooks still rely on that, but at least hook_views_data() probably does.

Another remaining concern here is there are hooks that we don't want to migrate to events but to something else entirely, like hook_mail() to mail templates in #1803948: [META] Adopt the symfony mailer component. It would be annoying to go to all of the trouble of switching hook_mail to a MailEvent class and then having to deprecate the MailEvent class. But this is more an issue of ordering/timing - it might mean we have to do all the additions and deprecations on quite a long timeline compared to usual. All of that is a non-issue with hux because hook_mail() would still be hook_mail() until it goes to templates.

I don't think you have to worry about old hooks as you could provide one event subscriber in core that subscribes to the new events but calls the old hooks. Then the whole event propagation still at some point gets influenced by the old hooks' results. When Drupal 11 is about to be released, we simply remove the event subscriber that polyfills hook implementations.

@catch This would mean that the calling code needs to be changed 2x.
I would prefer if you can just call ->dispatch() and it does all the rest under the hook, without the calling code having to be updated again in the future.

We are still talking in slack atm, so I don't want to go into too much detail.

But one thing:
Even though I still support the Hux direction, we should explore the event direction in more depth, and see how far we get, possibly with a technical POC.
Some questions to be addressed:

• How does the entire system look like in Drupal 12, after any kind of transition period?
• How can we do a smooth transition, and avoid too much BC breakage?
• How would we convert multiple-argument hooks?

In the end, we can compare and choose between different options. E.g.:

1. Go for "Hux in core" for all hooks, without the plan to convert them to events. (*)
2. Discard hooks and hux, go for events all the way.
3. Use Hux for hooks that don't easily convert to events, and as a transition solution. Then convert specific hooks to events one by one. at some point re-evaluate if we convert everything to events or keep the Hux system.

(*) EDIT: Earlier version of this post said "Discard events".
That's not really what I meant. I would keep existing events, but also keep hooks, without the intent to convert them in the future.
So what we discard would be the idea to convert hooks to events, but not events in general.

Additional notes from slack discusson in #contribute with @donquixote and @amateescu

1. Add EventDispatcher::dispatchVariadic($event_name, ...$args) - this would be a small addition to the event dispatcher, but it would allow for example a hook_form_alter() conversion with no signature changes for the listener method (and for any contrib hooks where they don't want to use an event class). People could still introduce an event class if they want to for specific hooks.

2. In EventDispatch::dispatch() look for a special event class/interface with ::getHookName() so we can dispatch the old hook. For payload hooks use an event_hook_mapping.yaml (or similar) and then find the hook to dispatch from that.

3.Possible approach for docs to replace *.api.php - this would equally work for hux or events, you'd be able to actually use the trait if wanted, but @group etc. makes it discoverable regardless.

Trait EntityLoadLoadTrait {
  /**
   * Useful docs with @group and etc.
   /*
  [#Hook node.load]
  public function entityLoad(EntityInterface $entity) {
    $this->doNodeLoad();
  }

  abstract protected function doEntityLoad(EntityInterface $entity) {
  }
}

use EntityLoadTrait

protected function doEntityload(EntityInterface $entity) {
  // stuff.
}

Discard events, go for "Hux in core" all the way.

This isn't going to be 100% possible due to kernel events and similar dispatched by Symfony. Unless we wrote some kind of event -> hux adapter, but then we can't stop people registering event listeners in services.yml and we'd not be able to drop event dispatcher as a dependency.

Discard events, go for "Hux in core" all the way.

This isn't going to be 100% possible due to kernel events and similar dispatched by Symfony.

Sorry, I did not express myself well.
What I meant is drop the idea that we want to convert all hooks to events, and make peace with most hooks remaining hooks.
I updated my comment.

I created a comparison table as a spreadsheet.
It is open for editing.
(To reduce the risk for it to be vandalized by bots, I scramble the url a bit.)
ht tps://doc s.go ogle.c om/spr eadsheets/d/1581ZT4ezs JLFAZIH7EW2n4NDyfQFENVEcApNxlYYOC4/edit#gid=0

I scramble the url a bit

I could not decrypt the URL.

@Chi, all you have to do is remove the spaces/newline.

Yep, missed the new line.

There are a few hooks that available for themes to implement. HOOK_form_alter, HOOK_theme_suggestions_alter, etc. If those are transformed to events, how will themes implement them? Does it mean that themes should be able to register services?

There are a few hooks that available for themes to implement. HOOK_form_alter, HOOK_theme_suggestions_alter, etc. If those are transformed to events, how will themes implement them? Does it mean that themes should be able to register services?

Good question!
This applies both to hux-ified hooks, and to event-ified hooks.

First thing to note is that technically a call to $theme_manager->alter() is completely separate from $module_handler->alter().
Calling code has to call both separately.
We could convert module hooks without converting theme hooks. Or we do theme hooks as a follow-up.

But let's say we do want themes to register services.

(Just a reminder for people who follow this and forgot how themes work:
Of all the themes available on a website, any number of them can be installed (in D7 we would say "enabled") at a time. The list of installed themes applies site-wide and only changes when configuration (core.extension.theme) changes.
Only one of the installed themes can be active at a time, and this can be determined dynamically, typically based on the page you are visiting.)

If we allow themes to register services, it has to be for all themes that are installed, no matter which is the active theme on a specific page.
Otherwise we would need different containers on different pages, which would not really work.

On the other hand, theme hooks should only be invoked for the active theme.
So the theme manager has to cache hook implementation methods per theme, not globally.
Or for events, there would have to be a special layer to the dispatcher that selects the active theme.

(just for the record, I still favor the hux direction)