From e0d5786975a8fcfc50860e88464131e6a97cb700 Mon Sep 17 00:00:00 2001
From: catch <catch@35733.no-reply.drupal.org>
Date: Tue, 21 Jul 2020 12:38:45 +0100
Subject: [PATCH] Issue #3086307 by alexpott, shaal, tstoeckler, mglaman,
Berdir, andypost, chr.fritsch, borisson_: Improve installer performance by
~20% by rebuilding the router after the entire installation is complete
rather than after each module
---
core/includes/install.inc | 3 +-
.../Drupal/Core/Extension/ModuleInstaller.php | 47 +++----
.../InstallerRouteProviderLazyBuilder.php | 26 ++++
.../NormalInstallerServiceProvider.php | 12 ++
.../Core/Routing/RouteProviderLazyBuilder.php | 2 +-
.../Core/Test/PerformanceTestRecorder.php | 117 ++++++++++++++++++
.../router_installer_test.info.yml | 5 +
.../router_installer_test.install | 19 +++
.../router_installer_test.routing.yml | 4 +
core/profiles/standard/standard.install | 4 -
.../Installer/InstallerPerformanceTest.php | 46 +++++++
.../Installer/InstallerRouterTest.php | 65 ++++++++++
.../Installer/InstallerTest.php | 9 ++
.../Installer/StandardInstallerTest.php | 5 +
14 files changed, 336 insertions(+), 28 deletions(-)
create mode 100644 core/lib/Drupal/Core/Installer/InstallerRouteProviderLazyBuilder.php
create mode 100644 core/lib/Drupal/Core/Test/PerformanceTestRecorder.php
create mode 100644 core/modules/system/tests/modules/router_installer_test/router_installer_test.info.yml
create mode 100644 core/modules/system/tests/modules/router_installer_test/router_installer_test.install
create mode 100644 core/modules/system/tests/modules/router_installer_test/router_installer_test.routing.yml
create mode 100644 core/tests/Drupal/FunctionalTests/Installer/InstallerPerformanceTest.php
create mode 100644 core/tests/Drupal/FunctionalTests/Installer/InstallerRouterTest.php
diff --git a/core/includes/install.inc b/core/includes/install.inc
index 95c9d2b83ab3..8e6eb21d81ad 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 3540a42ef0d0..bc2f9d032360 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/InstallerRouteProviderLazyBuilder.php b/core/lib/Drupal/Core/Installer/InstallerRouteProviderLazyBuilder.php
new file mode 100644
index 000000000000..252220aa73b7
--- /dev/null
+++ b/core/lib/Drupal/Core/Installer/InstallerRouteProviderLazyBuilder.php
@@ -0,0 +1,26 @@
+<?php
+
+namespace Drupal\Core\Installer;
+
+use Drupal\Core\Routing\RouteProviderLazyBuilder;
+use Symfony\Component\Routing\Route;
+
+/**
+ * A Route Provider front-end for use during the installer.
+ */
+class InstallerRouteProviderLazyBuilder extends RouteProviderLazyBuilder {
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getRouteByName($name) {
+ if ($name === '<none>' || $name === '<front>') {
+ // 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 parent::getRouteByName($name);
+ }
+
+}
diff --git a/core/lib/Drupal/Core/Installer/NormalInstallerServiceProvider.php b/core/lib/Drupal/Core/Installer/NormalInstallerServiceProvider.php
index a29b7d8a7758..a4a5b1457201 100644
--- a/core/lib/Drupal/Core/Installer/NormalInstallerServiceProvider.php
+++ b/core/lib/Drupal/Core/Installer/NormalInstallerServiceProvider.php
@@ -6,6 +6,7 @@
use Drupal\Core\DependencyInjection\ContainerBuilder;
use Drupal\Core\DependencyInjection\ServiceProviderInterface;
use Drupal\Core\Lock\NullLockBackend;
+use Symfony\Component\DependencyInjection\Reference;
/**
* Service provider for the installer environment.
@@ -49,6 +50,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(InstallerRouteProviderLazyBuilder::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 d5a55a9a3a5c..0b3f9fd10969 100644
--- a/core/lib/Drupal/Core/Routing/RouteProviderLazyBuilder.php
+++ b/core/lib/Drupal/Core/Routing/RouteProviderLazyBuilder.php
@@ -65,7 +65,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;
}
@@ -190,6 +189,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 000000000000..8be037bdf530
--- /dev/null
+++ b/core/lib/Drupal/Core/Test/PerformanceTestRecorder.php
@@ -0,0 +1,117 @@
+<?php
+
+namespace Drupal\Core\Test;
+
+use Drupal\Core\Routing\RoutingEvents;
+use Drupal\Core\State\StateInterface;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Symfony\Component\Yaml\Yaml;
+
+/**
+ * Records the number of times specific events occur.
+ *
+ * @see \Drupal\Core\Test\PerformanceTestRecorder::registerService()
+ */
+class PerformanceTestRecorder implements EventSubscriberInterface {
+
+ /**
+ * The state service for persistent storage if necessary.
+ *
+ * @var \Drupal\Core\State\StateInterface
+ */
+ protected $service;
+
+ /**
+ * @var array
+ */
+ protected static $record = [];
+
+ /**
+ * PerformanceTestRecorder constructor.
+ *
+ * @param bool $persistent
+ * Whether to save the record to state.
+ * @param \Drupal\Core\State\StateInterface|null $state
+ * (optional) The state service for persistent storage. Required if
+ * $persistent is TRUE.
+ */
+ public function __construct(bool $persistent, ?StateInterface $state) {
+ if ($persistent && !$state) {
+ throw new \InvalidArgumentException('If $persistent is TRUE then $state must be set');
+ }
+ $this->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 000000000000..33b9ed9b3d20
--- /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 000000000000..8988bf8b80ea
--- /dev/null
+++ b/core/modules/system/tests/modules/router_installer_test/router_installer_test.install
@@ -0,0 +1,19 @@
+<?php
+
+/**
+ * @file
+ * Install, update and uninstall functions for the router_installer_test module.
+ */
+
+use Drupal\Core\Url;
+
+/**
+ * Implements hook_modules_installed().
+ */
+function router_installer_test_modules_installed($modules) {
+ if (in_array('router_installer_test', $modules, TRUE)) {
+ // Ensure a URL can be generated for routes provided by the module during
+ // installation.
+ \Drupal::state()->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 000000000000..9de39ca76655
--- /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 35e283da2c73..990ca38d2c8a 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 000000000000..6ea5e7006229
--- /dev/null
+++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerPerformanceTest.php
@@ -0,0 +1,46 @@
+<?php
+
+namespace Drupal\FunctionalTests\Installer;
+
+use Drupal\Core\Routing\RoutingEvents;
+use Drupal\Core\Test\PerformanceTestRecorder;
+use Drupal\Tests\BrowserTestBase;
+
+/**
+ * Tests the interactive installer.
+ *
+ * @group Installer
+ */
+class InstallerPerformanceTest extends BrowserTestBase {
+
+ /**
+ * {@inheritdoc}
+ */
+ protected $defaultTheme = 'stark';
+
+ /**
+ * {@inheritdoc}
+ */
+ protected $profile = 'testing';
+
+ /**
+ * {@inheritdoc}
+ */
+ protected function prepareSettings() {
+ parent::prepareSettings();
+ PerformanceTestRecorder::registerService($this->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 000000000000..0e41e6a58c89
--- /dev/null
+++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerRouterTest.php
@@ -0,0 +1,65 @@
+<?php
+
+namespace Drupal\FunctionalTests\Installer;
+
+use Drupal\Core\Routing\RoutingEvents;
+use Drupal\Core\Serialization\Yaml;
+use Drupal\Core\Test\PerformanceTestRecorder;
+
+/**
+ * Tests router rebuilding during installation.
+ *
+ * @group Installer
+ */
+class InstallerRouterTest extends InstallerTestBase {
+
+ /**
+ * {@inheritdoc}
+ */
+ protected $profile = 'test_profile';
+
+ /**
+ * {@inheritdoc}
+ */
+ protected $defaultTheme = 'stark';
+
+ /**
+ * {@inheritdoc}
+ */
+ protected function prepareEnvironment() {
+ parent::prepareEnvironment();
+ $info = [
+ 'type' => '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 01fb53d92bbf..6a3f9002d968 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 48ebb9a801f4..5f9ac65f8431 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');
}
/**
--
GitLab