Don't rebuild the route on ModuleInstaller::install() (30% installer speedup)

Happy to test this on my installation profile to figure out what breaks for me.

Unpostponing.

+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
@@ -232,10 +225,8 @@ public function destruct() {
-    if (!isset($this->yamlDiscovery)) {
-      $this->yamlDiscovery = new YamlDiscovery('routing', $this->moduleHandler->getModuleDirectories());
-    }
-    return $this->yamlDiscovery->findAll();
+    $discovery = new YamlDiscovery('routing', $this->moduleHandler->getModuleDirectories());
+    return $discovery->findAll();

Wasn't the reasoning to leave this in that the container is rebuild anyway when the ModuleDirectories() thing changes?

Or is it just not needed anymore?

Looks good and almost ready!

+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
@@ -232,10 +225,8 @@ public function destruct() {
-    if (!isset($this->yamlDiscovery)) {
-      $this->yamlDiscovery = new YamlDiscovery('routing', $this->moduleHandler->getModuleDirectories());
-    }
-    return $this->yamlDiscovery->findAll();
+    $discovery = new YamlDiscovery('routing', $this->moduleHandler->getModuleDirectories());
+    return $discovery->findAll();

Since Fabianx asked we should document why we do not use the cached yaml discovery anymore: "Always instantiate a new YamlDiscovery object so that we always search on the up-to-date list of modules."

And we can start working on the CR draft. The issue summary already documents the side effects of this change we can copy.

@Berdir: any results on testing this on your install profile yet?

Not yet, I'm on beta7, so I will need to test this in combination with the previous patch, assuming it will even apply.

But we just noticed in a different project that we already changed the route rebuild from happening for each module before hook_install(), so the modules that are affected by this, e.g. captcha (which is displaying some messages which I think should just be dropped) are likely already broken.

Ok, applying those patches was tough, but it worked.

Works fine for me, the comment about captcha is bogus, we had to switch to internal: a long time ago already because already before those two issues, the rebuild happened too late for it to work.

Install time for my install profile:

Beta7:
173.62 sec, 163.24 MB

After #356399: Optimize the route rebuilding process to rebuild on write:
269.12 sec, 162.9 MB

This patch:
198.99 sec, 162.98 MB

Somehow still a bit slower, but not that bad, and much better than before. And as mentioned, zero problems, so +1 to RTBC from me when the comment above is addressed.

Cool, now let's also add the comment I pointed to in #13 and the CR draft.

OK, improved the CR a bit.

It seems that the drush integration has also been fixed in the meantime to call the Kernel Terminate event, so I removed that form the issue summary.

Looks RTBC to me now!

Let me see if I understand this correctly (and please update the CR some):

- If I use setRebuildNeeded() then the routes are rebuild at the end of the request
- If I use rebuild() the routes are immediately rebuld

What I don't get is:

If I use setRebuildNeeded() and then call url() - will the routes be rebuilded or not?

I assume the answer is, the routes will use the old routing table and hence I need to call rebuild() myself?

No, the routes will never be rebuilt if you call Url(). But that is not changed in this issue, we did this in #356399: Optimize the route rebuilding process to rebuild on write. So that would be out of scope for this change record, would you like to add something to the CR "Route rebuilding moved to an explicit model" at https://www.drupal.org/node/2452735 ?

This issue is only about the install process.

The change record is fine now :). RTBC + 1

This issue is a major task that will improve performance significantly and the disruption it introduces is limited. Per https://www.drupal.org/core/beta-changes, this is a good change to complete during the Drupal 8 beta phase. Committed ae3dc62 and pushed to 8.0.x. Thanks!