Investigating this issue, I discovered a very insidious bug in the module installer. It is exposed, under certain circumstances, by the content_translation module -- but by no means is that the only module potentially affected.

Given:

  • You don't have Language or Content Translation installed
  • You do have a module installed which provides at least one plugin manager which depends, directly or indirectly, on the url_generator service

If you attempt to install Content Translation (allowing the module system to install the Language module implicitly, since it is a dependency of Content Translation), you will get a RouteNotFoundException: Route "entity.configurable_language.collection" does not exist.

What's happening here is this:

  1. Through the plugin.cache_clearer service, the module installer instantiates all cacheable plugin managers (which is virtually all of them) and clears their caches. This means that your url_generator-using plugin manager is also being instantiated. The url_generator has been fully initialized as one of its dependencies, and it has a reference to normal, non-lazy route provider.
  2. Just before calling hook_install on the newly installed module(s), the module installer swaps in the lazy-building route provider in place of the normal one. However, the url_generator consumed by your plugin manager still has the old route provider. Because the url_generator is not a factory, that url_generator is the same URL generator as what you'd get from \Drupal::urlGenerator(). In other words, by this point, the global url_generator has a stale route provider, even though the module installer swapped the correct one into the global container.
  3. Content Translation tries to generate a link to a route that was just created as part of the same install cycle, but because the URL generator does not have the lazy route provider swapped in by the module installer, it does not find the new route, and fails with an exception.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

Status: Active » Needs review
FileSize
4.23 KB

I attach a failing test for your consideration!

Status: Needs review » Needs work

The last submitted patch, 2: 2913912-2-FAIL.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.4 KB
5.75 KB

Here's a potential fix - just swap out the route provider immediately after rebuilding the kernel and before the plugin cache clear.

It also fixes the comment that reference uninstall in the install hook.

dawehner’s picture

Nice and simple fix!

Status: Needs review » Needs work

The last submitted patch, 4: 2913912-4.patch, failed testing. View results

phenaproxima’s picture

Maybe I should have added hidden: true to the test module?

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.11 KB
6.33 KB

This will fix the broken test. @phenaproxima test modules need to be in the Testing package.

I think we need to move the test out of content_translation - because, whilst that module exposes the issue, any module using Url::fromRoute() in hook_install() where there are plugin managers that depend on the route provider will cause this.

dawehner’s picture

I think we need to move the test out of content_translation - because, whilst that module exposes the issue, any module using Url::fromRoute() in hook_install() where there are plugin managers that depend on the route provider will cause this.

+1, one less test which is bound to implementation detail.

amateescu’s picture

Status: Needs review » Needs work

Nice fix! Here's a small review :)

  1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -194,6 +194,10 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
    +        // In order to make installing transactional if anything uses routes.
    +        \Drupal::getContainer()->set('router.route_provider.old', \Drupal::service('router.route_provider'));
    +        \Drupal::getContainer()->set('router.route_provider', \Drupal::service('router.route_provider.lazy_builder'));
    
    @@ -283,10 +287,6 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
    -        // In order to make uninstalling transactional if anything uses routes.
    -        \Drupal::getContainer()->set('router.route_provider.old', \Drupal::service('router.route_provider'));
    -        \Drupal::getContainer()->set('router.route_provider', \Drupal::service('router.route_provider.lazy_builder'));
    

    Do we need to make the same change in \Drupal\Core\Extension\ModuleInstaller::uninstall() as well?

  2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    --- /dev/null
    +++ b/core/modules/content_translation/tests/modules/route_provider_install_test/route_provider_install_test.info.yml
    

    We need to move the test module outside of content_translation as well. And the name of the module is a bit confusing.. it sounds like it provides some routes but instead it just provides a plugin manager that uses the url_generator service..

  3. +++ b/core/modules/content_translation/tests/src/Functional/RouteProviderInstallTest.php
    @@ -0,0 +1,43 @@
    +class RouteProviderInstallTest extends BrowserTestBase {
    

    I take it that kernel tests do not use the module installer.. somehow?

  4. +++ b/core/modules/content_translation/tests/src/Functional/RouteProviderInstallTest.php
    @@ -0,0 +1,43 @@
    +   * Currently, this fails with a RouteNotFoundException complaining that the
    

    After the patch is committed, "currently" will lose its meaning, so I think this comment needs to be rewritten a bit :)

  5. +++ b/core/modules/content_translation/tests/src/Functional/RouteProviderInstallTest.php
    @@ -0,0 +1,43 @@
    +  public function testInstallation() {
    +    $this->container->get('module_installer')->install(['content_translation']);
    +  }
    

    Doesn't this mean that when content_translation decides to not use the route provider in its hook_install() for whatever reason, we won't be really testing anything?

Edit: Nevermind points 2 and 5, @alexpott said the same thing in #8.

alexpott’s picture

I tried to move the test into \Drupal\KernelTests\Core\Extension\ModuleInstallerTest() but this failed because it is KernelTest and there's magic in there that swaps out the route provider for a lazy building one - see \Drupal\simpletest\TestServiceProvider. Funky stuff.

alexpott’s picture

I considered #10.1 whilst making the change. What is interesting about uninstall is that we never swap router.route_provider.old but to be router.route_provider back again. I'm really not 100% certain about where we should swap it. Also the swapping feels a bit off because it happens in a possible loop. AH I SEE.. This is actually all okay because the kernel is being rebuilt and so the route provider becomes the correct one each time the kernel is rebuilt. There definitely should be more comments here :) but tldr; I think the uninstall loop is fine - I'm not even really convinced of why it is necessary to swap out for the lazy builder in the uninstall loop (apart from being a mirror image to install).

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.39 KB
6.42 KB

I've moved all the test coverage outside of the content_translation module and addressed #10. I've added commentary to the ModuleInstaller instead of the tests since I think this is where it might be the most useful.

No interdiff because of all the moves.

amateescu’s picture

  1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    --- /dev/null
    +++ b/core/modules/system/tests/modules/route_provider_install_test/route_provider_install_test.info.yml
    
    +++ b/core/modules/system/tests/modules/route_provider_install_test/route_provider_install_test.info.yml
    @@ -0,0 +1,6 @@
    +name: 'Route provider install test'
    
    +++ b/core/modules/system/tests/modules/route_provider_install_test/route_provider_install_test.services.yml
    @@ -0,0 +1,5 @@
    +  plugin.manager.route_provider_install_test:
    +    class: '\Drupal\route_provider_install_test\PluginManager'
    

    The name of the module is still confusing since it doesn't really provide any routes :)

  2. +++ b/core/tests/Drupal/FunctionalTests/Routing/RouteProviderInstallTest.php
    @@ -0,0 +1,24 @@
    + * @group content_translation
    

    Leftover content_translation stuff :)

The last submitted patch, 13: 2913912-13.test-only.patch, failed testing. View results

alexpott’s picture

thanks for the review @amateescu.
1. Well it is the route provider service during install that is causing the issues. I've renamed it lazy_route_provider_install_test because that is specifically what we are interested in. That the lazy route provider is being used.
2. Fixed.

amateescu’s picture

Ok then, ship it! :D

alexpott’s picture

Noticed a missing "r".

dawehner’s picture

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -194,6 +194,17 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
+        // Replace the route provider service with a version that will rebuild
+        // if routes used during installation. This ensures that a module's
+        // routes are available during installation. This has to occur before
+        // any services that depend on the it are instantiated otherwise those
+        // services will have the old route provider injected. Note that, since
+        // the container is rebuilt by updating the kernel, the route provider
+        // service is the regular one even though we are in a loop and might
+        // have replaced it before.
+        \Drupal::getContainer()->set('router.route_provider.old', \Drupal::service('router.route_provider'));
+        \Drupal::getContainer()->set('router.route_provider', \Drupal::service('router.route_provider.lazy_builder'));

I love this piece of documentation!

alexpott’s picture

@dawehner suggested the test could do with some assertions. Decided to assert on the results of the URL getting.

Also this is now blocking #2571235: [regression] Roles should depend on objects that are building the granted permissions so this issue is borderline critical.

dawehner’s picture

+++ b/core/tests/Drupal/FunctionalTests/Routing/LazyRouteProviderInstallTest.php
@@ -19,6 +19,8 @@ class LazyRouteProviderInstallTest extends BrowserTestBase {
+    $this->assertEquals('/admin', \Drupal::state()->get('Drupal\lazy_route_provider_install_test\PluginManager'));
+    $this->assertEquals('/router_test/test1', \Drupal::state()->get('router_test_install'));

I predict that this fails on a subdir installation

alexpott’s picture

@dawehner can predict the future.

The last submitted patch, 20: 2913912-20.patch, failed testing. View results

dawehner’s picture

+++ b/core/tests/Drupal/FunctionalTests/Routing/LazyRouteProviderInstallTest.php
@@ -19,8 +19,10 @@ class LazyRouteProviderInstallTest extends BrowserTestBase {
+    $this->assertRegExp('/\/admin$/', \Drupal::state()->get('Drupal\lazy_route_provider_install_test\PluginManager'));
+    $this->assertRegExp('/\/router_test\/test1$/', \Drupal::state()->get('router_test_install'));

You could use assertStringEndsWith, couldn't you?

alexpott’s picture

alexpott’s picture

Priority: Normal » Major

Given this is blocking #2571235: [regression] Roles should depend on objects that are building the granted permissions (which is critical) this is at least a major.

dawehner’s picture

Component: base system » extension system
Status: Needs review » Reviewed & tested by the community

The module installer is a sad place, just so much random hardcoded stuff. Its not something we can change quickly, but at least we document things a bit better with this issue, thank you!

  • catch committed 391b710 on 8.5.x
    Issue #2913912 by alexpott, phenaproxima: URL generator may have a stale...
catch’s picture

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

Fixed this on commit, appropriate for halloween though:

diff --git a/core/lib/Drupal/Core/Extension/ModuleInstaller.php b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
index 5374bd3..2fb866d 100644
--- a/core/lib/Drupal/Core/Extension/ModuleInstaller.php
+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -197,7 +197,7 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
         // Replace the route provider service with a version that will rebuild
         // if routes used during installation. This ensures that a module's
         // routes are available during installation. This has to occur before
-        // any services that depend on the it are instantiated otherwise those
+        // any services that depend on it are instantiated, otherwise those
         // services will have the old route provider injected. Note that, since
         // the container is rebuilt by updating the kernel, the route provider
         // service is the regular one even though we are in a loop and might

Committed 391b710 and pushed to 8.5.x. Thanks!

Leaving RTBC against 8.4.x for cherry-pick once 8.4.1 is out.

amateescu’s picture

@catch, don't forget the reviewers, they might be celebrating Halloween too :P

catch’s picture

Sorry got distracted by the local fix, which I then managed not to commit properly. Just reverted in 8.5.x, adding credit, when the 8.4.x freeze is over I'll re-commit to both, leaving at 8.4.x since it's valid for that branch.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 25: 2913912-25.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community

Kicking back to RTBC for cherry-pick to 8.4.x, and re-queued the patch for testing against 8.4.x. Obviously any further test runs against 8.5.x will fail, since the patch landed there.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 25: 2913912-25.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community

Nope.

  • catch committed b5e3cc5 on 8.5.x
    Revert "Issue #2913912 by alexpott, phenaproxima: URL generator may have...

  • catch committed 7f15792 on 8.5.x
    Issue #2913912 by alexpott, phenaproxima, dawehner, amateescu, catch:...

  • catch committed 39262ba on 8.4.x
    Issue #2913912 by alexpott, phenaproxima, dawehner, amateescu, catch:...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.5.x and cherry-picked to 8.4.x, thanks!

Status: Fixed » Closed (fixed)

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