ModuleHandler::resetImplementations should reset $this->invokeMap

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.

Test up, was a bit tricky to get this working tbh

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.

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.

longwave → closed merge request !11544

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.

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

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.

Reverted the commit for now.

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.

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

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.

The hook order issue and preprocess issue both got in and both affected module handler.

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 .

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.

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

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

We need to reset more properties now that 📌 Hook ordering across OOP, procedural and with extra types Active has landed.

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.

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 :)

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.

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?

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.

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.

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.

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.

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?

@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?

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

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.

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.

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

(add supporting organization)

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

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

catch → closed merge request !12042