Contrib.social

ModuleHandler::add() can prevent existing implementations from being added

Open on Drupal.org →
Created on 6 June 2025, 3 months ago

Problem/Motivation

The method ModuleHandler::addModule() allows to add a module, and then ModuleHandler::add() attempts to add hook implementations for that module.
The methods were deprecated in 📌 Investigate ModuleHandler::add Active , and will be removed in Drupal 12.
We can assume that currently these method are rarely used, but their usage is still possible.

We already understand some limitations:
- Only procedural implementations are added.
- The resulting combined list of implementations is not properly ordered.

However, there is a bigger problem:
Calling ->add() removes existing implementations, and also prevents regular implementations from being added later.
Also, when ->resetImplementations() is called, the new implementations are lost.

This is much worse than simply not adding any new implementations.

Steps to reproduce

We can produce a KernelTest with code like this:

$module_handler = \Drupal::service(ModuleHandlerInterface::class);
// Assume that modules 'module_a' and 'module_b' are currently installed.
// Assume that implementations of test_hook return the respective module names.
$this->assertSame(['module_a', 'module_b'], $module_handler->invokeAll('test_hook'));
$module_handler->addModule('module_c');
$this->assertSame(['module_c'], $module_handler->invokeAll('test_hook'));  // ouch!!!
$module_handler->resetImplementations();
$this->assertSame(['module_a', 'module_b'], $module_handler->invokeAll('test_hook'));  // Implementation from module_c is forgotten.

One problem is the `->resetImplementations()` called within `->add()`.
We could remove this, but then it would still be problematic, if we call ->add() before ->invokeAll():

$module_handler = \Drupal::service(ModuleHandlerInterface::class);
// Assume that modules 'module_a' and 'module_b' are currently installed.
// Assume that implementations of test_hook return the respective module names.
$this->assertSame(['module_a', 'module_b'], $module_handler->invokeAll('test_hook'));
$module_handler->addModule('module_c');
// If we remove ->resetImplementations() from within ->add(), the old implementations are not lost.
$this->assertSame(['module_a', 'module_b', 'module_c'], $module_handler->invokeAll('test_hook'));  // nice, I guess..
$module_handler->resetImplementations();
$this->assertSame(['module_a', 'module_b'], $module_handler->invokeAll('test_hook'));  // Implementation from module_c is forgotten.

and

$module_handler = \Drupal::service(ModuleHandlerInterface::class);
// Assume that modules 'module_a' and 'module_b' are currently installed.
// Assume that implementations of test_hook return the respective module names.
// Assume that ->invokeAll('test_hook') was not called yet, so the list is not initialized.
$module_handler->addModule('module_c');
$this->assertSame(['module_c'], $module_handler->invokeAll('test_hook'));  // ouch!!!
$module_handler->resetImplementations();
// Now the list can be properly initialized.
$this->assertSame(['module_a', 'module_b'], $module_handler->invokeAll('test_hook'));

Proposed resolution

The first option is to not add any new hook implementations on ->add(), because this is already defunct.

The second options is to attempt to fix the merging.
This can be easily done once we have the hook list closures in 🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active .
We can then simply decorate the existing closure on ->add().

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

🐛 Bug report
Status

Active

Version

11.0 🔥

Component

base system

Created by

🇩🇪Germany donquixote

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

Merge Requests

Comments & Activities

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024