Problem/Motivation

Installing/uninstalling modules triggers container rebuilds. Mid-request container rebuilds have proven to be the source of weird bugs in the past. Isolating this functionality into a self contained, strictly stateless service will help avoiding this kind bugs in the future.

Proposed resolution

Extract install() and uninstall() methods to a separate class/service.

Remaining tasks

Review.

User interface changes

None.

API changes

  • Introduce new module_installer service (implements \Drupal\Core\Extension\ModuleInstallerInterface).
  • Relocate install() method from ModuleHandlerInterface to ModuleInstallerInterface
  • Relocate uninstall() method from ModuleHandlerInterface to ModuleInstallerInterface

Impact on core/contrib: Installation and removal of modules is not something which is commonly performed from within production code. However, developers might need to update their web-tests.

Original report by @dawehner

This seems inconsistent - modules have a handler, themes have a handler and manager. For modules the handler fires hooks and for themes its the manager. I get why this is since the alter only get's fired for the active theme - which is something the ThemeManager knows about but not the ThemeHandler. As @dawehner said in IRC - perhaps we need to refactor ModuleHandler. :(

from #2228093: Modernize theme initialization

Files: 
CommentFileSizeAuthor
#76 2324055-76.patch139.28 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,431 pass(es). View
#76 interdiff-74-76.txt717 bytescilefen
#74 2324055-74.patch138.81 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,412 pass(es), 1 fail(s), and 0 exception(s). View
#71 2324055-71.patch138.8 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,386 pass(es). View
#71 interdiff_7998.txt2.22 KBdawehner
#66 2324055-66.patch138.74 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,539 pass(es), 0 fail(s), and 1 exception(s). View
#66 interdiff.txt625 bytesdawehner
#63 2324055-63.patch138.73 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#63 interdiff.txt1.87 KBdawehner
#61 interdiff.txt2.79 KBdawehner
#61 2324055-61.patch138.57 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#59 interdiff.txt9.85 KBdawehner
#59 2324055-59.patch138.62 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#53 interdiff.txt13.99 KBdawehner
#53 2324055-53.patch140.21 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#50 cross-branch-diff.txt7.52 KBznerol
#44 2324055-44.patch137.26 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,126 pass(es). View
#44 interdiff-40-44.txt768 bytescilefen
#40 2324055-40.patch136.38 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,108 pass(es), 1 fail(s), and 0 exception(s). View
#39 interdiff.txt2.22 KBznerol
#37 split_up_the_module-2324055-37.patch136.32 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,101 pass(es), 1 fail(s), and 0 exception(s). View
#37 interdiff-36-37.txt1.2 KBcilefen
#36 split_up_the_module-2324055-36.patch136.32 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,121 pass(es), 1 fail(s), and 0 exception(s). View
#32 2324055-32.patch137.21 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,379 pass(es), 0 fail(s), and 1 exception(s). View
#32 interdiff-25-32.txt773 bytescilefen
#25 interdiff.txt611 bytesdawehner
#25 2324055-25.patch136.32 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,368 pass(es), 1 fail(s), and 1 exception(s). View
#22 interdiff.txt939 bytesdawehner
#22 2324055-22.patch136.25 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,959 pass(es). View
#20 2324055-20.patch135.33 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,919 pass(es), 0 fail(s), and 4 exception(s). View
#20 interdiff.txt9.67 KBdawehner
#15 interdiff.txt1.86 KBdawehner
#15 2324055-15.patch129.33 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,654 pass(es). View
#14 interdiff.txt3.68 KBdawehner
#14 2324055-14.patch127.47 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,698 pass(es), 0 fail(s), and 56 exception(s). View
#10 interdiff.txt3.06 KBdawehner
#10 2324055-10.patch123.79 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#7 2324055-7.patch120.74 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,041 pass(es), 125 fail(s), and 2 exception(s). View
#7 interdiff.txt3.03 KBdawehner
#4 module_installer-2324055-4.patch120.18 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Comments

sun’s picture

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

dawehner’s picture

Probably not that much, this sounds like a concrete task in contrast to the other one.

Just wanted to open a follow up on the theme init issue.

catch’s picture

dawehner’s picture

Status: Active » Needs review
FileSize
120.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Let's see how this works out (the installer is broken but beside from that ... ;) )

Status: Needs review » Needs work

The last submitted patch, 4: module_installer-2324055-4.patch, failed testing.

almaudoh’s picture

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/

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.03 KB
120.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,041 pass(es), 125 fail(s), and 2 exception(s). View

@almaudoh
Good catch.

Wim Leers’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 7: 2324055-7.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
123.79 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
3.06 KB

This could be it.

jibran’s picture

Status: Needs review » Needs work

The last submitted patch, 10: 2324055-10.patch, failed testing.

andypost’s picture

Issue tags: +Needs profiling
dawehner’s picture

Status: Needs work » Needs review
FileSize
127.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,698 pass(es), 0 fail(s), and 56 exception(s). View
3.68 KB

@andypost
... This just moves code out of the way for the normal request. Please do the profiling :p

dawehner’s picture

FileSize
129.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,654 pass(es). View
1.86 KB

More fixes.

The last submitted patch, 14: 2324055-14.patch, failed testing.

dawehner’s picture

Issue tags: -Needs profiling

Anyone wants to give some feedback?

Fabianx’s picture

Issue tags: +Needs profiling

I assume that tag got missing accidentally?

znerol’s picture

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.

dawehner’s picture

FileSize
9.67 KB
135.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,919 pass(es), 0 fail(s), and 4 exception(s). View

Thank you for the review!

Status: Needs review » Needs work

The last submitted patch, 20: 2324055-20.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
136.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,959 pass(es). View
939 bytes

Fixed it.

znerol’s picture

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.

catch’s picture

Priority: Normal » Major
dawehner’s picture

FileSize
136.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,368 pass(es), 1 fail(s), and 1 exception(s). View
611 bytes

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

I would argue we should, given that the module handler itself, should point to the new cache backend,
which will be initialized, if we update the module handler.

jibran’s picture

Status: Needs review » Needs work

The last submitted patch, 25: 2324055-25.patch, failed testing.

larowlan’s picture

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

cilefen’s picture

Raised to critical because it is holding up a critical.

cilefen’s picture

Status: Needs work » Needs review
FileSize
813 bytes
137.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,369 pass(es), 0 fail(s), and 1 exception(s). View
cilefen’s picture

FileSize
773 bytes
137.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,379 pass(es), 0 fail(s), and 1 exception(s). View

The last submitted patch, 31: 2324055-31.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 32: 2324055-32.patch, failed testing.

dawehner’s picture

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?

Please let us keep things in scope ... you know this change for itself is small and self-contained, no internal change of behaviour.

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?

cilefen’s picture

Status: Needs work » Needs review
FileSize
136.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,121 pass(es), 1 fail(s), and 0 exception(s). View

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

cilefen’s picture

FileSize
1.2 KB
136.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,101 pass(es), 1 fail(s), and 0 exception(s). View
+++ 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.

znerol’s picture

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

znerol’s picture

FileSize
2.22 KB

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.

dawehner’s picture

FileSize
136.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,108 pass(es), 1 fail(s), and 0 exception(s). View

let's apply znerol's patch.

The last submitted patch, 36: split_up_the_module-2324055-36.patch, failed testing.

The last submitted patch, 37: split_up_the_module-2324055-37.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 40: 2324055-40.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
768 bytes
137.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,126 pass(es). View
tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Read through the entire patch, looks great.

This is really an awesome decoupling.

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work

Edit: Wrong issue

tstoeckler’s picture

Status: Needs work » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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!
znerol’s picture

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.

znerol’s picture

FileSize
7.52 KB
+++ 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.

alexpott’s picture

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.

znerol’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
140.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
13.99 KB

Yeah I think that we should do the injecting later, given that especially it provides 2 okayish to review patches.

Updated the docs and introduced a DependencyReinjectTrait which can use used by both ModuleInstaller and ConfigImporter and maybe more in the future.

Status: Needs review » Needs work

The last submitted patch, 53: 2324055-53.patch, failed testing.

effulgentsia’s picture

Issue tags: +D8 upgrade path, +blocker

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

znerol’s picture

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

Crell’s picture

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.

znerol’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
138.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
9.85 KB

Yeah this is fragile and there are more reasons to do stuff than: we can inject stuff now properly.
Decided to go with the suggestion of #58.

Status: Needs review » Needs work

The last submitted patch, 59: 2324055-59.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
138.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
2.79 KB

Mh :( dawehner--

Status: Needs review » Needs work

The last submitted patch, 61: 2324055-61.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.87 KB
138.73 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

There we go, maybe ... @see #61

Status: Needs review » Needs work

The last submitted patch, 63: 2324055-63.patch, failed testing.

znerol’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
625 bytes
138.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,539 pass(es), 0 fail(s), and 1 exception(s). View

Doh!

Status: Needs review » Needs work

The last submitted patch, 66: 2324055-66.patch, failed testing.

znerol’s picture

znerol’s picture

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

Status: Needs work » Needs review

dawehner queued 66: 2324055-66.patch for re-testing.

dawehner’s picture

FileSize
2.22 KB
138.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,386 pass(es). View

Oh right, thank you @znerol!!!

The last submitted patch, 66: 2324055-66.patch, failed testing.

znerol’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
$ 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
cilefen’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
138.81 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,412 pass(es), 1 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 74: 2324055-74.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
717 bytes
139.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,431 pass(es). View
znerol’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs change record updates

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed 826245d on 8.0.x
    Issue #2324055 by dawehner, cilefen, znerol: Split up the module manager...
dawehner’s picture

Thank you alex!

Updated the CR

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.