Add/remove event listeners provided by a module on module install/uninstall

Thanks for the event subscriber that I added in drupal-event-dispatcher-bug-poc-test.patch the \Drupal\Tests\system\Functional\Module\UninstallTest::testUninstallPage() fails because the new event subscriber that I added gets called after the "Module test" module is uninstalled and the thrown exception causes an HTTP 500.

Well, it is already late Friday here but I may have found a simple fix for the module. I am not asking a full review on this just yet, although anybody can provide feedback about the idea. I'll check this with a clear head on Monday :)

Well, it was too good to be true...

Well, I already had the failing POC test case when I created this issue, I hoped that I also have a fix now :)

After uninstall get the list of listeners from the old service and new service and then the remove the ones that aren't there from the old service...

Yeah, probably this is the next best option that I could try because as I can see there is no API that I could use to collect all services defined by the uninstalled module before it gets uninstalled. Registered services does not store a reference to the module that defines time. Assuming that all service ids contain the provider modules' names would be a bold guess.

Maybe now...

This patch verifies all event listeners not just those that listens on the KernelEvents::RESPONSE. I was not sure which event listeners should be validated and which not so I choose to verify all. This seemed the safest option.
If we do not want to validate all then probably we should add the KernelEvents::TERMINATE event to the list as well. Not sure what else could be triggered after the given state of the request.

Tbc

\o/

Let's handle when there is no event by type in the up to date event dispatcher anymore.

Originally I fixed the doc block of the updateKernel() method. Not sure whether I can do that as part of this issue or not.

   /**
    * Updates the kernel module list.
    *
-   * @param \Drupal\Core\Extension\Extension[] $module_filenames
+   * @param string $module_filenames
    *   The list of installed modules.
    */
-  protected function updateKernel(array $module_filenames) {
+  protected function updateKernel($module_filenames) {
     // This reboots the kernel to register the module's bundle and its services
     // in the service container. The $module_filenames argument is taken over as
     // %container.modules% parameter, which is passed to a fresh ModuleHandler
@@ -589,12 +589,14 @@
     };

Thanks for the review!

1. make sense, fixed
2. ty, fixed
3. well, this also came into my mind earlier but first I wanted to solve the task at hand :)
4. fixed
5. Well, works for me, I rewrote the test to use indicator value. My usual problem with the State service that even it could be used as shared storage between $this->container and \Drupal::service() in browser tests, it has a static cache where values can stick and they can cause false-positive test failures

Updated the test only patch.

Updated patch with the test and fix.

Additional comment regarding the 3, point. I almost submitted the patch when the core/modules/system/tests/src/Functional/Module/InstallUninstallTest.php had started fail. It was caused by this error:

Symfony\Component\Routing\Exception\RouteNotFoundException: Route "entity.configurable_language.collection" does not exist. in Drupal\Core\Routing\RouteProvider->getRouteByName() (line 201 of core/lib/Drupal/Core/Routing/RouteProvider.php).
Drupal\Core\Routing\UrlGenerator->getRoute('entity.configurable_language.collection') (Line: 271)
Drupal\Core\Routing\UrlGenerator->generateFromRoute('entity.configurable_language.collection', Array, Array, 1) (Line: 105)
Drupal\Core\Render\MetadataBubblingUrlGenerator->generateFromRoute('entity.configurable_language.collection', Array, Array, ) (Line: 753)
Drupal\Core\Url->toString() (Line: 29)
content_translation_install()
call_user_func_array('content_translation_install', Array) (Line: 392)
Drupal\Core\Extension\ModuleHandler->invoke('content_translation', 'install') (Line: 315)

Took some time while I figured out what went wrong, but it easier if I show the diff between the submitted and the non-submitted patch.

What is the main difference? All those additional tasks that are running before new events get registered. The failure with my original changes was caused by this code because it run after events got registered:

        \Drupal::getContainer()->set('router.route_provider.old', \Drupal::service('router.route_provider'));
        \Drupal::getContainer()->set('router.route_provider', \Drupal::service('router.route_provider.lazy_builder')); 

But after I fixed this there was a new failure caused by the unregistered entitites and entity view modules.

So I think the best way is the add the event registration to the end of the install process. Also applied the same logic on the uninstall process, although that has not failed as it was implemented before. Open for ideas what should be the best place to call the event register/unregister methods if not the current ones.

One thing I've tried to do is to make the install and uninstall methods kind of mirror opposites of each other.

Yes, this totally makes sense. How about this?

      // Clean up all entity bundles (including fields) of every entity type
      // provided by the module that is being uninstalled.
      // @todo Clean this up in https://www.drupal.org/node/2350111.
      $entity_manager = \Drupal::entityManager();
      $entity_type_bundle_info = \Drupal::service('entity_type.bundle.info');
      foreach ($entity_manager->getDefinitions() as $entity_type_id => $entity_type) {
        if ($entity_type->getProvider() == $module) {
          foreach (array_keys($entity_type_bundle_info->getBundleInfo($entity_type_id)) as $bundle) {
            \Drupal::service('entity_bundle.listener')->onBundleDelete($bundle, $entity_type_id);
          }
        }
      }

      // Remove event listeners from the initial event dispatcher that
      // belonged to the removed module.
      $this->removeEventListenersAfterModuleUninstall();

      // Allow modules to react prior to the uninstallation of a module.
      $this->moduleHandler->invokeAll('module_preuninstall', [$module]);

      // Uninstall the module.
      module_load_install($module);
      $this->moduleHandler->invoke($module, 'uninstall');

      // Remove all configuration belonging to the module.
      \Drupal::service('config.manager')->uninstall('module', $module);

Let's use the work remove rather than unregister. Using the event dispatcher interface as inspiration. Same with the docs and variable names in the method.

Also replaced "register" with "add" based on the same logic.

Meh, of course, it failed. Either the test case needs to be adjusted or the event listener removal must run after kernel update. I rather prefer the second option which the updated patch implements because I guess until the kernel has not updated a removed event listener, which is a service, still can be called as a service.

One possible way to adjust the test is to set an indicator value in module_test_uninstall() and use that in the ModuleInstallUninstallResponseSubscriber to set the other indicator value.

Additional info from our module's test suite. Tests with #18 passed but with #23 failed because the dynamic route subscriber tried to rebuild module related routes when some services provided by the module has been already uninstalled.

So probably the best place to do the event subscriber removal is somewhere in-between #18 and #23.

Well, this is getting worse and worse...

Managed to figure out why our test has failed. Even if this should ensure uninstalled entity types get _completely_ unregistered from the system but they do not. At least in this service, in the injected entity type manager's _static_ cache they still remain registered. The EntityDefinitionUpdateManager is unable to clear the static cache of a service which is running in a different "scope".

I tried out several ways to resolve this problem. Tried to inject the EntityDefinitionUpdateManager to the ModuleInstaller and use that in uninstall() instead of \Drupal::entityDefinitionUpdateManager(), etc. I also tried to run \Drupal::entityTypeManager()->clearCachedDefinitions(); before and after the updateKernel() method call in the uninstall() but it only worked this place where I left in this patch.

I am starting to feel like that it would be better to completely rewrite this service instead of trying to patch newly appearing issues. There are too many TODOs in this class already.

I guess this new issue with incomplete entity type deregistration would require a new POC test. Should it be handled in a new issue or not? It is also related to event dispatching in ModuleInstaler so it also makes sense to resolve this here.

What do you think about all of this @alexpott?

Tried to write a test case for the described issue in #24 #25, but I had no luck until this time. This made me think that maybe I was actually right in #23 and the event deregistration should happen in a different place in the uninstall() method. At least right now I do not see a better explanation for why our internal tests passed with #19 but failed with #23 without the change in #25.

Additional info to #23. I can move $this->removeEventListenersAfterModuleUninstall(); below in the uninstall() method until it is above the \Drupal::getContainer()->get('plugin.cache_clearer')->clearCachedDefinitions(); call our functional test passes. If I move it after the \Drupal::getContainer()->get('plugin.cache_clearer')->clearCachedDefinitions(); call then our test starts failing without #25 because the plugin.cache_cleaner clears some kind of caches, probably the entity_type.manager service's cache which is tagged with plugin_manager_cache_clear.

Next combination of change that seems to be working and somehow it still makes more sense than the fix in #25 although I am still trying to understand why this is working. Basically by moving the plugin.cache_cleaner call below the updateKernel() method call the EntityRouteProviderSubscriber still calls the uninstalled entity type's dynamic route provider, but the cache service defined by uninstalled module is still registered therefore the test does not fail like with #23: https://www.drupal.org/files/issues/2019-05-27/screeshot-2019-05-27-16-5....

No rule of three here, but the code is pretty much identical except for the addListener/removeListener calls

Well, I was thinking about this too and I am still not sure whether it makes sense to extract this diff creation to a method where the method that it gets called (addListener/removeListener) would be also a parameter or an anonymous function. Maybe with this refactoring, we would just replace a small copy-paste like-ish code to something that causes bigger complexity and it is not easier to maintain.

Also sharing a conversation between me and @catch from Slack here that relates to #32

I tried to modify Drupal core test and non-test modules to reproduce this issue, but tests always passed with #23. The last thing that I can think about that the container building handles core and contrib modules differently... but I haven't invested time to this bold theory.

catch [1 day ago]
@mxr576 there shouldn't be any difference between core and contrib modules fwiw, the only issue maybe is the order in which they get discovered.
@mxr576 having said that I can't see a particular reason to reject clearing the plugin manager cache there, although there's always the question of whether there's a previous reason it has to be there, it might be worth a git blame on the file to see if it switched places before.

So I did some digging:
* The latest commit that changed the \Drupal::getContainer()->get('plugin.cache_clearer')->clearCachedDefinitions(); line was this commit: https://github.com/drupal/core/commit/c6fb130bd0b0d272cbf16e95579181f1dd... which is related to this still open issue: https://www.drupal.org/node/2380293
* This is the previous commit https://github.com/drupal/core/commit/4646c059ea07ca013a25648e2e6048ffb1... but it is also related to the same issue

Also added the TODO item to the patch.

Bumped version, although this should be backported to 8.7.x.

Thanks for the review @catch, I fixed what you suggested.

Added [PP-1] Fix leaky and brittle container serialization solution as it describes and tries to resolve the issue with _serviceId.