Problem/Motivation

We add the url cache context in the access check
This is because we want to vary the result for 'checking access on the preview route' as opposed to 'checking it elsewhere'
But this is too granular as the result varies per url for nodes in menu trees etc.

Proposed resolution

Add a 'is preview link route' cache context.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan created an issue. See original summary.

jibran’s picture

diff --git a/preview_link.module b/preview_link.module
index b32f5fa..f742581 100644
--- a/preview_link.module
+++ b/preview_link.module
@@ -73,8 +73,7 @@ function preview_link_theme($existing, $type, $theme, $path) {
  */
 function preview_link_entity_access(EntityInterface $entity, $operation, AccountInterface $account) {
   $neutral = AccessResult::neutral()
-    ->addCacheableDependency($entity)
-    ->addCacheContexts(['url']);
+    ->addCacheableDependency($entity);
   if ($operation !== 'view' || !($entity instanceof ContentEntityInterface)) {
     return $neutral;
   }
diff --git a/src/Access/PreviewLinkAccessCheck.php b/src/Access/PreviewLinkAccessCheck.php
index c8499a3..3f6938f 100644
--- a/src/Access/PreviewLinkAccessCheck.php
+++ b/src/Access/PreviewLinkAccessCheck.php
@@ -42,8 +42,7 @@ class PreviewLinkAccessCheck implements AccessInterface {
    */
   public function access(EntityInterface $entity = NULL, $preview_token = NULL) {
     $neutral = AccessResult::neutral()
-      ->addCacheableDependency($entity)
-      ->addCacheContexts(['url']);
+      ->addCacheableDependency($entity);
     if (!$preview_token || !$entity) {
       return $neutral;
     }
Wim Leers’s picture

Issue tags: +D8 cacheability

Add a 'is preview link route' cache context.

+1 — that could even be a useful addition to core — see https://www.drupal.org/docs/8/api/cache-api/cache-contexts.

But … are you even absolutely sure you need a cache context? Route access results are assumed to be per-route: that's why access checks are defined on a route (as a route _requirements entry).

larowlan’s picture

But … are you even absolutely sure you need a cache context? Route access results are assumed to be per-route: that's why access checks are defined on a route (as a route _requirements entry).

This code is also invoked in general entity access, to allow view access to paragraphs that are attached to unpublished nodes, so that's where we need the context added, it won't add route by default.

larowlan’s picture

Status: Active » Needs review
FileSize
4.04 KB

Like so

larowlan’s picture

FileSize
4.04 KB
larowlan’s picture

@dpi/@jibran reckon you could run this on your site and repeat profiling?

dpi’s picture

Will be prioritised shortly.

dpi’s picture

Patch looks good and tests well.

+++ b/src/PreviewLinkCacheContext.php
@@ -0,0 +1,52 @@
+    return new CacheableMetadata();

Its probably implied, but should the 'routes' tag be added? RouteProvider uses this tag.

Sam152’s picture

Just ran into this. The 'url' cache context was added to all teasers of an entity type, which were rendered on a large number of pages. The result was a render cache table that grew indefinitely.

larowlan’s picture

FileSize
451 bytes
4.06 KB
Sam152’s picture

Status: Needs review » Reviewed & tested by the community

Running this in production now. It seems to have had a dramatic effect on overall site performance. I think going through access checks for every entity linked from the menu was really dragging the site down.

Thanks @dpi and @larowlan for the fix :)

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
Sam152’s picture

  • larowlan committed f39c3d1 on 8.x-1.x
    Issue #3037859 by larowlan, dpi, Sam152, Wim Leers, jibran: Remove the...
larowlan’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jibran’s picture

Is it suppose to be present on every page because I'm seeing it on all pages with empty value?

jibran’s picture

Ignore that. I was looking at the wrong field.

jibran’s picture

Is it suppose to be present on every page because I'm seeing it on all pages with empty value?

This certainly the case. @Sam152 or @larowlan can you confirm?

+++ b/src/PreviewLinkCacheContext.php
@@ -0,0 +1,52 @@
+    return $this->routeMatch->getRouteObject()->getOption('_preview_link_route') ?: FALSE;

Also getRouteObject() can return NULL. I'll create a followup for that.

fenstrat’s picture

I hit the case where the route object is null from #20, created a follow up #3100475: Cache context doesn't handle NULL route object.