- Issue created by @acbramley
- 🇺🇸United States nicxvan
Should be pretty straight forward to add a test like in the issue that brought you here.
I don't see an obvious reason not to do this.
- 🇦🇺Australia acbramley
Test up, was a bit tricky to get this working tbh
- 🇺🇸United States nicxvan
Oh I see how that was trickier to test now.
Test only fails as expected.
I don't think this needs an change record.
Seems pretty straightforward. -
longwave →
committed 0ed12ae5 on 11.x
Issue #3514197 by acbramley, nicxvan: ModuleHandler::...
-
longwave →
committed 0ed12ae5 on 11.x
Re-opening because this looks like it's breaking HEAD:
phpunit core/tests/Drupal/KernelTests/Core/Extension/ModuleHandlerTest.php --filter=testResetImplementationsClearsInvokeMap Clearing old webdriver sessions jq: error (at <stdin>:5): Cannot iterate over null (null) PHPUnit 10.5.38 by Sebastian Bergmann and contributors. Runtime: PHP 8.3.19 Configuration: /var/www/html/core/phpunit.xml F 1 / 1 (100%) Time: 00:00.381, Memory: 4.00 MB There was 1 failure: 1) Drupal\KernelTests\Core\Extension\ModuleHandlerTest::testResetImplementationsClearsInvokeMap Failed asserting that true is false. /var/www/html/core/tests/Drupal/KernelTests/Core/Extension/ModuleHandlerTest.php:52 FAILURES! Tests: 1, Assertions: 2, Failures: 1.
I think the issue with the test is that on uninstall of
module_test
, the container is rebuilt. So the$moduleHandler
variable is referencing an outdated module handler from the old container. Even withinvokeMap
set to empty, following the call stack ofgetHookListeners()
, it goes togetFlatHookListeners()
and then$this->eventDispatcher->getListeners()
. Since$this->eventDispatcher
is also referencing an outdated object from the old container, its listeners aren't refreshed, so AFAICT the list of hook implementations does not change.- 🇺🇸United States nicxvan
I created an MR with the fix @godotislate suggested in slack.
I'm not sure that this is really testing the fix now though since it's a new moduleHandler instance.To be honest I'm not sure if this fix needs a test for the new bugfix policy, this will be very hard to test, it's easy to see it's missing though.
I touched on this in #11, but looking at it more, I don't think
resetImplementations()
does much on its own. If we look atModuleHandler::add()
(deprecated, I know), after the call toresetImplementations()
, there's a call toHookCollectorPass::collectAllHookImplementations()
, and theinvokeMap
property of ModuleHandler is updated after that. When a module is uninstalled, there's a call tosetModuleList()
, which callsresetImplementations()
, but does not do anything after that as inadd()
.Otherwise, just calling
resetImplementations()
and thenhasImplementations()
subsequently just ends up pulling the implementations from the listeners list in the event dispatcher, which has not changed.I think for resetImplementations() to work on its own, 🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active needs to be done to decouple implementations from the event dispatcher, so that the list is managed directly by the module handler or another service where the list is similarly reset and rebuilt.
How much this is all necessary is also a question, because on module install/uninstall, the container is rebuilt with a new module handler and event dispatcher, so the lists should be up to date in the new service objects.
- 🇺🇸United States nicxvan
@longwave indicated we might be able to do this without tests, but the work in 🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active makes this unnecessary I think.
- 🇦🇺Australia acbramley
I'm just confused what changed between April 9 and May 3 to cause that to fail.
This issue spawned from 📌 Deprecate node_access_view_all_nodes(). Move its functionality in NodeAccessControlHandlerInterface Needs work which has a test that uninstalls a module to check a cache context that depends on the hooks being removed.
- 🇺🇸United States nicxvan
The hook order issue and preprocess issue both got in and both affected module handler.
- 🇦🇺Australia acbramley
I rebased the original issue and it still fails. I think it's because NodeAccessControlHandler uses
$this->moduleHandler()
which comes from EntityHandlerBase. After the module uninstall that access control handler still references the "old" module handler.There's also a mix of
- the access control handler being saved to a class prop in the test
- the test using$this->container->get
inassertUserCacheContext
- class prop services being reused as per aboveIt's a bit of a mess and I'm not sure what the correct fix is here.
There's been back and forth about replacing usages of
$this->container->get()
withDrupal::service()
, and/or avoiding service objects cached in test class properties in 🌱 Use \Drupal consistently in tests Needs work .- 🇦🇺Australia acbramley
Swapped that issue to use a new service instead of the access control handler and now the test passes as long as I have
\Drupal::moduleHandler()->resetImplementations()
without the changes here so we might just be able to close this. - 🇦🇺Australia acbramley
Got to the bottom of it and fixed with https://git.drupalcode.org/project/drupal/-/merge_requests/855/diffs?com...
The relevant change is:
\Drupal::currentUser()->setAccount($this->userMapping[$uid]);
So it was indeed an issue with the test using both
$this->container
and\Drupal::service
. SincedrupalLogin
uses$this->container
to set the current user, the cache context wasn't correctly picking up the new user when that was swapped to use\Drupal::service
... - 🇩🇪Germany donquixote
We need to reset more properties now that 📌 Hook ordering across OOP, procedural and with extra types Active has landed.
- 🇩🇪Germany donquixote
I pushed a solution to that old branch, but realized I should rather create a new branch after that old MR was merged and then reverted.
- Merge request !12042Resolve #3514197 "Modulehandler reset all hook properties" → (Closed) created by donquixote
- 🇩🇪Germany donquixote
At first I was thinking that test is not enough.
But it actually failed without the last change, which makes me think it is not so bad :) - 🇩🇪Germany donquixote
I am not sure if we need the change from last commit "Replace modulehandler" in 3514197-fix-moduleHandlerGet
https://git.drupalcode.org/project/drupal/-/merge_requests/12027/diffsIn fact I think it is counter-productive.
The method resetImplementations() is meant to clear the cache property within one ModuleHandler instance.
But the proposed change gets a new ModuleHandler instance after the container was rebuilt, which always has the updated lists.
That new version of the test would pass even if resetImplementations() does nothing. godotislate → changed the visibility of the branch 3514197-fix-moduleHandlerGet to hidden.
Nice fix. I totally missed accounting for entries in
$this->listenersByHook
and the fact that the listeners are filtered by the current module list event after being retrieved from the event dispatcher ingetFlatHookListeners()
.I'm still not sure that that testing hook implementations like this around module install/uninstall is meaningful, because of the container rebuild. While the test now passes for uninstall, it would not on install. If I change the test like to look like this:
/** * Tests that resetImplementations clears the invokeMap memory cache. * * @covers ::resetImplementations */ public function testResetImplementationsClearsInvokeMap(): void { $moduleHandler = \Drupal::moduleHandler(); /** @var \Drupal\Core\Extension\ModuleInstallerInterface $moduleInstaller */ $moduleInstaller = \Drupal::service('module_installer'); $moduleInstaller->install(['module_test']); $this->assertTrue($moduleHandler->hasImplementations('system_info_alter')); $moduleInstaller->uninstall(['module_test']); $this->assertFalse($moduleHandler->hasImplementations('system_info_alter')); }
The test will fail on
$this->assertTrue($moduleHandler->hasImplementations('system_info_alter'));
, because the old event dispatcher does not have hooks registered from the newly installedmodule_test
.I think for tests/code around container rebuilds, it doesn't make much sense to use service variables that point to the old container services. If anything, they're a source of a memory leak, since the references can't be garbage collected.
Not sure how else to test
resetImplementations
. I see thatmodule_set_weight()
has a called to
setModuleList()
. Are there tests around changing module weights and how that affects hook ordering?
- 🇩🇪Germany donquixote
We can fix 🐛 Regression: Empty hook implementation lists are not cached in ModuleHandler Active as part of this.
This will make the behavior more symmetric and makes it easier to write the test.I enhanced the tests to better reflect what resetImplementations does and when it is called.
I might still have some misconceptions in the comments.Not sure how else to test resetImplementations. I see that module_set_weight() has a called to setModuleList(). Are there tests around changing module weights and how that affects hook ordering?
We could use module_set_weight() in the test. But then we need two test modules with their hook implementations, and test the order.
I was originally thinking that the main use case is update page where only system module hook implementations are invoked.
But I don't see ->setModuleList() being called in that context. - 🇩🇪Germany donquixote
Found it.
So the one place where ->setModuleList() is called outside tests and install/uninstall is in the theme registry, for the update page:if ($this->kernel instanceof UpdateKernel) { $module_list = $this->moduleHandler->getModuleList(); $filter_list = array_intersect_key($module_list, ['system' => TRUE]); // Call ::build() with only the system module and then revert. $this->moduleHandler->setModuleList($filter_list); $this->build(); $this->moduleHandler->setModuleList($module_list);
Our test should replicate that.
- 🇩🇪Germany donquixote
Not sure how else to test resetImplementations. I see that module_set_weight() has a called to setModuleList(). Are there tests around changing module weights and how that affects hook ordering?
Actually this should have no effect on hook implementation order, which is determined at container build time.
There is no code in ModuleHandler that would change the order. Actually this should have no effect on hook implementation order, which is determined at container build time.
Is this a regression or an intentional change? Before, module weights would be part of what determines the order in which hook implementations run.
- 🇺🇸United States nicxvan
I think Don meant set module list would not have an effect.
The code in the update is not important to test around either. It is explicitly to prevent views from loading way to many things during update before certain registries can be cleaned or set up.
Module weight still should matter, there is an issue for alters and module weight we still need tests for. But this is not the issue.
- 🇩🇪Germany donquixote
Not sure how else to test resetImplementations. I see that module_set_weight() has a called to setModuleList(). Are there tests around changing module weights and how that affects hook ordering?
Actually this should have no effect on hook implementation order, which is determined at container build time.
Is this a regression or an intentional change? Before, module weights would be part of what determines the order in which hook implementations run.
Currently, the module weights in configuration determine the order.
If you call module_set_weight(), the new order will become active for hooks after a container rebuild.Originally, before OOP hooks via event dispatcher were introduced, it would have been possible to change the module order at runtime.
Now, it is only possible to remove modules at runtime (and thus remove the implementations), but not to change the order at runtime.I suspect the change came with the first move to OOP hooks.
But it is also possible that it came with the hook order MR.
It reveals a lack of tests, which imo should have been added before the first OOP hook MR.We had this issue but the MR was a bit complex, and not sure it would cover this case. 📌 Improve unit test coverage for ModuleHandler Active
Now I think the best is to improve coverage piecemeal.
But to really see the breakage we already may have done in the past, we need to open draft/test MRs to older versions to compare. Currently, the module weights in configuration determine the order.
If you call module_set_weight(), the new order will become active for hooks after a container rebuild.Sounds good.
Bringing it back to
resetImplementations()
, are there any uses left where it is called (either directly or viasetModuleList
) and there is not an impending container rebuild?- 🇩🇪Germany donquixote
@godotislate
Searching for ->setModuleList(, outside tests:
- In module_set_weight() it updates the configuration and sets the list in the existing ModuleHandler instance, but does not trigger a rebuild.
- In ModuleInstaller::doInstall() and ::uninstall(), it updates the list in the existing ModuleHandler instance, and we get a rebuild later (I suppose).
- In Drupal\Core\Theme\Registry::get(), if the kernel is an UpdateKernel, it temporarily sets the module list to be just the system module.Searching for ->resetImplementations(, outside tests:
- In ModuleHandler::add(), which we are going to remove.
- In ModuleHandler::setModuleList(), see above.I don't know about contrib, but it could only be some specialized module that wants to change how hooks or modules work.
So it seems that in older versions, we would immediately get new/updated hook implementations on install and uninstall with the old ModuleHandler instance from the old container.
With newer versions of Drupal this would already be problematic because even procedural implementations may call \Drupal::service() and require services that only exist in the newly built container. Actually this might work if the new container is already built and only the ModuleHandler is still the old instance.
With OOP hooks, all the services used in a hook implementation are coming from the same outdated container as the ModuleHandler itself. So we can remove implementations on uninstall but never add them on install.
Even the ModuleHandler::add() which only adds procedural implementations can only work if the new container is already present, for added procedural implementations that call Drupal::service(). My question is really whether
resetImplementations
is useful anymore, since it's likely that the list of implementations should be pulled from the new container instance.module_set_weight()
was a candidate, but you've found that the order does not apply until after a container rebuild.In Drupal\Core\Theme\Registry::get(), if the kernel is an UpdateKernel, it temporarily sets the module list to be just the system module.
So this might be the only remaining useful test case?- 🇩🇪Germany donquixote
(cross post, but still valid)
Long term we might drop the setModuleList() and resetImplementations().
Instead, we could:- Accept that module_set_weight() and install/uninstall have no effect on the old container.
- For the theme registry on update page, we could have an immutable setter ModuleHandler->withReducedModuleList(['system']), which would return a clone with the reduced module list and starting with empty hook cache properties.This would mean changing the ModuleHandlerInterface, so perhaps in 12.x.
But I think the change proposed here and the tests make sense in the spirit of preserving or restoring existing (old) functionality, and for the sake of consistency. As long as the method exists, we want it to behave as one would expect or as it used to.
- 🇩🇪Germany donquixote
So this might be the only remaining useful test case?
What I wonder is whether there is any code out there that relies on the reset in the old ModuleHandler instance after install or uninstall.
Such code would probably already be broken.. Maybe there should be a comment in
setModuleList
that hook implementations can only be reset to be limited to a subset of installed modules. For module[s] being installed, hook implementations should be retrieved from the new module handler after container rebuild.- 🇩🇪Germany donquixote
I came up with the following text:
/** * Sets an explicit list of currently active modules. * * After the module list has been replaced in this way, calls to * ->getModule(), ->getModuleList() and ->moduleExists() will behave according * to the new module list. * * Methods like ->load() and ->loadInclude() will behave according to the new * module list, except for modules that were already included. * * Hook implementations from modules not in this list are skipped on * subsequent calls to ->alter() or ->invoke*(). * * However, no new implementations will be added from modules that were not * part of the initial list passed in the constructor. Also, the order of * implementations does not change, except for alter hooks with a * multiple-value type argument. * * Restoring the original module list will restore the original hook * implementations. * * @param \Drupal\Core\Extension\Extension[] $module_list * An associative array whose keys are the names of the modules and whose * values are Extension objects. */ public function setModuleList(array $module_list = []);
But, I think the following would be more suitable, assuming we would create that new test method:
/** * Sets an explicit list of currently active modules. * * Calling this directly may not have the result one would expect, and should * be avoided outside of Drupal core. * * For more details see the respective test method. * * @param \Drupal\Core\Extension\Extension[] $module_list * An associative array whose keys are the names of the modules and whose * values are Extension objects. * * @see \Drupal\KernelTests\Core\Extension\ModuleHandlerTest::testSetModuleList() */ public function setModuleList(array $module_list = []);
Both of this sounds like follow-up material to me.
- 🇺🇸United States nicxvan
This looks great, test is more thorough and I see you managed to fix empty hook implementations not being cached at the same time!
Created 📌 Better document when to use setModuleList Active
- 🇩🇪Germany donquixote
We should get this merged before 11.2.0-beta1.
It fixes what could possibly be seen as regression. - 🇬🇧United Kingdom catch
Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!
- Status changed to Fixed
3 days ago 8:15am 2 June 2025