Problem/Motivation

We hard-code a router rebuild in ModuleInstaller::install(). This was added in #2589967: Rebuild routes immediately when modules are installed to prevent the request after submission of the module install form from getting a stale router table.

There are various places where we don't immediately need to do a router rebuild:

1. When batch installing modules during the installer - we only need the router rebuild after the last module is installed, not all the interim ones, because there is no way to hit a route until you get to the end of the installer.

2. In kernel tests where we're not hitting a route even though we're installing modules.

I think we should consider moving the router rebuild to the install form submit, at the end of the UI installer etc. where it is actually needed.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3492438

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

nicxvan created an issue. See original summary.

nicxvan’s picture

nicxvan commented
Related issues: +#3416522: Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller
catch’s picture

catch
he/him
Primary language English
commented
Issue tags: +Performance
Related issues: +#2422681: Remove the automatic cron run from the installer
nicxvan’s picture

nicxvan commented

Is this a duplicate of #2572285: Module enabling and router rebuilding should be done in one transaction

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes

It's more of a revert of #2589967: Rebuild routes immediately when modules are installed I think.

See the comments in ModuleInstaller:

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();
      }
      else {
        // Rebuild the router immediately if it is marked as needing a rebuild.
        // @todo Work this through a bit more. This fixes
        //   \Drupal\Tests\standard\Functional\StandardTest::testStandard()
        //   after separately out the optional configuration install.
        \Drupal::service('router.builder')->rebuildIfNeeded();
      }

Updated the issue summary with some background.

catch’s picture

catch
he/him
Primary language English
commented
Related issues: +#3356739: Memory usage increases linearly when (un)installing modules via config import

podarok made their first commit to this issue’s fork.

podarok opened merge request !14231

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs work

The container rebuild issue is a very tricky find.

podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
commented
Status: Needs work » Needs review

Javascript webtests fails unrelated

podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
commented
Assigned: Unassigned » podarok

ConfigImportAll test failing, looks like missed a place where I need to add a line of code, on me

nicxvan’s picture

nicxvan commented

Just commenting to note a discussion in slack between catch podarok and myself: https://drupal.slack.com/archives/C4M1EV8G5/p1767705669400739?thread_ts=...

podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
commented
Assigned: podarok » Unassigned

Ready for review

svicer’s picture

svicer commented
Status: Needs review » Reviewed & tested by the community

Tested MR !14231 via YUSAOpenY distribution (https://github.com/YCloudYUSA/yusaopeny/pull/331)

Environment: Drupal 11.3.2, PHP 8.3

Tested: YUSAOpenY profile installation with small_y and standard presets.

- 871 routes registered (small_y), 801 routes (standard)
- Module enable (dblog, syslog) works correctly
- Routes respond properly after installation
- No errors in watchdog

Patch applies cleanly and routing works as expected during module installation and runtime.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Pretty sure we can remove the static with the changes here.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

nicxvan’s picture

nicxvan commented

Reading the reviews it looks like we need to remove the street since it's not used.
Add typing, and we can't remove the static since it's a different property.