Reported by Berdir:

/**
 * Implements hook_entity_build_defaults_alter().
 */
function comment_entity_build_defaults_alter(array &$build, EntityInterface $entity, $view_mode = 'full', $langcode = NULL) {
  // Get the corresponding display settings.
  $display = EntityViewDisplay::collectRenderDisplay($entity, $view_mode);
  // Add the comment page number to the cache key if render caching is enabled.
  if (isset($build['#cache']) && isset($build['#cache']['keys']) && \Drupal::request()->query->has('page')) {
    foreach ($entity->getFieldDefinitions() as $field_name => $definition) {
      if ($definition->getType() === 'comment' && ($display_options = $display->getComponent($field_name))) {
        $pager_id = $display_options['settings']['pager_id'];
        $page = pager_find_page($pager_id);
        $build['#cache']['keys'][] = $field_name . '-pager-' . $page;
      }
    }
  }
  return $build;
}
CommentFileSizeAuthor
#22 interdiff.txt781 bytesWim Leers
#22 pager_cache_context-2428563-22.patch6.81 KBWim Leers
#16 interdiff.txt3.16 KBWim Leers
#16 pager_cache_context-2428563-16.patch6.81 KBWim Leers
#9 interdiff.txt2.29 KBWim Leers
#9 pager_cache_context-2428563-9.patch8.7 KBWim Leers
#8 interdiff.txt958 bytesWim Leers
#8 pager_cache_context-2428563-7.patch6.74 KBWim Leers
#4 pager_cache_context-2428563-3.patch6.91 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Title: Comment pager-dependent cache key should be a cache context » [PP-1] Comment pager-dependent cache key should be a cache context
Related issues: +#2428563: Introduce parameter-dependent cache contexts

Blocked on #2428563: Introduce parameter-dependent cache contexts.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Title: [PP-1] Comment pager-dependent cache key should be a cache context » Comment pager-dependent cache key should be a cache context
Assigned: Unassigned » Wim Leers

#2428563: Introduce parameter-dependent cache contexts landed, now unblocked. Taking a first stab at this.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Suppose better to apply CommentPagerCacheContext from comment field formater somehow

is it possible if:
1) bubbling is not supported yet #2429257: Bubble cache contexts
2) comment formatter could be changed to contrib provided one

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Assigned: Wim Leers » Unassigned
Status: Active » Needs review
FileSize
pager_cache_context-2428563-3.patch6.91 KB

Briefly discussed this with catch yesterday. We saw 2 general approaches:

  1. A QueryStringCacheContext: hashes the entire query string, and hence varies cache items using this context whenever a single detail about it is different.
  2. A QueryParameterCacheContext: varies cache items using a query parameter that a render array varies by.

I'm not a fan of approach 1 because it would cause even the typical Google Analytics query parameters that appear when used from feed readers to cause caches not to be used, even though Drupal never ever reacts to either of those query parameters.

I implemented approach 2.

But then I noticed that this doesn't actually work very well for pagers, because Drupal's pagers (see pager.inc) actually don't use separate query parameters… but the same query parameter: page, with the values for the different pagers encoded as an integer at a different location in a list of comma-separated page numbers (see pager_find_page()).
So I added a PagerCacheContext as well.

In doing so, I found a small bug in #2428563: Introduce parameter-dependent cache contexts: the CacheContexts service, when converting cache context tokens to keys, if given the same cache parameter-dependent cache context multiple times, with different parameters, it only keeps one of them. Very simple fix (with expanded test coverage) included.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

Also note this bit in the patch:

 * @todo Remove this entire hook, let the pager's rendering handle this
 *    automatically once https://www.drupal.org/node/2429257 lands. It might
 *    require converting #theme => pager to #type => pager, but that should not
 *    be a problem.
 */
function comment_entity_build_defaults_alter(array &$build, EntityInterface $entity, $view_mode = 'full', $langcode = NULL) {

i.e. once we have bubbling, we can automatically make sure that anything using a pager automatically sets the appropriate cache context, and hence it automatically bubbles up everywhere!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

Filed #2433591: Views using pagers should specify a cache context as a sister issue.

Status: Needs review » Needs work

The last submitted patch, 4: pager_cache_context-2428563-3.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Related issues: +#2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!)
FileSize
pager_cache_context-2428563-7.patch6.74 KB
interdiff.txt958 bytes

Filed an issue for that @todo cited in #5: #2433599: Ensure every (non-views) pager automatically associates a matching cache context.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs work » Needs review
FileSize
pager_cache_context-2428563-9.patch8.7 KB
interdiff.txt2.29 KB

Fixed test failures.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Related issues: -#2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) +#2433599: Ensure every (non-views) pager automatically associates a matching cache context

Oops. Chrome--

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented

This is off-topic, but this .missing is interested.

We should just still execute fields that have #access => FALSE so that we always have the same cache contexts and no variations :-p.

The PR looks good, gonna hopefully review later for RTBC.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

I still think that this context should apply within formatter, also this is a way to solve #11 about #access

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented 
+++ b/core/lib/Drupal/Core/Cache/UrlQueryParameterCacheContext.php
@@ -0,0 +1,56 @@
+    }
+    return 'qs.' . $query_parameter . '=' . $query_parameters[$query_parameter];
+  }

Do we need to sanitize this in any way?

Overall like a query parameter cache context, but perhaps we should have a cache_context.request.query

instead?

So we could also have cache_context.request.cookie, which is another important use case?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

Perhaps it's better to just omit UrlQueryParameterCacheContext, because it's not actually necessary to fix this issue? We can easily add it later.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented

Sounds good, lets get this in first.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
FileSize
pager_cache_context-2428563-16.patch6.81 KB
interdiff.txt3.16 KB

Alright, done.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/lib/Drupal/Core/Cache/CacheContexts.php
@@ -136,9 +137,11 @@ protected function getService($context_id) {
+   *   An array with the parsed results, with each result being an array
+   *   containing:
+   *   1. the cache context ID
+   *   2. the associated parameter (for a calculated cache context), or NULL if
+   *      there is no parameter.

@@ -148,7 +151,7 @@ public static function parseTokens(array $context_tokens) {
-      $contexts_with_parameters[$context_id] = $parameter;
+      $contexts_with_parameters[] = [$context_id, $parameter];

This is technically an API change (though it was just introduced, so disruption is zero.

--

RTBC!

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented 
+++ b/core/modules/comment/comment.module
@@ -197,17 +197,18 @@ function comment_field_config_delete(FieldConfigInterface $field) {
+ *
+ * @todo Remove this hook implementation in https://www.drupal.org/node/2429617

I still need to read all the issues around that, but why is removing that only possible there?

Shouldn't the bubble issue allow us to remove it already, assuming the pager actually adds this context as well?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

This is technically an API change (though it was just introduced, so disruption is zero.

Indeed, plus: this is an API function that is very, very, very rarely used. Only code that provides a UI that shows cache contexts is affected. In core, that's only BlockBase. In D8 contrib, likely nothing yet.

I still need to read all the issues around that, but why is removing that only possible there?

Because pager uses #theme => pager, not #type => pager. Preprocess functions cannot add cache tags. But #type => pager means we can have a #pre_render callback which adds it.

Shouldn't the bubble issue allow us to remove it already, assuming the pager actually adds this context as well?

Correct. But bubbling isn't in yet.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Ok, I'm not sure to which issue that @todo should actually point to. It currently points to #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!), but I think it should point to #2433599: Ensure every (non-views) pager automatically associates a matching cache context?

That would make sense to me, but pointing it to the first seems strange, because I'd expect need to solve those kind of issues before being able to use that kind of caching?

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented
Status: Reviewed & tested by the community » Needs work

Yes, lets put the @todo to this issue: #2433599: Ensure every (non-views) pager automatically associates a matching cache context

CNW, sadly on that ...

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs work » Reviewed & tested by the community
FileSize
pager_cache_context-2428563-22.patch6.81 KB
interdiff.txt781 bytes

#20: LOL! Oops!

Fixed.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed e57b339 on 8.0.x 
    Issue #2430341 by Wim Leers: Comment pager-dependent cache key should be...

Status: Fixed » Closed (fixed)

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