[META] Hooks via attributes on service methods (hux style)

Created on 11 June 2023, about 1 year ago
Updated 19 April 2024, about 2 months ago

Problem/Motivation

Currently in Drupal we have the old procedural hook system, we have symfony events, and we have tagged services and service collectors, to allow different modules or components broadcast or respond to "events" (in the broader sense).

The procedural hook system has a number of problems and limitations (some already mentioned in #2237831):

  • Possibility of name clashes / ambiguity, e.g. "field" + "_group_permission" vs "field_group" + "_permission".
  • No dependency injection: Hook implementations have to call \Drupal::service(...) to get their dependencies.
  • Most implementations are in *.module files, which can get crowded, make it hard to organize code, and can easily cause git merge conflicts, if multiple branches attempt to add new hook implementations.
  • There can be only one implementation per module (or the module has to "cheat", and pretend it is implementing on behalf of another module, one that might not even exist).
  • The procedural code feels out of place compared to the rest of Drupal.

Alternatives:

  • symfony events require a dedicated event class per event type instead of named parameters, which "creates additional boilerplate when defining events and additional levels of indirection when subscribing to events." (quote from #2237831). See ๐Ÿ“Œ Add events for matching entity hooks Needs work
  • tagged services and services collectors typically require a dedicated interface per "event type", and subscribers (= tagged services) to implement that interface.

None of these "alternatives" is a good candidate to fully and generically convert the existing hook system. All past attempts to do so (that I am aware of) only focused on very specific hooks, or families of hooks.

Proposed resolution

The Hux module allows to mark methods on services classes as hook implementation, using php attributes.

Something similar should be added in Drupal core, so that existing hook implementations can be converted to OOP code.

This seems to be the current consensus in ๐ŸŒฑ Allow module services to specify hooks Needs work .

class MyService {
  #[Hook('node_insert'), Hook('node_update')]
  public function nodeWrite(NodeInterface $node): void {..}
  #[Alter('form_node_edit_form')]
  public function nodeFormAlter(array $form, FormStateInterface $form_state): void {..}
}

Goals / requirements

Backwards support:

  • The first iteration of this should be fully compatible with existing procedural hook implementations and also on the calling code side.
  • There will be a mix of procedural and non-procedural implementations.
  • The old hook_module_implements_alter() will still work on the list of procedural implementations, but a new hook (or event?) could be added to reorder or alter the collection that includes OOP implementations.
  • The behavior and return value of any call to $module_handler->invoke() or $module_handler->invokeAll() etc...
    • ...must remain unchanged, if no new implementations were added.
    • ...must remain unchanged, if existing implementations are merely converted to the new system.
    • ...must only gradually change, if new implementations are added using the new system.
  • Modules can start implementing existing hooks using the new system, without any change in the module that publishes or invokes the hook.

Basic new features:

  • Methods on service classes can subscribe to hooks using attributes like #[Hook('node_update')].
  • One method can implement more than one hook.
  • Multiple methods in the same module can implement the same hook, resulting in multiple implementations per module.
    This introduces some challenges with return value of single $module_handler->invoke(..), see below.
  • There must be a mechanism to order different implementations. E.g. in hux there is a system of weights / priorities, but in core we might do something different.
  • There must be a mechanism to replace or remove existing implementations.
  • For the two requirements above, it might be necessary to have unique machine names for individual implementations.

Optional or advanced features:

  • There can be a discovery mechanism to register classes with hooks as services. E.g. in hux, this happens if these classes are in a Drupal\\$modulename\\Hooks namespace.
  • Modules can add additional attribute classes, to let the method implement one or more hooks. E.g. #[HookEntityOp(['node', 'taxonomy_term'], ['update', 'insert'])] would implement 4 different hooks.
  • Modules can add other callbacks as hook implementations, that don't rely on php attributes, and that can be added dynamically.
  • We might add additional methods to ModuleHandler, with more control of the results aggregation for single ->invoke(). See "Challenges" below.
  • We might add hooks (or anoother mechanism) to control the default results aggregation per hook name.

Challenges

  • Return value of single $module_handler->invoke():
    • Currently, it returns the return value of the one single implementation. The behavior for this case must not change!
    • With multiple implementations that return arrays, e.g. for hook_schema(), we would expect the results to be merged. Any null values would be skipped, if the "collected data" is already an array.
    • For more than one non-array non-null values, or a mix of array and non-array values, an exception could be thrown, because these cannot be merged, unless an aggregation method is specified somewhere.
  • Design a system to define order of implementations, and to remove or replace implementations. Options:
    • Before/after rules, as proposed for events in ๐Ÿ“Œ Add before/after ordering to events Needs work
    • Numeric weights/priorities, as currently in hux.
    • A hook or similar to alter the list of implementations, similar to hook_module_implements_alter(), but with support for callbacks.
    • Extending hook_module_implements_alter() in a BC-friendly way. This could be tricky.

Underlying architecture

The module handler ->invoke(), ->invokeAll() etc should operate with a list of callbacks, that can come from different sources.
The methods with attributes will be one such source, but it should be flexible enough to support other sources.

Proposed roadmap / target versions

  • Study and play with the hux module as a proof of concept.
  • Build the new system in latest version of Drupal 10 (10.2.x). Do not change the existing implementations and calling code, yet.
    This allows contrib to start using the new system, while minimizing disruption from upgrading to the new core version.
    Any possible BC conflict will only apply if there is a module that uses the new system.
  • Start converting existing implementations in a future version - 10.? or 11 - depends how disruptive this turns out to be.
  • Add additional changes that are not backwards compatible, consider to deprecate procedural hooks, in Drupal 11.

Remaining tasks

User interface changes

API changes

Methods in service classes can be marked as hook implementations using attributes.
Details are provided above.

Data model changes

Caches for the new kinds of implementations and discovery data.

Release notes snippet

๐ŸŒฑ Plan
Status

Active

Version

11.0 ๐Ÿ”ฅ

Component
Baseย  โ†’

Last updated about 5 hours ago

Created by

๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @donquixote
  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    Adding a "proposed roadmap".

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    For the two requirements above, it might be necessary to have unique machine names for individual implementations.

    For hook_module_implements_alter() we just use the module name, could the unique machine name be the fully qualified method name?

    General note that 11.x isn't open yet, see https://www.drupal.org/about/core/blog/new-drupal-core-branching-scheme-... โ†’ for how we're using that branch. So API additions can just target 10.x minor releases. I think we should deprecate procedural hooks quite far ahead, so removal in Drupal 12 seems reasonable at the moment, whether we deprecate in Drupal 10 or 11 probably depends on how far we get here.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    For hook_module_implements_alter() we just use the module name, could the unique machine name be the fully qualified method name?

    I would like to allow refactoring the classes, methods and namespaces in a module without breaking other modules that alter the hook implementations or declare dependencies of one implementation to another.
    This is why I don't like using class names and method names for this.

    This is different for service ids with autowire, here I totally support using the class name or (better) interface name as service id or alias.
    I would consider these service interfaces as part of the public API, whereas which class and which method implements a hook would be an implementation detail.

    This is based on my experience working on a module where I wanted to move and rename the hook implementations multiple times.

    On the other hand, this would give us another string parameter which would feel quite useless most of the time.
    So, not fully decided.

    fully qualified

    Btw "fully qualified" generally means including the leading namespace separator, so \Drupal\sytem\A\B::foo, whereas "qualified" means Drupal\system\A\B::foo(). See https://www.php.net/manual/en/language.namespaces.rules.php. I think the more generic option would be just the qualified name.

    Or we could omit the module namespace so it would become A\B::foo.

    The method name will usually be something that reflects the hook name, like "userUpdate". And usually the same class will not have more than one implementation for the same hook.

    So we could use the class name or the service id as a default, and an option to specify a custom id.
    I like class name because it can be written as ClassAlias::class in code.

    If I contradicted myself in this response, it is totally intentional, and reflects my undecidedness :)

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    (Merged "target version" section into "roadmap" section, and removed reference to 11.x for the current phase.)

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    I did have a look at this, and found some complications.

    To achieve full BC, we need to support:

    • Modules that implement hook_module_implements_alter().
    • Modules that extend the existing ModuleHandler class, and access the protected methods and properties.

    I think for all of this to work we need the new mechanism to be fully opt-in. Otherwise the ModuleHandler class will become a monster.

    The idea would be to have a new optional module in core that replaces the current module_handler service.
    Without this module enabled, you get full BC, but you don't get the new advanced hook system.
    With this module enabled, you get only limited BC regarding the points above, but you get the new advanced hook system.

    To make this easier, we can split out traits like HookDispatcherTrait and ModuleLoaderTrait out of ModuleHandler, and parent interfaces like HookDispatcherInterface and ModuleLoaderInterface out of ModuleHandlerInterface. This will make it easier to compose a new ModuleHandler class while using large chunks of the existing class, while at the same time having the old ModuleHandler class maintain full BC support for inheritors.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    Can we flip that approach? Existing sites get a module that installs on update hook and retains BC, new sites don't get it? Then we can easily jettison the BC layer by ditching that module.

  • ๐Ÿ‡ซ๐Ÿ‡ทFrance andypost

    Nice idea) if it possible

  • ๐Ÿ‡ท๐Ÿ‡บRussia Chi

    This probably needs establishing a new way of documenting hooks instead of module.api.php.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    Can we flip that approach? Existing sites get a module that installs on update hook and retains BC, new sites don't get it? Then we can easily jettison the BC layer by ditching that module.

    The idea with the new module was that other contrib and custom modules can add it as a dependency, if they use the new system.
    If we "flip that approach", these modules instead need to declare an incompatibility / conflict. I don't think this is even possible.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    Can we flip that approach? Existing sites get a module that installs on update hook and retains BC, new sites don't get it? Then we can easily jettison the BC layer by ditching that module.

    Also, no matter which way we do it, the old ModuleHandler class has to remain in its original namespace in Drupal\\Core\\Extension\\ModuleHandler.

    A new ModuleHandler class will have a new class name.

    This also impacts modules that use ModuleHandler instead of ModuleHandlerInterface as a parameter type.

    It would happen like this:

    • Contrib module "token" releases a new major version which requires the new core module advanced_hooks.
    • A website project is upgraded to the new version of token. They need to enable the new core module.
      This makes me wonder: How do we currently check new dependencies that come with a new version of a module?
    • The developers notice conflicts with some other contrib and custom modules that rely on the old ModuleHandler class.
      They look for new versions or patches to fix these problems.
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    Modules that implement hook_module_implements_alter().

    We need to support hook_module_implements_alter() in the sense that it should still run, but we don't need to support it getting the same data- i.e. if a hook implementation changes name, we can expect modules to change their implementation in a minor release. Core and contrib modules can add or remove hook implementations all the time anyway.

    Modules that extend the existing ModuleHandler class, and access the protected methods and properties.

    Protected methods and properties are considered @internal unless they're on a base class, which ModuleHandler isn't, so I don't think we need to provide bc here. If we find contrib modules that are extending ModuleHandler, we can open issues to warn them about changes and/or implement bc on a best effort basis where it's straightforward to do so. See https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... โ†’

    In both cases we don't need 'full bc', we just need the amount of backwards compatibility that we say we'll provide in the bc policy. Sometimes we might want to provide more than that, but if we do that's extra, not required. We're also going to need to convert core implementations (and invocations if there's a change) to the new system.

    I think we should try to use ModuleHandler in-place unless there's a very good reason not to, and the two above for me aren't good enough reasons.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    Protected methods and properties are considered @internal unless they're on a base class, which ModuleHandler isn't, so I don't think we need to provide bc here.

    This is good to hear :)
    I have not been following core development as much in the last years so I did not feel confident to make this choice. But if this is how we apply our BC policy, I am all for it.
    In that case I would want to split ModuleHandler into separate services internally, while still providing a class implementing the interface as a wrapper. But we can see about that.

    We need to support hook_module_implements_alter() in the sense that it should still run, but we don't need to support it getting the same data- i.e. if a hook implementation changes name, we can expect modules to change their implementation in a minor release. Core and contrib modules can add or remove hook implementations all the time anyway.

    This raises the question which data we should send to hook_module_implements_alter().
    Also, whether we allow it to reshuffle the new implementations, or just the old procedural ones, and we insert the new implementations later. Or we use a trick with 'early' and 'late' placeholders - see below.

    Currently, every key in the array sent to hook_module_implements_alter() is a module name.
    When dealing with alter hooks with multiple hook names, the keys sent to hook_module_implements_alter() are still module names, but each key may represent more than one implementation.

    The values are either 'false', or strings representing module file names ("group"), e.g. 'hello' for "$module.hello.inc".
    When dealing with alter hooks with multiple hook names, the values sent to hook_module_implements_alter() are always false, even for those implementations where the original array has a string.

    I had a look at existing implementations of hook_module_implements_alter() in a relatively big project, and this was my conclusion:

    • Most of them want their own implementation to run first, or to run last.
    • Some of them want to remove implementations of other modules.
    • I found at least one that wants to insert their implementation after that of another module, or multiple other modules. That one I found was ckeditor5_module_implements_alter(), which is actually in core.
    • I remember that in the past (Drupal 7 time) I used hook_module_implements_alter() to change the 'group' of hook implementations, to split up a *.module file that was growing too big. I never saw this done in contrib modules though. And later I just changed the strategy to simply have a list of `require_once` at the top of the *.module file, instead of messing with the hook info.
      I don't know if we want to continue to support this.

    I think we want to support all or most of these, but we also want to provide alternatives to achieve these tasks in a more elegant way, and in a way that allows different position for different hooks within the same module.

    I could imagine the following solution:

    • By default, every implementation from a module is grouped under the module name. The data sent to hook_module_implements_alter() only contains one entry that acts as a placeholder for all these implementations. These implementations don't even need a dedicated id, they can just be numbered automatically internally if needed.
    • Implementations can be marked to run "late" or "early", e.g. using an attribute or a property inside the attribute. For these, we could insert placeholder keys `.early` and `.late` in the data sent to hook_module_implements_alter(). Or we could just leave these out and prepend or append them after the hook_module_implements_alter() has run.
    • Implementations can have custom ids, but they don't need to. If they do, that id is used as key for hook_module_implements_alter(). But not sure about it.
    • For implementations with a custom id, we still need to keep the module name somewhere, because it needs to be passed to the callback sent to invokeAllWith(), and it is needed for single invoke().
    • Implementations can be on behalf of another module.
    • Perhaps we want to allow some implementations without a module. These would be skipped in invoke(), and a NULL or the id would be passed to the callback from invokeAllWith().
    • The array values sent to hook_module_implements_alter() will be `false`, to keep it simple. Or perhaps for those that already have a 'group' from hook_hook_info(), we can send the group, as we do now.
    • The actual callback data (service id and method name) is stored elsewhere, and combined later with the data that was altered in hook_module_implements_alter().
    • If after hook_module_implements_alter() has run, any entry in the list contains a string instead of false, we add this as a file to be included for that module.
      In fact, for every hook we keep a list of include files separate from the implementations, So before the first invokeAll(), we first include all the files.
    • Implementations can be marked as running before or after a specific other implementation, identified by id (if it has one) or by module name (then it targets all implementations from that module).
      This order could be applied before we call hook_module_implements_alter(), or after.

    We need multiple layers of cacheable or discoverable data:

    • Raw unordered list of implementations from services for all hooks. These are discovered in one go, not separately for each hook.
    • Raw unordered list of procedural implementations for a specific hook. These are discovered separately per hook.
    • (persistent cache?) Raw unordered list for a specific hook combined from different sources/types (services, procedural).
      The structure of these is suitable for sending to hook_module_implements_alter().
    • (persistent cache) Altered, sorted and serializable list for a specific hook.
      The structure of these is optimized for easy conversion into closures.
    • (persistent cache?) Altered, sorted and serializable list for a combination of hooks as needed e.g. for alter(['form', 'form_FORM_ID', ...]).
    • (runtime cache) List of callables/closures for a specific hook, with service ids resolved as objects and all module files loaded.
    • (runtime cache) List of callables/closures for a specific hook and specific module, for single invoke(), with service ids resolved as objects.
      We could simply reuse the previous one, but perhaps for single invoke() we don't want to load and prepare implementations from other modules.

    I already have something like this locally, but so far I was not sure how to handle alter() with multiple hooks, and single invoke().

  • ๐Ÿ‡ซ๐Ÿ‡ทFrance andypost
  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    I think we can completely avoid the custom ids per implementation.

    Implementations would be grouped and ordered by weight first.
    All the legacy/procedural implementations have weight = 0. Service method implementations can have negative, positive or zero weight.

    Within the same weight group, implementations are ordered by module keys and before/after indicator.
    For weight = 0, entries can further be ordered by hook_module_implements_alter().

    More details:

    Every implementation belongs to a module, which is used to filter implementations for single invoke(), and is passed as argument to the invokeAllWith() callback.
    For legacy/procedural implementations, these modules are the same, they are simply the function prefix.
    For service method implementations, by default the module is determined from the namespace.
    A different module can be specified in the attribute.

    By default, the order is determined by the module's position in the global module list.

    In addition, one or more 'before' keys or one or more 'after' keys can be specified. The keys have to be module names.
    By doing this, the implementation will be inserted before or after the position that an implementation for the target module would have.

    Finally, hook_module_implements_alter() is invoked on the keys of the weight = 0 group.
    This will typically only move the implementations for a specific module, not the ones that are inserted in between due to their 'before' or 'after' keys. All the legacy implementations only target known module names.
    This can leave an order where the 'before' and 'after' conditions are no longer true - which is fine.

    Neither the hook_module_implements_alter() nor the 'before' or 'after' keys can override the weights.

    -----

    In a future version, we might drop procedural implementations and the hook_module_implements_alter() hook.
    The weights and the 'before' / 'after' keys can already achieve everything we currently do with the alter hook.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia kim.pepper ๐Ÿ„โ€โ™‚๏ธ๐Ÿ‡ฆ๐Ÿ‡บSydney, Australia

    On the topic of deferring to module weights, there was a fair amount of discussion on that in ๐Ÿ“Œ Deprecate module_set_weight() Needs work

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    On the topic of deferring to module weights, there was a fair amount of discussion on that in #2968232: Deprecate module_set_weight()

    Thanks for the link!

    The module order would simply be the default ordering within a weight group.
    This same order I would assume is present in anything that is being discovered in Drupal - e.g. plugins.
    I think without module_set_weight(), the order is simply alphabetic by machine name - which is totally fine.
    We need a well-defined order to avoid randomness and make things reproducible.

    With the weights and the before/after setting, there will be no need to change the global weight of a module.
    Even today, hook_module_implements_alter() usually can do all we need, so that module_set_weight() is already unnecessary.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia kim.pepper ๐Ÿ„โ€โ™‚๏ธ๐Ÿ‡ฆ๐Ÿ‡บSydney, Australia

    The conclusion was that module weights were originally used to ensure load order, but we should be ordering by module dependencies (then alphabetical?)

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    The conclusion was that module weights were originally used to ensure load order, but we should be ordering by module dependencies (then alphabetical?)

    We could do this, independent of the change proposed here.
    It would still result in a canonical order of modules.

    I am a bit skeptical because there is a cost in BC, the benefit is limited, and the system would be overall less predictable.
    It is not even fully clear whether a dependent module's hook implementation must always run before or always after that of its dependee module. Maybe after for alter hooks, but before for non-alter hooks?
    And there were some arguments about ambiguity with e.g. conditional dependencies.

    This said, if we were to design this from scratch, with no regard for BC, I might be more favorable towards the module dependency order.
    And, we can still do it, it does not really conflict with the proposal discussed here.

    ---

    The main problem of competing hook_module_implements_alter() implementations can be resolved with the new weight system (in Hux module it is called priority, but it is currently not available in the Alter attribute):

    class C {
      // Make this run after other implementations.
      #[Alter('module_implements', weight: 999)]
      function moduleImplementsAlter(array $implementations, string $hook) {..}
    

    or using before/after:

    class C {
      // Make this run after other implementations.
      #[Alter('module_implements', after: 'other_module')]
      function moduleImplementsAlter(array $implementations, string $hook) {..}
    

    but actually, in most cases, it won't be needed at all, because there will be more powerful ways to order the target hook directly.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    > This probably needs establishing a new way of documenting hooks instead of module.api.php.

    Agreed. And this is an opportunity to move beyond the api.php files and FINALLY document more than just hooks.

  • Agreed. And this is an opportunity to move beyond the api.php files and FINALLY document more than just hooks.

    Should moving beyond api.php and thinking of a new means of documentation be a separate child ticket?

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    ๐Ÿ“Œ Hux-style hooks, proof of concept Needs work
    Green! But WIP.

    So far, there is no attributes discovery yet. All I've done so far is prepare a structure where we can then add the service method implementations.
    Green just means that I did not destroy the old hooks :)

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    Let's improve test coverage before we do this big refactoring:
    ๐Ÿ“Œ Improve unit test coverage for ModuleHandler RTBC

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    Hook attributes design

    Similar discussions are already in the hux issue queue, see โœจ Dynamic hook ID specific attributes Active and โœจ Advanced attributes to declare multiple hooks Closed: won't fix .

    Basic hook attributes

    I think these are quite uncontroversial in their basic usage.

      #[Hook('cron')]
      public function ...
    
      #[Alter('entity_type')]
      public function ...
    

    Specialized hook attributes

    We can make special attribute classes with their own parameters for specific hook types.
    E.g. FormAlter() or perhaps some entity hooks.

      #[FormAlter]
      public function formAlter(..)
     
      #[FormAlter('field_ui_display_overview_form')]
      public function fieldDisplayOverviewFormAlter(..)
    

    Hook name patterns?

    This could be used to abbreviate lists of hook attributes.
    No full regex is supported, only some basic features that allow to unfold it to a list of hook names.

      #[Hook('(node|comment)_(insert|update)']
      public function ...
    

    Meta information like weight, before/after, module.

    A hook implementation can:
    - have a weight other than zero, to make it happen earlier or later than hooks with weight 0.
    - have a before or after list, to make it happen before or after specific other modules. (I am explaining how I imagine the exact rules in one of my earlier posts).
    - be implemented on behalf of another module.

    One option is to squeeze all this info into the same attributes, using named parameters:

    #[Hook('node_insert', weight: 99)]
    #[Hook('node_update', module: 'views')]
    #[Alter('views_data', before: 'views')]
    

    This gets a bit annoying if we create a lot of specialized hook classes, and all of them get the full list of meta parameters (weight, module, before, after).

    Another option would be to have separate attributes for these things, but I don't really like it:

    #[Hook('node_insert'), Weight(99)]
    #[Hook('node_update'), ForModule('views')]
    #[Alter('views_data', Before('views')]
    

    Perhaps it is fine if the meta parameters are only in the basic hook attribute classes, so if you need to set a weight or before/after, you simply use the basic attribute class instead of specialized attributes.

    Dynamic binding

    I noticed hook implementations in core where all they do is delegate to a service method, but that service should really not be dealing with this specific hook.

    /**
     * Implements hook_entity_view_mode_presave().
     */
    function field_ui_entity_view_mode_presave(EntityViewModeInterface $view_mode) {
      \Drupal::service('router.builder')->setRebuildNeeded();
    }
    

    It would be wrong to put a Hook('entity_view_mode_presave') on \Drupal\Core\Routing\RouteBuilder::setRebuildNeeded(), because the route builder should really not know about entity view modes.

    Instead, we would create a dedicated class for this hook.

    class FieldUiRouteHooks {
    
      public function __construct(
        private readonly RouteBuilderInterface $routeBuilder,
      ) {}
    
      /**
       * Implements different hooks.
       */
      #[Hook('entity_view_mode_presave')]
      #[Hook('entity_form_mode_presave')]
      #[Hook('entity_view_mode_delete')]
      #[Hook('entity_form_mode_delete')]
      public function setRouteRebuildNeeded(): void {
        $this->routeBuilder->setRebuildNeeded();
      }
    
    }
    

    But could there be a better way to do this?
    Maybe we could register the RouteBuilder->setRebuildNeeded() as a hook implementation, but without using attributes.

    There are a number of places that load a service just to call a cache clear/reset method. I think this is not the ideal way to do it.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    Changing issue title to more easily distinguish it in lists.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    > Should moving beyond api.php and thinking of a new means of documentation be a separate child ticket?

    Possibly, as it will need input from the API module team and will be take some time to figure out. It needs to block this issue though.

    Given we have the opportunity to be more explicit with dynamic hooks, I'd suggest something like this:

      #[FormAlter(['form' => 'field_ui_display_overview_form')]
      public function fieldDisplayOverviewFormAlter(..)
    

    and

      #[Hook('entity_type' => ['node', 'comment'], 'operation' => ['insert', 'update'])
      public function ...
    
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    Maybe we could register the RouteBuilder->setRebuildNeeded() as a hook implementation, but without using attributes.

    Is this a use case for.... another YAML file?

    What's the benefit of:

     #[FormAlter('field_ui_display_overview_form')]
    

    vs.

    #[Hook('form_field_ui_display_overview_form_alter')]
    
  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    @catch

    Is this a use case for.... another YAML file?

    Maybe.
    Or maybe I am sabotaging this issue by adding distractions :)

    What's the benefit of:
    #[FormAlter('field_ui_display_overview_form')]
    vs.
    #[Hook('form_field_ui_display_overview_form_alter')]

    If you ask me: Mostly syntax candy.
    Also it means that the string literals will refer to actual "things" in Drupal, instead of composite hook names.
    This also means we can use constants like this:

    #[FormAlter(MyForm::FORM_ID)]
    #[EntityUpdate(Node::ENTITY_TYPE_ID)]

    In an IDE you can ctrl-click the attribute name, and get more information about the hook.
    And there could be validation about the ids - although it would be difficult in the attribute class, which has no access to the container.

    I don't have a clear opinion right now if this is worth the extra code, or not.

    You could also ask @dpi, he might have other reasons.

    For implementation, there are two directions we can take:

    • An interface for hook attributes, with methods like ->getHookNames(), ->getModule() etc.
    • A mechanism to register callbacks or handlers for different attribute classes.
      This is what symfony does for service tag attributes, look for "->registerAttributeForAutoconfiguration()" in https://symfony.com/doc/current/service_container/tags.html#adding-addit...
      It adds more flexibility while keeping the attribute classes simple. Also, the handler or callback can have access to services, whereas an attribute class should generally be simple and pure.
      The only thing I don't like here is that we have to craft a signature for this callback, which could become quite crowded to cover all possibilities.
  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    @joachim @solideogloria

    Should moving beyond api.php and thinking of a new means of documentation be a separate child ticket?

    Possibly, as it will need input from the API module team and will be take some time to figure out. It needs to block this issue though.

    I have nothing against more or different types of documentation.
    But I don't see how this is blocking for this issue.
    I also don't get what kind of documentation you are proposing.

    So far the attributes mechanism that is being proposed here does not change the basic anatomy of a hook name and signature.
    So the *.api.php files would still provide the relevant information. The only problem is they will look out of place, once we move a lot of implementations to service methods.

    In addition, any attribute class we introduce can have its own extensive phpdoc.

    Some projects have a /docs/ directory with markdown files.
    I am not opposed to that, but we would need to consider how this duplicates documentation on drupal.org.

    Given we have the opportunity to be more explicit with dynamic hooks, I'd suggest something like this:

      #[FormAlter(['form' => 'field_ui_display_overview_form')]
      public function fieldDisplayOverviewFormAlter(..)
    

    and

      #[Hook('entity_type' => ['node', 'comment'], 'operation' => ['insert', 'update'])
      public function ...
    

    I am sorry if I don't get it:
    Are you still talking about documentation here, or about attributes design and usage?

    If it is about attributes design, my feedback would be:

    • We don't need an array parameter with a dedicated hook attribute class. We can use named arguments, or just a regular argument, like #[FormAlter('field_ui_display_overview_form')]. Especially if there is only one main parameter (other parameters like weight etc would be optional and rarely used).
    • With the second snippet it is not really clear which hook we are refering to, because the attribute name is just "Hook". So it is not really clear how the array keys 'entity_type' and 'operation' should be interpreted.
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    If you ask me: Mostly syntax candy.
    Also it means that the string literals will refer to actual "things" in Drupal, instead of composite hook names.

    If you're using#[Hook('form_field_ui_display_overview_form_alter')] and then someone later adds #[FormAlter(MyForm::FORM_ID)], then does #Hook still work?

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    If you're using#[Hook('form_field_ui_display_overview_form_alter')] and then someone later adds #[FormAlter(MyForm::FORM_ID)], then does #Hook still work?

    The same method can be registered for more than one hook, and it can also be registered more than one time for the same hook.

    If you repeat the same attribute with the same hook name, or if you use multiple different attribute classes that all give the same hook name(s), it just means the method will be called multiple times.

    At least this is how it works in hux right now, and I think it makes sense.

  • First commit to issue fork.
  • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand quietone New Zealand

    @sonam_sharma, credit has been removed per How is credit granted for Drupal core issues โ†’

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States volkswagenchick San Francisco Bay Area

    @sonam_sharma I see you have opened a merge request on an issue that is a discussion and not code related.

    The amount of issues you and others from your organization have posted in the last few days leads other contributors to think you may be attempting to gamify the Drupal attribution system.

    When you open a merge request or post a file it ticks the credit box for attribution and creates more work for maintainers when they close the issues. Please refrain from opening a merge request unless you are working on code.

    I am going to go through some of the issues you are working on and give you individual examples on issue queue etiquette and how you can better improve your contributor experience working within the community.

    Here is a list of resources to help you on your contribution journey, thanks!
    Issue Queue Etiquette โ†’
    Contributor tasks โ†’
    Adding tags to issues and using tags โ†’
    How is credit granted for Drupal core issues โ†’

    @quietone has unticked the attribution box, as you did not contribute to the conversation, let me know if you have any questions.

    ~AmyJune, Core Mentor and CWG Community Health team member

  • ๐Ÿ‡ท๐Ÿ‡ดRomania claudiu.cristea Arad ๐Ÿ‡ท๐Ÿ‡ด

    Nice. However, I think something is missing from the IS: with events itโ€™s possible to stop propagation and this feature seems to be missed from the hook and hux listeners. Combined with a listeners ordering feature, the ability to stop downstream listers to act seems a quite powerful tool to me

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    Nice. However, I think something is missing from the IS: with events itโ€™s possible to stop propagation and this feature is missing from the hook and hux listeners. Combined with a listeners ordering feature, the ability to stop downstream listers to act is quite a powerful tool

    We could replicate this with a special invoke method, as alternative to the usual ->invokeAll() or ->invokeAllWith(), that would stop propagation based on a return value or something with a special parameter that could act very similar to an event object.

    I think for most existing hooks this is not really needed.

    For specific new hooks we can add a mechanism like this, but then we can also wonder if we should instead go with events directly.
    This is something we will have to figure out over time.

    Can you think of a hook that exists currently, or a hook you imagine you want to introduce, that would benefit from this feature?

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    Just want to weigh in here as it seems like things are getting quite complicated.

    Our project was one of the (if not THE) first projects to use Hux in production thanks to @dpi being heavily involved in it. We first introduced Hux to the project back in April 2022. Since then, all of my projects use Hux.

    I think it's wise that we keep the first iteration of hux-in-core simple. Some features in this list I've never used before, and I'm not sure whether they belong in core.

    Multiple methods in the same module can implement the same hook, resulting in multiple implementations per module.

    I've never needed this, although I know it is available in Hux. Would it be simpler if we excluded it? Merging the result of multiple hooks seems potentially dangerous.

    There can be a discovery mechanism to register classes with hooks as services

    +1 to keep using the namespace discovery.

    Modules can add additional attribute classes, to let the method implement one or more hooks

    There's other ways to do this already in Hux, do we need another layer?

    Design a system to define order of implementations, and to remove or replace implementations

    I think the priority/weight system in Hux works well, although I've never needed to use it.

    Do not change the existing implementations and calling code, yet.

    I think we'd want to implement at least 1 core hook using this new architecture to dogfood it. Seems like that's the approach for a lot of new systems in core.

    with events itโ€™s possible to stop propagation and this feature is missing from the hook and hux listeners

    I don't see how this would be a good idea for any module to do. The whole point of hooks is that any module can hook in and do stuff. If some random module stops that then that could have serious consequences? Could it not be solved with priority/weighting?

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    Hello @acbramley,
    thanks for the feedback!

    I think it's wise that we keep the first iteration of hux-in-core simple. Some features in this list I've never used before, and I'm not sure whether they belong in core.

    This seems a good idea at first.
    One motivation for me is to release something incomplete now, people start using it, and then we have disruptive BC breaks in the future.
    Another is I want to be able to convert existing hooks without side effects.

    A secondary goal is also to be compatible with Hux contrib, so that people can transition smoothly from contrib hux to core hux. But I think we can get there when the rest is in place.

    Also, for a number of the "new features" discussed here, there are already issues in the hux issue queue, which can be an interesting reference.

    What we can do is make a "feature-complete" POC branch, and then split out parts that we can commit early, if we determine that the full thing is too much. In fact this is what I am doing right now.

    Multiple methods in the same module can implement the same hook, resulting in multiple implementations per module.

    I've never needed this, although I know it is available in Hux. Would it be simpler if we excluded it?

    I am working on one module with hux (not published yet), and I make heavy use of the multiple implementations per module per hook.
    E.g. I have multiple implementations for group_update, group_insert etc.
    I don't want to miss it :)
    Yes it might be simpler in one place but then we have to enforce an artificial limitation.

    Merging the result of multiple hooks seems potentially dangerous.

    It is generally not a problem for ->invokeAll(), because we already do merge results. But it can be a problem for single module ->invoke(). See ๐Ÿ“Œ Single invoke should merge results if multiple implementations for same module Needs review .
    In Hux, later implementations simply replace previous values in single module ->invoke().
    I am proposing a conditional merging that preserves the result type.
    E.g. in a module I work on I would like to use this for multiple #[Hook('schema')], that should then be merged. But with current version of Hux, I can't.

    Note that only few hooks are used with single module ->invoke().

    +1 to keep using the namespace discovery.

    Current POC branch has this :)
    For now it is the same namespace as in Hux. We can discuss to change it, but I don't see anything wrong in how Hux does it.

    Modules can add additional attribute classes, to let the method implement one or more hooks

    There's other ways to do this already in Hux, do we need another layer?

    How? Last time I checked, we need to add multiple attributes on the method, as many as we want to implement hooks.
    This is still acceptable, but not as convenient as it can be :)
    In Hux queue the relevant issues are โœจ Dynamic hook ID specific attributes Active and โœจ Advanced attributes to declare multiple hooks Closed: won't fix .

    I think the priority/weight system in Hux works well, although I've never needed to use it.

    The weights in Hux have a problem, because they don't allow your implementation to run before the procedural ones.
    This is fine in most cases where order is not important.
    But as soon as it does become important, you suddenly want all the tools you can get to achieve the desired order.

    One goal here is to convert existing procedural hooks without messing with the order, and without breaking contrib modules that do hook_module_implements_alter().

    The before/after has been proposed elsewhere, and it would be a natural conversion for ckeditor5_module_implements_alter().

    But I think most implementations are better served with weights, considering that most hook_module_implements_alter() implementations just do "move me to the end" or "move me to the start", instead of setting a position relative to another module.

    Do not change the existing implementations and calling code, yet.

    I think we'd want to implement at least 1 core hook using this new architecture to dogfood it. Seems like that's the approach for a lot of new systems in core.

    Well in the POC branch I convert a number of hooks to see if it works.
    We can decide how much of that we want to commit or not..

    with events itโ€™s possible to stop propagation and this feature is missing from the hook and hux listeners

    I don't see how this would be a good idea for any module to do. The whole point of hooks is that any module can hook in and do stuff. If some random module stops that then that could have serious consequences? Could it not be solved with priority/weighting?

    I agree this might not be a good idea.
    Especially because it would depend heavily on order of implementations, which can be quite fragile.

    I would need to see good use cases for it.

    I don't think we need to support stopping a regular ->invokeAll() call.
    But maybe we can have a special ->invokeAllUntil() which would only be invoked for special hooks that need it.
    Actually the way to do it could be to return an iterator of implementing callbacks, so the calling code can stop when it sees a special return value.

  • ๐Ÿ‡ท๐Ÿ‡ดRomania claudiu.cristea Arad ๐Ÿ‡ท๐Ÿ‡ด

    I would need to see good use cases for it.

    I don't think we need to support stopping a regular ->invokeAll() call.
    But maybe we can have a special ->invokeAllUntil() which would only be
    invoked for special hooks that need it.

    I assume that we want to standardize on a unified way to listen to events (even this is not limitative). I guess we want to offer users a *recommended* way to dispatch and subscribe, considering that now we have the classic hooks, Symfony dispatch/subscribe and proposed hux. Then we need to have the all from the 3 worlds. For use-cases, check all Symfony subscribers that are calling ->stopPropagation() on the event instance.

  • ๐Ÿ‡ท๐Ÿ‡ดRomania claudiu.cristea Arad ๐Ÿ‡ท๐Ÿ‡ด
  • ๐Ÿ‡ท๐Ÿ‡ดRomania claudiu.cristea Arad ๐Ÿ‡ท๐Ÿ‡ด

    To summarize, IMHO, the new system will have to have the following characteristics (please add more!):

    • Easy to write a listener: The very popular current hook system is such an example and HUX as well. For Symfony event subscribers you'll need first to (1) define a service, (2) scaffold a class, and so on... -> BAD DX
    • Predictable ordering of listeners: Current hooks are poor, they depend on module weight and module name (on same weight). Symfony event subscribers are good, you can set priorities.
    • Ability to stop downstream listeners: Current hooks don't comply, you can only remove them via hook_module_implements_alter() but that's static, you cannot make this decision on run-time, while you listen. Symfony event disoatcher knows to do that. We need it too.
  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    To summarize, IMHO, the new system will have to have the following characteristics (please add more!):

    Just to be clear, it is not a completely "new system", we want to BC support existing hook implementations.
    This is why we choose this hux-like path, but it is also why we don't declare this the one and only event system to replace all others.

    Ability to stop downstream listeners

    I would argue this will only ever be needed for specific hooks.
    Typical types of use cases:

    • "does at least one of X exist?" (stop once we get TRUE result)
    • "stop at first failure" (e.g. for validation)
    • "find me one item, I don't care about the others".
    • "find me one task, I will call again later for other tasks"

    Most of the current hooks don't have this need.

  • ๐Ÿ‡ท๐Ÿ‡ดRomania claudiu.cristea Arad ๐Ÿ‡ท๐Ÿ‡ด

    Just to be clear, it is not a completely "new system", we want to BC support existing hook implementations.

    Still means, we'll drop it in a new major release. Otherwise why we would need 2 systems + a 3rd-

    I would argue this will only ever be needed for specific hooks.

    I don't think we can predict the scenario faced by a given listener at some point. Also, Symfony dispatcher makes no such assumptions when is dispatching, it just let any arbitrary listener to decide. Moreover, for the sake of consistency, I think we should not have different types of hooks.

    This is why we choose this hux-like path, but it is also why we don't declare this the one and only event system to replace all others.

    I'm expecting that the new system will deprecate classic hooks and remove it in a future major release. Also, it will be recommended (and loved by devs) way compared with Symfony dispatcher. There's no reason to keep 2 hook systems in place, if some will still need hux, they can use it as contrib module.

  • ๐Ÿ‡ท๐Ÿ‡บRussia Chi

    Request/Response events come from Symfony itself. So we can't ever drop them.

    For Symfony event subscribers you'll need first to (1) define a service, (2) scaffold a class, and so on... -> BAD DX

    We could avoid the first step if implemented defining event listeners via PHP attributes.
    https://symfony.com/doc/current/event_dispatcher.html#defining-event-lis...

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    > I have nothing against more or different types of documentation.
    > But I don't see how this is blocking for this issue.

    Because we can't introduce new APIs and new patterns without also documenting them.

    And then API module needs to know what the documentation looks like so it can use it to generate the docs.

    For example, on https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21..., if you read the source code, the calls to invoke() and invokeAll() link to the API docs for the hook.

    > I also don't get what kind of documentation you are proposing.

    If we are moving away from hook implementations that look like a function mymodule_hookname(), then it no longer makes sense to have api.php files with functions hook_hookname() to document those hooks. We need something else.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    If we are moving away from hook implementations that look like a function mymodule_hookname(), then it no longer makes sense to have api.php files with functions hook_hookname() to document those hooks. We need something else.

    So, if we fully move to attributes, then the natural replacement could be dummy classes with methods with attributes on them..
    Ideally in a place where they don't spam the autoload classmap.
    Or maybe just one big class per module.

    The method names would be arbitrary, because the relevant info is the string in the attribute.

    If we keep procedural implementations _and_ attributes, we should keep the main documentation in one place, and reference the other.

    For the start, the default should be "less effort".
    So we would keep the documentations in *.api.php files, and add a mechanism to link from attributes to the respective hook definition.
    Similar magic will hopefully be added to IDEs like PhpStorm in the future.

    We can then provide an option to have an alternative documentation which uses a method with attribute instead of a function in a *.api.php file.
    The magic tools would have to find out which of them exists.

    We could also have hybrid documentation, where we add the attribute on top of the hook_*() function. Although I don't really like it because of the redundancy.

    So, there are various options, but imo it could even be acceptable to keep the *.api.php files for a while.

    I am a bit worried if we introduce a new mechanism for documentation, that there will be a period where half of the modules still have their *.api.php file, while others use the new mechanism. Both would work, but to find anything you now need two separate searches.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    Also, for a number of the "new features" discussed here, there are already issues in the hux issue queue, which can be an interesting reference.

    Yeah I guess that's where I'm getting at, that the current Hux features (even excluding some) will be enough for the 95% use case for most people.

    E.g. I have multiple implementations for group_update, group_insert etc.

    Can I ask what benefit that gives you instead of just a single hook that calls multiple functions?

    Yes it might be simpler in one place but then we have to enforce an artificial limitation.

    If we look at it as an improvement on procedural hooks, they have this same limitation. I just don't see the rationale behind it if it's going to overly complicate things.

    How? Last time I checked, we need to add multiple attributes on the method, as many as we want to implement hooks.

    Yup that's exactly what I was referring to.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote
    E.g. I have multiple implementations for group_update, group_insert etc.

    Can I ask what benefit that gives you instead of just a single hook that calls multiple functions?

    Yes!
    So the goal of the module is to sync data from Drupal to a different platform.
    I created different "tracker" classes that each respond to different entity hooks in Drupal, and write records into tables that will later be sent to the other platform.
    Each tracker is responsible for its own type of data to send, but they often have to respond to different entity types in Drupal. So I have multiple trackers that respond to group update/insert/delete, multiple trackers that respond to group role update/insert/delete, user update/insert/delete, membership crud, etc.

    Without this possibility, I would have to write my own dispatcher. In fact this is exactly what I did, before I decided to use Hux.
    This project also would benefit from the Hook('group_(update|insert)')] shortcut that I mentioned.

    But I also remember many cases in the past and in other modules where either one piece of code should be triggered for more than one hook, or one implementation contained too many distinct pieces of logic, that should rather be split up. E.g. in Drupal 7 I remember nightmarish hook_form_alter() implementations.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    Ah yeah, that makes more sense if you're spitting hooks into separate classes to declare different dependencies I guess, although again I feel like that's a pretty low % use case.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    Ah yeah, that makes more sense if you're spitting hooks into separate classes to declare different dependencies I guess, although again I feel like that's a pretty low % use case.

    And we can split existing implementations for separation of concerns.

    And a good thing is, now the respective method no longer needs to be part of the public interface of a service.
    It will still be a public method on the class, but not on the interface, if there is one.

    But this reminds me, what if the order of these implementations within one module is important?

    Currently the plans I described here and the MR on ๐Ÿ“Œ Hux-style hooks, proof of concept Needs work allow to specify a global weight to run before or after implementations from all other modules (with default weight 0), and the before/after keys.
    But perhaps we need a micro weight to sort within the same module?

    E.g. a procedural implementation like this:

    function workspaces_form_alter(&$form, FormStateInterface $form_state, $form_id) {
      if ($form_state->getFormObject() instanceof EntityFormInterface) {
        \Drupal::service('class_resolver')
          ->getInstanceFromDefinition(EntityOperations::class)
          ->entityFormAlter($form, $form_state, $form_id);
      }
      \Drupal::service('class_resolver')
        ->getInstanceFromDefinition(FormOperations::class)
        ->formAlter($form, $form_state, $form_id);
    }
    

    In the MR this is being split up.
    But is the order important? Does EntityOperations::entityFormAlter() need to run before FormOperations::formAlter()?

    In the example this could be achieved with a `before: "workspaces"` on the `EntityOperations::entityFormAlter()`.
    But this does not really scale if we have more than 3. (there is only before, same and after), and it pollutes this possibility for other modules.

    Or it could use `weight: -5`, but this would pull it before all other implementations, not just relative to those in the same module.

    We could easily add and support a new key "microweight" or "minor_weight".
    But anything we add has to be maintained in the future. It is easier to add than to remove.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    If two hook implementations in the same module have a dependency in either direction on each other, that should be one hook implementation IMO. I can see how splitting otherwise unrelated hook implementations could be useful (similarly having one method that subscribes to both updates/insert hooks or two different form alters, which is much more common), but once you're worrying about order you can just go back to the single implementation.

    Predictable ordering of listeners: Current hooks are poor, they depend on module weight and module name (on same weight). Symfony event subscribers are good, you can set priorities.

    I don't entirely agree with this.

    Module weight and name are a bad way to order - doubt there's many disagreements about that, but hook_module_implements_alter() allows you to ensure that your hook runs before or after a specific hook from another module.

    Symfony priorities you end up in an arms race with other modules (-1000 etc.), and if you want to run between one listener and another listener, you end up having to document why you chose priority: 812 and hope neither listener changes their weight later.

    You end up with stuff like this:

      twig.extension:
        class: Drupal\Core\Template\TwigExtension
        arguments: ['@renderer', '@url_generator', '@theme.manager', '@date.formatter', '@file_url_generator']
        tags:
          - { name: twig.extension, priority: 100 }
      twig.extension.debug:
        class: Twig\Extension\DebugExtension
        tags:
          - { name: twig.extension, priority: 50 }
      twig.extension.varDumper:
        class: Drupal\Core\Template\DebugExtension
        tags:
          # This extension is loaded after the Twig Debug Extension because for Twig
          # Extensions, last extension loaded takes precedent. This allows this
          # extension to override the default Twig Debug Extension conditionally
          # when Symfony VarDumper is available.
          - { name: twig.extension, priority: 25 }
    

    before/after would be better than both systems - it's still possible to end up with conflicts, but generally only when you have a 'real' conflict or circular dependency, not just getting unlucky with numbers.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    Symfony priorities you end up in an arms race with other modules (-1000 etc.), and if you want to run between one listener and another listener, you end up having to document why you chose priority: 812 and hope neither listener changes their weight later.

    Arguably it is similar now with hook_module_implements_alter().
    Most them just want to run "before all others" or "after all others", but if they need anything more fine-grained, it gets complicated.

    So if we would convert hook_module_implements_alter() to a weights system, we might just set weight = 10 for anything that is pushed to the end, and weight = -10 for anything that is pushed to the start, and leave the rest to destiny (or rather the canonical order of modules).

    With the POC MR in the other issue, we do _not_ convert these to weights (yet), but rather hook_module_implements_alter() remains in place and does its job as before.
    (Or actually I think those hooks that I converted don't really need reordering. I just picked the easy targets)

    before/after would be better than both systems - it's still possible to end up with conflicts, but generally only when you have a 'real' conflict or circular dependency, not just getting unlucky with numbers.

    As said above, most current hook_module_implements_alter() implementations do _not_ specify a position relative to another module, but rather before or after everything else.
    Given this past experience, how realistic is it that this will change?

    There could be a major difference with symfony, in that perhaps in symfony it is more likely that you know about the other subscribers, whereas with hooks you want to position yourself relative to arbitrary yet unknown modules.

    In the POC MR in the other issue, the before/after keys work a bit different than you might expect:
    It only places you before or after the position an implementation of the target module would have if it is not itself repositioned.
    So if the canonical module order is ...node...user..., then the order of hooks might be like this:

    1. #[Hook('somehook', module: node, weight: 5)]
    2. #[Hook('somehook', module: node, before: node)]
    3. #[Hook('somehook', module: user, before: node)]
    4. function node_somehook()
    5. #[Hook('somehook', module: node)]
    6. #[Hook('somehook', module: node, after: node)]
    7. #[Hook('somehook', module: user, after: node)]
    8. #[Hook('somehook', module: node, before: user)]
    9. #[Hook('somehook', module: user, before: user)]
    10. function user_somehook()
    11. #[Hook('somehook', module: user)]
    12. #[Hook('somehook', module: node, after: user)]
    13. #[Hook('somehook', module: user, after: user)]
    14. #[Hook('somehook', module: user, weight: 5, before: node)]
    15. #[Hook('somehook', module: node, weight: 5)]
    16. #[Hook('somehook', module: user, weight: 5)]

    The hook_module_implements_alter() moves entire groups of implementations with the same module.
    (Actually I just found that it I might want to change how it moves the before/after parts.)

    So, with the current implementation in the MR, there can never be a circular requirement, because implementations do not position themselves relative to other implementations, but relative to modules.
    This way we don't have to invent ids for implementations.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    New related issue as preparation for this change: ๐ŸŒฑ Fix awkward details in hook system Active

  • ๐Ÿ‡ซ๐Ÿ‡ทFrance andypost

    one more benefit issue

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany geek-merlin Freiburg, Germany

    @donquixote: Great you give this topic a bumpโฃโฃโฃ

    That said, sorry for coming late to the party. But i have a very fundamental objection.

    Gut feeling: Having arbitrary methods still breathes function-style approach (so do symfony subscribers!).
    It hass makes static code analysis very hard, and makes mental groking harder, as hook methods can have arbitrary names.

    My dream hooks would look like that:
    - Every hook corresponds to an interface
    - Services implement that hook by implementing the interface (a la sf autoconfigure)

    Of course this raises the question of hook_TYPE_foo, but there are several ways to overcome that, which we can bikeshed.

    Yes, having the proven hux module is a stong point for simply taking that, but otoh IF we are changing core, we should think twice.

    (I remember that i coded a POC of this quite far, but got pulled away from that area. Will share soon-ish, so people can judge themself. )

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    Of course this raises the question of hook_TYPE_foo, but there are several ways to overcome that, which we can bikeshed.

    I originally had a kneejerk reaction against this because we're never going to be able to do 1-1 dynamic hooks to interface. But as I was writing up the objections, maybe there's a nice way to do it.

    One big advantage of an interface would be that it could replace module.api.php

    So something like:

    interface HookInterface {}
    
    interface FormAlterInterface extends HookInterface {
      public function alter(array &$form,FormStateInterface $form_state, string $form_id): void {}
    }
    
    For dynamic hooks, we can't do interface per hook, so autodiscovery falls down, but maybe we can do interface-per-dynamic-hook, like this:
    
    <?php
    interface DynamicHookInterface extends HookInterface {}
    
    interface FormIdAlterInterface extends DynamicHookInterface {
      public function alter(array &$form,FormStateInterface $form_state): void {}
    }
    

    DynamicHookInterface helps with docs, and it would tell us that we can't use pure-interface discovery on those classes.

    Could then auto-discover everything implementing HookInterface, and continue to rely on attributes for everything implementing DynamicHookInterface

    There are some drawbacks though:
    - you'd only be able to use a single class to implement multiple hooks if the interface signatures don't clash. i.e. you'd need separate classes for every FormIdAlterInterface implementation
    - a single method can't implement multiple hooks (hook_node_insert()/hook_node_update() is the common use-case).

    However if we continue to allow attribute-only hooks per the current hux model, modules could skip the interface just for those cases.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    There are some drawbacks though:

    • you'd only be able to use a single class to implement multiple hooks if the interface signatures don't clash. i.e. you'd need separate classes for every FormIdAlterInterface implementation
    • a single method can't implement multiple hooks (hook_node_insert()/hook_node_update() is the common use-case).

    I can name more drawbacks related to dynamic hooks.
    This assumes that e.g. hook_ENTITY_TYPE_update() would be a dynamic hook.

    • We can't have multiple methods on a class that all implement variations of the same "dynamic" hook. E.g. distinct methods for term update, node update, comment update etc. The same is if we want multiple form alters on the same service/class.
    • We can't narrow down parameter types. A method for hook_node_update() must have parameter type EntityInterface instead of NodeInterface, to satisfy Liskov.

    I already have real-world use cases where I would use exactly these features.

    At the same time, we don't get the full benefit that we usually get from interfaces.
    To me, the biggest benefit of interfaces is dependency abstraction in actual userland code.
    We still get the dependency abstraction, but none of the userland code ever interacts with the specific hook implementations. So there is not as big a need to have these contracts formalized as interfaces.

    In fact it could be more useful to wrap the complete stack of implementations in a class with interface, instead of the individual implementations. But then I still would vote against it.

    For dynamic hooks, the relationship between a generic interface and the specific hook variations is not really a subtype relationship, but rather a specialization, as we would have with generics. The closest we have to this is the generics notation from phpstan / psalm.

    ---

    One big benefit of the current approach is that it will work nicely with the current system, without adding anything per hook.
    It allows contrib and custom modules to modernize their code, without waiting for a new generation of Drupal.

    That is, if we accept the current documentation in *.api.php as sufficient to document a hook, even if we come up with alternative ways of documentation in the future.
    The attribute class for #[Hook] can itself have some starting documentation with links for further reading, and with hints to look for the hook_*() functions.
    We can then hope that IDEs (PhpStorm) add support for it, as it already does for procedural hooks.

    ----

    I should also mention that interfaces for a similar purpose are not really new: We already have tagged services with service collectors, which have some overlap with events and hooks.

    ----

    However if we continue to allow attribute-only hooks per the current hux model, modules could skip the interface just for those cases.

    If we make the interface completely optional, I would be ok with it. But this also means we can add interfaces in a follow-up.

    modules could skip the interface just for those cases.

    What exactly would this mean?
    Firstly, which module are we talking about - the one that is publishing the hook, or the one that is implementing it?
    As long as the module publishing the hook is not updated, the interface simply won't exist. But implementing modules would already start adding implementations with #[Hook] attribute. I assume we want to allow this, or not?
    If we allow this, then the interface has to remain optional all the way, until a major version update.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    Maybe there is a main conflict of whether we attempt to design the "perfect" new system to replace hooks and events, or if we are just gradually improving what we have.

    To me, it is as important how the drop rolls (*) than where it lands. And it does not really land, ever.
    (*) Or falls, or flows?

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia acbramley

    I'm a big -1 on Interfaces for every single hook, we would need a huge number of them and that sounds like it will be a bit of a nightmare to maintain.

    - you'd only be able to use a single class to implement multiple hooks if the interface signatures don't clash. i.e. you'd need separate classes for every FormIdAlterInterface implementation
    - a single method can't implement multiple hooks (hook_node_insert()/hook_node_update() is the common use-case).

    These sound like pretty major drawbacks

    However if we continue to allow attribute-only hooks per the current hux model, modules could skip the interface just for those cases.

    If we allow it without an interface, then doesn't that defeat the purpose?

  • ๐Ÿ‡ฎ๐Ÿ‡ฉIndonesia el7cosmos ๐Ÿ‡ฎ๐Ÿ‡ฉ GMT+7

    Having arbitrary methods still breathes function-style approach (so do Symfony subscribers!).

    While the subscribers do have arbitrary methods, the dispatched event class can have specific methods or implement interfaces, so they still have advantages of modern development (IDE, static analysis, etc)

    There are some drawbacks though:
    - you'd only be able to use a single class to implement multiple hooks if the interface signatures don't clash. i.e. you'd need separate classes for every FormIdAlterInterface implementation
    - a single method can't implement multiple hooks (hook_node_insert()/hook_node_update() is the common use-case).

    And event dispatcher also solves these drawbacks.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    Firstly, which module are we talking about - the one that is publishing the hook, or the one that is implementing it?

    Sorry, meant implementing in that context although perhaps both.

    I'm a big -1 on Interfaces for every single hook, we would need a huge number of them and that sounds like it will be a bit of a nightmare to maintain.

    I was mainly thinking about it to replace *.api.php on more or less a 1-1 basis. But there's another problem with it that hook dispatching only requires a hook name at the moment, we'd have to tie the hook name to the interface when we dispatch and that adds extra things to worry about. So there is really no benefit to it other than providing two ways of discovery and a place for documentation.

    If we make the interface completely optional, I would be ok with it. But this also means we can add interfaces in a follow-up.

    Yes this is true, so if we decided on it as a replacement for *.api.php we could do that later on.

  • Would it make sense to create something like a comparison table for the HUX/Event implementations?

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    There was a lot of discussion of events vs. hooks in ๐ŸŒฑ Allow module services to specify hooks Needs work , see for example #2237831-53: Allow module services to specify hooks โ†’ . This issue is a product of around a decade of discussions in previous issues about hooks vs. events. It would be good to summarise the differences in a comparison table, but would suggest that if people are reading this and thinking 'why not events?' that they read some of the older issues first too.

  • ๐Ÿ‡ซ๐Ÿ‡ทFrance andypost

    IIRC hooks discovery vs event registration hacks means hux should use own discovery

  • ๐Ÿ‡ง๐Ÿ‡ดBolivia vacho Cochabamba

    Nice idea. I am reviewing and some notes:

    - Also we need the idea to add new Implementations that should be entirely independents of the current hooks system.
    - As I see hux module looks nice to convert current hooks system into a POO way. We need to alter this in order to co-exists with the current but should be prepared to works without this (current hook system).

  • ๐Ÿ‡ง๐Ÿ‡ดBolivia vacho Cochabamba

    - Also we need something about weight of module into hooks system. I mean something to can get a priority order for my function with relation to another functions.

  • ๐Ÿ‡ง๐Ÿ‡ดBolivia vacho Cochabamba

    I am think that #[Hook('entity_access')] that Hux module provide is great to start Back Compatibility.

    namespace Drupal\my_module;
    
    use Drupal\hux\Attribute\Alter;
    use Drupal\hux\Attribute\Hook;
    use Drupal\hux\Attribute\ReplaceOriginalHook;
    
    /**
     * Usage examples.
     */
    final class MyModuleHooks {
    
      #[Hook('entity_access')]
      public function myEntityAccess(EntityInterface $entity, string $operation, AccountInterface $account): AccessResultInterface {
        // A barebones implementation.
        return AccessResult::neutral();
      }
    

    However to completely POO way we need to define a Parent Class where any module that needs to implement a new "Hook", extends from this and create the new modules. also at time to extend this class we can use #... to use some existing hook, however this should be target as deprecated to provide time to core and most another modules to remove completely.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    @vacho Most of what you mentioned is already covered in the current proposal - just read previous comments and the issue summary.
    We should avoid to repeat the same points for no reason.
    Yes, the new system as proposed here will look and work very similar to what you see in the Hux contrib module. The main difference is a better integration with the core system.

    However to completely POO way we need to define a Parent Class where any module that needs to implement a new "Hook", extends from this and create the new modules.

    It is not really necessary or useful to extends any kind of parent class for this purpose.
    This is because a class can provide any number of methods which act as hook implementations, with arbitrary names. So a base class or interface would just be empty and useless.
    symfony event subscribers often _do_ implement an interface, but this is because the registration method, not the event callback itself.

    There is a different mechanism in Drupal that typically _does_ use base classes (or rather, interfaces) - tagged services with methods that are called by service collectors. In some cases this can be an alternative to hooks. But this is already mentioned in the issue summary.

  • ๐Ÿ‡ง๐Ÿ‡ดBolivia vacho Cochabamba

    thanks for the comment @donquixote

    I mean this is the Hux way to define a class:

    use Drupal\hux\Attribute\Alter;
    use Drupal\hux\Attribute\Hook;
    use Drupal\hux\Attribute\ReplaceOriginalHook;

    /**
     * Usage examples.
     */
    final class MyModuleHooks {
    
      #[Hook('entity_access')]
      public function myEntityAccess(EntityInterface $entity, string $operation, AccountInterface $account): AccessResultInterface {
        // A barebones implementation.
        return AccessResult::neutral();
      }
    ....
    

    Where looks that we can avoid

    use Drupal\hux\Attribute\Alter;
    use Drupal\hux\Attribute\Hook;
    use Drupal\hux\Attribute\ReplaceOriginalHook;
    

    extending from a parent class that already implements this.

    BTW the discover looks out of drupal way for Hux class,. As you say Drupal usually define interfaces or base classes just like use Symfony EventSubscribers...

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States EclipseGc

    So I think I'm going to post here, but I'll cross post a link to this comment in ๐Ÿ“Œ Hux-style hooks, proof of concept Needs work .

    I want to come at this from a slightly different angle. The HUX style hook attribute stuff is very cool, and I'm not against it at all, but we have two competing systems in hooks vs event subscribers, and this issue/effort does little to resolve that. To that end, I'm going to provide a POC patch here. It requires a lot more work, but you can drush si a site with the patch applied, you can install new modules, and almost all hooks are working to the best of my testing (this will fail test coverage as I've not migrated any of that over).

    Goal

    1. Unify hooks and event subscribers
    2. Do so with NO Event class boilerplate
    3. Event subscriber methods should get the same parameters as corresponding hooks
    4. Event subscribers and hooks should be able to be interwoven in their execution order

    Caveats

    1. I've not attempted to migrate preprocess hooks (That is, I've not done any work on Registry.php)
    2. I've not looked at how update hooks fire, my solution might work for them... it might not.

    The How

    Others have attempted in the past to unify hooks and event subscribers, and we still don't have a solution. While poking at the problem space I noticed a few important things:

    1. We don't need "Event" classes if we don't call EventSubscriber::dispatch()
    2. Calling EventSubscriber::getListeners() manually (it's public) lets you invoke them however you like, with whatever parameters you like
    3. In Drupal 10.2, the ContainerAwareEventDispatcher was happy to take a string as a Callable. During this process I moved the patch to be against 11.x and found that we've moved to use the core Symfony EventDispatcher. It was a little more persnickity, but I got it working.

    In order to do this, I've got a compiler pass. It starts by finding all installed modules, and their weight as detailed in core.extension.yml. It then finds any method in .module and .install that COULD be a hook. That means it will wrongly identify functions with the module name prefix as being potential hooks even when they're not. I don't see this as being particularly harmful. The resulting map of function names is relatively small all things considered. Once this list is built, I manually invoke hook_hook_info() in the compiler pass. I use that to find other files that can hold particular hook implementations. Since we know which hooks will appear there, we can be sure to not mis-identify any that exist. We also note which file hook implementations are found in. This will be useful later.

    Once we've done this, we re-organize the information and manually invoke hook_module_implements_alter(). The result of that is used to modify the weights of each hook implementation that has been altered. This allows us to maintain an execution order that will keep things in line with Drupal's existing expections while allowing us to also bubble up that information in such a way that it can be stated in "priority" order to the event dispatcher. Doing this will allow regular event subscribers to set priority between existing hook implementations with different priorities.

    Once that's complete, we register our hooks on the event dispatcher as listeners to particular hooks and write our list of hooks and corresponding implementations to the container as a parameter any service can use.

    The Hook Caller

    As I mentioned earlier, the vanilla Symfony Event Dispatcher was not fond on taking function names as callables. In order to make that work I invented a HookCaller class that is small, and useful. It implements the magic __call() method, so we can call any hook implementation against it. It then uses the documented hook implementations from the previous step to figure out which file the implementation exists within, load that file, and then call the function. I initially sort of hated this solution, but now I kind of love it. It really simplifies a lot of complicated things that module handler/loader/stuff all did.

    The Module Handler

    My code here is still very much in flux and needs some love, but the important bits are there-ish. The invoke method is wrong. To do this "right" we need to identify EventSubscribers to particular hooks, what module's namespace they exist within, and ensure they're invoked too. The code is working right now, and does some weirdness around letting you invoke particular service methods, but that should be replaced by what I've just described.

    Similary, with the compiling of a list of hooks to the container, I suspect we no longer need the cache backend stuff that's happening here. I didn't dig in too far into that, it's just a hunch.

    Misc changes and thoughts

    I had to modify hook_page_top/bottom callers. I think that might be able to be moved back to it's original state with some changes in the hook.caller, but I was tired of messing with it, and this made it work.

    I just barely started to poke at the theme side of the equation before I decided to stop and get feedback on this. As mentioned earlier Registry.php will need some changes too.

    I added some methods to the ModuleHandler. Again, this is a POC. Some of that stuff can probably be done better than what I did here.

    Bringing this all back around to HUX, I bet a simple Trait could be written that generates the getSubscribedEvents() method body from method Attributes. There are a dozen ways that could be done... I'm just trying to keep it somewhat on topic here while I try to make hooks/events the same thing for existing Drupal hooks.

    Summary

    When we're in the issue queue, there's often a question of "is this a new hook? or event?". I've seen it go both ways, and no one ever seems completely satisfied with the answer. While traditional event subscribers with a custom event class are sort of a different thing from this, my solution here mean we can largely make both camps happy when inventing new "api" level considerations. We don't have to discuss if it's a hook or an event. We only have to discuss if it would be better served with a custom event class, or just passing parameters.

    I've attached the patch, and here's an example of what an event subscribe could look like with this patch applied. This code works, it's not a mock up.

    
    namespace Drupal\kris_test\EventSubscriber;
    
    use Drupal\Core\Form\FormStateInterface;
    use Drupal\Core\Routing\RouteMatchInterface;
    use Symfony\Component\EventDispatcher\EventSubscriberInterface;
    
    class ExampleHooks implements EventSubscriberInterface {
    
      /**
       * @inheritDoc
       */
      public static function getSubscribedEvents() {
        $events = [];
        $events['hook_help'][] = ['onHookHelp'];
        $events['hook_page_top'][] = ['onPageTop'];
        $events['hook_form_alter'][] = ['onFormAlter'];
        $events['hook_form_search_block_form_alter'][] = ['onFormSearchBlockFormAlter'];
        return $events;
      }
    
      #[Hook('help')]
      public function onHookHelp(string $route_name, RouteMatchInterface $rout_match) {
        return 'This is a test';
      }
    
      public function onPageTop(&$page_top) {
        $page_top['test'] = [
          '#markup' => 'this is a test'
        ];
      }
    
      public function onFormSearchBlockFormAlter(array &$form, FormStateInterface $form_state) {
        $form['more_test'] = [
          '#markup' => 'This is a search block specific customization'
        ];
      }
    
      public function onFormAlter(array &$form, FormStateInterface $form_state, string $form_id) {
        $form['test'] = [
          '#markup' => 'This is form: ' . $form_id
        ];
      }
    }
    
    
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    We don't need "Event" classes if we don't call EventSubscriber::dispatch()

    It's no longer a requirement for ::dispatch() either because there is no Event type hint, so e.g. hook_entity_load(EntityInterface $entity) could be called already. This is covered already in some of the discussion above.

    It would need an extra method or five to handle alter hooks and multiple arguments though.

    /**
       * @inheritDoc
       */
      public static function getSubscribedEvents() {
        $events = [];
        $events['hook_help'][] = ['onHookHelp'];
        $events['hook_page_top'][] = ['onPageTop'];
        $events['hook_form_alter'][] = ['onFormAlter'];
        $events['hook_form_search_block_form_alter'][] = ['onFormSearchBlockFormAlter'];
        return $events;
      }
    
      public function onHookHelp(string $route_name, RouteMatchInterface $rout_match) {
        return 'This is a test';
      }
    
    }
    

    From the hook implementation point of view, the discussions both here and in ๐Ÿ“Œ Hux-style hooks, proof of concept Needs work gravitated towards using attributes on methods for discovery rather than ::getSubcribedEvents() - this would be even less boilerplate then. See the examples on https://symfony.com/doc/current/event_dispatcher.html

    In order to do this, I've got a compiler pass. It starts by finding all installed modules, and their weight as detailed in core.extension.yml. It then finds any method in .module and .install that COULD be a hook. That means it will wrongly identify functions with the module name prefix as being potential hooks even when they're not. I don't see this as being particularly harmful. The resulting map of function names is relatively small all things considered.

    This is interesting as a bc layer, although I think along with this we would want to implement attribute discovery for procedural hooks, and then trigger deprecations if a hook is invoked that is not using attributes. That way we can force contrib to at least add the attributes so we can drop the get_defined_functions() logic, but also plenty will take the opportunity to move hooks to methods then too.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    Doing a similar API to hux with the event dispatcher was discussed in #2237831-51: Allow module services to specify hooks โ†’ downwards, worth reading that for extra context.

  • Plus, getSubscribedEvents is using strings for the callables, which won't be noticed by the IDE with regards to type hinting or finding references to functions. I think that, to the best we are able, we should avoid using strings to represent or point to functions.

    From the hook implementation point of view, the discussions both here and in ๐Ÿ“Œ Hux-style hooks, proof of concept Needs work gravitated towards using attributes on methods for discovery rather than ::getSubcribedEvents()

    I definitely prefer using attributes. It's closer to the newer Drupal way of solving tasks with an object-oriented approach.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany Fabianx

    What Kris did here is kinda orthogonal to the approach and does not care where the events come from.

    In essence adding support for //[@Hook(entity_insert)] style annotations to the existing patch is very simple:

    - We know the service to call
    - We know the method to call
    - We just map that into the hook_entity_insert event (along side all other events)

    The advantage is that:

    - Full BC (and possibility to clearly deprecate module style invocations)
    - Does not care if you use event listener to listen to the events or @Hook style invocation or .module file (for now)

    We get rid of a lot of run-time stuff, which we cache anyway per hook and instead create for the .module things more like a registry of potential invocations.

  • ๐Ÿ‡ท๐Ÿ‡บRussia Chi

    Re #77
    If we use event listeners instead of subscribers we could make use of PHP attributes as well.

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada Charlie ChX Negyesi ๐ŸCanada

    I read eclipsegc's patch. I liked it quite a bit. But I thought we could perhaps do even better.

    I observed ๐Ÿ“Œ Replace ContainerAwareEventDispatcher with Symfony EventDispatcher Fixed and read https://symfony.com/doc/current/event_dispatcher.html . I searched for AsEventListener to see how it is used and observed it is not used at all and realized while it is shipped with the event dispatcher, it is only wired in the full framework here.

    Inspired by all that, may I present my humble patch. Here's an example:

    
    namespace Drupal\node\Hooks;
    
    use Drupal\Core\Messenger\MessengerInterface;
    use Symfony\Component\EventDispatcher\Attribute\AsEventListener;
    
    #[AsEventListener]
    class Whatever {
    
      public function __construct(protected MessengerInterface $messenger) {
      }
    
      #[AsEventListener(event: 'hook_entity_load')]
      public function foobar() {
        $this->messenger->addMessage('here');
      }
    
    }
    

    That is all.

    1. Absolutely minimum boilerplate. There's no need for a service definition or getSubscribedEvents.
    2. Very little Drupalism and not really new at that: the class needs to be in a Drupal\modulename\Hooks namespace and according directory -- this pattern should be very familiar to Drupal developers because of the plugin subsystem. AsEventListener is a Symfony attribute and we use it as it is documented.
    3. Any number of hook classes, any number of hook methods, any method can implement as many hooks as it wants.
    4. very very little code -- less than eighty new lines is added to core. alter is not implemented yet but it should be trivial based on how invokeAllWith and invoke goes. Likely it will need some performance love, too.

    Of course, events registered normally using a service and getSubscribedEvents() work just as well -- this should only be necessary when autowire doesn't suffice.

    Conversion from the old to the new I am sure will be done with rector at the end but may I note it could be done by search-and-replace: take "Implements hook_theme()" and rewrite it into #[AsEventListener(event: 'hook_entity_theme')]. Implements was three lines of code, this is only one so it actually decreases the size of boilerplate :D An exemption to coding standard doxygen for AsEventListener methods and classes would be required to keep the boilerplate at minimum but since hook implementations also have no doxygen this can't be a problem.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    Reviewed the latest patch quickly (haven't said that for a while). I can't really think of any gotchas here except for hook_module_implements_alter() - but that would still work, implementations might just have to adjust and to run last you'd need to use the new API.

    1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
      @@ -1372,6 +1375,49 @@ protected function compileContainer() {
      +  public function registerHooks(ContainerBuilder $container): void {
      

      Does a service with the correct attribute 'just work' as a hook too with this approach? Or would we need an extra compiler pass for that.

    2. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
      @@ -392,8 +393,14 @@ public function hasImplementations(string $hook, $modules = NULL): bool {
      +    foreach ($this->eventDispatcher->getListeners('hook_' . $hook) as $listener) {
      

      tidy.

      I assume this means that class hooks always run after function hooks? It will potentially mean some hook_module_implements_alter() would need updating as implementations get converted, but also I think that is fine.

    3. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
      --- /dev/null
      +++ b/core/modules/node/src/Hooks/Whatever.php
      

      Would be nice to convert one real core hook implementation over.

    I also wonder if the approach here could be combined with @EclipseGc's bc layer for procedural hook-events so that everything runs through the dispatcher instead of the two separate calls?

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    This looks very interesting!

    So essentially we use the events mechanism to manage the hook implementations, but we use ModuleHandler to actually call/invoke them.

    This also means you are not supposed to invoke these special "hook" events via the event system.
    At least those hooks that have return values or that receive more than one parameter.

    This is probably fine.
    But do we want to "pollute" the event list with pseudo-events that cannot really be used as such?
    Or should we have a separate instance of the event system for hooks?

    I can't really think of any gotchas here except for hook_module_implements_alter() - but that would still work, implementations might just have to adjust and to run last you'd need to use the new API but that is fine.

    This also means that implementations of hook_module_implements_alter() that _remove_ an implementation will no longer work if that implementation gets converted.

    I assume this means that class hooks always run after function hooks? It will potentially mean some hook_module_implements_alter() would need updating as implementations get converted, but also I think that is fine.

    This is the same as currently in Hux.

    If we start replacing core hook implementations, there will be a BC impact due to the changed order.
    Maybe we truly target 11.x, or we accept the damage.
    Or we introduce the API but don't convert any existing core hook implementations before 11.0.

    An interesting question will be alter hooks in general, especially the combined ones like "form" + "form__somethingform".
    We could go for a similar solution, if we don't care to replicate the order of execution from the old system, which is quite complex.
    Again, the main BC impact will happen if we try to convert existing implementations.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    I think it is OK to do conversions in minor releases, we can have change records for each minor saying that hook_module_implements_alter() will need to be updated. The actual order of what goes into hook_module_implements_alter() is not guaranteed at all, and not subject to bc, we just need to warn people.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    This also means you are not supposed to invoke these special "hook" events via the event system.
    At least those hooks that have return values or that receive more than one parameter.

    This is probably fine.
    But do we want to "pollute" the event list with pseudo-events that cannot really be used as such?

    Could we have a subclass of event dispatcher that has a method (or methods) with the same logic? Then obviously you couldn't call dispatch() on those events, but (once the hook bc layer is dropped) it would eventually be possible to call those methods directly instead of via the module handler.

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada Charlie ChX Negyesi ๐ŸCanada
    1. Does a service with the correct attribute 'just work' as a hook too with this approach? Or would we need an extra compiler pass for that? No and -- maybe surprisingly -- I suspect the second is also no. This is a separate, much simpler issue, I strongly suspect lifting this ten lines of code from Symfony framework into DrupalKernel would cause the container to add proper kernel listener tags to services which have this attribute because all the code necessary is already included in symfony/dependency-injection but I haven't tested this yet. But, as I said, this seems like a completely separate issue to me. Could easily precede this one.
    2. To facilitate implements alter we could gather all the necessary info into an array: keys are hook, class, method value is priority. Then run a new implements alter on them. I am not quite sure how the container behaves at the point this code runs so it's possible it'd be better if we used magic named classes for this purpose alone -- much like we do with FooServiceProvider -- instead of a hook_event_hooks_implements_alter or some such. Do we have experience in instantiating a service in DrupalKernel::compileContainer? I doubt that's wise, really.
    3. A new service could be defined which uses the existing dispatcher class or extends it and throws a logicexception on dispatch. Could even proxy the event dispatcher, all the same. Perhaps move the new hook-firing logic in there and eventually merge modulehandler into it and drop modulehandler on the long run (DrupalCon Portland looks good :P).
    4. Then using the altered array just directly run $module_dispatcher_definition->addMethodCall('addListener', [$event, [new ServiceClosureArgument(new Reference($class)), $method], $priority]); instead of tagging. I had an earlier version doing these calls. It's not necessary to add a tag.
    5. I wouldn't worry much about BC, of course old hooks can't just die from one release to the next but I expect the conversion to be automated and very swift. I do not see a problem with any hook names, the conversion is: "in module foo, given a function which name starts with foo_ and has doxygen which says implements hook_ take the function name, replace foo with hook, that's the event name". The converter script doesn't need to know about Drupal: a directory containing foo.info.yml and subdirs belongs to module foo unless it contains another bar.info.yml. This is essentially what EclipseGc's code does, sorta. I just don't want to do it runtime, let's just convert stuff. I do not know much about phpstan but we have excellent people who do -- but if push comes to shove I can write it with nikic/php-parser in less than a day -- I would find out the event name and the function starting and ending line with it but do the actual conversion with just string operations. But that's just how I would do it. Name a D Day and convert core in a single commit that day. Prepare for it with test issues, of course. On that day, mark procedural hooks deprecated and drop them from the next major. So maybe introduce it in 11.1 or 11.2 and drop procedural in Drupal 12. Or does such deprecation need to wait for D12 and drop in D13? Rapid conversion alleviates the pain with having to deal with two sorts of module implements alter.

    I am happy with the feedback so far, thanks catch and donquixote. I do not see any major pain points raised nor anything which would require adding a lot of code. My goal here is to keep both boilerplate and added core complexity down to an absolute minimum.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    I am happy with the feedback so far, thanks catch and donquixote. I do not see any major pain points raised nor anything which would require adding a lot of code. My goal here is to keep both boilerplate and added core complexity down to an absolute minimum.

    My previous attempt at this at ๐Ÿ“Œ Hux-style hooks, proof of concept Needs work had _a lot_ more code.
    Unfortunately I did not have consistent time and attention for it.

    The main reason for the complexity, from what I remember,
    It was a lot more ambitious in terms of BC, and also tried to cover wishlist items like relative ordering etc.

    I like the simpler approach proposed here, but it does mean we lose _some_ functionality and BC, especially if we convert existing hook implementations. There might be more nasty details that I no longer remember atm.

    I wouldn't worry much about BC, of course old hooks can't just die from one release to the next but I expect the conversion to be automated and very swift.

    I am not worried about the effort of the conversion.
    My BC concern is about custom and contrib code which:
    - relies on a specific order of execution
    - uses hook_module_implements_alter() to add, remove or rearrange functions
    - relies on conditionally included files ($module.$group.inc)
    - implements a hook on behalf of another module
    - (perhaps more stuff which I don't remember)

    But, if we aim for 11.x then I think this is fine.

    So, perhaps like this:
    - In 10.x we introduce a BC-friendly mechanism that allows contrib and custom code to define hooks in the new way.
    - In 11.x we convert the core hooks.
    - In 12.x we drop the old system (as you already proposed).

    I do not know much about phpstan but we have excellent people who do -- but if push comes to shove I can write it with nikic/php-parser in less than a day -- I would find out the event name and the function starting and ending line with it but do the actual conversion with just string operations.

    You could have a look at the conversion commits here, https://git.drupalcode.org/project/drupal/-/merge_requests/4347/commits

    Could we have a subclass of event dispatcher that has a method (or methods) with the same logic? Then obviously you couldn't call dispatch() on those events, but (once the hook bc layer is dropped) it would eventually be possible to call those methods directly instead of via the module handler.

    Or we could keep it as a separate class, which just uses the symfony event dispatcher internally.
    This would follow the composition over inheritance idea, and avoid interface creep.

    Having one big event dispatcher class and interface which also supports hook-style dispatching would mean that components that need both only need one dependency injected. But is this a good reason? Not sure.

    One conceptual / philosophical question is whether we think of these new OOP hooks as events, or as something separate, which just uses the event system to collect subscribers.
    I think it matters for DX:
    If we advertise it as one system, then developers will have the same expectations for hooks and events. This could lead to a situation of surprise or disappointment.
    If we advertise it as separate systems (as it is now), then developers can have separate mental models with separate expectations. In this case we should also keep a separate attribute #[Hook(..)].

    A developer only sees the dispatcher methods and the subscribed method (signature and return), and both feel different in hooks vs events, even if internally we use the event system for both.

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada Charlie ChX Negyesi ๐ŸCanada

    You are right, these are not quite events because, after all, events have event classes and these do not.

    And while using AsEventListener has the advantage of reusing a Symfony thing, truth to be told the code reuse is rather minimal here -- as I noted while the AsEventListener attribute class itself ships with symfony/event-dispatcher there's literally nothing else, it's never used for anything. Using a Hook attribute instead would have the advantage of allowing module as an attribute argument and wouldn't be a lot of code -- it seems like ~20 LoC, it's just a constructor. The registration code could rather easily autofill the module key making the runtime code even simpler (yay) but it would allow hook implementations to specify it themselves thus checking off the implement on the behalf of another module feature. Overall, it's a good idea.

    Of the features listed, well, you can't mix the order of converted and non-converted code. Otherwise, ordering is possible using priority as with events. While temporarily it'll be weird to use the opposite of weight not having two different ways for events vs hooks is IMO good. My previous comment suggested creating an implements alter magic named class for the purpose of, well, implements altering the new style ones.

    Conditionally included, sure, these are autoloaded classes.

    I will post a new patch tomorrow.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    My previous comment suggested creating an implements alter magic named class for the purpose of, well, implements altering the new style ones.

    In Hux you can put `#[Hook('something_alter')]` or `#[Alter('something')]` on any method of a hux-enabled class.
    I think this DX is preferable over a magic named class.
    (The #[AsEventListener] would also still be better than any kind of magic naming.)

    In practice, I like to name these classes after their purpose, and often have one class implement multiple hooks with related functionality, or multiple classes implement the same hook for separate functionality.

    Typical class names might be "AssignSourceId" or "CoAuthorNodeAccess" or "HideSomethingField".
    The class name does not reveal which hook(s) are used to achieve the result, and we might in fact decide to use a different hook without renaming the class.
    (Of course others might have different conventions how to name their classes)

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    You are right, these are not quite events because, after all, events have event classes and these do not.

    Now that the event dispatcher itself allows any object to be passed in rather than requiring an Event class, I think they count as events again. But as soon as we get to multiple parameters and alters etc. that's no longer true.

    A new class sounds good, we could even make 'dispatch()' available on it and just pass through.

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada Charlie ChX Negyesi ๐ŸCanada

    Special named classes are only for the "implements alter" because I am just not sure whether it's doable or desirable to fire off an event in the middle of DrupalKernel::compileContainer(). Every other alter is, of course, the same pattern as hooks. Whether we want a different attribute for those, I do not know. Maybe?

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    I am just not sure whether it's doable or desirable to fire off an event in the middle of DrupalKernel::compileContainer().

    It's not, but we have an issue open for adding before/after to events instead of priority (maybe via an attribute?). Or potentially modules could implement a compiler pass to run after hook collection and alter things in that? If we do something like that, hook_module_implements_alter() could be deprecated.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    Now that the event dispatcher itself allows any object to be passed in rather than requiring an Event class, I think they count as events again. But as soon as we get to multiple parameters and alters etc. that's no longer true either.

    So the main differences are:
    - by-reference parameters
    - multiple parameters
    - return value (and the mechanism to combine the return values)

    So... yeah mostly not events then but definitely an extension of them.

    So we could say Drupal has an extended events system that supports single-parameter classical event subscribers, but also multiple-parameter dispatching with return etc.
    So, symfony event system could be seen as a subset of the extended Drupal event system.

    Ofc the term "event" implies that there is an actual event object. Or at least this would be a typical understanding of it.
    So do we call it an "extended event system" or rather a "dispatching system"?
    (Conceptually, "event" does not necessarily mean the object, it can also just mean a thing that happens)

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany donquixote

    Special named classes are only for the "implements alter" because I am just not sure whether it's doable or desirable to fire off an event in the middle of DrupalKernel::compileContainer().

    Alright. Still not sure that magic naming is the best solution, but..

    Other question, do we want `hook_implements_alter()` to work for non-procedural implementations?
    With the current PR this would not be the case.
    This means whenever a procedural hook gets converted, any code that targets it with hook_implements_alter() would need to be converted too.

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada Charlie ChX Negyesi ๐ŸCanada
    1. We had a bit of discussion whether we want to allow multiple implementations for a module and the end result was a yes. Except for hooks that are called by invoke. We would need to document those. There are very few: mail, library_info_build, help in runtime and install/uninstall/schema/update_last_removed in install and that's about it. I am not 100% what happens to the install time hooks they might need to remain procedural? Far future problem.
    2. I am introducing a new Hook attribute here. Makes sense: these are not quite events.
    3. I moved the conversion of the module implements info into a compiler pass and then I realized this actually allows implements alter with zero added code because service provider alter can be used to do this. I added a PoC for a conversion, note "This module's implementation of form_filter_format_form_alter() must happen after theeditor module's implementation" doesn't quite make sense if we are allowing multiple implementations per module: it needs to be a specific method of a specific class, rather. While there's some vague OOP design concern here about how a module needs to know about what another does, well, implementation alter is already against another specific piece of functionality in another module so nothing new here.
    4. The hook priority defaults to negative module weight. This way all the actual ordering is delegated to the event dispatcher. Normally the module weight is at hand, needs some adjustments around module (un)install but nothing much. Might need a separate issue though.
    5. I included a real conversion of node_user_cancel, it's one of the trickier ones because it uses module handler but the example shows the power of autowire solving this.
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium kristiaanvandeneynde Antwerp, Belgium
    +++ b/core/modules/node/src/Hooks/NodeHooks.php
    @@ -0,0 +1,48 @@
    +  public function __construct(
    +    EntityTypeManagerInterface $entityTypeManager,
    +    #[AutowireServiceClosure('module_handler')]
    +    protected \Closure $moduleHandler,
    +  ) {
    +    $this->nodeStorage = $entityTypeManager->getStorage('node');
    +  }
    

    Not 100% sure about this autowiring attribute.

    I got that you're doing this so we don't have to declare all of our Drupal\MODULE\Hooks classes as services and by using autowiring this way we can still inject dependencies from the container. However, if I understood correctly, AutowireServiceClosure is intended to be used for lazy loading of services or to declare a dependency that may not be available yet when the class is instantiated. But when hooks are invoked, we should have a fuly built container, no?

    After all, you're registering the classes on the container with autowire turned on, so doesn't that mean we do not have to manually add the AutowireServiceClosure attribute? Seems like it would beat the whole purpose of autowiring if we still have to specify the attribute.

    Aside from the above, I'm loving the approach you're taking here. Nice work.

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada Charlie ChX Negyesi ๐ŸCanada

    It was only modulehandler that needed a closure in an earlier version because you got a circular dependency error but I got rid of it. As you can see entity type manager was already autowired, the next version which I intend to post in a separate issue won't need it. Thanks for making me try it again :)

Production build 0.69.0 2024