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