Commit 559479f8 authored by catch's avatar catch

Issue #2164367 by alexpott, tim.plunkett, dawehner: Rebuild router as few...

Issue #2164367 by alexpott, tim.plunkett, dawehner: Rebuild router as few times as possible per request.
parent f2769f4d
......@@ -267,7 +267,9 @@ services:
- [fromRequest, ['@request']]
router.route_provider:
class: Drupal\Core\Routing\RouteProvider
arguments: ['@database']
arguments: ['@database', '@router.builder']
tags:
- { name: event_subscriber }
router.matcher.final_matcher:
class: Drupal\Core\Routing\UrlMatcher
router.matcher:
......@@ -310,7 +312,12 @@ services:
arguments: ['@database']
router.builder:
class: Drupal\Core\Routing\RouteBuilder
arguments: ['@router.dumper', '@lock', '@event_dispatcher', '@module_handler', '@controller_resolver']
arguments: ['@router.dumper', '@lock', '@event_dispatcher', '@module_handler', '@controller_resolver', '@state']
router.rebuild_subscriber:
class: Drupal\Core\EventSubscriber\RouterRebuildSubscriber
arguments: ['@router.builder']
tags:
- { name: event_subscriber }
path.alias_manager.cached:
class: Drupal\Core\CacheDecorator\AliasManagerCacheDecorator
arguments: ['@path.alias_manager', '@cache.path']
......
......@@ -4889,7 +4889,6 @@ function drupal_flush_all_caches() {
// Important: This rebuild must happen last, so the menu router is guaranteed
// to be based on up to date information.
\Drupal::service('router.builder')->rebuild();
menu_router_rebuild();
// Re-initialize the maintenance theme, if the current request attempted to
// use it. Unlike regular usages of this function, the installer and update
......
......@@ -475,13 +475,7 @@ function menu_get_item($path = NULL, $router_item = NULL) {
$router_items[$path] = $router_item;
}
if (!isset($router_items[$path])) {
// Rebuild if we know it's needed, or if the menu masks are missing which
// occurs rarely, likely due to a race condition of multiple rebuilds.
if (\Drupal::state()->get('menu_rebuild_needed') || !\Drupal::state()->get('menu.masks')) {
menu_router_rebuild();
\Drupal::service('router.builder')->rebuild();
Cache::deleteTags(array('local_task' => 1));
}
\Drupal::service('router.builder')->rebuildIfNeeded();
$original_map = arg(NULL, $path);
$parts = array_slice($original_map, 0, MENU_MAX_PARTS);
......@@ -2536,13 +2530,13 @@ function menu_router_rebuild() {
$transaction = db_transaction();
try {
// Ensure the route based router is up to date.
\Drupal::service('router.builder')->rebuildIfNeeded();
list($menu) = menu_router_build(TRUE);
_menu_navigation_links_rebuild($menu);
// Clear the menu, page and block caches.
menu_cache_clear_all();
_menu_clear_page_cache();
// Indicate that the menu has been successfully rebuilt.
\Drupal::state()->delete('menu_rebuild_needed');
}
catch (Exception $e) {
$transaction->rollback();
......@@ -2744,6 +2738,9 @@ function _menu_navigation_links_rebuild($menu) {
}
// Find any item whose router path does not exist any more.
// Do not delete entries with an empty path as this can remove menu links in
// the process of being created.
$paths[] = '';
$query = \Drupal::entityQuery('menu_link')
->condition('router_path', $paths, 'NOT IN')
->condition('external', 0)
......
<?php
/**
* @file
* Contains \Drupal\Core\EventSubscriber\RouterRebuildSubscriber.
*/
namespace Drupal\Core\EventSubscriber;
use Drupal\Core\Cache\Cache;
use Drupal\Core\Routing\RouteBuilderInterface;
use Drupal\Core\Routing\RoutingEvents;
use Symfony\Component\EventDispatcher\Event;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpKernel\Event\PostResponseEvent;
use Symfony\Component\HttpKernel\KernelEvents;
/**
* Rebuilds the router and menu_router if necessary.
*/
class RouterRebuildSubscriber implements EventSubscriberInterface {
/**
* @var \Drupal\Core\Routing\RouteBuilderInterface
*/
protected $routeBuilder;
/**
* Constructs the RouterRebuildSubscriber object.
*
* @param \Drupal\Core\Routing\RouteBuilderInterface $route_builder
* The route builder.
*/
public function __construct(RouteBuilderInterface $route_builder) {
$this->routeBuilder = $route_builder;
}
/**
* Rebuilds routers if necessary.
*
* @param \Symfony\Component\HttpKernel\Event\PostResponseEvent $event
* The event object.
*/
public function onKernelTerminate(PostResponseEvent $event) {
$this->routeBuilder->rebuildIfNeeded();
}
/**
* Rebuilds the menu_router and deletes the local_task cache tag.
*
* @param \Symfony\Component\EventDispatcher\Event $event
* The event object.
*/
public function onRouterRebuild(Event $event) {
menu_router_rebuild();
Cache::deleteTags(array('local_task' => 1));
}
/**
* Registers the methods in this class that should be listeners.
*
* @return array
* An array of event listener definitions.
*/
static function getSubscribedEvents() {
$events[KernelEvents::TERMINATE][] = array('onKernelTerminate', 200);
$events[RoutingEvents::FINISHED][] = array('onRouterRebuild', 200);
return $events;
}
}
......@@ -84,7 +84,7 @@ class ThemeHandler implements ThemeHandlerInterface {
*
* @var \Drupal\Core\Routing\RouteBuilder
*/
protected $routerBuilder;
protected $routeBuilder;
/**
* The system listing info
......@@ -460,7 +460,7 @@ protected function getSystemListingInfo() {
*/
protected function resetSystem() {
if ($this->routeBuilder) {
$this->routeBuilder->rebuild();
$this->routeBuilder->setRebuildNeeded();
}
$this->systemListReset();
......
......@@ -9,6 +9,7 @@
use Drupal\Component\Discovery\YamlDiscovery;
use Drupal\Core\Controller\ControllerResolverInterface;
use Drupal\Core\KeyValueStore\StateInterface;
use Symfony\Component\EventDispatcher\Event;
use Symfony\Component\EventDispatcher\EventDispatcherInterface;
use Symfony\Component\Routing\RouteCollection;
......@@ -23,7 +24,7 @@
* Because this class makes use of the modules system, it cannot currently
* be unit tested.
*/
class RouteBuilder {
class RouteBuilder implements RouteBuilderInterface {
/**
* The dumper to which we should send collected routes.
......@@ -68,7 +69,7 @@ class RouteBuilder {
protected $controllerResolver;
/**
* Construcs the RouteBuilder using the passed MatcherDumperInterface.
* Constructs the RouteBuilder using the passed MatcherDumperInterface.
*
* @param \Drupal\Core\Routing\MatcherDumperInterface $dumper
* The matcher dumper used to store the route information.
......@@ -81,19 +82,17 @@ class RouteBuilder {
* @param \Drupal\Core\Controller\ControllerResolverInterface $controller_resolver
* The controller resolver.
*/
public function __construct(MatcherDumperInterface $dumper, LockBackendInterface $lock, EventDispatcherInterface $dispatcher, ModuleHandlerInterface $module_handler, ControllerResolverInterface $controller_resolver) {
public function __construct(MatcherDumperInterface $dumper, LockBackendInterface $lock, EventDispatcherInterface $dispatcher, ModuleHandlerInterface $module_handler, ControllerResolverInterface $controller_resolver, StateInterface $state = NULL) {
$this->dumper = $dumper;
$this->lock = $lock;
$this->dispatcher = $dispatcher;
$this->moduleHandler = $module_handler;
$this->controllerResolver = $controller_resolver;
$this->state = $state;
}
/**
* Rebuilds the route info and dumps to dumper.
*
* @return bool
* Returns TRUE if the rebuild succeeds, FALSE otherwise.
* {@inheritdoc}
*/
public function rebuild() {
if (!$this->lock->acquire('router_rebuild')) {
......@@ -157,11 +156,29 @@ public function rebuild() {
$this->dumper->addRoutes($collection);
$this->dumper->dump(array('provider' => 'dynamic_routes'));
$this->state->delete(static::REBUILD_NEEDED);
$this->lock->release('router_rebuild');
$this->dispatcher->dispatch(RoutingEvents::FINISHED, new Event());
return TRUE;
}
/**
* {@inheritdoc}
*/
public function rebuildIfNeeded() {
if ($this->state->get(static::REBUILD_NEEDED, FALSE)) {
return $this->rebuild();
}
return FALSE;
}
/**
* {@inheritdoc}
*/
public function setRebuildNeeded() {
$this->state->set(static::REBUILD_NEEDED, TRUE);
}
/**
* Returns the YAML discovery for getting all the .routing.yml files.
*
......
<?php
/**
* @file
* Definition of Drupal\Core\Routing\RouteBuilderInterface.
*/
namespace Drupal\Core\Routing;
interface RouteBuilderInterface {
const REBUILD_NEEDED = 'router_rebuild_needed';
/**
* Rebuilds the route info and dumps to dumper.
*
* @return bool
* Returns TRUE if the rebuild succeeds, FALSE otherwise.
*/
public function rebuild();
/**
* Rebuilds the route info and dumps to dumper if necessary.
*
* @return bool
* Returns TRUE if the rebuild occurs, FALSE otherwise.
*/
public function rebuildIfNeeded();
/**
* Sets the router to be rebuilt next time rebuildIfNeeded() is called.
*/
public function setRebuildNeeded();
}
......@@ -10,14 +10,39 @@
/**
* This builds a static version of the router.
*/
class RouteBuilderStatic {
class RouteBuilderStatic implements RouteBuilderInterface {
/**
* Rebuilds router.
* Marks a rebuild as being necessary.
*
* @var bool
*/
protected $rebuildNeeded = FALSE;
/**
* @inheritdoc
*/
public function rebuild() {
// @todo Add the route for the batch pages when that conversion happens,
// http://drupal.org/node/1987816.
}
/**
* @inheritdoc
*/
public function rebuildIfNeeded(){
if ($this->rebuildNeeded && $this->rebuild()) {
$this->rebuildNeeded = FALSE;
return TRUE;
}
return FALSE;
}
/**
* @inheritdoc
*/
public function setRebuildNeeded() {
$this->rebuildNeeded = TRUE;
}
}
......@@ -8,6 +8,7 @@
namespace Drupal\Core\Routing;
use Drupal\Component\Utility\String;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\Routing\Exception\RouteNotFoundException;
use Symfony\Component\Routing\RouteCollection;
......@@ -18,7 +19,7 @@
/**
* A Route Provider front-end for all Drupal-stored routes.
*/
class RouteProvider implements RouteProviderInterface {
class RouteProvider implements RouteProviderInterface, EventSubscriberInterface {
/**
* The database connection from which to read route information.
......@@ -34,6 +35,13 @@ class RouteProvider implements RouteProviderInterface {
*/
protected $tableName;
/**
* The route builder.
*
* @var \Drupal\Core\Routing\RouteBuilderInterface
*/
protected $routeBuilder;
/**
* A cache of already-loaded routes, keyed by route name.
*
......@@ -46,11 +54,14 @@ class RouteProvider implements RouteProviderInterface {
*
* @param \Drupal\Core\Database\Connection $connection
* A database connection object.
* @param \Drupal\Core\Routing\RouteBuilderInterface $route_builder
* The route builder.
* @param string $table
* The table in the database to use for matching.
*/
public function __construct(Connection $connection, $table = 'router') {
public function __construct(Connection $connection, RouteBuilderInterface $route_builder, $table = 'router') {
$this->connection = $connection;
$this->routeBuilder = $route_builder;
$this->tableName = $table;
}
......@@ -99,7 +110,12 @@ public function getRouteCollectionForRequest(Request $request) {
$collection = $this->getRoutesByPath($path);
if (!count($collection)) {
// Try rebuilding the router if it is necessary.
if (!$collection->count() && $this->routeBuilder->rebuildIfNeeded()) {
$collection = $this->getRoutesByPath($path);
}
if (!$collection->count()) {
throw new ResourceNotFoundException(String::format("The route for '@path' could not be found", array('@path' => $path)));
}
......@@ -269,4 +285,19 @@ public function getAllRoutes() {
return new LazyLoadingRouteCollection($this->connection, $this->tableName);
}
/**
* {@inheritdoc}
*/
public function reset() {
$this->routes = array();
}
/**
* {@inheritdoc}
*/
static function getSubscribedEvents() {
$events[RoutingEvents::FINISHED][] = array('reset');
return $events;
}
}
......@@ -40,4 +40,9 @@ public function getRoutesByPattern($pattern);
*/
public function getAllRoutes();
/**
* Resets the route provider object.
*/
public function reset();
}
......@@ -64,8 +64,9 @@ public function testBlockNotInHiddenRegion() {
\Drupal::config('system.theme')
->set('default', $theme)
->save();
\Drupal::service('router.builder')->rebuild();
menu_router_rebuild();
// Enabling a theme will cause the kernel terminate event to rebuild the
// router. Simulate that here.
\Drupal::service('router.builder')->rebuildIfNeeded();
// Ensure that "block_test_theme" is set as the default theme.
$this->drupalGet('admin/structure/block');
......
......@@ -48,8 +48,6 @@ function setUp() {
),
);
theme_enable(array_keys($this->themes));
$this->container->get('router.builder')->rebuild();
menu_router_rebuild();
// Array filled with valid and not valid color values
$this->colorTests = array(
......
......@@ -669,8 +669,9 @@ public function testThemeDiscovery() {
// Enable the test theme and rebuild routes.
$theme = 'config_translation_test_theme';
theme_enable(array($theme));
\Drupal::service('router.builder')->rebuild();
menu_router_rebuild();
// Enabling a theme will cause the kernel terminate event to rebuild the
// router. Simulate that here.
\Drupal::service('router.builder')->rebuildIfNeeded();
$this->drupalLogin($this->admin_user);
......
......@@ -130,7 +130,7 @@ function field_ui_entity_info(&$entity_info) {
function field_ui_entity_bundle_create($entity_type, $bundle) {
// When a new bundle is created, the menu needs to be rebuilt to add our
// menu item tabs.
\Drupal::state()->set('menu_rebuild_needed', TRUE);
\Drupal::service('router.builder')->setRebuildNeeded();
}
/**
......@@ -139,7 +139,7 @@ function field_ui_entity_bundle_create($entity_type, $bundle) {
function field_ui_entity_bundle_rename($entity_type, $bundle_old, $bundle_new) {
// When a bundle is renamed, the menu needs to be rebuilt to add our
// menu item tabs.
\Drupal::state()->set('menu_rebuild_needed', TRUE);
\Drupal::service('router.builder')->setRebuildNeeded();
}
/**
......@@ -237,14 +237,14 @@ function field_ui_library_info() {
* Implements hook_view_mode_presave().
*/
function field_ui_view_mode_presave(EntityViewModeInterface $view_mode) {
\Drupal::state()->set('menu_rebuild_needed', TRUE);
\Drupal::service('router.builder')->setRebuildNeeded();
}
/**
* Implements hook_view_mode_delete().
*/
function field_ui_view_mode_delete(EntityViewModeInterface $view_mode) {
\Drupal::state()->set('menu_rebuild_needed', TRUE);
\Drupal::service('router.builder')->setRebuildNeeded();
}
/**
......
......@@ -18,7 +18,7 @@ class ExtensionViewsFieldTest extends ViewUnitTestBase {
/**
* {@inheritdoc}
*/
public static $modules = array('file', 'file_test_views');
public static $modules = array('file', 'file_test_views', 'user');
/**
* Views used by this test.
......
......@@ -13,7 +13,6 @@
function menu_install() {
// Add a link for each custom menu.
\Drupal::service('router.builder')->rebuild();
menu_router_rebuild();
$system_link = entity_load_multiple_by_properties('menu_link', array('link_path' => 'admin/structure/menu', 'module' => 'system'));
$system_link = reset($system_link);
......
......@@ -539,7 +539,7 @@ public function preSave(EntityStorageControllerInterface $storage_controller) {
}
// Find the route_name.
if (!isset($this->route_name)) {
if ($result = \Drupal::service('router.matcher.final_matcher')->findRouteNameParameters($this->link_path)) {
if (!$this->external && $result = \Drupal::service('router.matcher.final_matcher')->findRouteNameParameters($this->link_path)) {
list($this->route_name, $this->route_parameters) = $result;
}
else {
......
......@@ -121,7 +121,7 @@ public function save(EntityInterface $entity) {
}
if ($entity->isNew()) {
$entity->mlid = $this->database->insert($this->entityInfo->getBaseTable())->fields(array('menu_name' => 'tools'))->execute();
$entity->mlid = $this->database->insert($this->entityInfo->getBaseTable())->fields(array('menu_name' => $entity->menu_name))->execute();
$entity->enforceIsNew();
}
......
......@@ -205,11 +205,7 @@ public function preSave(EntityStorageControllerInterface $storage_controller) {
*/
public function postSave(EntityStorageControllerInterface $storage_controller, $update = TRUE) {
parent::postSave($storage_controller, $update);
$this->state()->set('menu_rebuild_needed', TRUE);
// @todo The above call should be sufficient, but it is not until
// https://drupal.org/node/2167323 is fixed.
\Drupal::service('router.builder')->rebuild();
$this->routeBuilder()->setRebuildNeeded();
}
/**
......@@ -239,13 +235,13 @@ public static function sort($a, $b) {
}
/**
* Wraps the state storage.
* Wraps the route builder.
*
* @return \Drupal\Core\KeyValueStore\StateInterface
* @return \Drupal\Core\Routing\RouteBuilderInterface
* An object for state storage.
*/
protected function state() {
return \Drupal::state();
protected function routeBuilder() {
return \Drupal::service('router.builder');
}
/**
......
......@@ -34,8 +34,6 @@ function setUp() {
$this->searching_user = $this->drupalCreateUser(array('search content', 'access content', 'access comments', 'skip comment approval'));
// Login with sufficient privileges.
$this->drupalLogin($this->searching_user);
// Test with all search modules enabled.
\Drupal::state()->set('menu_rebuild_needed', TRUE);
}
/**
......
......@@ -35,13 +35,9 @@ function setUp() {
// Login as a user that can create and search content.
$this->search_user = $this->drupalCreateUser(array('search content', 'administer search'));
$this->drupalLogin($this->search_user);
// Enable the extra type module for searching.
\Drupal::service('router.builder')->rebuild();
}
function testSearchPageHook() {
$this->container->get('router.builder')->rebuild();
$keys = 'bike shed ' . $this->randomName();
$this->drupalGet("search/dummy_path/{$keys}");
$this->assertText('Dummy search snippet', 'Dummy search snippet is shown');
......
......@@ -8,6 +8,7 @@
namespace Drupal\system\Entity;
use Drupal\Core\Config\Entity\ConfigEntityBase;
use Drupal\Core\Entity\EntityStorageControllerInterface;
use Drupal\system\MenuInterface;
/**
......
......@@ -186,7 +186,6 @@ protected function doTestMenuName() {
// rebuild.
menu_test_menu_name('changed');
\Drupal::service('router.builder')->rebuild();
menu_router_rebuild();
$menu_links = entity_load_multiple_by_properties('menu_link', array('router_path' => 'menu_name_test'));
$menu_link = reset($menu_links);
......
......@@ -7,10 +7,11 @@
namespace Drupal\system\Tests\Menu;
use Drupal\Core\Routing\RouteBuilderInterface;
use Drupal\simpletest\WebTestBase;
/**
* Tests rebuilding the menu by setting 'menu_rebuild_needed.'
* Tests rebuilding the router.
*/
class RebuildTest extends WebTestBase {
public static function getInfo() {
......@@ -22,9 +23,9 @@ public static function getInfo() {
}
/**
* Test if the 'menu_rebuild_needed' variable triggers a menu_rebuild() call.
* Tests that set a router rebuild needed works.
*/
function testMenuRebuildByVariable() {
function testMenuRebuild() {
// Check if 'admin' path exists.
$admin_exists = db_query('SELECT path from {menu_router} WHERE path = :path', array(':path' => 'admin'))->fetchField();
$this->assertEqual($admin_exists, 'admin', "The path 'admin/' exists prior to deleting.");
......@@ -36,9 +37,9 @@ function testMenuRebuildByVariable() {
$admin_exists = db_query('SELECT path from {menu_router} WHERE path = :path', array(':path' => 'admin'))->fetchField();
$this->assertFalse($admin_exists, "The path 'admin/' has been deleted and doesn't exist in the database.");