Follow-up to #2324055: Split up the module manager into runtime information and extension information.

Problem/Motivation

The ModuleInstaller is littered with calls to \Drupal::service.

Proposed resolution

Properly inject services and ensure that instances are reinjected after every container rebuild.

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue priority Major because dependency management in low level system components allows us to discover system fragilities that are otherwise superhard to find.
Prioritized changes This is prioritised because it is a followup from #2324055: Split up the module manager into runtime information and extension information.
Disruption Not disruptive since the module_installer is a new service and injecting its dependencies should have no impact on calling code.
CommentFileSizeAuthor
#129 2380293-REVERT2.patch17.85 KBmpdonadio
#125 2380293-REVERT.patch17.69 KBmpdonadio
#113 interdiff.txt1.61 KBdawehner
#113 2380293-112.patch17.74 KBdawehner
#108 interdiff.txt1.87 KBdawehner
#108 2380293-106.patch19.36 KBdawehner
interdiff.txt1.87 KBdawehner
2380293-106.patch19.36 KBdawehner
#99 interdiff-2380293-99.txt2.91 KBdamiankloip
#99 2380293-99.patch19.45 KBdamiankloip
#96 properly_inject-2380293-96.patch19.22 KBwillzyx
#90 properly_inject-2380293-90.patch19.23 KBcilefen
#86 interdiff-84-86.txt9 KBwillzyx
#86 properly_inject-2380293-86.patch18.89 KBwillzyx
#84 properly_inject-2380293-84.patch24.55 KBwillzyx
#72 properly_inject-2380293-72.patch21.25 KBwillzyx
#71 properly_inject-2380293-71.patch21.29 KBwillzyx
#63 interdiff-61-63.txt2.85 KBwillzyx
#63 properly_inject-2380293-63.patch21.24 KBwillzyx
#61 interdiff-55-61.txt369 byteswillzyx
#61 properly_inject-2380293-61.patch21.14 KBwillzyx
#55 interdiff-53-55.txt1.15 KBwillzyx
#55 properly_inject-2380293-55.patch21.39 KBwillzyx
#53 interdiff-49-53.txt1.62 KBmpdonadio
#53 properly_inject-2380293-53.patch21.74 KBmpdonadio
#49 properly_inject-2380293-49.patch22.66 KBwillzyx
#49 interdiff-44-49.txt4.67 KBwillzyx
#44 interdiff-39-44.txt1.02 KBmpdonadio
#44 properly_inject-2380293-44.patch22.42 KBmpdonadio
#40 properly_inject-2380293-39.patch22.43 KBmpdonadio
#33 2380293-33.patch21.83 KBdawehner
#31 2380293-31.patch21.57 KBdawehner
#29 2380293.29.patch20.71 KBalexpott
#29 27-29-interdiff.txt1.24 KBalexpott
#27 2380293.27.patch20.57 KBalexpott
#27 25-27-interdiff.txt3.13 KBalexpott
#25 2380293-25.patch18.43 KBdawehner
#25 interdiff.txt1.77 KBdawehner
#23 2380293-23.patch19 KBjibran
#23 interdiff.txt1.86 KBjibran
#21 2380293-21.patch19 KBjibran
#21 interdiff.txt347 bytesjibran
#18 2380293-17.patch19 KBjibran
#17 2380293-17.patch0 bytesjibran
#17 interdiff.txt2.64 KBjibran
#15 2380293-15.patch18.26 KBdawehner
#15 interdiff.txt884 bytesdawehner
#13 2380293-13.patch17.7 KBdawehner
#13 interdiff.txt957 bytesdawehner
#10 2380293-10.patch17.39 KBdawehner
#10 interdiff.txt683 bytesdawehner
#7 2380293-7.patch16.73 KBdawehner
#7 interdiff.txt4.34 KBdawehner
#5 2380293-5.patch15.85 KBdawehner
#5 interdiff.txt4.27 KBdawehner
#3 2380293-3.patch14.93 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

znerol’s picture

Title: Split up the module manager into runtime information and extension information. » Properly inject services into ModuleInstaller
alexpott’s picture

Issue summary: View changes
Priority: Normal » Major
Status: Postponed » Active
dawehner’s picture

Status: Active » Needs review
FileSize
14.93 KB

Let's see whether this works ...

Status: Needs review » Needs work

The last submitted patch, 3: 2380293-3.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.27 KB
15.85 KB

There we go.

Berdir’s picture

Looks great.

  1. +++ b/core/core.services.yml
    @@ -224,6 +224,9 @@ services:
    +  logger.channel.system:
    +    parent: logger.channel_base
    +    arguments: ['system']
    

    Having a logger named 'system' in core.services.yml is a bit weird...

    Not sure if we should change it here, but there's nothing that mandates that channel must be a module name. Something like modules or so would make more sense to me.

  2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -458,6 +555,16 @@ protected function updateKernel($module_filenames) {
         // dependencies.
         $container = $this->kernel->getContainer();
         $this->moduleHandler = $container->get('module_handler');
    +    $this->configFactory = $container->get('config.factory');
    +    $this->configInstaller = $container->get('config.installer');
    +    $this->pluginCacheClearer = $container->get('plugin.cache_clearer');
    +    $this->routeRebuildIndicator = $container->get('router.builder_indicator');
    +    $this->entityManager = $container->get('entity.manager');
    +    $this->streamWrapperManager = $container->get('stream_wrapper_manager');
    +    $this->themeHandler = $container->get('theme_handler');
    +    $this->logger = $container->get('logger.channel.system');
    +    $this->configManager = $container->get('config.manager');
    +    $this->keyValueFactory = $container->get('keyvalue');
    

    Long list.. wondering if we want to use a pattern similar to DependencySerializationTrait, loop over properties and look for _serviceId and then update?

dawehner’s picture

FileSize
4.34 KB
16.73 KB

Long list.. wondering if we want to use a pattern similar to DependencySerializationTrait, loop over properties and look for _serviceId and then update?

Tried that in #2324055-53: Split up the module manager into runtime information and extension information. already, let's see whether this works better this time.

Status: Needs review » Needs work

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

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
683 bytes
17.39 KB

Maybe this works?

Status: Needs review » Needs work

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

Berdir’s picture

I think the kernel doesn't actually change?

But the issue about improving Container::get() would fix that, one way or another, so we should get back to that :)

dawehner’s picture

Status: Needs work » Needs review
FileSize
957 bytes
17.7 KB

Rerolled #7 and read the failure.

Status: Needs review » Needs work

The last submitted patch, 13: 2380293-13.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
884 bytes
18.26 KB

doh!

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

This looks great to me.

We discussed synthetic services in #2307869: Remove Drupal's Container::get(), we can consider to implement setting the _serviceId there in set(), as we've discussed already. Or keep it like this.

jibran’s picture

FileSize
2.64 KB
0 bytes

I find some minor issues. Uploaded the patch with the fixes.

  1. +++ b/core/lib/Drupal/Core/DependencyInjection/DependencyReinjectionTrait.php
    @@ -0,0 +1,31 @@
    +namespace Drupal\Core\DependencyInjection;
    +use Symfony\Component\DependencyInjection\ContainerInterface;
    

    space missing.

  2. +++ b/core/lib/Drupal/Core/DependencyInjection/DependencyReinjectionTrait.php
    @@ -0,0 +1,31 @@
    +trait DependencyReinjectionTrait {
    

    Re-injection is a proper word so I think the trait name should be DependencyReInjectionTrait.

  3. +++ b/core/lib/Drupal/Core/DependencyInjection/DependencyReinjectionTrait.php
    @@ -0,0 +1,31 @@
    +   * Updates all external dependencies and refetch it from the container.
    

    re-fetch

  4. @@ -562,7 +562,7 @@ protected function removeCacheBins($module) {
                   // be extremely rare.
                   if ($tag['name'] == 'cache.bin' && isset($definition['factory_service']) && isset($definition['factory_method']) && !empty($defini
                     try {
    -                  $factory = \Drupal::service($definition['factory_service']);
    +                  $factory = $this->kernel->getContainer()->get($definition['factory_service']);
                       if (method_exists($factory, $definition['factory_method'])) {
                         $backend = call_user_func_array(array($factory, $definition['factory_method']), $definition['arguments']);
                         if ($backend instanceof CacheBackendInterface) {
    
  5. Also can we do this?

jibran’s picture

FileSize
19 KB

With patch.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: 2380293-17.patch, failed testing.

Berdir’s picture

You didn't rename the file. Also, reinjection is a word: http://www.merriam-webster.com/medical/reinjection

jibran’s picture

Status: Needs work » Needs review
FileSize
347 bytes
19 KB

Thanks for pointing that out renamed the file.

Also, reinjection is a word: http://www.merriam-webster.com/medical/reinjection

medicinal word not English word :D

dawehner’s picture

jibran’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.86 KB
19 KB

Let's not bikeshed here. Reverted back to Reinjection. RTBC as per #16.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/DependencyInjection/DependencyReinjectionTrait.php
    @@ -0,0 +1,32 @@
    +trait DependencyReinjectionTrait {
    ...
    +  protected function updateDependencies(ContainerInterface $container) {
    

    How about UpdateDependenciesTrait? Since that is the name of the method it provides. It is in the dependency injection namespace so that is probably enough about injection.

  2. +++ b/core/lib/Drupal/Core/DependencyInjection/DependencyReinjectionTrait.php
    @@ -0,0 +1,32 @@
    +   * Updates all external dependencies and re-fetch it from the container.
    

    it => them

  3. +++ b/core/lib/Drupal/Core/DependencyInjection/DependencyReinjectionTrait.php
    @@ -0,0 +1,32 @@
    +/**
    + * Provides a trait which automatically updates dependencies from the container.
    + */
    

    This should have more docs about when to use this and when not to.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.77 KB
18.43 KB

Agreed, your name is better. Addressed the feedback

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for fixing the feedback. The interdiff doesn't include renaming :P but anyways RTBC

+++ b/core/lib/Drupal/Core/DependencyInjection/UpdateDependenciesTrait.php
@@ -0,0 +1,35 @@
+ * Contains \Drupal\Core\DependencyInjection\DependencyReinjectionTrait.

minor issue can be fixed on commit.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.13 KB
20.57 KB
  • Added a unit test
  • Improved the documentation
  • And mirrored the handling of containers found in DependencySerializationTrait
dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/DependencyInjection/UpdateDependenciesTrait.php
    @@ -0,0 +1,45 @@
    +      // Special case the container, which might not have a service ID.
    +      elseif ($value instanceof ContainerInterface) {
    +        $this->$key = $container;
    +      }
    

    Doesn't the kernel and class loader both don't have it either? Would be worth to quickly research.

  2. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/UpdateDependenciesTraitTest.php
    @@ -0,0 +1,46 @@
    +  }
    +}
    

    newline :P

alexpott’s picture

FileSize
1.24 KB
20.71 KB

re #28.1

    protected function getHttpMiddleware_KernelPreHandleService()
    {
        return $this->services['http_middleware.kernel_pre_handle'] = new \Drupal\Core\StackMiddleware\KernelPreHandle($this->get('http_kernel.basic'), $this->get('kernel'));
    }

The kernel comes through get() so it will get a _serviceId if your service depends on it. Same if your service depends on the class_loader.

I think more problematic are container parameters. Add some documentation about what to do then. So the question now is - it is feasible for app.root to change during a module install. I don't think so because I can't see anyway a module could successfully swap out the kernel.

fixed #28.2 - is this actually a standard?

dawehner’s picture

I think more problematic are container parameters. Add some documentation about what to do then. So the question now is - it is feasible for app.root to change during a module install. I don't think so because I can't see anyway a module could successfully swap out the kernel.

Checked it, we also have the property set on the class loader ...

fixed #28.2 - is this actually a standard?

Take that: https://www.drupal.org/node/608152 :P

dawehner’s picture

FileSize
21.57 KB

Just a reroll.

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
21.83 KB

Interesting ... so yeah we don't support lazy classes yet actually.

dawehner queued 33: 2380293-33.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 33: 2380293-33.patch, failed testing.

Status: Needs work » Needs review

dawehner queued 33: 2380293-33.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 33: 2380293-33.patch, failed testing.

dawehner’s picture

Issue tags: +Needs reroll

Anyone wants to reroll this one?

AjitS’s picture

Assigned: Unassigned » AjitS

Me ;-)

mpdonadio’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
22.43 KB

Applied patch to 8e0bcfef0870e916eb9b8b07ad35f26e98bad486 and rebased against a fresh pull of 8.0.x. Six merge conflicts in ModuleInstaller, but I think this is good.

AjitS’s picture

Assigned: AjitS » Unassigned

Saw the tweet a little late ;-)

Status: Needs review » Needs work

The last submitted patch, 40: properly_inject-2380293-39.patch, failed testing.

dawehner’s picture

'The service "module_installer" has a dependency on a non-existent service "router.builder_indicator".' i

Yeah, we need the router.builder service, or somehow named like that, now.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
22.42 KB
1.02 KB

Let's try this.

Status: Needs review » Needs work

The last submitted patch, 44: properly_inject-2380293-44.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review

Got a

FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].

Retesting.

Status: Needs review » Needs work

The last submitted patch, 44: properly_inject-2380293-44.patch, failed testing.

willzyx’s picture

Status: Needs work » Needs review
FileSize
4.67 KB
22.66 KB

Status: Needs review » Needs work

The last submitted patch, 49: properly_inject-2380293-49.patch, failed testing.

mpdonadio’s picture

#2090115: Don't install a module when its default configuration has unmet dependencies changed \Drupal\Core\Config\ConfigInstaller::findPreExistingConfiguration() from a public method to a protected one, and removed it from the interface. According to that, we need to refactor to use ::checkConfigurationToInstall() instead, but we are already calling that a few lines up?

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

I am waiting on a TestBot run to finish, but I took a look at #2090115: Don't install a module when its default configuration has unmet dependencies, and think the reroll has some extra hunks in it that can come out. If that passes, I will post it here.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs work » Needs review
FileSize
21.74 KB
1.62 KB

Looking at #2090115: Don't install a module when its default configuration has unmet dependencies, it looks like these two hunks snuck back in. Still have failures. When I have time, I'll look at the patch in #3 side by side with this one to see if anything else pops out.

Status: Needs review » Needs work

The last submitted patch, 53: properly_inject-2380293-53.patch, failed testing.

willzyx’s picture

the code in the patch is the exact equivalent (with injected services) of the code of HEAD but the tests still fail. Code looks good the problem is not in it.. Seems we have some problems with lazy-loaded services (router.builder and config.installer) and that the services proxy does not work properly

jibran’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 55: properly_inject-2380293-55.patch, failed testing.

willzyx’s picture

Now that #2408371: Proxies of module interfaces don't work is in, see if anything changes

The last submitted patch, 55: properly_inject-2380293-55.patch, failed testing.

willzyx’s picture

Status: Needs work » Needs review
FileSize
21.14 KB
369 bytes

it should pass. After #2408371: Proxies of module interfaces don't work it should work properly

Status: Needs review » Needs work

The last submitted patch, 61: properly_inject-2380293-61.patch, failed testing.

willzyx’s picture

#61 is a good example of why we should avoid faulty dependency on concrete classes and use interfaces :/

dawehner’s picture

#61 is a good example of why should avoid faulty dependency on concrete classes and use interfaces :/

OH yeah, we just recently introduced that interface ...

willzyx’s picture

finally the patch is green.. is this ready to go?

znerol’s picture

It looks like ConfigImporter could use UpdateDependenciesTrait and then replace reInjectMe() with updateDependencies().

Status: Needs review » Needs work

The last submitted patch, 63: properly_inject-2380293-63.patch, failed testing.

dawehner’s picture

Issue tags: +Needs reroll

.

willzyx’s picture

willzyx’s picture

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This patch provides some progress, let's do it.

plach’s picture

Status: Reviewed & tested by the community » Needs work

The entity manager was not injected on purpose to avoid unnecessary API changes when doing #2350111: Introduce an event to decouple the module installer from the entity manager.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

@plach
I'm sorry but constructors aren't APIs. If you think they are, things are wrong.

dawehner’s picture

Mh, maybe I just don't get your point.

plach’s picture

Call it API or not, but when doing #2350111: Introduce an event to decouple the module installer from the entity manager we will have to remove that parameter again from the constructor, which is an unnecessary change.

dawehner’s picture

Call it API or not, but when doing #2350111: Introduce an event to decouple the module handler from the entity manager we will have to remove that parameter again from the constructor, which is an unnecessary change.

Well, we don't HAVE to right, if we care about the removal at that point?

dawehner’s picture

It would be great if we can make babysteps and actual progress.

plach’s picture

Feel free to proceed here, but I really don't see the point of injecting the entity manager.

dawehner’s picture

Well, I totally agree with you that the design of that class is wrong. It just does too much. But you know, better inject and be honest about it, than to lie.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 72: properly_inject-2380293-72.patch, failed testing.

mpdonadio’s picture

Issue tags: +Needs reroll

This needs a reroll, I think b/c #2542748: Automatic entity updates can fail when there is existing content, leaving the site's schema in an unpredictable state, but I didn't feel comfortable with how to handle the conflict in ModuleInstaller().

willzyx’s picture

Status: Needs work » Needs review
FileSize
24.55 KB

Rerolled and injected also the entity.definition_update_manager service. I can't provide interdiff.txt because interdiff crashes when I try to produce an interdiff file for this patch, sorry.

Status: Needs review » Needs work

The last submitted patch, 84: properly_inject-2380293-84.patch, failed testing.

willzyx’s picture

Status: Needs work » Needs review
FileSize
18.89 KB
9 KB

both of the failures are related to out of memory (see log). Probably avoid to inject entity.manager and entity.definition_update_manager services (like in HEAD) can limit the memory usage (and also should help with #2350111: Introduce an event to decouple the module installer from the entity manager)

Status: Needs review » Needs work

The last submitted patch, 86: properly_inject-2380293-86.patch, failed testing.

Nikolay Shapovalov’s picture

Issue tags: -Needs reroll
almaudoh’s picture

Version: 8.0.x-dev » 8.1.x-dev
cilefen’s picture

Status: Needs work » Needs review
FileSize
19.23 KB

This is a simple reroll against 8.1.x. Note that there are still \Drupal::'s in ModuleInstaller that need refactoring. This class has a large list of dependencies in its current form.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I really like how this is looking right now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 90: properly_inject-2380293-90.patch, failed testing.

dawehner’s picture

Issue tags: +Needs reroll

Let's reroll it against 8.2.x. I think at this time, it should be part of 8.2.x only

mpdonadio’s picture

For anyone looking at tackling this, it isn't a straightforward rebase/merge. There are a decent amount of weird conflicts in ModuleInstaller, but I didn't start to dig into which issues mucked this up.

willzyx’s picture

Status: Needs work » Needs review
FileSize
19.22 KB

rerolled

mpdonadio’s picture

Issue tags: -Needs reroll

Compared the two patches, and this looks like a good reroll.

dawehner’s picture

It is great to see this patch moving forward.

  1. +++ b/core/lib/Drupal/Core/DependencyInjection/UpdateDependenciesTrait.php
    @@ -0,0 +1,48 @@
    +
    +/**
    + * @file
    + * Contains \Drupal\Core\DependencyInjection\UpdateDependenciesTrait.
    + */
    
    +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/UpdateDependenciesTraitTest.php
    @@ -0,0 +1,47 @@
    +
    +/**
    + * @file
    + * Contains \Drupal\Core\DependencyInjection\UpdateDependenciesTraitTest.
    + */
    +
    

    Let's drop that quickly ...

  2. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/UpdateDependenciesTraitTest.php
    @@ -0,0 +1,47 @@
    +    $container = $this->getMock('Symfony\Component\DependencyInjection\ContainerInterface');
    +    $container->expects($this->any())
    +      ->method('get')
    +      ->with('test_service')
    +      ->willReturn($service);
    

    For some reason we never mock the container, but just create a new instance of it. This makes it easier to read IMHO

  3. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/UpdateDependenciesTraitTest.php
    @@ -0,0 +1,47 @@
    +    $trait = $this->getMockForTrait('Drupal\Core\DependencyInjection\UpdateDependenciesTrait');
    +    $trait->dependency = clone $service;
    +    $trait->container = clone $container;
    +    $object = new \StdClass();
    +    $trait->object = $object;
    +
    +    $update_dependencies_method = new \ReflectionMethod($trait, 'updateDependencies');
    +    $update_dependencies_method->setAccessible(TRUE);
    

    IMHO we should provide a concrete class using that trait, which provides a public method we can call. It feels more realistic than a mock.

damiankloip’s picture

Let's give dawehner what he wants.

damiankloip’s picture

+++ b/core/core.services.yml
@@ -476,6 +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.modules', '@config.manager', '@keyvalue']

As a side note, the amount of dependencies here is absurd. A true indication that we should pursue the issue to split the module handler.

dawehner’s picture

Thank you damian!

As a side note, the amount of dependencies here is absurd. A true indication that we should pursue the issue to split the module handler.

Well, this is the module installer only now ... but yeah this is why we for example want to have a module installation event to not have to deal with all those things directly in there.
In general though its just about being honest.

+++ b/core/lib/Drupal/Core/DependencyInjection/UpdateDependenciesTrait.php
@@ -0,0 +1,48 @@
+/**
+ * @file
+ * Contains \Drupal\Core\DependencyInjection\UpdateDependenciesTrait.
+ */

Can be dropped on commit

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

.

damiankloip’s picture

dawehner’s picture

Exactly!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record
+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -287,7 +382,7 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
-        \Drupal::logger('system')->info('%module module installed.', array('%module' => $module));
+        $this->logger->info('%module module installed.', array('%module' => $module));

I think we need a CR for this change of where stuff is being logged. The log channel is changing.

Also need to fix a couple of minor coding standards things...

FILE: .../lib/Drupal/Core/DependencyInjection/UpdateDependenciesTrait.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 3 | ERROR | [x] Namespaced classes, interfaces and traits should not
   |       |     begin with a file doc comment
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 24ms; Memory: 3.75Mb

PHPCS: core/lib/Drupal/Core/Extension/ModuleInstaller.php passed
PHPCS: core/modules/system/src/Tests/Module/InstallUninstallTest.php passed

FILE: ...l/Tests/Core/DependencyInjection/UpdateDependenciesTraitTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 59 | ERROR | [x] The closing brace for the class must have an empty
    |       |     line before it
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 31ms; Memory: 3.75Mb
dawehner’s picture

Also need to fix a couple of minor coding standards things...

Nice!

We agreed that we should reintroduce the system one and not use a new one. Added a follow up on #2721289: Potentially rename the system logging channel to core to deal with that.

alexpott’s picture

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

Status: Needs work » Reviewed & tested by the community
FileSize
19.36 KB
1.87 KB

The last submitted patch, 2380293-106.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 108: 2380293-106.patch, failed testing.

alexpott’s picture

+++ b/core/modules/system/src/Tests/Module/InstallUninstallTest.php
@@ -133,7 +133,7 @@ public function testInstallUninstall() {
-        $this->assertLogMessage('system', "%module module installed.", array('%module' => $module_to_install), RfcLogLevel::INFO);
+        $this->assertLogMessage('modules', "%module module installed.", array('%module' => $module_to_install), RfcLogLevel::INFO);

@@ -255,7 +255,7 @@ protected function assertSuccessfulUninstall($module, $package = 'Core') {
-    $this->assertLogMessage('system', "%module module uninstalled.", array('%module' => $module), RfcLogLevel::INFO);
+    $this->assertLogMessage('modules', "%module module uninstalled.", array('%module' => $module), RfcLogLevel::INFO);

Needs to revert this change.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
17.74 KB
1.35 KB

Passes locally.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
17.74 KB
1.61 KB

Thank you @mpdonadio!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5efda3c and pushed to 8.2.x. Thanks!

  • alexpott committed 5efda3c on 8.2.x
    Issue #2380293 by dawehner, willzyx, jibran, mpdonadio, alexpott,...
neclimdul’s picture

+++ b/core/core.services.yml
@@ -476,6 +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']
     tags:
       - { name: service_collector, tag: 'module_install.uninstall_validator', call: addUninstallValidator }
     arguments: ['@app.root', '@module_handler', '@kernel', '@router.builder']

Woops... double declaring the arguments. Broke PECL YAML, follow up. #2721741: Fix double argument declaration in core.service.yml

neclimdul’s picture

Status: Fixed » Active

So I slept on it and I can't think of a way to undo the changes in this patch that are now blocking a major performance bug. #2531564: Fix leaky and brittle container serialization solution

So since this is a task and that is a bug one option is reverting this issue. However there is clearly a lot of awesome work here and I'm definitely one for injecting requirements so I'm open for discussions on how we can resolve this.

dawehner’s picture

neclimdul’s picture

Personally the idea that a class cares _how_ it got its dependencies injected is terrifying.

tstoeckler’s picture

Not sure if this is related to #117 / #188 but this seems to have majorly regressed the memory usage when installing via Drush.

I ran drush --verbose --debug si -y before and after this commit and before the commit I have 52.55 MB and after I have 166.26 MB. I ran both twice with the same results.

Hat tip to @lokapujya for finding this!

cilefen’s picture

What about in a UI install?

neclimdul’s picture

The route builder, config manager, key value factory and uninstall validators(surprise!) are only used in uninstall so they probably weren't being created before. That's a surprising amount of memory for those but that would be my guess as to the memory issue.

Since Drush just calls install_drupal() the same as the UI installer, if we bisect the memory regression to this commit it almost certainly behaves the same.

cilefen’s picture

Oh, some dependencies are not lazy, so they and all their dependencies and so on, are built.

alexpott’s picture

Can someone roll a revert of this patch. I think we should revert and continue in this issue.

mpdonadio’s picture

Status: Active » Needs review
FileSize
17.69 KB
$ git revert 5efda3cf636bb6a0bc2c46129a08702af11c304c
$ git rm core/tests/Drupal/Tests/Core/DependencyInjection/UpdateDependenciesTraitTest.php
$ git revert --continue
$ git diff origin/8.2.x > 2380293-REVERT.patch

Status: Needs review » Needs work

The last submitted patch, 125: 2380293-REVERT.patch, failed testing.

mpdonadio’s picture

Confused. I did a `git apply --reverse 2380293-112.patch` w/ some hand edits to resolve conflicts, and ended up with the exact same patch to back this out.

Mile23’s picture

The reversion probably doesn't deal with the change to the services file due to duplicate entries in a later commit.

That said: I think this should stick around a little while, and we can fix things in the dependencies that need fixing. If 8.2.x weren't dev, then we'd need to revert.

Also, there are still 20 occurrences of \Drupal:: in ModuleInstaller.

Also: \Drupal is an antipattern. :-)

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
17.85 KB

Think the followup is the problem.

$ git revert 8df66f2 
$ git revert 5efda3c
$ rm core/tests/Drupal/Tests/Core/DependencyInjection/UpdateDependenciesTraitTest.php
$ git revert --continue
$ git diff origin/8.2.x > 2380293-REVERT2.patch
mpdonadio’s picture

  1. +++ b/core/core.services.yml
    @@ -479,9 +476,9 @@ services:
    +    arguments: ['@app.root', '@module_handler', '@kernel', '@router.builder']
         lazy: true
    

    Let's inject the router builder.

  2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -128,41 +54,14 @@ class ModuleInstaller implements ModuleInstallerInterface {
    +  public function __construct($root, ModuleHandlerInterface $module_handler, DrupalKernelInterface $kernel) {
    

    We don't need the router builder!

This is present up through d7b430a.

dawehner’s picture

I totally agree that this patch should be reverted.

For me the next two parallel steps are:

  1. Figure out where this leak was coming from
  2. Do #2389193: Provide a way to determine whether a given instance was retrieved from the container as it gives us a more sane way to reinject dependencies.
alexpott’s picture

Status: Needs review » Needs work

Reverted 2c22ebb and pushed to 8.2.x. Thanks!

  • alexpott committed 2c22ebb on 8.2.x
    Issue #2380293 revert by dawehner, willzyx, mpdonadio, jibran, alexpott...
neclimdul’s picture

re: #130, ha! I guess it probably isn't the router.builder that's the leak then unless the container is smart enough to not instantiate unused parameters. ;)

re: #131 After 1, or possibly as part of it, we could inject the container and manage the "injection" of the uninstall services internally. That would allow it to us the same logic to "reinject" services during the the uninstalls. It is still an anti-pattern but it can probably leverage most of the work done here and gets everything in the right place for further refactors.

Another possibility, though its really hard to grok with the complexity of the uninstall method, would be to have a specific "uninstaller" object that we could repeatedly instantiate and inject rebuilt services into. Then we make ModuleInstaller::uninstall() only do some precalculations and instantiate/run those objects.

I think at some level we'll have to accept a level of anti-pattern here because loops preserve state and we are looping over global state changes. We have to do something to work around that and I don't think we can get caught trying to make this perfect.

tstoeckler’s picture

Mile23’s picture

  • alexpott committed 5efda3c on 8.3.x
    Issue #2380293 by dawehner, willzyx, jibran, mpdonadio, alexpott,...
  • alexpott committed 2c22ebb on 8.3.x
    Issue #2380293 revert by dawehner, willzyx, mpdonadio, jibran, alexpott...

  • alexpott committed 5efda3c on 8.3.x
    Issue #2380293 by dawehner, willzyx, jibran, mpdonadio, alexpott,...
  • alexpott committed 2c22ebb on 8.3.x
    Issue #2380293 revert by dawehner, willzyx, mpdonadio, jibran, alexpott...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Mile23’s picture

This issue could happen in pieces here: #2729597: [meta] Replace \Drupal with injected services where appropriate in core

ModuleInstaller has calls to \Drupal::logger() and \Drupal::keyValue() and others that could be replaced one at a time along with all other usages.

  • alexpott committed 5efda3c on 8.4.x
    Issue #2380293 by dawehner, willzyx, jibran, mpdonadio, alexpott,...
  • alexpott committed 2c22ebb on 8.4.x
    Issue #2380293 revert by dawehner, willzyx, mpdonadio, jibran, alexpott...

  • alexpott committed 5efda3c on 8.4.x
    Issue #2380293 by dawehner, willzyx, jibran, mpdonadio, alexpott,...
  • alexpott committed 2c22ebb on 8.4.x
    Issue #2380293 revert by dawehner, willzyx, mpdonadio, jibran, alexpott...

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jibran’s picture

dawehner’s picture

We can try. I believe the fundamental challenges might still apply :)

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

markhalliwell’s picture

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mxr576’s picture

FYI, I added this issue as related to #3056604 because in my patch I had to change when \Drupal::getContainer()->get('plugin.cache_clearer')->clearCachedDefinitions(); gets called when a module is uninstalled.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

alexpott’s picture

Note that injecting a new service into the module installer caused a memory leak - see #3126003: Increased memory usage during install

alexpott’s picture

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

larowlan’s picture

Closed related issue as a duplicate - applying credit

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.