Problem/Motivation

Since #2913912: URL generator may have a stale route provider during module installation installing the admin_toolbar_tools module (part of https://www.drupal.org/project/admin_toolbar) is causing the route builder to throw a \RuntimeException('Recursive router rebuild detected.');. This is then caught by \Drupal\Core\EventSubscriber\MenuRouterRebuildSubscriber::menuLinksRebuild() and the error is logged but basically eaten. The recursive rebuild occurs because the lazy route provider is still injected into \Drupal\Core\Menu\LocalTaskManager which has been instantiated due to the plugin cache clearer.

Here is the corresponding bug report in the admin_toolbar module - #2926844: admin_toolbar_tools is not creating links on Appearance, Extend and People when installed.

Proposed resolution

It seems as though there are two options:

  1. Completely rebuild the container yet again before calling \Drupal::service('router.builder')->rebuild(); in the ModuleInstaller
  2. Add flag to lazy route provider to ensure it does not cause recursive rebuilds. This is the only solution that prevents recursive rebuilds if anything causes a route rebuild during a hook_install() after admin_toolbar_tools has been installed.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#16 2930715-2-16.patch4.92 KBalexpott
#16 4-16-interdiff.txt750 bytesalexpott
#15 2930715-2-15.patch39.38 KBalexpott
#15 4-15-interdiff.txt750 bytesalexpott
#4 2930715-2-4.patch4.74 KBalexpott
#4 2930715-2-4.test-only.patch2 KBalexpott
#3 2930715-2-3.patch2.74 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Issue summary: View changes
Related issues: +#2926844: admin_toolbar_tools is not creating links on Appearance, Extend and People when installed.

n/a

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Issue summary: View changes
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
2930715-2-3.patch2.74 KB

Here's a fix that works and might be good for any rebuild that occurs during a module install once admin_toolbar_tools is installed. Now to write a test.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Issue tags: -Needs tests
FileSize
2930715-2-4.test-only.patch2 KB
2930715-2-4.patch4.74 KB

Here's a test that shows the problem and proves that the fix fixes it.

The last submitted patch, 4: 2930715-2-4.test-only.patch, failed testing. View results

Status: Needs review » Needs work

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

mlncn’s picture

mlncn CreditAttribution: mlncn at Agaric for Drutopia, National Institute for Children's Health Quality commented
Status: Needs work » Reviewed & tested by the community

Pending agreement by the testbot, this resolves the admin_toolbar_tools problem we faced on fresh site installation and allows it to install correctly on its own also.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented

I discussed this issue with @dawehner - it was at his suggestion that I tried using the routing events to overcome the problem. Using events does introduce a slight brittleness since there is no way to say ensure my event goes first or last as we need here - hence the priorities of 3000 and -3000. The other alternatives are adding a method to the route builder to return the value of \Drupal\Core\Routing\RouteBuilder::$building which might be more robust but it requires an API addition to \Drupal\Core\Routing\RouteBuilderInterface.

@dawehner and I also briefly discussed having a completely different container during module install with an alternate implementation of RouteBuilder that also protects itself against recursive route rebuilding. I feel that this might cause more problems than it solves because of having alternate containers that are only active during module install is likely to lead to increased complexity of install time not less - which should be the aim.

It would be great if @dawehner also gave his +1 due to his deep understanding of both module install and the routing system.

Status: Reviewed & tested by the community » Needs work

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Needs work » Reviewed & tested by the community

DrupalCI error

Status: Reviewed & tested by the community » Needs work

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

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented
Status: Needs work » Reviewed & tested by the community

Testbot hickup

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented 
+++ b/core/lib/Drupal/Core/Routing/RouteProviderLazyBuilder.php
@@ -31,6 +32,15 @@ class RouteProviderLazyBuilder implements PreloadableRouteProviderInterface, Pag
+   *
+   * Used to prevent recursive router rebuilds during module installation.
+   *

@@ -51,7 +61,7 @@ public function __construct(RouteProviderInterface $route_provider, RouteBuilder
   protected function getRouteProvider() {
-    if (!$this->rebuilt) {
+    if (!$this->rebuilt && !$this->rebuilding) {
       $this->routeBuilder->rebuild();

It would be nice to mention on possible case where this can happen. One question which could be answered: Why does this recursive rebuild happens on install time, but not on runtime.

acbramley’s picture

acbramley CreditAttribution: acbramley at PreviousNext for Transport for NSW commented

This also fixes the same problem when enabling the workbench_moderation module.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
4-15-interdiff.txt750 bytes
2930715-2-15.patch39.38 KB

It doesn't happen on runtime because the service has the old route provider injected - not a lazy one that rebuilds on demand. The fresh route provider is only available the next request in regular runtime. Module install is unfortunately exceptionally special.

Added more docs as per #13.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
4-16-interdiff.txt750 bytes
2930715-2-16.patch4.92 KB

Oops forgot to rebase.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Thank you @alexpott for adding another round of helpful docs.

  • catch committed 0d2a45a on 8.5.x 
    Issue #2930715 by alexpott, dawehner: Recursive rebuild caused by...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed 0d2a45a and pushed to 8.5.x. Thanks!

  • catch committed 35b2990 on 8.4.x 
    Issue #2930715 by alexpott, dawehner: Recursive rebuild caused by...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.5.x-dev » 8.4.x-dev

And.. cherry-picked to 8.4.x. Via @alexpott as well as the error getting logged "Also without the patch modules like admin_toolbar_extra won't create all their links until a cache rebuild - see https://www.drupal.org/project/admin_toolbar/issues/2926844"

Status: Fixed » Closed (fixed)

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