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 and MenuLinkContent 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 the dynamic_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 existing DenyAdminRoutes class.
  • Mark DenyAdminRoutes as @deprecated.
  • Remove (or comment out) dynamic_page_cache_deny_admin_routes from dynamic_page_cache.services.yml and add a dynamic_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

effulgentsia created an issue. See original summary.

effulgentsia’s picture

Issue summary: View changes
effulgentsia’s picture

Issue summary: View changes
effulgentsia’s picture

Issue summary: View changes
Wim Leers’s picture

Thanks so much for creating this issue, and doing such a thorough write-up, @effulgentsia!

  1. 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.

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.

  1. 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.

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).

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 existing DenyAdminRoutes class.
  • Mark DenyAdminRoutes as @deprecated.
  • Remove (or comment out) dynamic_page_cache_deny_admin_routes from dynamic_page_cache.services.yml and add a dynamic_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.

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.

Wim Leers’s picture

In other words: we have good reasons, and I love your concrete proposal.

If you're +1, then I'll work on the patch!

Wim Leers’s picture

Wim Leers’s picture

Assigned: Unassigned » effulgentsia

Clarifying that this is hard-blocked on @effulgentsia.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Status: Active » Closed (outdated)
Wim Leers’s picture

Assigned: effulgentsia » Unassigned