Split up the module manager into runtime information and extension information.

I'm not sure whether this is a (partial?) duplicate of this related issue?

Just a quick scan through, found one nit:

+++ b/core/modules/system/src/Form/ModulesUninstallConfirmForm.php
@@ -69,8 +69,8 @@ class ModulesUninstallConfirmForm extends ConfirmFormBase {
+  public function __construct(ModuleHandlerInterface $module_installer, KeyValueStoreExpirableInterface $key_value_expirable, ConfigManagerInterface $config_manager, EntityManagerInterface $entity_manager) {

s/ModuleHandlerInterface/ModuleInstallerInterface/

This patch looks great!

But in IRC, you mentioned this is a potential performance improvement patch. That potential comes only from moving some code into a module_installer service, out of module_handler, and hence needing to load less code, right?

While this patch is 120K, "all" it really does, is introduce that module_installer service and move ~400 LoC into it.

So unless I'm missing something, I think the performance benefit will likely not be noticeable. That being said, this does improve maintainability/understandability.

What is the difference between #2004784: Move module install/uninstall implementations into ModuleInstaller and this patch?

I assume that tag got missing accidentally?

This is great, it makes a ton of sense to extract the installer/uninstaller logic from module handler.

1. +++ b/core/core.services.yml
   @@ -288,6 +288,9 @@ services:
      module_handler:
        class: Drupal\Core\Extension\ModuleHandler
        arguments: ['%container.modules%', '@kernel', '@cache.bootstrap']
   

   I guess module_handler does not need the @kernel anymore.

2. +++ b/core/lib/Drupal/Core/Installer/Form/SiteConfigureForm.php
   @@ -8,6 +8,7 @@
    use Drupal\Core\Extension\ModuleHandlerInterface;
   

   Can be removed.

3. +++ b/core/lib/Drupal/Core/Installer/Form/SiteConfigureForm.php
   @@ -55,15 +56,15 @@ class SiteConfigureForm extends FormBase {
   -   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
   +   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_installer
       *   The module handler.
   

   ModuleInstallerInterface, The module installer.

4. +++ b/core/modules/system/src/Form/ModulesUninstallConfirmForm.php
   @@ -9,6 +9,7 @@
    use Drupal\Core\Config\ConfigManagerInterface;
    use Drupal\Core\Entity\EntityManagerInterface;
   +use Drupal\Core\Extension\ModuleInstallerInterface;
    use Drupal\Core\Form\ConfirmFormBase;
    use Drupal\Core\Form\FormStateInterface;
    use Drupal\Core\Url;
   

   Some lines further down there is a spurious use statement for ModuleHandlerInterface.

5. +++ b/core/modules/system/src/Tests/Extension/ThemeHandlerTest.php
   @@ -311,13 +311,14 @@ function testThemeInfoAlter() {
        $module_handler = $this->container->get('module_handler');
   +    $module_installer = $this->container->get('module_installer');
   

   This is dangerous. References to services need to be refreshed after a container rebuild. ModuleHandlerTest has a protected method which always fetches a new instance from the container. We should do the same here.

I think this issue is very important, raising to at least major is appropriate for the following reason: Installing/uninstalling modules triggers container rebuilds. Mid-request container rebuilds have proven to be the source of weird bugs in the past. I expect that moving this functionality to a separate service will help avoiding this kind bugs in the future.

1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -0,0 +1,438 @@
   +/**
   + * Default implementation of the module installer.
   + *
   + * It registers the module in config, install its own configuration,
   + * installs the schema, updates the Drupal kernel and more.
   + */
   

   Might be the right place to #2282371: Document the fact that it is not safe to reuse services across container rebuilds.

2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -0,0 +1,438 @@
   +    $this->cacheBackend = $container->get('cache.bootstrap');
   

   The cacheBackend property does not exist in ModuleInstaller. I'm wondering whether we should refresh the module_handler reference here though.

I don't know but this seems related #2372745: KernelTestBase ignores extensions in site-specific directories

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -0,0 +1,438 @@
+ // If any modules were newly installed, invoke hook_modules_installed().
+ if (!empty($modules_installed)) {
+ $this->moduleHandler->invokeAll('modules_installed', array($modules_installed));
+ }

Mile23 asked on irc if we fire events for modules being installed - worth adding here while we have a chance or punt to a follow up?

Raised to critical because it is holding up a critical.

oops - this one

@dawehner Ok. Here is the reroll of #22.

+++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
@@ -0,0 +1,62 @@
+   * Uninstalls a given list of disabled modules.

There is no such thing as disabled. We may as well change this.

+++ b/core/modules/taxonomy/src/Tests/VocabularyCrudTest.php
@@ -194,8 +194,8 @@ function testUninstallReinstall() {
+    $this->container->get('module_installer')->uninstall(array('taxonomy'));
+    \Drupal::service('module_installer')->install(array('taxonomy'));

Different usages were not introduced by this patch, but let's make them the same.

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -0,0 +1,438 @@
+    $this->cacheBackend = $container->get('cache.bootstrap');

The cacheBackend property does not exist on ModuleInstaller.

I think the suggestion in #23 turned out to be a bad idea, so what about continue with the patch in #22 and then maybe improve later?

Why? The old code refreshed the cache-backend for some reason I guess.

Ok, I believe I know what is causing the exception in #32. In ModuleInstaller::uninstall() the variable $entity_manager is assigned before the loop. This reference is never updated after the container rebuild. This is also true for the $schema_store variable. The attached interdiff fixes the problems for me (in the ConfigImportAllTest).

Reading through the code again it stands out that all services are always fetched from the container - except module_handler, that one is injected into the constructor and needs to be refreshed in the updateKernel method. In my opinion there is no reason to special case the module_handler, therefore I suggest to also fetch that from the container when needed.

Read through the entire patch, looks great.

This is really an awesome decoupling.

Edit: Wrong issue

1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -0,0 +1,439 @@
   +  public function __construct(ModuleHandlerInterface $module_handler, DrupalKernelInterface $kernel) {
   +    $this->moduleHandler = $module_handler;
   +    $this->kernel = $kernel;
   +  }
   ...
   +    $extension_config = \Drupal::config('core.extension');
   ...
   +        \Drupal::getContainer()->get('plugin.cache_clearer')->clearCachedDefinitions();
   +        \Drupal::service('router.builder_indicator')->setRebuildNeeded();
   ...
   +        $entity_manager = \Drupal::entityManager();
   ...
   +        $config_installer = \Drupal::service('config.installer');
   ...
   +        \Drupal::service('config.installer')->installDefaultConfig('module', $module);
   ...
   +        \Drupal::service('stream_wrapper_manager')->register();
   ...
   +        // @todo ThemeHandler cannot be injected into ModuleHandler, since that
   +        //   causes a circular service dependency.
   +        // @see https://drupal.org/node/2208429
   +        \Drupal::service('theme_handler')->refreshInfo();
   ...
   +        \Drupal::logger('system')->info('%module module installed.', array('%module' => $module));
   ...
   +    $extension_config = \Drupal::config('core.extension');
   ...
   +      $entity_manager = \Drupal::entityManager();
   ...
   +      \Drupal::service('config.manager')->uninstall('module', $module);
   ...
   +      $extension_config = \Drupal::config('core.extension');
   ...
   +      \Drupal::getContainer()->get('plugin.cache_clearer')->clearCachedDefinitions();
   +      \Drupal::service('router.builder_indicator')->setRebuildNeeded();
   ...
   +      \Drupal::service('theme_handler')->refreshInfo();
   ...
   +      \Drupal::logger('system')->info('%module module uninstalled.', array('%module' => $module));
   ...
   +      $schema_store = \Drupal::keyValue('system.schema');
   

   We should be properly injecting all the dependencies now. Also As we can see from the @todo this refactor is correct because I think this gets around the circular dependency between the module handler and the themehandler

2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -0,0 +1,439 @@
   +                  $factory = \Drupal::service($definition['factory_service']);
   

   Instead of using \Drupal::service() here I think we should get the container from the the kernel. See updateKernel() below.

3. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -0,0 +1,439 @@
   +    // After rebuilding the container we need to update the injected
   +    // dependencies.
   +    $container = $this->kernel->getContainer();
   +    $this->moduleHandler = $container->get('module_handler');
   

   This code can be improved see the ConfigImporter::reinjectMe() code.

4. Can remove use Drupal\Component\Serialization\Yaml; and use Drupal\Core\DrupalKernelInterface; from moduleHandler.

5. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -107,12 +100,11 @@ class ModuleHandler implements ModuleHandlerInterface {
       * @see \Drupal\Core\DrupalKernel
       * @see \Drupal\Core\CoreServiceProvider
       */
   -  public function __construct(array $module_list = array(), DrupalKernelInterface $kernel, CacheBackendInterface $cache_backend) {
   +  public function __construct(array $module_list = array(), CacheBackendInterface $cache_backend) {
   

   Need to update the docblock

6. ModuleHandler has all its dependencies injected - that is awesome!

Re 1,2,3: This patch is risky enough already, let's please not do this here and instead open a follow-up for cleanup.

+++ b/core/lib/Drupal/Core/Extension/ModuleInstallerInterface.php
@@ -0,0 +1,62 @@
+

Stray newline.

In order to simplify the review of actual changes to the extracted install() and uninstall() methods, the following command can be used to produce a diff with copy-detection (the low threshold is necessary because of the huge amount of code which still remains in ModuleHandler):

git diff -C40% 8.0.x core/lib/Drupal/Core/Extension

Attached is the result with irrelevant hooks removed.

This patch is risky enough already

Hmm why is the current patch risky? I think the risks of the patch are only exposed if we properly inject it. If we don't then there is not real difference between this patch and the current state of HEAD. Part of proving the patch is worthwhile is injecting everything.

Also not sure why this patch as the "needs profiling" tag since it does not make any changes that would impact performance. The issue summary does nothing to describe the change, the solution or why we should be doing this.

there is not real difference between this patch and the current state of HEAD.

I disagree. The patch changes the behavior of the install() and uninstall() functions. In HEAD, the same instance of ModuleHandler is reused to invoke the various hooks (caches get reset correctly though). With the patch, the instance is renewed after every container rebuild.

In addition the patch contains a considerable amount of changes to test-code. This is okay and necessary when performing an Extract Class refactoring. However, in general we should not touch tests when refactoring code.

Cleaning up the methods can be done in a separate, non-critical, non-api-change followup which does not need to touch any tests. That's what we should do.

Updated the issue summary. I cannot see any reason for the "Needs profiling" tag.

#2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference is currently postponed on this, so adding the "D8 upgrade path" tag to this. Also adding the "blocker" tag per the "More ways to help" section of Drupal core updates for November 8, 2014.

We have a working patch in #44, we could go back there and just include #48.4...6 (excluding 1...3 for reasons stated above).

Actual unit tests are impacted by whether something is injected or not. If not, then you're not doing a unit test.

I'm with Alex on this one. Inject needed services. If they can't be for some reason, better to know now.

@Crell please no, not in this issue, see #52 why not. Aside from the technical risk, mixing two different refactorings here would make it impossible to create a cross branch diff like #50, and that again would make it difficult to accurately review the patch.

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -0,0 +1,462 @@
+  public function __construct($root, ModuleHandlerInterface $module_handler, DrupalKernelInterface $kernel) {
+    $this->root;

Not quite an assignment...

#2376039: Undefined property ContainerAwareEventDispatcherTest::results in run-tests.sh

The exception thrown in ConfigImportAllTest is the same as in #32, apply (#39) in order to resolve it.

$ git apply 2324055-71.patch
error: patch failed: core/modules/aggregator/src/Tests/AggregatorConfigurationTest.php:54
error: core/modules/aggregator/src/Tests/AggregatorConfigurationTest.php: patch does not apply
error: patch failed: core/modules/aggregator/src/Tests/UpdateFeedItemTest.php:67
error: core/modules/aggregator/src/Tests/UpdateFeedItemTest.php: patch does not apply

Tested with the browser: Installation works, module installation/uninstallation works.

Tested with drush: drush site-install works, drush pm-enable breaks with PHP Fatal error: Call to undefined method Drupal\Core\Extension\ModuleHandler::install(). drush pm-uninstall is not affected because it still uses the procedural wrapper. Pull request on Drush: #1009.

The cross branch interdiff is the same as in #50 (hunks have slightly different offset). The changes are still valid. Also did read through the entire diff again and I think that this is ready now.

Minor (maybe fix on commit): The stray newline at the end of ModuleInstallerInterface.php is still present.

After commit, the following change record needs an update: Module/hook system functions replaced with module_handler service

Follow-up: #2380293: Properly inject services into ModuleInstaller

This issue is a critical task and is allowed per https://www.drupal.org/core/beta-changes. Committed 826245d and pushed to 8.0.x. Thanks!

Let's get https://www.drupal.org/node/1894902 updated and add a bit about 8.x to 8.x to that. Thanks.