Rebuild routes immediately when modules are installed

Let's start simple.

I still think we want #2572293: Race condition triggerable by a single user due to router rebuild in kernel.terminate and maybe some extension to destructable to allow similar things to happen in finish_request and not terminate.

Oh yeah this basically now requires all kernel tests to have the table in place and would slow them down a bit :(

Yeah having a properly working site is way more important than test speed :)

Yeah sure, just saying. I think one approach would be to just add it to the base class.

Agreed, either add it to be base class or introduce an in-memory (null might not work, tests possibly rely on it) router storage.

Maybe different approach - get router builder as a parameter of module installer, and use it in install()?
Patch attached

Is it just me that this issue is more like a routing issue?

I think we need to rebuild routing prior to running the install hook so that messages or anything done in there can rely on the module's routes see #2342015: Content Translation module still implements hook_enable

How to work around this is in #2663660-2: Aggregator module renders Drupal inoperative as soon as a feed is added.

I can confirm that this solves the issue of using routes in the hook_requirements (#2572459: Route "acquia_connector.setup" does not exist).

The previous test seems to have failed testing due to unrelated issues, so marking it needs review to kick tests off again.

Ran into this bug over at #2469431-216: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts, in hook_page_attachments().

This is also causing #2679547-4: Fix random fail in ConfigImportAllTest caused by ModuleInstaller not rebuilding routes immediately, which happens to be surfaced by BigPipe.

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -294,8 +301,8 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
-      \Drupal::service('router.builder')->setRebuildNeeded();
       $this->moduleHandler->invokeAll('modules_installed', array($modules_installed));
+      $this->routeBuilder->rebuild();

I think the route rebuild should happen before hook_installed so at least one hook can rely on the routing system being right.

#2679547-5: Fix random fail in ConfigImportAllTest caused by ModuleInstaller not rebuilding routes immediately, which happens to be surfaced by BigPipe got committed with a work-around until this issue is fixed. This issue should remove the changes made there.

Related? #2684189: RouteNotFoundException: Route "dblog.overview" does not exist when uninstalling the dblog module

Potentially ...

We can't inject the route builder into the module installer because the container is rebuilt during module install so we'd be using the wrong route builder if we do that.

#2380293: Properly inject services into ModuleInstaller

Hmmm patch doesn't apply to 8.0.x so we need to make this an 8.1.x-dev issue at least.

We need explicit tests for install and uninstall..

This isn't compatible alternative routing implementations - we just moved the definition of that table to the implementation too. Can't the router rebuild do that check?

This if is entirely not needed anymore now that we lazy create the table.

Expanded the documentation and removed the if.

So #31 is going to fail because of the rest module - it has an implicit dependency on the node module. Imo we need make that dependency concrete. Easiest way to see this is to run Drupal\KernelTests\Config\DefaultConfigTest with the patch in #31 applied.

The workaround would be to skip hal/rest module from the DefaultConfigTest

We could just suppress error during installation - that way we still get the default config testing but we not getting the side effect testing of the trigger error in \Drupal\rest\Routing\ResourceRoutes.

Mh, I would honestly prefer to just skip rest/hal in the test. Hiding the error hides it for every module, not just rest.

@dawehner I disagree. The point of this test is that after installing a module its active config matches the default config it provides. Using the @ symbol doesn't hide anything. All the tests that we expect to occur as part of this test occur.

I guess we could just install node if hal or rest is under test but to me skipping a module is worse than doing an @.

So the last issue we have is that people using \Drupal\Core\Url in hook_install will still have potentially really inconsistent bugs. Because if they rely on a dependencies routes it'll work if the module was installed in a previous request but if it is installed in the same request it is going to break... it will depend on how boxes are ticked on the module install page.

Discuss a bit with @dawehner on IRC. The options seems to be:

• Somehow decorate the route provider service during module install to one that rebuilds lazily.
• Call the route rebuild during the install loop before hook_install() - which will potentially result in lots of unnecessary rebuilds.

Oops... not quite the right interdiff...

While the discussion in #42 is important it is not in scope of this issue to be honest. The problem already exists ...

For #1 that'd be more or less what we used to have with Drupal 7-style rebuild needed right?

Well, but just during the module installation process. Other processes would still hit the other router table.

re #44... from the issue summary:

However this makes module enabling non-transactional (see related issues) - the state of the installed modules and their routes gets out of sync, which can lead to race conditions and fatal errors.

I think the discussion in #42 is very much in scope.

Here's a horrible implementation

Something a little nicer...

Uninstall updates the container and removes the old service so the swap is not necessary... not sure we need to use the lazy building route provider during uninstall... leaving it in for discussion.

Fixing some docs and making sure we don't do unnecessary rebuilds.

I think this is as good as it can get now. In general its though quite sad that we need to do special things for routing, as similar problems could exist for all kind of other subsystems as well.

You probably talk about #2605956: Port #2605684 to the new KernelTest but yeah alex certainly was aware of that.

re #57

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -294,7 +298,15 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
+      if (!\Drupal::service('router.route_provider.lazy_builder')->hasRebuilt()) {
+        // Rebuild routes after installing module. This is done here on top of
+        // \Drupal\Core\Routing\RouteBuilder::destruct to not run into errors on
+        // fastCGI which executes ::destruct() after the module installation
+        // page was sent already.
+        \Drupal::service('router.builder')->rebuild();
+      }

This guarantees we've got an up-to-date router table after module installation regardless of whether it has been rebuilt by the lazy_builder service. So the rebuilt router is definitely available earlier than it is in HEAD and there is no danger that it is not available on the request subsequent to installing a module.

Re the simpletest route provider - yes we can remove it and make it use this one - but I vote for doing that in (a repurposed) #2672762: Move core/modules/simpletest/src/RouteProvider.php to the Drupal\Tests namespace/folder

Tested #55 and still cleanly applies and fixes the issue. I would love to have this fix out fast. Many separate issues are created for this problem from different points of view and mostly in the issue queue of a contrib module. Some have the issue (like me: fastcgi) and others can't reproduce (mamp or so). See Google results also.

Should we start relate issues from contrib to this post, or?

I'm going to also test with a few modules (on PHP-FPM/mod_proxy_fcgi) to see if this patch fixes issues like #2701313: When installing via UI: RouteNotFoundException: Route "facets.overview" does not exist.; I've been running into this all over the place, and also get occasional bug reports from other people installing modules using the UI. Would be nice to get this fixed so the deluge of issues can stop.

+1 on getting this into 8.1.x if at all possible.

[Edit: Tested on Facets, Honeypot, and the rest of the modules I was getting the install WSOD with last Friday—+1 to the RTBC, it makes installing modules actually work.]

Thanks for the commit and the testing guys!

I'm definitely +1 to this being an rc target - it solves a whole class of problems the hard to find the cause of.

This apparently also resolved #2707433: Errors installing from config on 8.1.0-rc1.

#2792293: RouteNotFoundException after enabling the Language module

Long time ago, but there may be a memory impact/leak from this change on large sites #3356739: Memory usage increases linearly when (un)installing modules via config import