Make empty route lookup cacheable

Alright, here's a first stab at this.

I went ahead and copied Symfony's RouterListener instead of extending as it is marked final. That leads to a bunch of duplicated code, but as far as I can tell, that's the preferred way to do it these days 🤷.

Also note that the deprecation notice points to a fake change record as of now, I'd like to see some confirmation on the general approach and also on the concrete deprecation before writing a proper change notice that can be linked to.

Right, sorry, I wanted to get this "out the door" on Friday evening, so was a bit terse on explanation/context. The way I tested that this "works" is by actually verifying with XDebug that the cacheable exception correctly arrives in DefaultExceptionHtmlSubscriber::makeSubrequest() so that the request no longer gets the max-age 0.

DynamicPageCacheSubscriber already runs before that point. Will need to take some time to pull apart exactly what the functional change is with this other than unblocking #2352009: Bubbling of elements' max-age to the page's headers and the page cache and/or check if there's anything missing here still. Will hopefully get to that on Monday. And, yes, will definitely need some test coverage.

Status update

OK, next attempt at pushing this forward. Expanded the issue summary now to make the (non-)interactions with Dynamic Page Cache / and Page Cache and also the scope of this in general more clear. Also re-titling for this purpose. Also made the proposed resolution more explicit and included more detail. Some notable things from there:

• I added a new url.route_path cache context which is distinct from url.path in that it does not contain the base path. There is a lengthy comment in RouteProvider::$cacheContexts on why that is needed.
• In copying over Symfony's RouterListener I found that you can currently do throw new NotFoundHttpException(previous: new NoConfigurationException()) in a controller to trigger the Symfony welcome page. That doesn't seem to be terribly sensible for Drupal and is presumably simply an oversight, so I left it out of the Drupal implementation. This is just to reduce the amount of (duplicated) code, it could just as well be copied over if there's something that I'm missing.

Comment replies

Also added some test coverage of the new behavior so that this is now almost ready from my point of view, but before posing the last remaining question from my end, going through the latest comments now in detail:

#7

Sorry about the dynamic page cache confusion. dynamic_page_cache runs after routing, so obviously it can't cache the result of not finding a route.

That's broadly correct, although a bit simplistic, as you also found out in #10. Anyway, added that detail to the issue summary now.

This includes a lot of changes to make the route provider able return CacheableMetadata, many methods are fully removed/deprecated. BC is going to be complex around this. Yes, I think we need to cover deprecated method and the constructor for that too, there are a bunch of overrides in contrib, such as domain.module. Possibly this would allow domain module to no longer have the override, not sure.

I guess you are talking about DomainSourceRouteProvider here. I would have thought that since it's not public API it would be fine to remove e.g. RouteProvider::getRouteCollectionCacheId(), but yes, that would break domain_source, apparently. I don't think it's particularly complex to change this to deprecate more internal methods instead of removing them. I just wanted to reduce the amount of legacy baggage, but if more BC is wanted, that's fine too. Do you want this for RouteProvider::getRouteCollectionCacheId() explicitly then, or for all the removed methods?

From searching contrib it seems the following modules override RouteProvider::getCollectionForRequest() so broadly that they wouldn't be affected (just from skimming the code, didn't dig too deeply): purl, micro_site, social_auth_buttons, urct and only group_domain also (only) relies on RouteProvider::getRouteCollectionCacheId().

So not sure, but maybe just bringing that one back would be sufficient? (See also below for some thoughts on that method specifically.)

Is all that really required to resolve this or can we do an intermediate step here?

I have laid out the problem and the reasoning for the solution pretty in-depth now in the issue summary, feel free to tell me of a less invasive way to fix this problem, but I don't think there is one. There are a couple of minor things that I chose to do that are not strictly necessary but leave the resulting code in less of a bad shape as it otherwise would be in. All of these can be reverted, though, if you prefer:

1. I've changed RouteProvider to use constructor promotion. This was because I needed to newly inject CacheContextsManager so the constructor needed to be changed anyway
2. I've removed RouteProvider::getRouteCollectionCacheId(). This is because we need the ContextCacheKeys object now, to bubble its cache tags. So leaving the call like it is and just changing RouteProvider::getRouteCollectionCacheId() internally would necessarily result in repeated calls to RouteProvider::getContextCacheKeys() or alternatively we could keep a per-request cache of the CacheContextKeys object
3. I've removed Router::getInitialRouteCollection() because it is a one-line wrapper around RouteProvider::getRouteCollectionForRequest() and removing it makes it more clear that we're later asking the same object - and with the same parameters - for the cacheable metadata that we asked for the route collection. This makes it closer to what we conceptually are doing, namely fetching a "cacheable route collection result", than with that wrapper
4. I've added a return type-hint to WorkspaceRequestSubscriber::onKernelRequest() because that line needed to be changed anyway

If there's anything else that feels gratuitous to you feel free to point that out explicitly.

#8/#9

Yes, indeed, this doesn't actually lead to anything being cached that previously wasn't. The new title and improved issue summary should hopefully clarify that.

#10

But on the second request, you get a x-drupal-cache: HIT, even though cache control is no-cache, private.

The no-cache header is just set by FinishResponseSubscriber since (at least by default) the system.performance:cache.page.max_age is 0. As far as I can tell this is completely independent of the actual cacheable metadata.

And even dynamic page cache, despite reporting uncacheable here, is a hit if you disable the page_cache bin. I suppose that's the headers of the sub-request? Looking at the cache entries, for page_cache it's the /not-found URL, for dynamic page cache, it's the system.404 route.

(this is on main just to be clear for readers)

Well, yes, there is a cache hit in Dynamic Page Cache for the subrequest, but then DynamicPageCacheSubscriber::onResponse() is run for the main request and there it currently detects the 0 max-age and thus, adds the UNCACHEABLE (poor cacheability) header. It's arguable whether that is correct or not, but it's certainly confusing.

despite the change to dynamic-page-cache and max-age going permanent and miss-but-cacheable. I'm not sure if that's in scope of this issue or not and didn't investigate why this happens yet.

(this is with the MR just to be clear for readers)

So the max-age being -1 now is correct, I would say, as that is specifically what is being tackled here and I think is a sensible change. The dynamic page cache header with the MR (as of writing this) is leaked from the subrequest now. This is because in DynamicPageCacheSubscriber::onResponse() we no longer set the UNCACHEABLE (poor cacheability) (which in itself makes sense) and then just exit because !isset($this->requestPolicyResults[$request]). It seems wrong to return MISS or (yes, indeed) HIT while we are never going to get a hit for the main request, but not sure how to handle this, this was basically the remaining question I had.

As part of investigating this I created a RouteCachingNotFoundTest to verify the different headers on main and with this MR. The test passes with this MR and the diff is a reverse-diff of what is needed to make it pass on main, i.e. if we already had a test like that in main the diff would be what would be needed in this MR. Hope that helps for the review. Not yet sure if it makes sense to add the test as part of the MR (which is why I am providing it separately for now), since as discussed above there is no additional caching happening. Depending on how we decide to handle the Dynamic Page Cache headers, though, it might make sense to test that part, not sure.

I now get "url.query_args" in the cache contexts, for node/X, where X is a non-existing ID, I only get the same as on head: "url.query_args:_wrapper_format", so it varies by all query args.

Yes, and I think that's correct. The route provider already varies its cache by all query parameters, so this just reflects the reality.

languages:language_url is also newly added (language interface was already there), as is url.route_path.

Again, this just "exposes" what was already the case.

I'm not so sure what is actually affected by that.

Right, concretely nothing actually is. In the issue summary I elaborate on a theoretical scenario where the cache context itself bubbles a cache tag, which would now be picked up by the page cache and is currently not. But yeah, in practice, this doesn't make any difference.

Remaining questions

So as a kind of tl:dr;, I think the two concrete things that are left here, as of now are:

1. What about the BC of the RouteProvider methods?
2. What about the leaking of the Dynamic Page Cache headers from 404 subrequests?

So just one addendum that came to mind while righting the above:

One simple way to address the Dynamic Page Cache header leakage would be in the place in DynamicPageCacheSubscriber::onResponse() where we exit for 403s/404s:

    // Don't cache the response if Dynamic Page Cache's request subscriber did
    // not fire, because that means it is impossible to have a Dynamic Page
    // Cache hit. This can happen when the master request is for example a 403
    // or 404, in which case a subrequest is performed by the router. In that
    // case, it is the subrequest's response that is cached by Dynamic Page
    // Cache, because the routing happens in a request subscriber earlier than
    // Dynamic Page Cache's and immediately sets a response, i.e. the one
    // returned by the subrequest, and thus causes Dynamic Page Cache's request
    // subscriber to not fire for the master request.
    // @see \Drupal\Core\Routing\AccessAwareRouter::checkAccess()
    // @see \Drupal\Core\EventSubscriber\DefaultExceptionHtmlSubscriber::on403()
    $request = $event->getRequest();
    if (!isset($this->requestPolicyResults[$request])) {
      return;
    }

to just add always add a UNCACHEABLE (403/404) (or similar) header. Since that is currently the only exist point (except !$event->getRequest()->isMethodCacheable()) that doesn't set a header explicitly, that kind of feels like a simple enough change.

OK, went ahead with that. Went with "UNCACHEABLE (error)" now, as some of the tests revealed that this is also returned on 50x responses. Definitely open to suggestions, if someone thinks of something better. Since 40x responses are categorized as "client errors" I think it's technically correct, but it feels somewhat strange to return the string "error" for a 403/404.

Also reverted the deletions of the internal methods now and added proper deprecations (including tests) and also wrote a draft change record now.

As far as I can tell none of the contrib modules that override RouteProvider would break with this (although, again, did not test that).

The CI runners seem to be acting up at the moment, in general the tests should be green.

Fixed a bunch of things and replied in the Gitlab thread. Leaving at needs work for now, because the open thread probably needs to be resolved in one way or the other for this to move on. I'm not on Slack, but am reachable privately via e-mail for example, or on Mastodon.

OK, in order to reduce the scope here I moved the changes to the Dynamic Page Cache headers into (the pre-existing) #3451483: Improve Dynamic Page Cache headers for HTML and JSON 4xx responses.

Also, went ahead and implemented the "reduced" proposal now. Replied more in depth in the Gitlab thread, but, yeah, basically hoping that that will allow this issue to get resolved even though I don't particularly care for the solution.

Oh and for posterity I pushed the previous approach to a separate branch in the issue fork and just pushed the reverts on top of the existing branch so that the discussion can stay in the same place.

I wasn't sure if the rest.module tests were failing without the changes or if that's added test coverage (do rest requests behave different than before?)

Yes, they are failing because if you don't add the expected cache headers, the base test class asserts that the headers are not present. And that is what changes on those specific lines: Before there were no debug cache headers, now you can see:

  'X-Drupal-Cache-Tags' => '4xx-response http_response',
  'X-Drupal-Cache-Contexts' => '',
  'X-Drupal-Cache-Max-Age' => '-1 (Permanent)',

in the browser test output HTML thingy.

What about adding some extra test coverage on \Drupal\Tests\page_cache\Functional\PageCacheTest::testPageCacheAnonymous403404 about the header changes that this causes? I think it's just the max-age now that the dynamic page cache changes are moved to #2352009: Bubbling of elements' max-age to the page's headers and the page cache

Yes, it's just the max age header that goes from "0 (Uncacheable)" to "-1 (Permanent)" for 404s. Added assertions for all debug headers, though, to be more explicit.

Ahh good point, did that now 👍️

Yup, thanks for the heads up. Rebased now.