Support hooks (Hook attribute) in components

Yes, this would help a lot. Also, the restriction to the Hooks namespace is problematic even in modules:

I implemented hundreds of hooks in the hux POC, where there was the choice to use either the Hooks namespace with auto-service registration, or ANY service. In >90% of the cases, from a code organization perspective, the hook was best to live where it belonged semantically, not in the Hooks namespace.

Or stated it differently: For sane code organization, restrikting hooks to Drupal\my_module\Hooks is the same PITA as to have to put them in the module file.

While migrating a module to OOP hooks I noticed that the restriction of "one implementation per module" is still there. Now is rather silent as there is no error about it. For example having two classes in the \Drupal\my_module\Hook namespace that have the same hook annotations.

I would be really convenient to have multiple implementations per module to organize related features into the same class. E.g. a module that has two field widget third party settings and each one has its hook_field_widget_third_party_settings_form with a corresponding hook_field_widget_complete_form_alter. One implementation for each of the third party setting separated in different classes.

Is this something that can be tackled in this issue or should I open a new one?

Yeah we need to update that cr. The one per module restriction was added back when we had to fix hook module implements alter.

However you can still mark one method with more than one hook.

Can you link to the issue? Just want to investigate if there is a possibility to be added back.

I updated the CR to clarify the point about the multiple implementations
https://www.drupal.org/node/3442349 →

https://www.drupal.org/project/drupal/issues/3484754 🐛 Ordering of alter "extra hooks" is a gigantic mess Active

Thank you, it can one we remove hmia

Thanks for the link.
The project I migrated is https://git.drupalcode.org/project/change_labels/-/tree/1.x?ref_type=heads
but is already frankensteined with traits to workaround this issue that I'm talking about.

To showcase the problem I created a patch for automated_cron with similar hook_field_widget_third_party_settings_form implementations.
Only the "second" implementation end up displaying its form in the widget settings.

Maybe this is a special case since the calling module is important to later determine where to save the third party setting.

Moving one of the classes to another module (and changing the namespace) makes both form items to appear.

@rodrigoaguilera My understanding is that having multiple implementations of the same hook in a module makes re-ordering hooks via hook_module_implements_alter() convoluted, so there will be only 1 hook per module allowed until the whole procedural hook BC layer is removed in the future.

As for this issue: I think the need can be addressed by something similar to WIP MRs in 📌 Support #[AsEventListener] attribute on classes and methods Active . If we do the following, then registered services that have the #[Hook] and #[AsEventListener] attributes should get registered correctly as event listeners.

• Make the Hook attribute a subclass of AsEventListener
• Add something like this:
  

      $container->registerAttributeForAutoconfiguration(AsEventListener::class, static function (
        ChildDefinition $definition,
        AsEventListener $attribute,
        \ReflectionClass|\ReflectionMethod $reflector,
      ) {
        $tagAttributes = get_object_vars($attribute);
        if ($reflector instanceof \ReflectionMethod) {
          if (isset($tagAttributes['method'])) {
            throw new LogicException(sprintf('AsEventListener attribute cannot declare a method on "%s::%s()".', $reflector->class, $reflector->name));
          }
          $tagAttributes['method'] = $reflector->getName();
        }
        $definition->addTag('kernel.event_listener', $tagAttributes);
      });
  

There is the risk that someone registers a Hook service in their .services.yml, and the class for the service is in the Hook namespace, that the listener would get added twice and fire twice per event, but it can be documented that services in the Hook namespace should not be added to services.yml, and services outside that namespace should.

The hmia patch did not remove the multiple implementations feature, at least not intentionally, if it's missing that's a bug, it is collecting into $this->implementations[$hook->hook][$module][$class][] = $hook->method; and the moduleImplementsAlters property is only used to determine module order. By all means it should work. I would recommend running \Drupal::getContainer()->getParameter('hook_implementations_map')['field_widget_third_party_settings_form'] from drush php and checking what's in there. If you see both of your classes then the problem is in either ModuleHandler or the closure used by EditFormDisplayEditForm::thirdPartySettingsForm (it's using closure $hook, that's one possible problem down). Truth to be told, I don't think either nicxvan or myself have tested invokeAllWith by itself which is what's used here but at the same time invokeAll relies on it so the chances of it being broken is quite low. But, it's software, and even worse, it's software written by me, so it being broken is not as impossible as one might desire.

As for supporting the hook attribute in any service, that's not a particularly hard problem, it's just a performance/memory nightmare as you'd need to reflect every class and method defining a service to see whether they have a #Hook attribute. Of course, it's not my call but if it were then I'd split Rodrigo's problem into a separate issue and postpone this one on 📌 Add a core FileCache implementation that is database-backed Active . Just a suggestion.

it's just a performance/memory nightmare as you'd need to reflect every class and method defining a service to see whether they have a #Hook attribute

Yeah, forgot to mention that the MRs for 📌 Support #[AsEventListener] attribute on classes and methods Active need to be rebased, but I was holding off on that until 📌 Add a filecache-backed attribute parser Active goes in.

Apologies for the noise. Indeed multiple implementations are supported but there is special cases like hook_field_widget_third_party_settings_form that won't work the multiple implementations. Will file an issue for that.

I changed the CR to reflect that
https://www.drupal.org/node/3442349 →

Indeed let's move to a separate issue, the fix is as simple as $settings_form[$module] = ($settings_form[$module] ?? []) + $hook( that a trivial bug / missing feature it's not worth changing main CR over a small bug. Like, of all hooks only field_widget_third_party_settings_form/field_formatter_third_party_settings_form is affected and it won't be affected for long.

Filed 🐛 Support multiple implementations for hook_field_widget_third_party_settings_form Active

For example \Drupal\Core\Extension\ModuleHandler::invoke() does in fact only support a single hook for a given module, I assumed that's true in general, and based on that adjusted ultimate_cron in 📌 Fix 11.1 test fails Active to still check for hooks and invoke them through the module handler. Might need to change that then, but the nice thing about this approach is that it doesn't care about where that hook lives exactly, same config works in D10 and D11.1+.

Back to the topic.

> As for supporting the hook attribute in any service, that's not a particularly hard problem, it's just a performance/memory nightmare as you'd need to reflect every class and method defining a service to see whether they have a #Hook attribute

Two ideas to make that less of a nightmare.

a) What if we'd make the discovery/reflection opt in? We do it for everything in the Hooks namespace, but if you explicitly add a tag to your service, we scan that too? Means autoconfig of services won't work.
b) We keep the Hook namespace restriction, but expand it to components. That only helps core (for now?), but it's similar to plugin discovery, where we support that too (various plugins are for example in (Drupal\Core\Entity\Plugin). So if we want to move the cache component part of system_cron() to the Cache component, we'd add a Drupal\Core\Cache\Hooks\CacheHooks class?

Hrmmmm, I will update the CR to talk about ::invoke the original IS did it just didn't make it into the CR -- in short, those are an anomaly too, those are not really hooks :) and luckily there are very few of them.

To be on topic. Sure, adding a service tag to indicate it's a hook and then adding those classes into HookCollectorPass is certainly a possibility. Let's think aloud what that would take, first it needs an optional $container passed to collectAllHookImplementations (optional because ModuleHandler::add calls it). Once that's there code should read something like:

        foreach ($container->findTaggedServiceIds('drupal_hook') as $service_id => $tag) {
          $container->getDefinition($service_id)->getClass()
          foreach (static::getHookAttributesInClass($class) as $attribute) {
            $this->addFromAttribute($attribute, $class, $module);
          }
        }

That's nice -- but there's no $module. That information just doesn't exist and while for service YAMLs it could be persisted with a little effort -- DrupalKernel does index $this->serviceYamls by $module even if it doesn't pass it on -- services created by providers do not have this information.

I am guessing you could make $module optional in addFromAttribute and grab the module from the class name:

    if ($hook->module) {
      $module = $hook->module;
    }
    elseif (!$module) {
      $module = explode('\\', $class)[1];
    }
    if (!$module) {
      throw new WhateverException;
    }

this will work for Drupal modules and when it doesn't, well, just specify $module in #[Hook]. Most of the time it is irrelevant anyways -- invokeAllWith needs it because it calls $callback($listener, $module); where $module is cheerfully ignored in invokeAll and indeed in almost all code using invokeAllWith but still, that signature can't be changed without serious upheaval and so something needs to be supplied there.

@ghost of drupal past #11, @berdir #16, et al

it's just a performance/memory nightmare as you'd need to reflect every class and method defining a service to see whether they have a #Hook attribute

Maybe you are not aware of $container->registerAttributeForAutoconfiguration?
See #10 on how to implement.

The $container->registerAttributeForAutoconfiguration is directly copied from Symfony Framework bundle: https://github.com/symfony/symfony/blob/e128d76b3dfd5147fe1263a807e8ce83..., but it effectively just does the reflection looping that is the performance issue in question.

> but it effectively just does the reflection looping that is the performance issue in question.

Which means:
- That compiler pass is already in place, so no nightmare to fear.
- It's the central place to do the reflection, with no performance penelty for any additional consumer.

So IF that is used, no reason for performance fear left.

Which means:
- That compiler pass is already in place, iterating over all services, and no nightmare happened.

It only iterates with reflection if attributes are registered for autoconfiguration via $container->registerAttributeForAutoconfiguration. Right now there aren't any in Drupal core, so this reflection iteration does not occur.

Supporting magic naming instead of or in addition to tagged services is a good idea, thanks. But I don't think it merits a separate issue, indeed it should be discussed here:

1. support tagged services
2. support magic named classes
3. support both

Implementations wise, this is once again a minor tweak to HookCollectorPass. In this case, the filter iterator needs a minor adjustment: the iterator should iterate all of src instead of src/Hook but only return php files if the subPath starts with Hook or the file name ends in Hooks. This is not a problem, much as it was trivial to support tagged services the same is true for magic named classes.

No Symfony magic can avoid the problem outlined -- catch however is solving it with caching. Indeed, the less Symfony used, all the better as we have just seen they are incapable of keeping even the most basic semver. Of course, that's just my two cents, of course someone will eventually refactor the entire HookCollectorPass to use registerAttributeForAutoconfiguration and then no one ever will be able to debug the ensuing problems -- I know, I read the entire code flow for the latter to evalute for the hook oop patch, the complexity is horrifying. But, at least, it'll use Symfony, yay! But I digress.

> registerAttributeForAutoconfiguration is only used by the full Symfony framework, not the components as shipped and I am fairly sure it requires the Symfony Config component.

Hmm, i am using it i a quite big customer project with >300 contrib and custom modules to autoregister hux classes. No package added, except ~10 lines of custom code, and no extraordinary container build times.

I created a duplicate of this (which I closed) - ✨ Allow any service to subscribe to hooks Active which lists some more reasons to do this:

We have a lovely new hooks system that integrates the module handler and the event dispatcher to invoke hooks. In doing this we had to move a hook. core_field_views_data to \Drupal\views\Hook\ViewsViewsHooks::fieldViewsData() even though what it does - provide entity reference field integration with views could be considered to be core's responsibility.

Another example could be kernel tests to enable hook testing whether the test and the test hook implementation live side-by-side - see 📌 Make hook testing with kernel tests very simple Active

Also this could be used to make things like

  /**
   * Implements hook_cron().
   */
  #[Hook('cron')]
  public function cron(): void {
    $this->workspaceManager->purgeDeletedWorkspacesBatch();
  }

from \Drupal\workspaces\Hook\WorkspacesHooks simpler. \Drupal\workspaces\WorkspaceManager::purgeDeletedWorkspacesBatch could subscribe to the hook - although we'd have to ensure anything that decorates it is also subscribed...