Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
- #2827797: ResourceResponse(Subscriber) + Dynamic Page Cache: making authenticated ResourceResponses significantly faster surfaced that there are REST routes (such as GET requests for
Shortcut
andMenuLinkContent
entities) that are marked as administrative routes (_admin_route
) by virtue of starting with an/admin
URL path, and that this excludes those resources from benefiting from Dynamic Page Cache due to thedynamic_page_cache_deny_admin_routes
response policy. - #2874938: AdminRouteSubscriber must only mark HTML routes as administrative is the issue for deciding what we mean by "administrative" for non-HTML routes, and potentially not marking REST routes as administrative.
- Regardless of that issue, it might make sense to change
dynamic_page_cache_deny_admin_routes
to a policy that's just scoped to denying caching HTML admin routes, since the reasons for that policy might not apply to non-HTML routes. This issue is for discussing and implementing such a change.
Background
- The Dynamic Page Cache module was developed in #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!). For most of that issue's lifetime, it included a policy to deny caching of admin routes due to a concern that throughout core and contrib, there would exist admin routes for which cacheability metadata was lacking due to developer omission, and that therefore caching such routes would result in stale content being served, which in some cases could also be a security bug. This concern, among others, was discussed in #2556889: [policy, no patch] Decide if SmartCache is still in scope for 8.0 and whether remaining risks require additional mitigation, where the policy of denying caching of administrative routes was included as one of several risk mitigation strategies.
- On the day that Dynamic Page Cache was committed to core, the PHPDoc explaining the policy was changed from:
This policy rule denies caching of responses generated for admin routes, because the cacheability metadata of most admin route responses is lacking, which would lead to stale content being shown and the site being perceived as broken.
to
This policy rule denies caching of responses generated for admin routes, because admin routes have very low cache hit ratios due to low traffic and form submissions."
which remains the PHPDoc in the
Drupal\dynamic_page_cache\PageCache\ResponsePolicy\DenyAdminRoutes
class today. - If the reason for the policy is what is currently stated in the PHPDoc, then that reason does not automatically apply to non-HTML routes. URLs that might have low hit ratios for HTML usage might have high hit ratios for REST usage. For example, GET requests of
MenuLinkContent
entities. - If the reason for the policy is what was originally stated in the PHPDoc prior to that patch's commit, then that might also not apply to non-HTML routes. Creating non-HTML routes is less commonplace than creating HTML routes, so it might be reasonable to expect developers who do so to bear the responsibility of providing cacheability metadata.
Proposed resolution
Change the default policy of dynamic_page_cache
module to be scoped to only denying HTML admin routes. For BC, in order to make it easy for sites to use the old policy, one possible implementation might be:
- Create a new policy,
DenyHtmlAdminRoutes
, to sit alongside the existingDenyAdminRoutes
class. - Mark
DenyAdminRoutes
as@deprecated
. - Remove (or comment out)
dynamic_page_cache_deny_admin_routes
fromdynamic_page_cache.services.yml
and add adynamic_page_cache_deny_html_admin_routes
instead. - Provide a change record of this change and include instructions of how a site owner can modify their
services.yml
file to add back the old policy.
Remaining tasks
Discuss.
User interface changes
None.
API changes
No API change, but there is a behavior change per the proposed resolution.
Data model changes
None.
Comments
Comment #2
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #3
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #4
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #5
Wim LeersThanks so much for creating this issue, and doing such a thorough write-up, @effulgentsia!
Correct.
REST
GET
requests to URLs that just happen to start with/admin
would be pretty much guaranteed to have extremely high cache hit ratios. All of the entity types in Drupal core whose canonical route URL happens to have that prefix are in fact entity types that are typically modified very rarely, thus implying high cache hit ratios.Also correct.
In fact, it's rather unlikely that somebody would add new REST resource plugins that use URLs that have the
/admin
path prefix. So, it's fair to say that most likely, most of the REST requests that would ever use/admin
URLs already exist: they're for entities, and\Drupal\rest\Plugin\rest\resource\EntityResource
already is guaranteed to have correct cacheability metadata (we have lots of test coverage for it thanks to #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method and follow-ups, and any bug we find is fixed for all entity types at once).That's a more prudent/safe approach than I would have proposed, but I do think that this is exactly right: take no risk at all with regards to Backwards Compatibility Breaks if at all possible. This avoids lots of frustration and pain, at the minor cost of having some mostly-duplicated code that is deprecated for removal in 9.0.0.
Comment #6
Wim LeersIn other words: we have good reasons, and I love your concrete proposal.
If you're +1, then I'll work on the patch!
Comment #7
Wim LeersNote that this soft-blocks #2765959-188: Make 4xx REST responses cacheable by (Dynamic) Page Cache + comprehensive cacheability test coverage.
Comment #8
Wim LeersClarifying that this is hard-blocked on @effulgentsia.
Comment #10
Wim Leers#2874938: AdminRouteSubscriber must only mark HTML routes as administrative is in, which makes this issue obsolete! 👌
Comment #11
Wim Leers