Proposed commit message:

Issue #2428563 by Wim Leers, Fabianx: Introduce parameter-dependent cache contexts

Problem/Motivation

- Cache Contexts define the state of the system / the viewer in how objects should be rendered.

In the case of the menu trail (which is in reality usually a subset of the page cache context), this is dependent on the menu being shown.

However cache contexts currently are global for one thing, a user, a role, a page, node grants, etc.

Proposed resolution

To avoid having to register a cache context service for every menu instead, cache contexts are given a parameter.

To distinguish the method from the normal CacheContextInterface, a CalculatedCacheContextInterface is introduced, whose getContext method takes a parameter.

Those cache contexts _should_ be container aware so that the getLabel() method can be called without getting all the dependencies needed for calculating things.

Remaining tasks

- Fix nits.

User interface changes

- None.

API changes

Introduction of CalculatedCacheContextInterface, which allows developers to specify cache contexts that need a parameter.

See also:

See #2329101-19: CacheableInterface only has a getCacheKeys() method, no getCacheContexts(), leads to awkward implementations & #2329101-21: CacheableInterface only has a getCacheKeys() method, no getCacheContexts(), leads to awkward implementations.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because this is a gap in the API.
Issue priority Major because it's important that D8 core sets the right example for contrib developers WRT cache contexts. Without this API addition, some cache contexts cannot be expressed as … cache contexts. That forces developers to use the semantically wrong cache keys, which is a big DX problem — if we continue to have HEAD's bizarre/confusing/frustrating splitbrain approach, developers will consider the recommended approach caching another "DrupalWTF" (rightfully).
But it also has long-term negative effects for Drupal 8: it'd have prevented many Drupal 8 contrib modules to not set the correct caching metadata, hence causing incorrect caching, because cache contexts will soon bubble (see #2429257: Bubble cache contexts), and cache keys won't. In other words: if something actually is a cache context, but cannot be expressed as such because the API is lacking (well, missing even!), then it is hence impossible to write correct code!
Part of the critical #2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance meta.
Disruption Zero disruption. Only a small bit of disruption in the 0.001% chance that somebody in already written D8 custom or contrib code is actually altering the menu block.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Fabianx’s picture

From gut feeling I think this would muddy the waters of the very clear cache context system.

e.g. I would rather propose we use a #post_render_cache callback to render that, then to introduce parametrized cache contexts.

Of course there might be something I am missing. :)

Wim Leers’s picture

Wim Leers’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
14.87 KB

Done.

Introduces the concept of a "calculated cache context", which is different in only one way: it accepts a single (string) parameter. This can be encoded in the specified cache context as follows: <context ID>:<parameter> — e.g.: 'menu.active_trail:tools', where tools is the name of a menu.

This also allowed me to remove MenuActiveTrail::getCacheKey(), because it is now obsolete.

Status: Needs review » Needs work

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

Fabianx’s picture

This is fine now that we talked about it. The difference is:

cache_context.trail_tools_menu

vs.

cache_context.trail:tools_menu

e.g. a dedicated service configured with the parameter vs. a parametrized service.

This is still globally resolvable, which is important as some of those might need to be resolved at hook_boot() / very early kernel stage.

However for most practical purposes something rendered cache_context.trail can just be upgraded to cache_context.page.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
FileSize
14.4 KB

Will still not yet apply, but is a cleaner patch already. Will reroll to make it apply.

Wim Leers’s picture

Fabianx’s picture

+++ b/core/core.services.yml
@@ -33,6 +33,11 @@ services:
+  cache_context.menu.active_trail:
+    class: Drupal\Core\Cache\MenuActiveTrailCacheContext
+    arguments: ['@menu.active_trail']

I am not sure. I think making those container aware is better and loading things on demand.

Reason:

->label() which can be used in different contexts would lead to loading the menu trail service - even if it might not be needed. (and getService() gets called to check the interface).

And I think we should keep cache contexts as lightweight as possible?

---

Overall this looks great already!

Nice work!

Wim Leers’s picture

Title: Parameter-dependent cache contexts » [PP-1] Parameter-dependent cache contexts
Assigned: Wim Leers » Unassigned
Status: Needs work » Postponed

+1 to #8.

I first thought I could work on this even while #2329101: CacheableInterface only has a getCacheKeys() method, no getCacheContexts(), leads to awkward implementations is not yet committed (that's why the patches above don't apply: they're rolled on top of that one). But I can't. It's impossible to detect which cache keys are actually cache contexts… because they're mixed together. And that's exactly what #2329101 solves.

Wim Leers’s picture

FileSize
14.19 KB
2.35 KB

#8 implemented.

Wim Leers’s picture

Issue tags: -Needs tests
FileSize
18.69 KB

Added tests.

Wim Leers’s picture

Title: [PP-1] Parameter-dependent cache contexts » Parameter-dependent cache contexts
Status: Postponed » Needs review

The last submitted patch, 11: cachecontext_parameter-2428563-11.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, cachecontext_parameter-2428563-11.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
22.03 KB
4.58 KB

Reroll against HEAD. Changes:

  • updated parameter names and parameter docs to be highly consistent (with each other and with method names) instead of at least fairly confusing
  • updated docs throughout, to clarify what the role is of cache contexts, and what the two types of cache context tokens are (either A) just like before, B) the newly introduced: <context>:<parameter>)

I felt those additional changes were necessary to keep it clear what the CacheContexts service does and what the distinction is between these two types of cache contexts.

Wim Leers’s picture

FileSize
8.01 KB

This is the actual interdiff for #15. Sorry.

Status: Needs review » Needs work

The last submitted patch, 15: cachecontext_parameter-2428563-15.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
22.66 KB
1.51 KB

Forgot to update two assertions. Will be green.

Wim Leers’s picture

Fabianx’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Cache/MenuActiveTrailCacheContext.php
    @@ -0,0 +1,36 @@
    +    $active_trail = $this->container->get('menu.active_trail')
    +      ->getActiveTrailIds($menu_name);
    +    return 'menu_trail.' . implode('|', $active_trail);
    

    I don't know exactly what this functions return, but maybe this should be a hash?

    Also again the question on coming in with a parameter and returning without that parameter and a '.'.

    Maybe use:

    'menu.active_trail:' . $menu_name . '.' . $trail_hash?

  2. +++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
    @@ -206,9 +197,12 @@ public function getCacheTags() {
    +      'user.roles',
    +      'menu.active_trail:' . $menu_name,
    +    ];
    

    I love the simplicity of this!

  3. +++ b/core/tests/Drupal/Tests/Core/Cache/CacheContextsTest.php
    @@ -17,20 +19,19 @@
    +      'foo',
    +      'baz:parameter',
    +    ]);
     
    -    $expected = ['bar'];
    +    $expected = ['bar', 'baz.cnenzrgre'];
         $this->assertEquals($expected, $new_keys);
    

    It confuses me that I put in baz then ':' and receive baz and '.'.

    Should we not standardize that?

    e.g. it might be needed (not sure where) to calculate from a CID back to cache contexts.

  4. +++ b/core/tests/Drupal/Tests/Core/Cache/CacheContextsTest.php
    @@ -100,3 +126,26 @@ public function getContext() {
    +    if (!is_string($parameter) || strlen($parameter) ===  0) {
    +      throw new \Exception;
    +    }
    

    nit - should it not be \Exception() to be consistent with the rest?

Overall:

I think maybe getCacheContext() should just return the value, and convertTokensTo... should then add the replaced cache_context in front and separate with a '.'?

Fabianx’s picture

Spoke with Wim:

My proposed changes can be follow-up as the code is just moved.

Would still like to see the $menu_name in the corresponding CID, leaving at CNW for that as that parameter part is new.

I am fine with a follow-up for hash + what those cache contexts return.

Fabianx’s picture

Title: Parameter-dependent cache contexts » Introduce parameter-dependent cache contexts
Issue summary: View changes
Fabianx’s picture

Fabianx’s picture

Wim Leers’s picture

Status: Needs work » Needs review
Related issues: +#2430397: When mapping cache contexts to cache keys, include the cache context ID for easier debugging
FileSize
22.68 KB
1.36 KB
  1. Done, as per #21.
  2. :)
  3. So the follow-up issue you filed is #2430397: When mapping cache contexts to cache keys, include the cache context ID for easier debugging. Adding that to the related issues.
  4. Done.
Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, looks great now! Thanks!

Fabianx’s picture

Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

This issue is a major task. To evaluate whether we should make this change during the beta, we need to update the issue summary according to https://www.drupal.org/core/beta-changes to explain what improvements it makes and what the disruption of the change would be. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

Wim Leers’s picture

Category: Task » Bug report
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update +API addition

Added beta evaluation, updated CR.

catch’s picture

The issue summary is out of date - I just committed the book issue that doesn't use this.

So is this just for #2430341: Comment pager-dependent cache key should be a cache context now?

Wim Leers’s picture

The book one (#2429261: Replace the hardcoded cache key on the book navigation block with a 'book navigation' cache context) does not need a parameter.

This issue for both MenuActiveTrail (converted as part of this patch) as well as indeed #2430341: Comment pager-dependent cache key should be a cache context, and the cases that contrib/custom modules will surely encounter.

catch’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/lib/Drupal/Core/Cache/CacheContexts.php
    @@ -61,64 +69,87 @@ public function getAll() {
    +   *   values the associated parameter (for a calculated cache context), if any.
    

    This sentence doesn't parse for me. Should it be:

    "An array with the parsed results, with the cache context IDs as keys, and the associated parameter as value (for a calculated cache context) or NULL if there is no parameter." or similar?

  2. +++ b/core/lib/Drupal/Core/Cache/CacheableInterface.php
    @@ -33,6 +35,8 @@ public function getCacheKeys();
    +   * These identify a specific variation/representation of the object.
    

    Just a general note there's a slight grey area here for something like a view mode with the terminology. I'd expect this to be part of the cache key, but it's more colloquially a 'variation/representation' of an object. Not to fix here of course.

The menu trail implementation in the patch makes sense. I'm not 100% on the comment pager patch as it stands - discussed with Wim in irc and we can continue in that issue.

Fabianx’s picture

The menu_trail is a cache_context as its a sub set of the cache_context.page - and as such usually implicitly bound to that.

This is not true for the view_mode.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
22.72 KB
941 bytes
  1. You're right — thanks!
  2. Right.
Wim Leers’s picture

Opened #2432837: Make cache contexts hierarchical (e.g. 'user' is more specific than 'user.roles') for #33, since that has been brought up several times now. Better to have a dedicated issue, even if only for discussing it.

  • catch committed dfaf2e6 on 8.0.x
    Issue #2428563 by Wim Leers:  Introduce parameter-dependent cache...
catch’s picture

Status: Reviewed & tested by the community » Fixed

OK happy with this now.

Committed/pushed to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

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