ModuleHandler::resetImplementations should reset $this->invokeMap

Created on 20 March 2025, 3 months ago

Problem/Motivation

When working on https://git.drupalcode.org/project/drupal/-/merge_requests/855 I couldn't get NodeAccessGrantsCacheContextTest::testCacheContext to pass.

The reason is, we call $this->moduleHandler()->hasImplementations('node_grants')

hasImplementations calls getHookListeners which reads from the invokeMap cache if it's set. However, there's no way to force reset this cache in a kernel test. When debugging the above test, I noticed node_access_test was still in the invokeMap for node_grants even after it was uninstalled.

Steps to reproduce

See above test

Proposed resolution

$this->invokeMap = []; in resetImplementations

Remaining tasks

Agree
Review
Commit

User interface changes

None

Introduced terminology

None

API changes

None

Data model changes

None

Release notes snippet

N/A

🐛 Bug report
Status

Active

Version

11.0 🔥

Component

base system

Created by

🇦🇺Australia acbramley

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

Merge Requests

Comments & Activities

  • Issue created by @acbramley
  • Merge request !11544Clear invokeMap in resetImplementations → (Closed) created by acbramley
  • Pipeline finished with Success
    3 months ago
    Total: 1170s
    #452769
  • 🇺🇸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

  • Pipeline finished with Success
    about 2 months ago
    Total: 925s
    #468861
  • 🇺🇸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.

  • 🇬🇧United Kingdom longwave UK

    I agree no CR is required here, this is just an obscure bug fix.

    Committed 0ed12ae and pushed to 11.x. Thanks!

    Don't think this needs backporting but if there is good reason please reopen.

  • 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 with invokeMap set to empty, following the call stack of getHookListeners(), it goes to getFlatHookListeners() 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

    nicxvan changed the visibility of the branch 11.x to hidden.

  • Merge request !12027Replace modulehandler → (Open) created by nicxvan
  • 🇺🇸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.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 263s
    #487832
  • 🇬🇧United Kingdom catch

    Reverted the commit for now.

    • catch committed 09633691 on 11.x
      Revert "Issue #3514197 by acbramley, nicxvan: ModuleHandler::...
  • I touched on this in #11, but looking at it more, I don't think resetImplementations() does much on its own. If we look at ModuleHandler::add() (deprecated, I know), after the call to resetImplementations(), there's a call to HookCollectorPass::collectAllHookImplementations(), and the invokeMap property of ModuleHandler is updated after that. When a module is uninstalled, there's a call to setModuleList(), which calls resetImplementations(), but does not do anything after that as in add().

    Otherwise, just calling resetImplementations() and then hasImplementations() 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 in assertUserCacheContext
    - class prop services being reused as per above

    It'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() with Drupal::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

    Re #22 yeah I'm familiar with that issue 😁

  • 🇦🇺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. Since drupalLogin 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.

  • Pipeline finished with Success
    about 1 month ago
    Total: 2103s
    #489525
  • 🇩🇪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/diffs

    In 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.

  • 🇦🇺Australia acbramley

    Agree with #30, thanks for following up :)

  • 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 in getFlatHookListeners().

    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 installed module_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 that module_set_weight() has a called to

    setModuleList()

    . Are there tests around changing module weights and how that affects hook ordering?

  • Pipeline finished with Success
    about 1 month ago
    Total: 508s
    #489659
  • 🇩🇪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 via setModuleList) 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

    (add supporting organization)

  • 🇩🇪Germany donquixote

    We should get this merged before 11.2.0-beta1.
    It fixes what could possibly be seen as regression.

    • catch committed d5a57be0 on 11.2.x
      Issue #3514197 by donquixote, acbramley, nicxvan, godotislate, longwave...
    • catch committed 46820e24 on 11.x
      Issue #3514197 by donquixote, acbramley, nicxvan, godotislate, longwave...
  • 🇬🇧United Kingdom catch

    Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

  • Status changed to Fixed 3 days ago
Production build 0.71.5 2024