Rebuild router as few times as possible per request

I think this used to work in most cases, but unfortunately menu_get_item() is not guaranteed to be called on every page, so it's not always triggered...
And unless it is, the assertions will only happen on the second request, not the first.

I would have thought that the second request of a drupalPostForm() would handle it though?
Just trying this out for now.

Might as well get this passing, even if we don't go this way...

I guess this also needs the same check you're deleting:

-  // @todo Figure out why the cache rebuild is trigged but the route table
-  //   does not exist yet. 
-  if (db_table_exists('router')) {

Some unit tests already were installing the router table, removed the duplication.
This fixes the unit tests, but added back in a couple fixes for web tests.

If you save a view you want to ensure that a possible change does get reflected in the routing definition, at least in the UI.

Other places which might need to be touched: AdvancedSettingsForm.

Maybe forget my last patch, though not rebuild immediately opens new potential bugs, like a missing router definition, when the menu items are setup.
This is really fragile.

This essentially obsoletes #2167323: \Drupal::state()->set('menu_rebuild_needed') is not guaranteed to be checked on the next request.
If we choose to go this way, we should also remove the hunk from menu.inc (and close that and make this critical).

1. +++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
   @@ -149,12 +151,26 @@ public function rebuild() {
   +   * @return bool
   +   *   Returns TRUE if the rebuild succeeds, FALSE otherwise.
   +   */
   

   The documentation is a bit odd, as I would understand that FALSE means the route rebuild failed. Maybe something like: "Returns TRUE if the rebuild was done, FALSE otherwise."

2. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -48,10 +56,13 @@ class RouteProvider implements RouteProviderInterface {
   +  public function __construct(Connection $connection, $table = 'router', RouteBuilder $route_builder = NULL) {
   
   +++ b/core/core.services.yml
   @@ -268,7 +268,7 @@ services:
   +    arguments: ['@database', 'router', '@router.builder']
   

   Is there any reason why we mark the router builder here as optional?

Optional would be @?router, that's just a string?

@berdir
Yes, but I think there is no reason that this was intended, given that we just call methods on the object.

Marking #2167323: \Drupal::state()->set('menu_rebuild_needed') is not guaranteed to be checked on the next request as a dupe of this one. Haven't come up with a better title, but this is bigger than just views now.

The amount of conditionals being injected into plenty of spots here looks extremely concerning, hard to follow, and fragile.

In #2167323-6: \Drupal::state()->set('menu_rebuild_needed') is not guaranteed to be checked on the next request, @catch pointed out that this entire (ugly) rebuild-needed state flag was originally introduced in 2007 for D6 in:

#202955: Access denied after install - menu_rebuild() calls

The variable/state flag was introduced despite @Dries clearly objected to it back then already.

The original scenario of dynamically defined routes in hook_menu() no longer exists (in the offending form) in D8.

In addition, the very original root cause no longer applies, since the Drupal installer performs module installations in a full and regular Drupal environment today.

A rebuilt router is only required upon first route lookup/access. That could possibly happen (1) in a hook_install() implementation (albeit rare) or (2) when outputting a link in a message to a newly saved entity that is based on a dynamically defined route (also rare).

What we want to achieve is to (re)build the router only once within a single request, regardless of how many modules are being installed and regardless of how much configuration and how many entities are being saved.

Therefore, I'd recommend to do this:

1. Keep the RouteBuilder::setNeedsRebuild() flag of this patch
2. But kill the state entry entirely
3. Also replace the immediate router rebuild with setting the flag only
4. And perform the rebuild at the end of the request via Terminate event
5. And perform the rebuild upon first route lookup/access in case the needs-rebuild flag is set.

The title of this issue has "views" in it... but its scope is wider. tim.plunkett says it's the cause of #2042807-111: Convert search plugins to use a ConfigEntity and a PluginBag as well. This issue might need a new title and a better summary?

1. +++ b/core/modules/search/lib/Drupal/search/EventSubscriber/SearchConfigSubscriber.php
   @@ -0,0 +1,62 @@
   +        $this->routeBuilder->setRebuildNeeded();
   +        //\Drupal::service('router.builder')->setRebuildNeeded();
   

   There is no need to rebuild it twice.

2. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
   @@ -789,6 +789,11 @@ public function run(array $methods = array()) {
   +          if ($this->container instanceOf ContainerBuilder && $this->container->has('router.builder')) {
   

   I wonder whether it is enough to check just for Container

3. +++ b/core/includes/menu.inc
   @@ -2537,13 +2532,13 @@ function menu_router_rebuild() {
   diff --git a/core/lib/Drupal/Core/Config/Config.php b/core/lib/Drupal/Core/Config/Config.php
   
   diff --git a/core/lib/Drupal/Core/Config/Config.php b/core/lib/Drupal/Core/Config/Config.php
   index e026717..60e8ef7 100644
   
   index e026717..60e8ef7 100644
   --- a/core/lib/Drupal/Core/Config/Config.php
   
   --- a/core/lib/Drupal/Core/Config/Config.php
   +++ b/core/lib/Drupal/Core/Config/Config.php
   
   +++ b/core/lib/Drupal/Core/Config/Config.php
   +++ b/core/lib/Drupal/Core/Config/Config.php
   @@ -97,6 +97,13 @@ class Config {
   

   This change seems to come from a total different patch.

4. +++ b/core/lib/Drupal/Core/EventSubscriber/RouterRebuildSubscriber.php
   @@ -0,0 +1,59 @@
   +
   +  public function onRouterRebuild(Event $event) {
   +    menu_router_rebuild();
   +  }
   

   Some documentation which explains that this ensures the routes being there when rebuilding the menu router. I really like the idea.

5. +++ b/core/lib/Drupal/Core/Routing/RouteBuilderStatic.php
   @@ -10,14 +10,31 @@
   +   * @inheritdoc
   +   */
   +  public function rebuildIfNeeded(){
   +    // @todo Add the route for the batch pages when that conversion happens,
   +    //   http://drupal.org/node/1987816.
   +  }
   +
   +  /**
   +   * @inheritdoc
   +   */
   +  public function setRebuildNeeded() {
   +    // @todo Add the route for the batch pages when that conversion happens,
   +    //   http://drupal.org/node/1987816.
   +
   +  }
   +
   

   One of these todos seems to be enough.

6. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -99,6 +110,11 @@ public function getRouteCollectionForRequest(Request $request) {
   +    if (!count($collection) && $this->routeBuilder->rebuildIfNeeded()) {
   

   I kind of prefer to call the public count method directly.

7. +++ b/core/modules/search/lib/Drupal/search/EventSubscriber/SearchConfigSubscriber.php
   @@ -0,0 +1,62 @@
   +    $events['config.save'][] = array('onConfigSave', 40);
   

   I don't really see a requirement to rebuild the router really early, can you maybe document why this is needed?

The new approach looks much much better + cleaner — awesome!

1. +++ b/core/lib/Drupal/Core/Routing/RouteProviderInterface.php
   @@ -40,4 +40,9 @@ public function getRoutesByPattern($pattern);
   +  /**
   +   * Resets the static route cache.
   +   */
   +  public function resetStaticCache();
   

   A static cache appears to be specific to the default implementation -- as long as the method is only called internally, we don't need to add it to the interface?

2. +++ b/core/modules/color/lib/Drupal/color/Tests/ColorTest.php
   @@ -48,8 +48,6 @@ function setUp() {
        theme_enable(array_keys($this->themes));
   -    $this->container->get('router.builder')->rebuild();
   -    menu_router_rebuild();
   
   +++ b/core/modules/config_translation/lib/Drupal/config_translation/Tests/ConfigTranslationUiTest.php
   @@ -665,8 +665,9 @@ public function testThemeDiscovery() {
        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();
   
   +++ b/core/modules/field_ui/field_ui.module
   @@ -138,7 +138,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();
   
   +++ b/core/modules/system/system.admin.inc
   @@ -35,8 +35,7 @@ function system_theme_default() {
          // implementations, and doing the variable_set() before the theme_enable()
          // could result in a race condition where the theme is default but not
          // enabled.
   -      \Drupal::service('router.builder')->rebuild();
   -      menu_router_rebuild();
   +      \Drupal::service('router.builder')->setRebuildNeeded();
          \Drupal::cache('cache')->deleteTags(array('local_task' => 1));
   

   I'm not sure I understand why we're still rebuilding the router after enabling/disabling a theme?

   D7 and below only required this, because hook_menu() (e.g., block_menu()) was defining routes based on the list of enabled themes. But in D8, that no longer exists.

   The only part that still exists are local tasks (and possibly menu links), but those are not routes? Thus, we shouldn't have to rebuild the router; we just need to trigger a rebuild of local tasks/menu links...?

   That said, happy to ignore this here and re-evaluate that situation in a separate follow-up issue.

3. +++ b/core/modules/menu/menu.install
   @@ -13,7 +13,6 @@
    function menu_install() {
      // Add a link for each custom menu.
      \Drupal::service('router.builder')->rebuild();
   -  menu_router_rebuild();
   

   I wonder why menu_install() has to manually trigger/force a rebuild?

4. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
   @@ -789,6 +789,11 @@ public function run(array $methods = array()) {
              $this->setUp();
   +          // If set up is broken we can get here without a container.
   +          // @see \Drupal\simpletest\Tests\BrokenSetUpTest
   +          if ($this->container instanceOf ContainerBuilder && $this->container->has('router.builder')) {
   +            $this->container->get('router.builder')->rebuildIfNeeded();
   +          }
              if ($this->setup) {
   

   Hm. I'm very concerned about this addition.

   @alexpott explained in IRC that this is needed if setUp() has done something that sets the state. However:

   The sole responsibility of TestBase::run() is to dispatch test runs. It is not supposed to contain functional test environment logic. All of that code lives in prepareEnvironment() & Co, and more specifically, setUp().

   TestBase::run() is executed for any kind of test, including unit tests. The comment refers to a Simpletest test (testing the test runner), which is not only a web test, but additionally, all the Simpletest tests are extraordinarily special beasts → if this code is needed for those tests only, then we should move it into affected test classes (or introduce a base SimpleTestWebTestBase, which would be a good idea anyway).

5. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/RebuildTest.php
   @@ -36,9 +37,10 @@ function testMenuRebuildByVariable() {
   -    // Now we enable the rebuild variable and send a request to rebuild the menu
   +    // Now we set the rebuild state flag and send a request to rebuild the menu
        // item. Now 'admin' should exist.
   -    \Drupal::state()->set('menu_rebuild_needed', TRUE);
   +    \Drupal::state()->set(RouteBuilderInterface::REBUILD_NEEDED, TRUE);
   

   Any particular reason for why is this instance setting the state flag manually/differently?

btw: re: #38:

MenuLinkStorageController::save() creates a dummy row to get an mlid early on.

A single dummy row using an empty path doesn't sound thread-safe? Multiple users creating new menu links at the same time will blow up? Shouldn't we use at least a path of 'temp/hash(rand())' or similar there?

A quick comparison of this using php core/scripts/run-tests.sh --url http://d8/ "Views module integration" is 11m35 (HEAD) vs 10m24 (this patch).

Generally this patch is looking great.

1. +++ b/core/lib/Drupal/Core/Routing/RouteBuilderInterface.php
   @@ -0,0 +1,35 @@
   +   * Set the router to be rebuilt next next it is needed.
   

   typo there. Also, is rebuild() ever called now?

2. +++ b/core/lib/Drupal/Core/Routing/RouteBuilderStatic.php
   @@ -10,14 +10,31 @@
   +  public function rebuildIfNeeded(){
   +    // @todo Add the route for the batch pages when that conversion happens,
   ...
   +  public function setRebuildNeeded() {
   +    // @todo Add the route for the batch pages when that conversion happens,
   

   Couldn't this logic just be implemented around the one original @todo? Just needs to set the rebuild flag in memory?

3. +++ b/core/tests/Drupal/Tests/Core/Routing/RouteBuilderTest.php
   @@ -254,6 +266,44 @@ public function testRebuildWithProviderBasedRoutes() {
   +   * Tests that the route rebuilding if necessary works.
   

   Looks like a strange sentence, can't we just say 'Tests the rebuildIfNeeded method' or something?

4. +++ b/core/lib/Drupal/Core/EventSubscriber/RouterRebuildSubscriber.php
   @@ -0,0 +1,69 @@
   +    $this->cacheBackend->deleteTags(array('local_task' => 1));
   

   This just does tag clearing for the default cache bin, we need to use Cache::deleteTags() instead here. It's just coincidence they share the cache_tags table. If anyone switches implementations, this could break things.

   I guess that means you also don't need to inject the cache backend?

5. +++ b/core/lib/Drupal/Core/EventSubscriber/RouterRebuildSubscriber.php
   @@ -0,0 +1,69 @@
   +    $events[KernelEvents::TERMINATE][] = array('onKernelTerminate', 200);
   

   Should we be doing this at the end of a request? as opposed to the start of a request? I guess that a 'six of one, half a dozen of the other' type argument.

   EDIT: talking to dawehner, he pointed out this will technically get fired after the response has been sent, so is definitely the place to be.

Also, please rename this issue. It is not really an accurate description of what the patch is doing anymore :)

Issue needs title and summary that reflect what it is actually addressing. Neither is remotely accurate.

Updated the issue summary.

Wow, this got stuck for quite a while.

+1 on that

+++ b/core/modules/config_translation/lib/Drupal/config_translation/Tests/ConfigTranslationUiTest.php
@@ -669,8 +669,9 @@ public function testThemeDiscovery() {
-    menu_router_rebuild();
+    // Enabling a theme will cause the kernel terminate event to rebuild the
+    // router. Simulate that here.
+    \Drupal::service('router.builder')->rebuildIfNeeded();

With local tasks being split out, I wonder whether we really need to do this any more, this is just a straight switch here but might be worth a follow up to try to factor that out.

Couldn't find anything to complain about in the patch itself, this clears up a lot of crap.

Committed/pushed to 8.x, thanks!

Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release

https://drupal.org/node/2185947

Updated that one.

Thanks!