Move route preloading into the UrlGenerator: don't simply preload on every HTML request

Patch no longer applies cleanly.

I do not fully understand what this patch is doing and why.

Thanks, that helped! I clarified it further based on my new understanding. Feel free to correct if it's inaccurate.

Yes that looks right (and sorry dawehner for the incorrect correction). There was a bit of discussion around this in #2508445: Cache the query in RouteProvider::preloadRoutes(). Still makes sense to me - it's the generator that needs the pre-loading, so it should do it.

Yes patch looks completely fine for a minor to me - just constructor arguments and a new protected method. The actual logic change shouldn't make any difference. If routes aren't pre-loaded we can still get them, just one at a time until that happens.

This would be interesting. I've also noticed that the the list of routes that get preloaded can get really big in projects with a lot of additional routes. And most of those are not actually needed. Wondering if this would make sense as a cache collector, where we only load the routes we need per route name?

based on the current route name I'd say, so all node pages would share the same cache. Would need to test a bit, but I suspect that this would result in 10% or less of the current pre-loaded list that is currently loaded, several hundred routes seems pretty common to have in that list.

Had a quick look at one large custom project, it is preloading 437 routes and strlen(serialize($routes)) is 679313 in the preload method. There are tons of views and page manager routes, even REST routes which I guess are rather unlikely to be linked.

even REST routes which I guess are rather unlikely to be linke

They would indeed be very unlikely to get linked.

It seems to me that this could also use a heuristic, at least for the case of REST/JSON API/GraphQL routes since they're unlikely to be linked to: exclude routes routes that have a _format route whose value is not html.

I guess we want to keep logic like the admin exclude because admins will probably require those frequently while other users don't?

Also not quite sure yet where and how the cache collector would live. Unlike this preloading, which can be a completely separate subscriber, a collector-based approach needs to know which routes were loaded, so we'd need something like a getLoadedRoutes or so?

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Re-rolled patch.

> Does it require BC Shim ? as this is not a public service.

This is a really good question. I think that we don't need the BC shim because of https://www.drupal.org/about/core/policies/core-change-policies/bc-polic....

I re-read the discussion from five years ago, and I think if we end up with a working and viable patch here for the original approach, we should probably split the cache collector idea out to its own issue - that might be even better, but needs thought on exactly where to put it and how it would work.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

There are some open threads and a test failure in the MR.

Closed #3123181: "non admin routes" cache fragmentation as a duplicate

Forgot/didn't find this issue, created a new one instead: #3503843: Remove automatic preloading of all "public" routes, cache routes in fast chained bin. Might be a duplicate, but I'm wondering there if we need preloading at all.

We pretty much agree now to remove the preloading and change how we cache the routes, so closing this as a duplicate of #3503843: Remove automatic preloading of all "public" routes, cache routes in fast chained bin