diff --git a/core/includes/install.inc b/core/includes/install.inc index 95c9d2b83a..8e6eb21d81 100644 --- a/core/includes/install.inc +++ b/core/includes/install.inc @@ -619,9 +619,8 @@ function drupal_install_system($install_state) { ->set('profile', $install_state['parameters']['profile']) ->save(); - // Install System module and rebuild the newly available routes. + // Install System module. $kernel->getContainer()->get('module_installer')->install(['system'], FALSE); - \Drupal::service('router.builder')->rebuild(); // Ensure default language is saved. if (isset($install_state['parameters']['langcode'])) { diff --git a/core/lib/Drupal/Core/Extension/ModuleInstaller.php b/core/lib/Drupal/Core/Extension/ModuleInstaller.php index 3540a42ef0..bc2f9d0323 100644 --- a/core/lib/Drupal/Core/Extension/ModuleInstaller.php +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php @@ -7,6 +7,7 @@ use Drupal\Core\DrupalKernelInterface; use Drupal\Core\Entity\EntityStorageException; use Drupal\Core\Entity\FieldableEntityInterface; +use Drupal\Core\Installer\InstallerKernel; use Drupal\Core\Serialization\Yaml; /** @@ -209,16 +210,18 @@ public function install(array $module_list, $enable_dependencies = TRUE) { $this->moduleHandler->load($module); module_load_install($module); - // 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 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')); + if (!InstallerKernel::installationAttempted()) { + // 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 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')); + } // Allow modules to react prior to the installation of a module. $this->moduleHandler->invokeAll('module_preinstall', [$module]); @@ -331,17 +334,19 @@ public function install(array $module_list, $enable_dependencies = TRUE) { // If any modules were newly installed, invoke hook_modules_installed(). if (!empty($modules_installed)) { - // If the container was rebuilt during hook_install() it might not have - // the 'router.route_provider.old' service. - if (\Drupal::hasService('router.route_provider.old')) { - \Drupal::getContainer()->set('router.route_provider', \Drupal::service('router.route_provider.old')); - } - 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(); + if (!InstallerKernel::installationAttempted()) { + // If the container was rebuilt during hook_install() it might not have + // the 'router.route_provider.old' service. + if (\Drupal::hasService('router.route_provider.old')) { + \Drupal::getContainer()->set('router.route_provider', \Drupal::service('router.route_provider.old')); + } + 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->moduleHandler->invokeAll('modules_installed', [$modules_installed, $sync_status]); diff --git a/core/lib/Drupal/Core/Installer/NormalInstallerServiceProvider.php b/core/lib/Drupal/Core/Installer/NormalInstallerServiceProvider.php index a29b7d8a77..e203bec9b0 100644 --- a/core/lib/Drupal/Core/Installer/NormalInstallerServiceProvider.php +++ b/core/lib/Drupal/Core/Installer/NormalInstallerServiceProvider.php @@ -6,6 +6,8 @@ use Drupal\Core\DependencyInjection\ContainerBuilder; use Drupal\Core\DependencyInjection\ServiceProviderInterface; use Drupal\Core\Lock\NullLockBackend; +use Drupal\Core\Routing\RouteProviderLazyBuilder; +use Symfony\Component\DependencyInjection\Reference; /** * Service provider for the installer environment. @@ -49,6 +51,17 @@ public function register(ContainerBuilder $container) { $container->getDefinition('extension.list.module')->setClass(InstallerModuleExtensionList::class); $container->getDefinition('extension.list.theme')->setClass(InstallerThemeExtensionList::class); $container->getDefinition('extension.list.theme_engine')->setClass(InstallerThemeEngineExtensionList::class); + + // Don't register the lazy route provider in the super early installer. + if (get_called_class() === NormalInstallerServiceProvider::class) { + $lazy_route_provider = $container->register('router.route_provider.installer'); + $lazy_route_provider + ->setClass(RouteProviderLazyBuilder::class) + ->setDecoratedService('router.route_provider') + ->addArgument(new Reference('router.route_provider.installer.inner')) + ->addArgument(new Reference('router.builder')) + ->addTag('event_subscriber'); + } } } diff --git a/core/lib/Drupal/Core/Routing/RouteProviderLazyBuilder.php b/core/lib/Drupal/Core/Routing/RouteProviderLazyBuilder.php index d5a55a9a3a..bf2a11c038 100644 --- a/core/lib/Drupal/Core/Routing/RouteProviderLazyBuilder.php +++ b/core/lib/Drupal/Core/Routing/RouteProviderLazyBuilder.php @@ -2,8 +2,10 @@ namespace Drupal\Core\Routing; +use Drupal\Core\Installer\InstallerKernel; use Symfony\Component\EventDispatcher\EventSubscriberInterface; use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\Routing\Route; /** * A Route Provider front-end for all Drupal-stored routes. @@ -65,7 +67,6 @@ public function __construct(RouteProviderInterface $route_provider, RouteBuilder protected function getRouteProvider() { if (!$this->rebuilt && !$this->rebuilding) { $this->routeBuilder->rebuild(); - $this->rebuilt = TRUE; } return $this->routeProvider; } @@ -81,6 +82,12 @@ public function getRouteCollectionForRequest(Request $request) { * {@inheritdoc} */ public function getRouteByName($name) { + if (InstallerKernel::installationAttempted() && ($name === '' || $name === '')) { + // During the installer template_preprocess_page() uses the routing system + // to determine the front page. At this point building the router for this + // is unnecessary work. + return new Route('/'); + } return $this->getRouteProvider()->getRouteByName($name); } @@ -190,6 +197,7 @@ public function routerRebuilding() { */ public function routerRebuildFinished() { $this->rebuilding = FALSE; + $this->rebuilt = TRUE; } } diff --git a/core/lib/Drupal/Core/Test/PerformanceTestRecorder.php b/core/lib/Drupal/Core/Test/PerformanceTestRecorder.php new file mode 100644 index 0000000000..8be037bdf5 --- /dev/null +++ b/core/lib/Drupal/Core/Test/PerformanceTestRecorder.php @@ -0,0 +1,117 @@ +state = $state; + } + + public function getCount(string $type, string $name): int { + $count = 0; + if ($this->state) { + $record = $this->state->get('drupal.performance_test_recorder', []); + $count += $record[$type][$name] ?? 0; + } + $count += self::$record[$type][$name] ?? 0; + return $count; + } + + /** + * Records the occurrence of an event. + * + * @param string $type + * The type of event to record. + * @param string $name + * The name of the event to record. + */ + public function record(string $type, string $name): void { + if ($this->state) { + $record = $this->state->get('drupal.performance_test_recorder', []); + isset($record[$type][$name]) ? $record[$type][$name]++ : $record[$type][$name] = 1; + $this->state->set('drupal.performance_test_recorder', $record); + } + else { + isset(self::$record[$type][$name]) ? self::$record[$type][$name]++ : self::$record[$type][$name] = 1; + } + } + + /** + * Records a router rebuild. + */ + public function onRouteBuilderFinish() { + $this->record('event', RoutingEvents::FINISHED); + } + + /** + * {@inheritdoc} + */ + public static function getSubscribedEvents() { + $events = []; + $events[RoutingEvents::FINISHED][] = ['onRouteBuilderFinish', -9999999]; + return $events; + } + + /** + * Registers core.performance.test.recorder service. + * + * @param string $services_file + * Path to the services file to register the service in. + * @param bool $persistent + * Whether the recorder should be in persistent mode. The persistent mode + * records using the state service so that the recorder will work on the + * site under test when requests are made. However, if we want to measure + * something used by the state system then this will be recursive. Also in + * kernel tests using state is unnecessary. + */ + public static function registerService(string $services_file, bool $persistent): void { + + $services = Yaml::parse(file_get_contents($services_file)); + if (isset($services['services']['core.performance.test.recorder'])) { + // Once the service has been marked as persistent don't change that. + $persistent = $persistent || $services['services']['core.performance.test.recorder']['arguments'][0]; + } + $services['services']['core.performance.test.recorder'] = [ + 'class' => PerformanceTestRecorder::class, + 'arguments' => [$persistent, $persistent ? '@state' : NULL], + 'tags' => [['name' => 'event_subscriber']], + ]; + file_put_contents($services_file, Yaml::dump($services)); + } + +} diff --git a/core/modules/system/tests/modules/router_installer_test/router_installer_test.info.yml b/core/modules/system/tests/modules/router_installer_test/router_installer_test.info.yml new file mode 100644 index 0000000000..33b9ed9b3d --- /dev/null +++ b/core/modules/system/tests/modules/router_installer_test/router_installer_test.info.yml @@ -0,0 +1,5 @@ +name: 'Router installer test' +type: module +description: 'Support module for routing testing.' +package: Testing +version: VERSION diff --git a/core/modules/system/tests/modules/router_installer_test/router_installer_test.install b/core/modules/system/tests/modules/router_installer_test/router_installer_test.install new file mode 100644 index 0000000000..8988bf8b80 --- /dev/null +++ b/core/modules/system/tests/modules/router_installer_test/router_installer_test.install @@ -0,0 +1,19 @@ +set(__FUNCTION__, Url::fromRoute('router_installer_test.1')->toString()); + } +} diff --git a/core/modules/system/tests/modules/router_installer_test/router_installer_test.routing.yml b/core/modules/system/tests/modules/router_installer_test/router_installer_test.routing.yml new file mode 100644 index 0000000000..9de39ca766 --- /dev/null +++ b/core/modules/system/tests/modules/router_installer_test/router_installer_test.routing.yml @@ -0,0 +1,4 @@ +router_installer_test.1: + path: '/router_installer_test/test1' + requirements: + _access: 'TRUE' diff --git a/core/profiles/standard/standard.install b/core/profiles/standard/standard.install index 35e283da2c..990ca38d2c 100644 --- a/core/profiles/standard/standard.install +++ b/core/profiles/standard/standard.install @@ -21,10 +21,6 @@ function standard_install() { $user->roles[] = 'administrator'; $user->save(); - // We install some menu links, so we have to rebuild the router, to ensure the - // menu links are valid. - \Drupal::service('router.builder')->rebuildIfNeeded(); - // Populate the default shortcut set. $shortcut = Shortcut::create([ 'shortcut_set' => 'default', diff --git a/core/tests/Drupal/FunctionalTests/Installer/InstallerPerformanceTest.php b/core/tests/Drupal/FunctionalTests/Installer/InstallerPerformanceTest.php new file mode 100644 index 0000000000..6ea5e70062 --- /dev/null +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerPerformanceTest.php @@ -0,0 +1,46 @@ +siteDirectory . '/services.yml', FALSE); + } + + /** + * Ensures that the user page is available after installation. + */ + public function testInstaller() { + // Ensures that router is not rebuilt unnecessarily during the install. + // Currently it is built once during the install in install_finished() and + // once in \Drupal\Tests\BrowserTestBase::installDrupal() when + // \Drupal\Core\Test\FunctionalTestSetupTrait::resetAll() calls + // drupal_flush_all_caches() + $this->assertSame(2, \Drupal::service('core.performance.test.recorder')->getCount('event', RoutingEvents::FINISHED)); + } + +} diff --git a/core/tests/Drupal/FunctionalTests/Installer/InstallerRouterTest.php b/core/tests/Drupal/FunctionalTests/Installer/InstallerRouterTest.php new file mode 100644 index 0000000000..0e41e6a58c --- /dev/null +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerRouterTest.php @@ -0,0 +1,65 @@ + 'profile', + 'core_version_requirement' => '*', + 'name' => 'Router testing profile', + 'install' => [ + 'router_test', + 'router_installer_test', + ], + ]; + // File API functions are not available yet. + $path = $this->siteDirectory . '/profiles/test_profile'; + mkdir($path, 0777, TRUE); + file_put_contents("$path/test_profile.info.yml", Yaml::encode($info)); + + $settings_services_file = DRUPAL_ROOT . '/sites/default/default.services.yml'; + copy($settings_services_file, $this->siteDirectory . '/services.yml'); + PerformanceTestRecorder::registerService($this->siteDirectory . '/services.yml', TRUE); + } + + /** + * Confirms that the installation succeeded. + */ + public function testInstalled() { + $this->assertSession()->statusCodeEquals(200); + // Ensures that router is not rebuilt unnecessarily during the install. It + // is rebuilt during: + // - router_test_install() + // - router_installer_test_modules_installed() + // - install_finished() + $this->assertSame(3, \Drupal::service('core.performance.test.recorder')->getCount('event', RoutingEvents::FINISHED)); + $this->assertStringEndsWith('/core/install.php/router_installer_test/test1', \Drupal::state()->get('router_installer_test_modules_installed')); + $this->assertStringEndsWith('/core/install.php/router_test/test1', \Drupal::state()->get('router_test_install')); + } + +} diff --git a/core/tests/Drupal/FunctionalTests/Installer/InstallerTest.php b/core/tests/Drupal/FunctionalTests/Installer/InstallerTest.php index 01fb53d92b..6a3f9002d9 100644 --- a/core/tests/Drupal/FunctionalTests/Installer/InstallerTest.php +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerTest.php @@ -2,6 +2,9 @@ namespace Drupal\FunctionalTests\Installer; +use Drupal\Core\Routing\RoutingEvents; +use Drupal\Core\Test\PerformanceTestRecorder; + /** * Tests the interactive installer. * @@ -39,6 +42,8 @@ public function testInstaller() { $this->assertArrayHasKey('testing', $extensions); $this->assertEquals(1000, $extensions['testing']->weight); + // Ensures that router is not rebuilt unnecessarily during the install. + $this->assertSame(1, \Drupal::service('core.performance.test.recorder')->getCount('event', RoutingEvents::FINISHED)); } /** @@ -60,6 +65,10 @@ protected function setUpLanguage() { * {@inheritdoc} */ protected function setUpProfile() { + $settings_services_file = DRUPAL_ROOT . '/sites/default/default.services.yml'; + // Copy the testing-specific service overrides in place. + copy($settings_services_file, $this->siteDirectory . '/services.yml'); + PerformanceTestRecorder::registerService($this->siteDirectory . '/services.yml', TRUE); // Assert that the expected title is present. $this->assertEqual('Select an installation profile', $this->cssSelect('main h2')[0]->getText()); $result = $this->xpath('//span[contains(@class, :class) and contains(text(), :text)]', [':class' => 'visually-hidden', ':text' => 'Select an installation profile']); diff --git a/core/tests/Drupal/FunctionalTests/Installer/StandardInstallerTest.php b/core/tests/Drupal/FunctionalTests/Installer/StandardInstallerTest.php index 48ebb9a801..5f9ac65f84 100644 --- a/core/tests/Drupal/FunctionalTests/Installer/StandardInstallerTest.php +++ b/core/tests/Drupal/FunctionalTests/Installer/StandardInstallerTest.php @@ -20,6 +20,11 @@ class StandardInstallerTest extends ConfigAfterInstallerTestBase { public function testInstaller() { // Verify that the Standard install profile's default frontpage appears. $this->assertRaw('No front page content has been created yet.'); + // Ensure that the contact link enabled in standard_install() works as + // expected. + $this->clickLink('Contact'); + $this->assertSession()->statusCodeEquals(200); + $this->assertSession()->addressEquals('contact'); } /**