Introduce an event to decouple the module installer from the entity manager

+1, #2561639: Wrong placeholder for exception shown when new fields are created during module installation was very strange to see entity field code directly in ModuleInstaller

Let's see how much breaks

1. +++ b/core/core.services.yml
   @@ -544,6 +544,11 @@ services:
   +    arguments: ['@entity_type.manager', '@entity_field.manager', '@entity.definition_update_manager']
   
   +++ b/core/lib/Drupal/Core/EventSubscriber/ModuleInstallerEntitySubscriber.php
   @@ -0,0 +1,121 @@
   +class ModuleInstallerEntitySubscriber implements EventSubscriberInterface {
   

   Missing a constructor. Also, from what I remember, pulling each service from the container during each loop was important when installing a module that adds new entity types.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/ModuleInstallerEntitySubscriber.php
   @@ -0,0 +1,121 @@
   +  public function onModuleUnInstall(ModuleEvent $event) {
   

   onModuleUninstall

Also, isn't there a preUninstall portion too?

So there is currently:

        $this->moduleHandler->invokeAll('module_preinstall', array($module));
        $this->moduleHandler->invoke($module, 'install');
      $this->moduleHandler->invokeAll('modules_installed', array($modules_installed));

so I guess we would have also pre, and after ideally?

There we go.

Patch still applies. Anyone interested in a review exchance?

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -212,36 +212,10 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
+        $event_dispatcher = $this->kernel->getContainer()->get('event_dispatcher');

@@ -383,31 +357,10 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
+      $event_dispatcher = $this->kernel->getContainer()->get('event_dispatcher');

$this->kernel->getContainer()?!
Why can't the event dispatcher be injected? If it's for a good reason, we should probably have a comment here so no one tries to inject it later.

Good point!

There is ModuleInstaller::updateKernel() which is supposed to refresh dependencies injected into ModuleInstaller. Therefore it should be safe to inject the event dispatcher dependency and reuse the existing mechanism to refresh its reference where appropriate.

Good point!

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -212,7 +223,8 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
-        $event_dispatcher = $this->getEventDispatcher();
...
+        $event_dispatcher = $this->kernel->getContainer()->get('event_dispatcher');
...
         $event_dispatcher->dispatch(ModuleEvents::MODULE_INSTALLED, $event);

Hmm, is there any reason for not using the injected event dispatcher? Wasn't that the whole point of the change in #14? :)

Ups, you are totally right.

+++ b/core/lib/Drupal/Core/EventSubscriber/ModuleInstallerEntitySubscriber.php
@@ -0,0 +1,137 @@
+  public function onModuleUninstall(ModuleEvent $event) {
...
+        // The module being installed may be adding new fields to existing

I know this is just moved code but the comment here is wrong, we're in the "uninstall" part of the code :)

I know this is just moved code but the comment here is wrong, we're in the "uninstall" part of the code :)

Sure, let's fix that.

+++ b/core/lib/Drupal/Core/Extension/ModuleEvent.php
@@ -0,0 +1,41 @@
+   * The installed / uninstall module.
+   *
+   * @var string
+   */
+  protected $module;
...
+   * Returns the installed / uninstall module.
+   *
+   * @return string

Should we mention that this is actually the (machine) name of the module being installed / uninstalled?

Can't find anything else to complain about so let's do this :)

Should we mention that this is actually the (machine) name of the module being installed / uninstalled?

Sure, we can do that.

+1 for RTBC, thanks @znerol for pointing out the ModuleInstaller::updateKernel() bit!

#2679547: Fix random fail in ConfigImportAllTest caused by ModuleInstaller not rebuilding routes immediately, which happens to be surfaced by BigPipe

Let's try it again.

+++ b/core/core.services.yml
@@ -479,7 +479,7 @@ services:
-    arguments: ['@app.root', '@module_handler', '@kernel', '@config.factory', '@config.installer', '@plugin.cache_clearer', '@router.builder', '@stream_wrapper_manager', '@theme_handler', '@logger.channel.system', '@config.manager', '@keyvalue']
+    arguments: ['@app.root', '@module_handler', '@kernel', '@config.factory', '@config.installer', '@plugin.cache_clearer', '@router.builder', '@stream_wrapper_manager', '@theme_handler', '@logger.channel.system', '@config.manager', '@keyvalue', '@event_dispatcher']

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -306,36 +317,8 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
-        $entity_manager = \Drupal::entityManager();
-        $update_manager = \Drupal::entityDefinitionUpdateManager();

@@ -489,31 +472,8 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
-      $update_manager = \Drupal::entityDefinitionUpdateManager();

Was wondering why we only add dependencies and not remove them, but I see we're removing \Drupal:: calls.

Are any of these other services viable to remove in this patch, or is that a follow-up?

I fixed at least one test failure in CommentLanguageTest (and likely some others as well), and improved some docblocks.

Awesome @Xano!

Nothing significant, just removing a final stray @file tag

Back to RTBC

The testbot failures are the same as those of the daily automatic testing. So back to RTBC.

My mistake: no failures with the daily automatic testing.

Patch was green already.

Added a change record in the meantime.

Reroll of #34.

RE-RTBC as of previous RTBCs

We need to refresh the event dispatcher because if a module was being installed as part of a loop and listened to this event it wouldn't have it fired. See \Drupal\Core\Extension\ModuleInstaller::updateKernel()

There was an issue somewhere to add a trait to do this.

#2380293: Properly inject services into ModuleInstaller is the other one.

+++ b/core/lib/Drupal/Core/Extension/ModuleEvents.php
@@ -0,0 +1,17 @@
+  const MODULE_UNINSTALLED = 'core.modules.unstalled';

*uninstalled ?

We need to refresh the event dispatcher...

Happy to work on this with a little more direction.

+++ b/core/lib/Drupal/Core/EventSubscriber/ModuleInstallerEntitySubscriber.php
@@ -0,0 +1,149 @@
+use Drupal\Core\Extension\ModuleEvents;

This use statement is repeated.

My mistake. Patch addressing maintainer concerns forthcoming.

Patch addresses #48 and #51

I am seeing some memory errors when I run a site install using #54 - removing the $this->eventDispatcher = $container->get('event_dispatcher'); added resolves this, so maybe something else needs to be done?

I am not certain this will work correctly with the UI-based installer, at least based on some testing we have done using this patch and the new event in the config_rewrite project. Do we currently have tests for the installer that ensure events are triggered?

How does #54 address #48? Is the interdiff incomplete?

+++ b/core/lib/Drupal/Core/EventSubscriber/ModuleInstallerEntitySubscriber.php
+++ b/core/lib/Drupal/Core/Extension/ModuleEvent.php

Why are these specific to modules — why are they not generic across all types of extensions?

I am seeing some memory errors when I run a site install using #54

The only thing I can think of is that this is somehow triggering a recursive loop, perhaps due to some PHP error that is caught/handled in a certain way. Have you already checked the PHP error log?

Why are these specific to modules — why are they not generic across all types of extensions?

Well, at least as of now a module installation is a completly separate process to theme installation, like a container rebuild just happens on a module rebuild, sure that's worth to consider though.

Reroll of #54

Testbot seems suspect. See results, need a manual review here if someone can run the tests locally.

https://www.drupal.org/pift-ci-job/600858

You can restart the test run with the 'Add test/retest' link.

Doing that now.

Thanks, Paul! It looks like its still having issues. Is there something we can do?

There is a periodic failure on some environments, but not all:

The cache entry expiry time uses the cache_ttl_4xx setting. Expire: 1485617538 Created: 1485613944.835

Otherwise, there are instances of PHP 5.6 & MySQL 5.5 CI error

I am not sure how to address either of these issues with the testbot, but it seems strange the bot failing for *any* reason would immediately mark the issue as needs work.

I will not flip it back to Needs Review, but there is nothing in the test results that tells me the patch is the source of CI errors or random cache expiry time problems.

Rerolled #54 again. #61 was missing a file. Also fixed some docs and injected logger service to the event subscriber.

I realize that we don't need tests for event subscriber so removing the tag.

Other than the discussion on #59 I don't think anything else is pending.

Do we need profiling here?

Anyone up for review.

Looks pretty good to me. Nice work, @jibran et. al!

1. +++ b/core/core.services.yml
   @@ -502,7 +502,7 @@ services:
   -    arguments: ['@app.root', '@module_handler', '@kernel', '@router.builder']
   +    arguments: ['@app.root', '@module_handler', '@kernel', '@event_dispatcher']
   

   Why was the router builder removed?

2. +++ b/core/lib/Drupal/Core/EventSubscriber/ModuleInstallerEntitySubscriber.php
   @@ -0,0 +1,159 @@
   +  /**
   +   * Gets the entity definition update manager.
   +   *
   +   * The manager is retrieved dynamically, because the container is rebuilt
   +   * after module installation.
   +   *
   +   * @return \Drupal\Core\Entity\EntityFieldManagerInterface
   +   *   The entity definition update manager.
   +   */
   +  protected function getEntityFieldManager() {
   +    return $this->kernel->getContainer()->get('entity_field.manager');
   +  }
   

   The doc comment is inaccurate :)

3. +++ b/core/lib/Drupal/Core/EventSubscriber/ModuleInstallerEntitySubscriber.php
   @@ -0,0 +1,159 @@
   +      if ($entity_type->getProvider() == $module) {
   

   Should probably use === here, and in other similar comparisons.

4. +++ b/core/lib/Drupal/Core/Extension/ModuleEvent.php
   @@ -0,0 +1,39 @@
   +  /**
   +   * Constructs a new instance.
   +   *
   +   * @param string $module
   +   *   The module's machine name.
   +   */
   +  public function __construct($module) {
   +    $this->module = $module;
   +  }
   

   Is it possible to pass the entire Extension object, rather than just the name? If not, can we rename getModule() to getModuleName(), for clarity's sake?

5. +++ b/core/lib/Drupal/Core/Extension/ModuleEvents.php
   @@ -0,0 +1,17 @@
   +  /**
   +   * This event is fired when modules are installed.
   +   */
   +  const MODULE_INSTALLED = 'core.modules.installed';
   +
   +  /**
   +   * This event is fired when modules are uninstalled.
   +   */
   +  const MODULE_UNINSTALLED = 'core.modules.uninstalled';
   

   Don't these doc comments need @Event? I've seen that elsewhere in core...

I would normally say this needs tests, but this is the sort of deep change that would break just about every single test in core if it didn't work...so honestly, I feel pretty good about it.

Re-roll.

Now to address #73...

1. I suspect route builder was removed because it is redundant, it should have been deleted as art of #2380293: Properly inject services into ModuleInstaller. Technically it is beyond the scope of this ticket, but I see no harm in removing it.
2. Corrected the comment.
3. Replaced == with ===.
4. The module object is not available, it is just the name that is being passed around, so changed the function to getModuleName().
5. Added @Event to comment.

Thanks, @Jo Fitzgerald.

This one seems extremely straightforward. The fact that the tests all pass with it solidly proves that it works, given how foundational the module installer is to the proper functioning of Drupal.

I see no reason to hold off on RTBC.

+++ b/core/lib/Drupal/Core/EventSubscriber/ModuleInstallerEntitySubscriber.php
@@ -0,0 +1,159 @@
+    $this->kernel = $kernel;
...
+   * The manager is retrieved dynamically, because the container is rebuilt
+   * after module installation.
...
+    return $this->kernel->getContainer()->get('entity.definition_update_manager');

This kinda feels wrong and problematic. If we can't do proper injection this event is going to be fraught with trickiness. For example what happens if a module has a service that is subscribed to this event. Does it fire on its own install. Does it fire on its own uninstall? There's no test coverage of these type of complexities and there needs to be.

+++ b/core/lib/Drupal/Core/EventSubscriber/ModuleInstallerEntitySubscriber.php
@@ -0,0 +1,159 @@
+  /**
+   * The application kernel to retrieve the rebuilt service container from.
+   *
+   * @var \Drupal\Core\DrupalKernelInterface
+   */
+  protected $kernel;
+

This feels fishy. I see there is code inside the module installer (at the end) to update the event subscriber, so shouldn't this allow stuff to continue working without having to go through the kernel? I could imagine people would make a lot of mistakes when they just inject services normally, instead of injecting the kernel. Did someone do the research to understand that this is the only possible option?

+++ b/core/lib/Drupal/Core/Extension/ModuleEvent.php
@@ -0,0 +1,39 @@
+<?php
+
+namespace Drupal\Core\Extension;
+
+use Symfony\Component\EventDispatcher\Event;
+
+/**
+ * Describes a module-related event.
+ */
+class ModuleEvent extends Event {
+
+  /**
+   * The module's machine name.
+   *
+   * @var string
+   */
+  protected $module;
+
+  /**
+   * Constructs a new instance.
+   *
+   * @param string $module
+   *   The module's machine name.
+   */
+  public function __construct($module) {
+    $this->module = $module;
+  }
+
+
+  /**
+   * Returns the module's machine name.
+   *
+   * @return string
+   */
+  public function getModuleName() {
+    return $this->module;
+  }
+
+}

#2206347: Use event system in ModuleHandler, which is essential a duplicate, had a ExtensionEvent. I think this would be a good idea, given all of them could share that.

#2206347: Use event system in ModuleHandler is old but still relevant here. I think we should resocpe #2206347 and postpone on this. Once the events are in we can remove the cache bin code from ModuleInstaller as well.

I'd like to see this continue somehow but I believe doing this on the extension level is the best approach? It'd be really good if install profiles, themes and modules all acted pretty much the same way and fired similar events during installation and otherwise.