Support hooks (Hook attribute) in any registered service

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

Thank you, it can be added back once we remove hmia. But we can't reorder between.

Did your module not even pick up the multiple? Imd acrylamide like to see the example. Can you share the code?

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 #3376163: Support #[AsEventListener] attribute on classes and methods. 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.

The problem is in EditFormDisplayEditForm::thirdPartySettingsForm as it stores the return value of $hook into $settings_form[$module].

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 #3486503: Add a persistent cache for file-based discovery based on FileCache at least. But it's possible #3395260: Investigate possibilities to parse attributes without reflection would be needed. 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 #3376163: Support #[AsEventListener] attribute on classes and methods need to be rebased, but I was holding off on that until #3478621: Add filecache to OOP hook attribute parsing 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 #3489707: Support multiple implementations for hook_field_widget_third_party_settings_form and field_formatter_third_party_settings_form in the same module

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. Edit: added "Also, a module can implement almost all hooks multiple times as documented on the Hook attribute itself."

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('something maybe drupal_hook? maybe drupal.hook? whatever') as $service_id => $tag) {
          $class = $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.

Someone else will need to turn this comment into PR.

@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, iterating over all services, and no nightmare happened.
- It's the central place to do the reflection, with no performance penelty for any additional consumer.

So IF $container->registerAttributeForAutoconfiguration is used, i see no rational 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.

As noted above, the problem is how every class reflected by PHP stays in memory until the end of the request regardless whether Symfony or Drupal does the reflection. There's work being done elsewhere to utilize crafty caching and/or nikic/php-parser to avoid the massive memory hit. Both magic naming and tagging would make an initial process faster but ultimately might not be necessary if these efforts bore fruit.

This memory hit is new. Up until now only plugins started using attributes but from 11.1 all hook implementations will become OOP.

So really the decision is

1. support tagged services
2. support magic named classes
3. support both
4. punt and once caching made attribute collections cheaper just allow any class any method to use #[Hook]

Ps.: 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.

> 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) - #3502472: Allow any service to subscribe to hooks 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 #3502432: Make hook testing with kernel tests very simple

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...

So IF $container->registerAttributeForAutoconfiguration is used, i see no rational reason for performance fear left.

So while core does not use $container->registerAttributeForAutoconfiguration, with

autoconfigure: true

and several interfaces registered for autoconfiguraiotn in CoreServiceProvider::register() with $container->registerForAutoconfiguration() (no Attribute), it does turn out that at compile time all core services are iterated over and reflected.

Given this, @ghost of drupal past mentioned that all service classes are being loaded into memory at compile time anyway, so iterating with reflection on these classes to discover class-level `Hook` attributes has a negligible affect performance. @nicxvan also ran performance tests iterating reflection through each of a class's methods, and the hit on performance was not significant even with the number of methods in the class in 100,000s. Without an additional performance concern, it seems like a good idea to go forward.

Just to add to it, here is the code we used for benchmarking:

  $r = new \ReflectionClass($object);
  $time = microtime(TRUE);
  $n = count($r->getMethods());
  print (microtime(TRUE) - $time) . \PHP_EOL;

  $time = microtime(TRUE);
  for ($i = 0; $i < $n; $i++) {
    new stdClass;
  }
  print (microtime(TRUE) - $time) . \PHP_EOL;
  print ($n) . \PHP_EOL . \PHP_EOL;

We ran it with different classes with different numbers of methods.
The numbers returned are the time to check the class, the time to create the new stdClass and the number of methods:

0.24680495262146
0.058710098266602
1000000

0.015760898590088
0.0052649974822998
100000

0.0014050006866455
0.00056791305541992
10000

0.00011301040649414
7.2002410888672E-5
1000

1.5974044799805E-5
1.0013580322266E-5
100

2.8610229492188E-6
2.1457672119141E-6
10

Note that this is probably soft-blocked by #3485896: Hook ordering across OOP, procedural and with extra types i.e replace hook_module_implements_alter, or at least there will likely be a giant merge conflict resolution needed between the two.

Yes let's please postpone this

A couple thoughts on issues that might need accounting for, without confirming in code:

• If someone has registered a class in their module's Hook folder as a service in their *.services.yml, whether by mistake or holdover from D10 compatibility, probably need to make sure that that class method is not called twice for the hook
• If someone alters an existing service using a ServiceModifier or service decorator, then presumably the hook methods in the original class, and for decorated services, hook methods in every class except the outermost decorator, should not run. So only the outermost class's hook methods should be invoked

If there are complications with either scenario that aren't easy to resolve, then maybe perhaps to reduce scope it can be documented that hooks aren't supported for altered services in whatever use cases and punt those to be done later.

MR 12222 is up.

Notes:

• Any module service that is not abstract or synthetic can use Hook attributes to implement hooks
• This does not apply to services that (somehow?) do not have a class defined, services in the Drupal\Core and Drupal\Component namespaces and non-Drupal classes
• Kernel test classes can implement hooks. Use register() to register the kernel test class itself as service and any Hook attributes in the class will be picked up
• For services provided and/or altered by a service provider/modifier, or decorated services:
  • Hook implementations that exist in the new service class will be picked up
  • For altered or decorated services, if the new service class extends the original service class, and the hook implementation method is not overridden, the hook method will run once
  • For altered or decorated services, if the new service class extends the original service class, and the hook implementation method is overridden with no Hook attribute, the hook method will not be invoked
  • For altered or decorated services, if the new service class does not extend the original service class, none of the original service class's hook implementations will be invoked.
  • It is not possible to use a service modifier to alter Hook\ class services that are not registered services. This is because, in order to support altered services, HookCollectorPass has been moved to run after the ModifyServiceDefinitionsPass, so the the Hook class definitions do not exist when alter() in a service modifier runs (see more below*). If Hook\ class behavior needs to be changed, using a service decorator is recommended

* For services definitions provided by or modified by service providers to be picked correctly by HookCollectorPass, the defintion additions and alterations need to be registered before HookCollectorPass runs. The only way to get around this otherwise would be to make the HookCollectorPass essentially run twice, once before and once after the ModifyServiceDefinitionsPass. Before moving HookCollectorPass to run after ModifyServiceDefinitionsPass, hook implementations in services registered or modified in ModifyServiceDefinitionsPass would not run anyway. One issue that came up is that if any service provider instantiates the module handler, the hook_implementation_map container parameter needs to exist before that, so that parameter is initialized to an empty array first.

For reference, here's output of a representative local run of drush si standard -y -vvvv on HEAD:

 [info] Starting bootstrap to root [0.15 sec, 2.91 MB]
 [info] Drush bootstrap phase 1 [0.15 sec, 2.91 MB]
 [info] Try to validate bootstrap phase 1 [0.15 sec, 2.91 MB]
 [info] Try to validate bootstrap phase 1 [0.15 sec, 2.92 MB]
 [info] Try to bootstrap at phase 1 [0.15 sec, 2.92 MB]
 [info] Drush bootstrap phase: bootstrapDrupalRoot() [0.15 sec, 2.92 MB]
 [info] Change working directory to /var/www/html [0.15 sec, 2.91 MB]
 [info] Initialized Drupal 11.3-dev root directory at /var/www/html [0.15 sec, 2.92 MB]
 [info] Drush bootstrap phase: bootstrapDrupalSite() [0.16 sec, 3.19 MB]
 [debug] Could not find a Drush config file at sites/default/drush.yml. [0.16 sec, 3.19 MB]
 [info] Initialized Drupal site drupal.ddev.site at sites/default [0.16 sec, 3.19 MB]
 [info] Drush bootstrap phase: bootstrapDrupalConfiguration() [0.16 sec, 3.19 MB]
 [info] Executing: command -v sqlite3 [0.16 sec, 3.41 MB]
 You are about to:
 * DROP all tables in your 'sites/default/files/.sqlite' database.

 // Do you want to continue?: yes.

 [info] Sites directory sites/default already exists - proceeding. [0.16 sec, 3.48 MB]
 [info] sql:query: .tables [0.16 sec, 3.48 MB]
 [info] Executing: sqlite3  sites/default/files/.sqlite   < /tmp/drush_uaOpuf [0.16 sec, 3.49 MB]
 [info] sql:query: DROP TABLE block_content; DROP TABLE block_content__body; DROP TABLE block_content__field_body; DROP TABLE block_content__field_content_link; DROP TABLE block_content__field_copyright; DROP TABLE block_content__field_disclaimer; DROP TABLE block_content__field_media_image; DROP TABLE block_content__field_summary; DROP TABLE block_content__field_title; DROP TABLE block_content_field_data; DROP TABLE block_content_field_revision; DROP TABLE block_content_revision; DROP TABLE block_content_revision__body; DROP TABLE block_content_revision__field_body; DROP TABLE block_content_revision__field_content_link; DROP TABLE block_content_revision__field_copyright; DROP TABLE block_content_revision__field_disclaimer; DROP TABLE block_content_revision__field_media_image; DROP TABLE block_content_revision__field_summary; DROP TABLE block_content_revision__field_title; DROP TABLE cache_container; DROP TABLE config; DROP TABLE content_moderation_state; DROP TABLE content_moderation_state_field_data; DROP TABLE content_moderation_state_field_revision; DROP TABLE content_moderation_state_revision; DROP TABLE file_managed; DROP TABLE file_usage; DROP TABLE help_search_items; DROP TABLE history; DROP TABLE inline_block_usage; DROP TABLE key_value; DROP TABLE locale_file; DROP TABLE locales_location; DROP TABLE locales_source; DROP TABLE locales_target; DROP TABLE media; DROP TABLE media__field_media_audio_file; DROP TABLE media__field_media_document; DROP TABLE media__field_media_image; DROP TABLE media__field_media_oembed_video; DROP TABLE media__field_media_video_file; DROP TABLE media_field_data; DROP TABLE media_field_revision; DROP TABLE media_revision; DROP TABLE media_revision__field_media_audio_file; DROP TABLE media_revision__field_media_document; DROP TABLE media_revision__field_media_image; DROP TABLE media_revision__field_media_oembed_video; DROP TABLE media_revision__field_media_video_file; DROP TABLE menu_link_content; DROP TABLE menu_link_content_data; DROP TABLE menu_link_content_field_revision; DROP TABLE menu_link_content_revision; DROP TABLE menu_tree; DROP TABLE node; DROP TABLE node__body; DROP TABLE node__field_body; DROP TABLE node__field_cooking_time; DROP TABLE node__field_difficulty; DROP TABLE node__field_ingredients; DROP TABLE node__field_media_image; DROP TABLE node__field_number_of_servings; DROP TABLE node__field_preparation_time; DROP TABLE node__field_recipe_category; DROP TABLE node__field_recipe_instruction; DROP TABLE node__field_summary; DROP TABLE node__field_tags; DROP TABLE node__layout_builder__layout; DROP TABLE node_access; DROP TABLE node_field_data; DROP TABLE node_field_revision; DROP TABLE node_revision; DROP TABLE node_revision__body; DROP TABLE node_revision__field_body; DROP TABLE node_revision__field_cooking_time; DROP TABLE node_revision__field_difficulty; DROP TABLE node_revision__field_ingredients; DROP TABLE node_revision__field_media_image; DROP TABLE node_revision__field_number_of_servings; DROP TABLE node_revision__field_preparation_time; DROP TABLE node_revision__field_recipe_category; DROP TABLE node_revision__field_recipe_instruction; DROP TABLE node_revision__field_summary; DROP TABLE node_revision__field_tags; DROP TABLE node_revision__layout_builder__layout; DROP TABLE path_alias; DROP TABLE path_alias_revision; DROP TABLE router; DROP TABLE search_dataset; DROP TABLE search_index; DROP TABLE search_total; DROP TABLE sequences; DROP TABLE shortcut; DROP TABLE shortcut_field_data; DROP TABLE shortcut_set_users; DROP TABLE taxonomy_index; DROP TABLE taxonomy_term__parent; DROP TABLE taxonomy_term_data; DROP TABLE taxonomy_term_field_data; DROP TABLE taxonomy_term_field_revision; DROP TABLE taxonomy_term_revision; DROP TABLE taxonomy_term_revision__parent; DROP TABLE user__roles; DROP TABLE user__user_picture; DROP TABLE users; DROP TABLE users_data; DROP TABLE users_field_data; DROP TABLE watchdog;  [0.17 sec, 3.51 MB]
 [info] Executing: sqlite3  sites/default/files/.sqlite   < /tmp/drush_SYPKPZ [0.17 sec, 3.51 MB]
 [notice] Starting Drupal installation. This takes a while. [0.24 sec, 3.49 MB]
 [debug] Calling install_drupal(Composer\Autoload\ClassLoader, array, array) [0.24 sec, 3.49 MB]
 [notice] Performed install task: install_select_language [0.4 sec, 12.08 MB]
 [notice] Performed install task: install_select_profile [0.4 sec, 12.08 MB]
 [notice] Performed install task: install_load_profile [0.4 sec, 12.08 MB]
 [notice] Performed install task: install_verify_requirements [0.4 sec, 12.43 MB]
 [notice] Performed install task: install_verify_database_ready [0.4 sec, 12.43 MB]
 [info] sqlite module installed. [0.5 sec, 17.62 MB]
 [info] system module installed. [0.61 sec, 20.98 MB]
 [notice] Performed install task: install_base_system [0.61 sec, 21.01 MB]
 [notice] Performed install task: install_bootstrap_full [0.61 sec, 21.01 MB]
 [info] user module installed. [0.98 sec, 27.73 MB]
 [info] path_alias module installed. [0.98 sec, 27.73 MB]
 [info] big_pipe module installed. [0.98 sec, 27.73 MB]
 [info] config module installed. [0.98 sec, 27.73 MB]
 [info] help module installed. [0.98 sec, 27.53 MB]
 [info] page_cache module installed. [0.98 sec, 27.53 MB]
 [info] dynamic_page_cache module installed. [0.98 sec, 27.53 MB]
 [info] automated_cron module installed. [0.98 sec, 27.53 MB]
 [info] announcements_feed module installed. [0.98 sec, 27.53 MB]
 [info] block module installed. [0.98 sec, 27.53 MB]
 [info] filter module installed. [0.99 sec, 27.85 MB]
 [info] views module installed. [0.99 sec, 27.87 MB]
 [info] field module installed. [0.99 sec, 27.68 MB]
 [info] text module installed. [0.99 sec, 27.68 MB]
 [info] block_content module installed. [1 sec, 27.79 MB]
 [info] breakpoint module installed. [1 sec, 27.79 MB]
 [info] file module installed. [1 sec, 27.59 MB]
 [info] editor module installed. [1 sec, 27.59 MB]
 [info] ckeditor5 module installed. [1 sec, 27.59 MB]
 [info] image module installed. [1.01 sec, 27.82 MB]
 [info] media module installed. [1.31 sec, 34.62 MB]
 [info] language module installed. [1.32 sec, 34.67 MB]
 [info] locale module installed. [1.32 sec, 34.71 MB]
 [info] config_translation module installed. [1.32 sec, 34.71 MB]
 [info] contact module installed. [1.32 sec, 34.45 MB]
 [info] workflows module installed. [1.32 sec, 34.19 MB]
 [info] content_moderation module installed. [1.32 sec, 34.19 MB]
 [info] node module installed. [1.35 sec, 34.99 MB]
 [info] content_translation module installed. [1.36 sec, 35.01 MB]
 [info] contextual module installed. [1.36 sec, 34.75 MB]
 [info] datetime module installed. [1.36 sec, 34.75 MB]
 [info] taxonomy module installed. [1.37 sec, 34.92 MB]
 [info] dblog module installed. [1.37 sec, 34.65 MB]
 [info] layout_discovery module installed. [1.37 sec, 34.65 MB]
 [info] field_ui module installed. [1.37 sec, 34.65 MB]
 [info] history module installed. [1.37 sec, 34.65 MB]
 [info] layout_builder module installed. [1.37 sec, 34.65 MB]
 [info] link module installed. [1.37 sec, 34.65 MB]
 [info] media_library module installed. [1.52 sec, 42.65 MB]
 [info] menu_link_content module installed. [1.52 sec, 42.65 MB]
 [info] menu_ui module installed. [1.69 sec, 50.33 MB]
 [info] path module installed. [1.69 sec, 50.33 MB]
 [info] options module installed. [1.69 sec, 50.33 MB]
 [info] responsive_image module installed. [1.69 sec, 50.33 MB]
 [info] search module installed. [1.69 sec, 50.33 MB]
 [info] toolbar module installed. [1.69 sec, 50.33 MB]
 [info] shortcut module installed. [1.69 sec, 50.29 MB]
 [info] views_ui module installed. [1.69 sec, 50.29 MB]
 [notice] Performed install task: install_profile_modules [1.71 sec, 45.49 MB]
 [info] claro theme installed. [1.72 sec, 44.5 MB]
 [info] umami theme installed. [1.72 sec, 44.54 MB]
 [notice] Performed install task: install_profile_themes [1.74 sec, 45 MB]
 [warning] The "field_block:node:page:field_body" block plugin was not found [3.11 sec, 78.42 MB]
 [warning] The "extra_field_block:node:page:links" block plugin was not found [3.11 sec, 78.42 MB]
 [warning] The "extra_field_block:node:page:content_moderation_control" block plugin was not found [3.11 sec, 78.42 MB]
 [warning] The "field_block:node:page:field_body" block plugin was not found [3.11 sec, 78.42 MB]
 [warning] The "extra_field_block:node:page:links" block plugin was not found [3.11 sec, 78.42 MB]
 [warning] The "extra_field_block:node:page:content_moderation_control" block plugin was not found [3.11 sec, 78.42 MB]
 [warning] The "field_block:node:article:field_tags" block plugin was not found [3.13 sec, 77.49 MB]
 [warning] The "field_block:node:article:field_media_image" block plugin was not found [3.13 sec, 77.49 MB]
 [warning] The "field_block:node:article:field_body" block plugin was not found [3.13 sec, 77.49 MB]
 [warning] The "extra_field_block:node:article:links" block plugin was not found [3.13 sec, 77.49 MB]
 [warning] The "extra_field_block:node:article:content_moderation_control" block plugin was not found [3.13 sec, 77.49 MB]
 [warning] The "field_block:node:article:field_tags" block plugin was not found [3.14 sec, 77.49 MB]
 [warning] The "field_block:node:article:field_media_image" block plugin was not found [3.14 sec, 77.49 MB]
 [warning] The "field_block:node:article:field_body" block plugin was not found [3.14 sec, 77.49 MB]
 [warning] The "extra_field_block:node:article:links" block plugin was not found [3.14 sec, 77.49 MB]
 [warning] The "extra_field_block:node:article:content_moderation_control" block plugin was not found [3.14 sec, 77.49 MB]
 [warning] The "field_block:node:recipe:field_tags" block plugin was not found [3.31 sec, 82.28 MB]
 [warning] The "field_block:node:recipe:field_recipe_category" block plugin was not found [3.31 sec, 82.28 MB]
 [warning] The "field_block:node:recipe:field_summary" block plugin was not found [3.32 sec, 82.28 MB]
 [warning] The "field_block:node:recipe:field_media_image" block plugin was not found [3.32 sec, 82.28 MB]
 [warning] The "field_block:node:recipe:field_difficulty" block plugin was not found [3.32 sec, 82.28 MB]
 [warning] The "field_block:node:recipe:field_number_of_servings" block plugin was not found [3.32 sec, 82.28 MB]
 [warning] The "field_block:node:recipe:field_cooking_time" block plugin was not found [3.32 sec, 82.28 MB]
 [warning] The "field_block:node:recipe:field_preparation_time" block plugin was not found [3.32 sec, 82.28 MB]
 [warning] The "field_block:node:recipe:field_media_image" block plugin was not found [3.32 sec, 82.28 MB]
 [warning] The "field_block:node:recipe:field_ingredients" block plugin was not found [3.32 sec, 82.28 MB]
 [warning] The "field_block:node:recipe:field_recipe_instruction" block plugin was not found [3.32 sec, 82.28 MB]
 [warning] The "extra_field_block:node:recipe:content_moderation_control" block plugin was not found [3.32 sec, 82.28 MB]
 [warning] The "field_block:node:recipe:field_tags" block plugin was not found [3.32 sec, 82.29 MB]
 [warning] The "field_block:node:recipe:field_recipe_category" block plugin was not found [3.32 sec, 82.29 MB]
 [warning] The "field_block:node:recipe:field_summary" block plugin was not found [3.32 sec, 82.29 MB]
 [warning] The "field_block:node:recipe:field_media_image" block plugin was not found [3.32 sec, 82.29 MB]
 [warning] The "field_block:node:recipe:field_difficulty" block plugin was not found [3.32 sec, 82.29 MB]
 [warning] The "field_block:node:recipe:field_number_of_servings" block plugin was not found [3.32 sec, 82.29 MB]
 [warning] The "field_block:node:recipe:field_cooking_time" block plugin was not found [3.32 sec, 82.29 MB]
 [warning] The "field_block:node:recipe:field_preparation_time" block plugin was not found [3.32 sec, 82.29 MB]
 [warning] The "field_block:node:recipe:field_media_image" block plugin was not found [3.32 sec, 82.29 MB]
 [warning] The "field_block:node:recipe:field_ingredients" block plugin was not found [3.32 sec, 82.29 MB]
 [warning] The "field_block:node:recipe:field_recipe_instruction" block plugin was not found [3.32 sec, 82.29 MB]
 [warning] The "extra_field_block:node:recipe:content_moderation_control" block plugin was not found [3.32 sec, 82.29 MB]
 [info] demo_umami_content module installed. [4.72 sec, 104.79 MB]
 [info] demo_umami module installed. [4.77 sec, 99.85 MB]
 [notice] Performed install task: install_install_profile [4.77 sec, 98.92 MB]
 [error]  Import of string "<span class="visually-hidden">Mostrar </span>@title<span class="visually-hidden"> medios </span> <span class="active-tab visually-hidden"> (seleccionados) </ lapso>" was skipped because of disallowed or malformed HTML. [7.16 sec, 99.48 MB]
 [notice] Translations imported: 8854 added, 0 updated, 0 removed. [7.69 sec, 99.55 MB]
 [warning] 1 disallowed HTML string(s) in files: translations://drupal-11.3-dev.es.po. [7.69 sec, 99.55 MB]
 [notice] Performed install task: install_import_translations [7.69 sec, 99.01 MB]
 [info] update module installed. [7.89 sec, 110.12 MB]
 [notice] Performed install task: install_configure_form [8.46 sec, 111.8 MB]
 [notice] The configuration was successfully updated. 203 configuration objects updated. [9.01 sec, 97.03 MB]
 [notice] Performed install task: install_finish_translations [9.01 sec, 96.99 MB]
 [notice] Performed install task: install_finished [9.09 sec, 97.83 MB]
 [success] Installation complete.  User name: admin  User password: J79hpJRQ87 [9.09 sec, 97.82 MB]

And here's the same output run against the MR branch:

 [info] Starting bootstrap to root [0.78 sec, 2.91 MB]
 [info] Drush bootstrap phase 1 [0.78 sec, 2.91 MB]
 [info] Try to validate bootstrap phase 1 [0.78 sec, 2.91 MB]
 [info] Try to validate bootstrap phase 1 [0.78 sec, 2.92 MB]
 [info] Try to bootstrap at phase 1 [0.78 sec, 2.92 MB]
 [info] Drush bootstrap phase: bootstrapDrupalRoot() [0.78 sec, 2.92 MB]
 [info] Change working directory to /var/www/html [0.78 sec, 2.91 MB]
 [info] Initialized Drupal 11.3-dev root directory at /var/www/html [0.78 sec, 2.92 MB]
 [info] Drush bootstrap phase: bootstrapDrupalSite() [0.78 sec, 3.19 MB]
 [debug] Could not find a Drush config file at sites/default/drush.yml. [0.78 sec, 3.19 MB]
 [info] Initialized Drupal site drupal.ddev.site at sites/default [0.78 sec, 3.19 MB]
 [info] Drush bootstrap phase: bootstrapDrupalConfiguration() [0.78 sec, 3.19 MB]
 [info] Executing: command -v sqlite3 [0.79 sec, 3.41 MB]
 You are about to:
 * DROP all tables in your 'sites/default/files/.sqlite' database.

 // Do you want to continue?: yes.

 [info] Sites directory sites/default already exists - proceeding. [0.79 sec, 3.48 MB]
 [info] sql:query: .tables [0.79 sec, 3.48 MB]
 [info] Executing: sqlite3  sites/default/files/.sqlite   < /tmp/drush_OHkNT4 [0.79 sec, 3.49 MB]
 [info] sql:query: DROP TABLE block_content; DROP TABLE node_revision; DROP TABLE block_content__body; DROP TABLE node_revision__body; DROP TABLE block_content_field_data; DROP TABLE node_revision__comment; DROP TABLE block_content_field_revision; DROP TABLE node_revision__field_image; DROP TABLE block_content_revision; DROP TABLE node_revision__field_tags; DROP TABLE block_content_revision__body; DROP TABLE path_alias; DROP TABLE comment; DROP TABLE path_alias_revision; DROP TABLE comment__comment_body; DROP TABLE router; DROP TABLE comment_entity_statistics; DROP TABLE search_dataset; DROP TABLE comment_field_data; DROP TABLE search_index; DROP TABLE config; DROP TABLE search_total; DROP TABLE file_managed; DROP TABLE sequences; DROP TABLE file_usage; DROP TABLE shortcut; DROP TABLE help_search_items; DROP TABLE shortcut_field_data; DROP TABLE history; DROP TABLE shortcut_set_users; DROP TABLE key_value; DROP TABLE taxonomy_index; DROP TABLE menu_link_content; DROP TABLE taxonomy_term__parent; DROP TABLE menu_link_content_data; DROP TABLE taxonomy_term_data; DROP TABLE menu_link_content_field_revision; DROP TABLE taxonomy_term_field_data; DROP TABLE menu_link_content_revision; DROP TABLE taxonomy_term_field_revision; DROP TABLE menu_tree; DROP TABLE taxonomy_term_revision; DROP TABLE node; DROP TABLE taxonomy_term_revision__parent; DROP TABLE node__body; DROP TABLE user__roles; DROP TABLE node__comment; DROP TABLE user__user_picture; DROP TABLE node__field_image; DROP TABLE users; DROP TABLE node__field_tags; DROP TABLE users_data; DROP TABLE node_access; DROP TABLE users_field_data; DROP TABLE node_field_data; DROP TABLE watchdog; DROP TABLE node_field_revision;  [0.79 sec, 3.5 MB]
 [info] Executing: sqlite3  sites/default/files/.sqlite   < /tmp/drush_8ep5L0 [0.79 sec, 3.5 MB]
 [notice] Starting Drupal installation. This takes a while. [0.85 sec, 3.49 MB]
 [debug] Calling install_drupal(Composer\Autoload\ClassLoader, array, array) [0.85 sec, 3.49 MB]
 [notice] Performed install task: install_select_language [1.03 sec, 12.1 MB]
 [notice] Performed install task: install_select_profile [1.03 sec, 12.1 MB]
 [notice] Performed install task: install_load_profile [1.03 sec, 12.1 MB]
 [notice] Performed install task: install_verify_requirements [1.04 sec, 12.45 MB]
 [notice] Performed install task: install_verify_database_ready [1.04 sec, 12.45 MB]
 [info] sqlite module installed. [1.14 sec, 17.64 MB]
 [info] system module installed. [1.2 sec, 21 MB]
 [notice] Performed install task: install_base_system [1.21 sec, 21.03 MB]
 [notice] Performed install task: install_bootstrap_full [1.21 sec, 21.03 MB]
 [info] user module installed. [1.58 sec, 27.84 MB]
 [info] path_alias module installed. [1.58 sec, 27.83 MB]
 [info] big_pipe module installed. [1.58 sec, 27.83 MB]
 [info] config module installed. [1.58 sec, 27.83 MB]
 [info] help module installed. [1.58 sec, 27.63 MB]
 [info] page_cache module installed. [1.58 sec, 27.63 MB]
 [info] dynamic_page_cache module installed. [1.58 sec, 27.63 MB]
 [info] automated_cron module installed. [1.58 sec, 27.63 MB]
 [info] announcements_feed module installed. [1.58 sec, 27.63 MB]
 [info] block module installed. [1.58 sec, 27.63 MB]
 [info] filter module installed. [1.59 sec, 28.02 MB]
 [info] views module installed. [1.59 sec, 28.04 MB]
 [info] field module installed. [1.59 sec, 27.85 MB]
 [info] text module installed. [1.59 sec, 27.85 MB]
 [info] block_content module installed. [1.6 sec, 27.9 MB]
 [info] breakpoint module installed. [1.6 sec, 27.9 MB]
 [info] file module installed. [1.6 sec, 27.7 MB]
 [info] editor module installed. [1.6 sec, 27.7 MB]
 [info] ckeditor5 module installed. [1.6 sec, 27.7 MB]
 [info] image module installed. [1.61 sec, 27.92 MB]
 [info] media module installed. [1.94 sec, 34.8 MB]
 [info] language module installed. [1.95 sec, 34.85 MB]
 [info] locale module installed. [1.95 sec, 34.9 MB]
 [info] config_translation module installed. [1.95 sec, 34.89 MB]
 [info] contact module installed. [1.96 sec, 34.63 MB]
 [info] workflows module installed. [1.96 sec, 34.37 MB]
 [info] content_moderation module installed. [1.96 sec, 34.37 MB]
 [info] node module installed. [1.99 sec, 35.17 MB]
 [info] content_translation module installed. [1.99 sec, 35.2 MB]
 [info] contextual module installed. [2 sec, 34.94 MB]
 [info] datetime module installed. [2 sec, 34.94 MB]
 [info] taxonomy module installed. [2 sec, 35.1 MB]
 [info] dblog module installed. [2 sec, 34.84 MB]
 [info] layout_discovery module installed. [2 sec, 34.84 MB]
 [info] field_ui module installed. [2 sec, 34.84 MB]
 [info] history module installed. [2 sec, 34.84 MB]
 [info] layout_builder module installed. [2 sec, 34.84 MB]
 [info] link module installed. [2 sec, 34.84 MB]
 [info] media_library module installed. [2.1 sec, 42.83 MB]
 [info] menu_link_content module installed. [2.1 sec, 42.83 MB]
 [info] menu_ui module installed. [2.29 sec, 50.61 MB]
 [info] path module installed. [2.29 sec, 50.61 MB]
 [info] options module installed. [2.29 sec, 50.61 MB]
 [info] responsive_image module installed. [2.29 sec, 50.61 MB]
 [info] search module installed. [2.29 sec, 50.61 MB]
 [info] toolbar module installed. [2.29 sec, 50.61 MB]
 [info] shortcut module installed. [2.3 sec, 50.56 MB]
 [info] views_ui module installed. [2.3 sec, 50.56 MB]
 [notice] Performed install task: install_profile_modules [2.31 sec, 45.76 MB]
 [info] claro theme installed. [2.33 sec, 44.77 MB]
 [info] umami theme installed. [2.33 sec, 44.82 MB]
 [notice] Performed install task: install_profile_themes [2.34 sec, 45.27 MB]
 [warning] The "field_block:node:page:field_body" block plugin was not found [3.82 sec, 78.62 MB]
 [warning] The "extra_field_block:node:page:links" block plugin was not found [3.82 sec, 78.62 MB]
 [warning] The "extra_field_block:node:page:content_moderation_control" block plugin was not found [3.82 sec, 78.62 MB]
 [warning] The "field_block:node:page:field_body" block plugin was not found [3.82 sec, 78.61 MB]
 [warning] The "extra_field_block:node:page:links" block plugin was not found [3.82 sec, 78.61 MB]
 [warning] The "extra_field_block:node:page:content_moderation_control" block plugin was not found [3.82 sec, 78.61 MB]
 [warning] The "field_block:node:article:field_tags" block plugin was not found [3.85 sec, 77.69 MB]
 [warning] The "field_block:node:article:field_media_image" block plugin was not found [3.85 sec, 77.69 MB]
 [warning] The "field_block:node:article:field_body" block plugin was not found [3.85 sec, 77.69 MB]
 [warning] The "extra_field_block:node:article:links" block plugin was not found [3.85 sec, 77.69 MB]
 [warning] The "extra_field_block:node:article:content_moderation_control" block plugin was not found [3.85 sec, 77.69 MB]
 [warning] The "field_block:node:article:field_tags" block plugin was not found [3.85 sec, 77.69 MB]
 [warning] The "field_block:node:article:field_media_image" block plugin was not found [3.85 sec, 77.69 MB]
 [warning] The "field_block:node:article:field_body" block plugin was not found [3.85 sec, 77.69 MB]
 [warning] The "extra_field_block:node:article:links" block plugin was not found [3.85 sec, 77.69 MB]
 [warning] The "extra_field_block:node:article:content_moderation_control" block plugin was not found [3.85 sec, 77.69 MB]
 [warning] The "field_block:node:recipe:field_tags" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "field_block:node:recipe:field_recipe_category" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "field_block:node:recipe:field_summary" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "field_block:node:recipe:field_media_image" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "field_block:node:recipe:field_difficulty" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "field_block:node:recipe:field_number_of_servings" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "field_block:node:recipe:field_cooking_time" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "field_block:node:recipe:field_preparation_time" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "field_block:node:recipe:field_media_image" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "field_block:node:recipe:field_ingredients" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "field_block:node:recipe:field_recipe_instruction" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "extra_field_block:node:recipe:content_moderation_control" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "field_block:node:recipe:field_tags" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "field_block:node:recipe:field_recipe_category" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "field_block:node:recipe:field_summary" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "field_block:node:recipe:field_media_image" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "field_block:node:recipe:field_difficulty" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "field_block:node:recipe:field_number_of_servings" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "field_block:node:recipe:field_cooking_time" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "field_block:node:recipe:field_preparation_time" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "field_block:node:recipe:field_media_image" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "field_block:node:recipe:field_ingredients" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "field_block:node:recipe:field_recipe_instruction" block plugin was not found [4.11 sec, 82.48 MB]
 [warning] The "extra_field_block:node:recipe:content_moderation_control" block plugin was not found [4.11 sec, 82.48 MB]
 [info] demo_umami_content module installed. [5.53 sec, 104.99 MB]
 [info] demo_umami module installed. [5.6 sec, 100.06 MB]
 [notice] Performed install task: install_install_profile [5.61 sec, 99.12 MB]
 [error]  Import of string "<span class="visually-hidden">Mostrar </span>@title<span class="visually-hidden"> medios </span> <span class="active-tab visually-hidden"> (seleccionados) </ lapso>" was skipped because of disallowed or malformed HTML. [8.5 sec, 99.81 MB]
 [notice] Translations imported: 8854 added, 0 updated, 0 removed. [8.93 sec, 99.88 MB]
 [warning] 1 disallowed HTML string(s) in files: translations://drupal-11.3-dev.es.po. [8.93 sec, 99.88 MB]
 [notice] Performed install task: install_import_translations [8.93 sec, 99.34 MB]
 [info] update module installed. [9.11 sec, 110.46 MB]
 [notice] Performed install task: install_configure_form [9.67 sec, 112.14 MB]
 [notice] The configuration was successfully updated. 203 configuration objects updated. [10.29 sec, 97.37 MB]
 [notice] Performed install task: install_finish_translations [10.29 sec, 97.33 MB]
 [notice] Performed install task: install_finished [10.37 sec, 98.17 MB]
 [success] Installation complete.  User name: admin  User password: 3WU4QBARFJ [10.37 sec, 98.16 MB]

Anecdotally, run time and max memory usage do not seem to be affected too much, even with all module service classes being reflected.

Added the CR https://www.drupal.org/node/3526437.

The long, long bit about altered and decorated services in #33 should probably be in the CR as well, but I'm a little tired of typing for now. I can update the CR after there's agreement on that behavior makes sense.

It is not possible to use a service modifier to alter Hook\ class services that are not registered services

OK, refactored to split the changes in HookCollectorPass to a separate pass for hook discovery registered services that runs after ModifyServiceDefinitionsPass. The change now allows Hook\ class services that are not registered services to be altered by a service modifier in another module. The code changes are not the prettiest, but I think they are OK.

That all being said, changes here are crashing into other in-progress work on OOP hooks in #3506930: Separate hooks from events, #3519561: Introduce ImplementationList objects per hook, to simplify ModuleHandler, and #3526411: [pp-2] Move hook_implementations_map generation to late in service container compilation to make altering hooks classes more DX friendly (among others I'm not aware of?), so I think some sort of priority of what goes in first probably would make things clearer.

It was good to see this working, though, especially the part of making it possible for kernel tests to subscribe to hooks. I think that will be a big DX win once it's in.

Thank you!

I may have to create a meta, but in my mind for now this is the order I think we should work on these:

#3506930: Separate hooks from events which includes #3519561: Introduce ImplementationList objects per hook, to simplify ModuleHandler

#3526411: [pp-2] Move hook_implementations_map generation to late in service container compilation to make altering hooks classes more DX friendly may not be needed after that change and we may not want to do that at all we need to explore it a bit more.

This issue can likely go after the decoupling from events I would think.

This issue can likely go after the decoupling from events I would think.

I think this plan is fine. One thing to note is that this issue, or at least this MR, also fixes a bug (or addresses a feature gap) where currently altering or decorating the autodiscovered Hook\ classes doesn't work quite right. This is because the hook implementation map has the hook class name derived from the PHP file on disk as one of its keys ($this->hookImplementationsMap[$hook][{CLASS NAME}]). The event dispatcher resolves the callable correctly to the altered/decorator class, so calling get_class() on that will make the look up in the map fail (and also cause a PHP warning).

On a cursory glance, I think #3526411: [pp-2] Move hook_implementations_map generation to late in service container compilation to make altering hooks classes more DX friendly also tries to address this same bug, but I haven't looked at #3506930: Separate hooks from events enough to see if it's addressed there separately.

In any case, I think there is some urgency to get this bug addressed, regardless of which issue is done in.

Yes, that bug is concerning, the thing that has me hesitating on both is we're adding an extra compiler pass. Then for this one I'm concerned about performance for two reasons.

1. We need to reflect far more, we had thought that symfony was already reflecting and caching it so we dropped that concern, but we don't have the symfony flag set.
2. We are now rechecking on each hook if it's been changed.

I haven't looked at the event dispatcher issue with an eye to this gap, but I don't think it addresses it.

I need to get a better idea on timing, if this can go into 11.x now that would be good, but I think with 11.2 coming soon we may have to wait a bit, I'm unclear on timing.

@berdir raised a question about performance concerns iterating through all service definitions. I did a quick test and thought it was worth bringing back to conversation here.

So re-visiting my comments #22 and #26:

• core.services.yml and almost all core module services.yml files with service definitions in them have autoconfigure: true as defaults (datetime being at least one exception), so basically all these services are defined as autoconfigured
• In vendor/symfony/dependency-injection/Compiler/RegisterAutoconfigureAttributesPass.php, all autoconfigured services are reflected
• There are other passes that also use reflection, one noteworthy one being the pass that applies to services that are defined as autowired
• Service definitions in contrib or custom modules that aren't autoconfigured or autowired likely will not be reflected at compile time
• For example, I installed a new standard site, then installed ctools, and did observe in the debugger that ContainerBuilder::getReflectionClass() does not get called on any of the ctools services when the container is compiled ctools install

If a large majority of the services are already being reflected a compile time, then all those classes are already loaded into memory and my guess is that iterating through another time and reflecting will not have a large performance impact, and #27 seems to back this up.

There could be a noticeable performance impact on a (probably typical) project with dozens of custom and contrib modules with a large number of service definitions that are not autoconfigured or autowired. All those services classes would be loaded into memory and reflected when they otherwise wouldn't.

Updating postponement and adding three actual issue postponing this.

I think that since #3544715: Add oop support to hooks currently supported by themes is in, this is unblocked now. The MR needs refactoring since it was done on the event-based hook collector.

One thing to confirm is if we need to restrict the attribute to classes outside of the hook namespace.

If not then I think we want to go the opposite way and require it to exist on methods only and do this: #3523124: [pp-1] Drop $method parameter from #[Hook] attributes, allow only on methods

I've started looking at this again and trying to refactor the MR diff into latest 11.x after all the hook collector changes, which are significant.

One thing I noticed is this comment in HookCollectorPass::collectAllHookImplementations()

   * @todo Pass only $container and make protected when ModuleHandler::add() is
   *   removed in Drupal 12.0.0.

Since the call to collectAllHookImplementations() from ModuleHandler::add() was removed in #3528899: ModuleHandler::add() removes implementations from other modules, I think it will help here a lot to change collectAllHookImplementations() to protected, static, with only the $container argument in this issue per that @todo, so I'll likely proceed that way.

I think that's reasonable

So there are complexities between altering services with decorators or module service providers, and the Order* and Remove attribute functionality. These were kinda obscured when the hook collector leveraged the event system, and some things magically worked because the event system resolves callables at run time with the use of service closures.

To simplify things a bit, let's stipulate that all the Hook\ namespace classes are not meant to be altered or decorated in any way, and that Order/Remove hooks are the only to affect the behavior of existing hooks. We'd just document that trying to alter or decorate a Hook class is not supported and may result in unexpected or unpredictable behavior.

This leaves us with hook-implementing services that we're looking to do here. Conceivably people will declare their services that have both Hook and non-Hook methods, and likely there will be people who will want to alter or decorate those services. In which case, what is the expected behavior? As mentioned, with the event system, hook functions would only be invoked on the class that actually resolved from the service ID at runtime. With the new implementation of how hook classes are registered, this won't happen (for decorated services at least) without some significant refactoring.

Anyway, the outstanding questions are:

• What happens with if the replacement service class does not have the Hook method of the original class? Or if it does have the method, but its implementation does not have the attribute?
• How would the Order/Remove hooks work since they reference specific class names?
• Or do we document that any decorated or altered service definition is not expected to work predictably with the hook system?

@godotislate #48: [Proposal to not support service-with-hook decoration.]
The idea to limit scope sounds reasonable. TBH i do not understand the ramifications or alternatives of this (maybe i missed a relevant comment when vgrepping.)

I see two different cases:
a) One foo.service both implentis an interface AND implents a hook (not being part of that interface). While i would NOT recommend this for "clean-ness", it totally can be done. Another bar.service decorates foo.service interface methods, not caring for the hook.
=> My expectation is that the hook still runs in the (now renamed) bar.service.inner (ex foo.service).

b) One foo.service implements (say) hookEntityPreSave, and another component wants to wrap that hook.
=> This has relevant complexities, and it has my +1 to say it's unsupported and defer it to a separate issue "Allow hook implementations to be wrapped".

Some thoughts on b)
- Hux has a dedicated attribute for it.
- My gut feeling is not NOT support decorating the hook-implementing service, as it may contain multiple hook implementations. And it's not a well-defined interface that we're decorating, but the "interface" is a single hook implementation and its signature, NOT all public methods, because Hook service classes tend to have hook implementations added over time, For the same reason, let's stick with the term "wrap a hook implementation", because wrapping a hook implementationis technically not decorating.

@godotislate Is this along your lines of thought?

Re #49, I think we are basically in agreement. Though I would add that module service provider altering service definitions are also an issue here, and after trying some experimentation and consideration, I think

• The Order* parameters and ReorderHook/RemoveHook attributes should be the canonical and only API to manage other hooks
• The behavior of hook implementations in services altered by module service providers or service decorators should be unsupported and just documented as such, especially because trying to align the results of being altered by a service provider versus being altered by a decorator looks to be very complicated

Given this some more thought, and this is a plan I think is implementable:

• Services in the Drupal\Core namespace can have Hook implementations
• If a service definition has its class changed by a decorator or module service provider, all Hook/RemoveHook/ReorderHook attributes in the new class will be ignored completely. The methods in the original class that are Hook implementations will still be invoked with the hook
• Services definitions that are dynamically registered in a module service provider can have Hook implementations
• Any service definition that does not have the class property set will not be part of Hook discovery. This includes services that are created from a factory or a parent without a class. In addition, abstract and synthetic

services are excluded from Hook discovery. Maybe an exception is possible for service definitions that are NULL, such as Drupal\example\Service: ~, but that will need some investigation

• Also to be investigated for performance reasons is whether discovery should be limited to services that have a certain tag. This likely will need an example site with a lot of contrib/custom modules with service definitions that are not autowired or autoconfigured

General implementation plan:

• Update HookCollectorPass to look for Hook attributes in registered services as well, except those that don't have a class. Exclude discovery on definitions that are abstract, synthetic, or have a decorates property
• In HookCollectorPass, track the IDs of currently registered services
• Add a second HookCollectorPass in CoreServiceProvider to run after the ModifyServiceDefinitionsPass
• In the second pass, compare the new complete list of IDs of registered services versus the list of the IDs in the previous run
• Do discovery on only the IDs in the diff between the new and old lists, and recompute the Hook ordering

Providing an example of the issue with altered services for clarity.

We have a service that has two hook methods:

class OriginalService {

  #[Hook('test_a')]
  public function testA()  {
    ...
  }

  #[Hook('test_b')]
  public function testB() {
    ...
  }
...
}

We have another service that removes the OriginalService::testA hook:

class RemoveHookService {

  #[Hook('test_a')]
  #[RemoveHook(
    hook: 'test_a', 
    class: OriginalService::class, 
    method: 'testA',
  )]
  public function testA()  {
    ...
  }
...
}

And another service that reorders OriginalService::testB:

class ReorderHookService {

  #[Hook('test_b')]
  #[ReorderHook(
      hook: 'test_b', 
      class: OriginalService::class, 
      method: 'testB',
      order: new OrderBefore(['something_else']),
  )]
  public function testb()  {
    ...
  }
...
}

Now let's say we have a new class AlteredService that a module service modifier changes the definition of the OriginalService service:

class OtherModuleServiceProvider implements ServiceModifierInterface {
 
  public function alter(ContainerBuilder $container) {
    $definition = $container->getDefinition(OriginalService::class);
    $defintion->setClass(AlteredService::class);
  }

}

And AlteredService looks like this:

class AlteredService {

  #[Hook('test_a')]
  public function testA()  {
    ...
  }

  #[Hook('test_b')]
  public function testB() {
    ...
  }
...
}

Or similarly a new class DecoratedService that a service decorator uses to replace OriginalService:

services:
  Drupal\decorator_example\DecoratedService:
    class: Drupal\decorator_example\DecoratedService
    decorates: OriginalService
    ..

class DecoratedService {

  #[Hook('test_a')]
  public function testA()  {
    ...
  }

  #[Hook('test_b')]
  public function testB() {
    ...
  }
...
}

The complexity is this:
The RemoveHook and ReorderHook in ReorderHookService and ReorderHookService target methods in OriginalService. But if the modules that provide AlteredService or DecoratedService are installed, then OriginalService has been replaced, so what behavior should the RemoveHook and ReorderHook have?

So in order to not have to deal with that complexity, I think the simplest way would be for testA and testB in OriginalService not to be affected by the AlteredService or DecoratedService methods at all. Meaning that even if the service definition has been changed, the hook methods in OriginalService are still invoked, unless a RemoveHook somewhere targets them.

The idea in #51 is that the hook methods in AlteredService and DecoratedService would not be registered as hook implementations at all.

An alternative idea would be that the hooks in OriginalService, and AlteredService or DecoratedService, are all registered. For decorated services, this would actually mean that every hook method in every class in the decorator chain would be registered. I think to implement this, we'd have to do two collector passes, before and after ModifyServiceDefinitionsPass, and loop through all the service definitions both times. (The Hook namespace discovery would still only happen the first time). This way the classes for the service definitions before being modified would be registered, as well as the classes after.

Had a quick convo with @nicxvan about this on Slack, and a couple more notes:

• Doing #51 or #52 and having the OriginalService hooks still work might be a problem, because the callableResolver might not be able to instantiate the OriginalService class correctly at run time. (I was observing something different in testing, but what I was seeing could have a different explanation)
• If we think decorated/altered services should replace/remove the original service methods, Remove/Reorder would likely be ignored if there's no match, but there might need to be refactoring in that Remove/Reorder should target service_id:method instead of class_name:method (except when the class name is the service ID)
• Edited to add: But Remove/Reorder target service IDs, that means that they would work on the class the service ID resolves to. So in the example in #52, they would work against AlteredService or DecoratedService hooks

This is very tricky and I think will need wider consensus.

Honestly we needed to think about what is most expected and least likely to break things.

I would think we'd try to pattern it around how we handle RemoveHook and ordering which is if anything doesn't align we discard the directive.

Honestly I think the outermost level is responsible for calling inner and maintaining parity.

I think the trickier situation is when only one layer has the attribute or a layer is skipped.

I lean towards last being canonical.

If you're decorating or altering is up to you to keep up to date. I think the most predictable would be we should discard inner directives.

I'm 100% open to other thoughts and opinions though.

I see that #3523124: [pp-1] Drop $method parameter from #[Hook] attributes, allow only on methods was postponed for this issue, because we were considering to put attributes on class level for performance.
I think this is no longer relevant, is it?
I think it would feel odd to have different attribute placement rules for classes in \Hook\ and elsewhere.

If performance is a concern, we should somehow tag services that have hooks.
A module could even tag all their services using defaults.
But I think we are already over that, based on the comments I saw.

I think we can resume the discussion in the other issue.

If performance is a concern, we should somehow tag services that have hooks.
A module could even tag all their services using defaults.

I think looping through all the methods in a class and all the registered services (or all classes in a directory) are separate performance issues.

If/when we resolve outstanding architecture questions in the IS, it would make sense to do a performance test on container building with a site that contains a lot of contrib and custom modules whose services are largely not autoconfigured or autowired.

Since autoconfigure is in effect for almost all core services, those classes are already being reflected, so reflecting them again for Hook discovery does not increase memory use and is likely not a significant CPU issue. On a site where a lot of contrib/custom services are not already being reflected for autoconfigure or autowire, then the amount of memory discovery uses loading the classes for reflection comes into play more. And yes, this could be addressed per @berdir's suggestion earlier that registered services with Hooks would need both a service tag and the Hook attributes.

Services in the Drupal\Core namespace can have Hook implementations

What would be the value of $module when these implementations are invoked?

Also in other cases where services have hooks, can we always reliably determine a module name?
E.g. if a module registers a service using a class from a non-Drupal package in /vendor/, could that have hooks? And what would be $module?
(Seems unfair if we don't allow that, but core is allowed to do it :) )

The case of module invokers that use the $module for anything is quite rare, but for now it is still part of the contract, and it can be expected to be a valid module name (e.g. "core" is not).

What would be the value of $module when these implementations are invoked?

core. We already did this in #3502432: Make hook testing with kernel tests very simple.

I think looping through all the methods in a class and all the registered services (or all classes in a directory) are separate performance issues.

I would expect that scanning the methods on an already reflected class will have a very small impact compared to loading the class file, unless we do something expensive like parsing the doc comment of each method - which we don't. Or if we would rely on something like static reflection parser, which does not load the full file and can be faster if we only parse the top.

OK, I think I figured out a way forward that avoids some complexity. It's up for review: MR 15722.

It's getting late, so will summarize briefly here and then update IS/CR later.

The main idea is to make the hook attribute Order API the only way to alter and reorder hooks, and to prevent interaction with altering container services by decorators or module service modifiers/providers.
How this is done:

• When hook services are registered, the hook tag is added to them
• If the class which contains hooks is an already registered service, another service definition is registered with the id of "hook.{class name}". This is done to allow decorators and service providers to alter a registered service without changing the actual hook implementations
• In HookCollectorKeyValueWritePass, before saving the maps to keyvalue, the services tagged hook are looped through to make sure that the class associated with the service has not changed. This prevents service modifiers or decoration from changing the actual hook services
• In discovery, services that decorate other services, services that are deprecated, or services provided dynamically by a service provider are excluded
• Hook ordering and removing through the order property or ReOrder/Remove hooks still works the same with the identifiers being class names and not service IDs. If you want to target hook implementation on a registered service, use the class name instead of the service ID

Updated the IS and CR.

Had a thought that maybe it'd be fine to allow service decorators to be included in hook discovery. My thought about excluding them was to make service modifiers and service decorators have parallel functionality. But since decorators and modifiers don't work identically anyway, it could make sense to include decorators, if for no other reason than for somehow to define a service decorator that has a RemoveHook method that targets the hook implementation in the original service class.

The idea would be that hook implementations in every decorating class and the original service would all have their hooks run, and that Order/Remove/etc would still target the actual class they are in.

The protection that validates that "hook"-tagged services are not altered or decorated would remain in place.

Bumped the CR version to 11.5, since this seems unlikely to get in for 11.4. I don't see any deprecations in the MR, so we should be good there.

@rodrigoaguilera said in #3

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?

I opened an issue a few months ago for a similar need with hook_theme #3558998: Allow multiple hook_theme implementations per module

Ok I took a first pass review, I'm glad I'm seeing the less complex version :D.

I have some questions, I am concerned about the complexity, I don't fully get the bit in the later compiler pass.

I have not reviewed the tests fully, but I will need to.

OK, I opened draft MR 15981 to demonstrate what the current state of replacing a Hook class via a service modifier alter() method or decoration is. For the following examples, replacing the class via altering or decorating ends up being functionally the same.

• Assuming module_a has class Drupal\module_a\Hook\ATestHooks with method testHook that implements hook test_hook, then if module_b replaces that class with Drupal\module_b\BTestHooks in the hook service definition, and BTestHooks has or inherits testHook, then BTestHooks::testHook will run when test_hook is invoked, and ATestHooks::testHook does not run
• If BTestHooks::testHook does not exist, there will be an invalid argument exception when test_hook is invoked
• If module_c has class Drupal\module_c\Hook\CTestHooks with a method testCHooks that implements test_hook that looks like:
  

     #[Hook('test_hook', order: new OrderBefore(classesAndMethods: [[ATestHooks::class, 'testHook']]))]
    public function testCHooks(array &$variables): void {
    }
  

  Then testCHooks will run before BTestHooks::testHook. ATestHooks::testHook still does not run. The classesAndMethodsparameter is slightly misleading; it actually targets service IDs because of how the callable resolver works. It's just that in the class of Hook\ classes, the service ID is the FQCN.

• ReorderHook and RemoveHook work similarly. If module_c has Hook class methods that target ATestHooks::testHook, then BTestHooks::testHook is actually the method is that ordered against or removed.

(Note that for decorating, the above applies only to the outermost decorator. Inner decorators are ignored.)

I think this is confusing and can lead to errors. What is proposed in MR 15722 is that decorating or altering Hook\ class service definitions is prohibited. This means that hook will always run in the original Hook class. While this is a breaking change for any projects that were replacing Hook services, I'm not sure what BC we're obligated here to, because this pattern was not established as part of API. Perhaps we could do some kind of deprecation here, in which case we should define what that deprecation behavior looks like.

In addition, as the MR expands hook discovery to any registered service, this means that "classes" still can be targeted, without needing to change the "classesAndMethods" or "class" parameter names, because the collector will register a second ID (of the form "hook.{FQCN}" for the service. It's also prohibited to decorate or alter the hook. classes, which is not a break, since these are new.

I made changes to the MR and CR so that decorating or altering Hook\ services is deprecated.

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

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

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

Rebased after merge conflict in HookCollectorPass from #3592577: Ensure that hook attributes are never parsed from a stale opcache.