Problem/Motivation

#3485896: Hook ordering across OOP, procedural and with extra types i.e replace hook_module_implements_alter Introduces a modern way to order hooks.
This does not handle the edge case when a module inserts itself between two implementations of another module.

Currently in ModuleHandler::getHookListeners(), we are grouping implementations by module $this->invokeMap[$hook][$module][] = $callable;

Steps to reproduce

Proposed resolution

Because ::invoke() needs to find implementations by module, this structure should remain in place however to fix the map another one like $this->implementationsMap[$hook][] = ['module' => $module, 'callable' => $callable]; should be added. The check whether invoke can or can not run should be in yet another property also gathered in getHookListeneres: $this->hasMultipleImplementationsPerModuleAndHook[$hook][$module] = TRUE and if it's set at all then invoke should die as it does now. It means "invoke all" and "invoke" now relies on separate structures but oh well.

Remaining tasks

Change getHookListeners:

          if (isset($this->moduleList[$module])) {
            if (isset($this->invokeMap[$hook][$module])) {
              $this->hasMultipleImplementationsPerHookAndModule[$hook][$module] = TRUE:
            }
            $this->invokeMap[$hook][$module][] = $callable;
            $this->hookImplementations[$hook][] = [$callable, $module];
          }
...
return $this->hookImplementations[$hook];

Add

protected function getHookListenersByHookAndModule($hook, $module = NULL) {
  $this->getHookListeners($hook);
  if (isset($module)) {
    if (isset($this->hasMultipleImplementationsPerHookAndModule[$hook][$module])) {
      throw new MultipleImplementationsException;
    }
    return $this->invokeMap[$hook][$module];
  }
    if (isset($this->hasMultipleImplementationsPerHookAndModule[$hook])) {
      throw new MultipleImplementationsException;
    }
    return $this->invokeMap[$hook];
}

Change hasImplementations, invoke, triggerDeprecationError to use getHookListenersByHookAndModule instead, they should catch the exception and act appropriately -- only invoke cares. Upgrade invokeAllWith appropriately: foreach ($this->getHookListeners($hook) as [$listener, $module]) and remove the second loop. alter is fun. While the second call to getHookListeners is a loop which is trivial to upgrade like the previous one, the initial call is not a loop, it should be made into one but only when needed which will be like never:

try {
  $hook_listeners = $this->getHookListenersByModule($hook); 
}
catch (MultipleImplementationsException) {
  $hook_listeners = [];
  foreach ($this->getHookListeners($hook) as [$listener, $module]) {
    $hook_listeners[$module][] = $listener;
  }
}

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Comments

nicxvan created an issue. See original summary.

ghost of drupal past’s picture

Title: [pp-1] Explore ordering multiple hook implementations relatively to other hooks » [pp-1] Ordering a hook between two hook implementations of a single module is broken
Category: Task » Bug report
Issue summary: View changes
nicxvan’s picture

Title: [pp-1] Ordering a hook between two hook implementations of a single module is broken » Ordering a hook between two hook implementations of a single module is broken
Related issues: +#3485896: Hook ordering across OOP, procedural and with extra types i.e replace hook_module_implements_alter, +#3519564: Handle multiple implementations of hooks better
nicxvan’s picture

Component: base system » extension system

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.