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.