Problem/Motivation

CacheContextsManager::optimizeTokens() contains logic to remove redundant cache contexts. For example, if a rendered entity has one field that varies by 'user' and another that varies by 'user.permissions', the rendered entity can safely be cached just by 'user', because for any given user, his or her permissions do not vary by any other context. This optimization can also be leveraged by ESI integration modules, where reducing the context variations / Vary headers can have a large impact.

However, while AccountPermissionsCacheContext::getContext() doesn't vary by any context other than 'user', it does change when various configuration changes, such as when permissions are added to or removed from one of the user's roles. Therefore, when 'user.permissions' is optimized away in favor of 'user', the cache tags for those roles needs to be added to the cached item, so that the cache is invalidated when those roles are edited.

This is a general problem, not limited to user.permissions. For example, whenever the route.menu_active_trails:main_menu context is optimized away into route, the item needs to be tagged with all of the objects (in this example, the main menu's cache tag) that affect the active trail.

Proposed resolution

Remember that "caching" is basically "avoiding unnecessary computations". Therefore, optimizing a context away can be thought of as caching the result of the context service's getContext() method. In this case, it's an implicit cache (the value is discarded rather than stored), but the effect is the same: on a cache hit, the getContext() method is not called, hence: computations avoided.

So, the proposal is to do for cache context services what we do elsewhere for expressing that something is cacheable: use CacheableDependencyInterface. But, given that we also have calculated cache contexts, that receive parameters, we cannot actually implement CacheableDependencyInterface (since the cacheability metadata may depend on the parameter given to the calculated cache context, e.g. the route.menu_active_trails:main_menu cache context needs the config:menu.main_menu cache tag).
Therefore, we choose to use the next best thing: we update the two cache context interfaces:

interface CacheContextInterface {
  …
  /**
    * @return \Drupal\Core\Cache\CacheableMetadata
    */
  public function getCacheableMetadata();
}

and

interface CalculatedCacheContextInterface {
  …
  /**
    * @return \Drupal\Core\Cache\CacheableMetadata
    */
  public function getCacheableMetadata($parameter = NULL);
}

Then, the logic in CacheContexts::convertTokensToKeys() needs to return not a string[] containing the keys, but a new value object containing those keys plus cacheability metadata that is bubbled.

In thinking this through/analyzing this, we've also come to realize that HEAD is wrong in optimizing away user.node_grants when user is present, since node grants can have arbitrary complexity. But a specific site still may have perfectly cacheable node grants.
Thanks to the fact that cache contexts now return cacheability metadata, including max-age, we can now actually correctly leverage that for fixing this bug: the node grants cache context must return a CacheableMetadata object with max-age = 0. This tells the cache-context-optimizing-away logic that if it chooses to optimize the user.node_grants cache context away because user is also present, that it'll need to make the resulting cache item cached for at most zero seconds: i.e. it makes it uncacheable. So, this allows the logic to instead decide to not optimize the user.node_grants cache context away, because that'd in fact be worse than optimizing it away.
However, a specific site can override the default node grants cache context, and return a max-age of e.g. 3600 (1 hour), because its node grants are cacheable for at least an hour. On those sites, it'd then be possible to do the ['user.node_grants', 'user'] -> ['user'] optimization after all, but consequently only cache the result for an hour at most.

Remaining tasks

  • Tests.
  • Update the patch to have getCacheableMetadata() never return NULL, but always return a CacheableMetadata object.

User interface changes

None

API changes

  1. Added CacheContextInterface::getCacheableMetadata()
  2. Added CalculatedCacheContextInterface::getCacheableMetadata($parameter = NULL)
  3. Return value of CacheContextsManager::convertTokensToKeys() is changed

Data model changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because stale data is returned when configuration/content changes.
Issue priority Unclear if Critical or Major. It's kind of an information disclosure bug (permissions change, but you still see something you now shouldn't), but you can only see what you already once saw, so unclear if that would qualify as a true security vulnerability needing an SA.
Disruption Not disruptive, because the new interfaces are optional.
CommentFileSizeAuthor
#127 2512866-127-interdiff.txt7.45 KBBerdir
#127 2512866-127.patch53.73 KBBerdir
#123 2512866-123-interdiff.txt749 bytesBerdir
#123 2512866-123.patch52.58 KBBerdir
#121 2512866-121-interdiff.txt6.52 KBBerdir
#121 2512866-121.patch52.58 KBBerdir
#119 2512866-119-interdiff.txt763 bytesBerdir
#119 2512866-119.patch51.33 KBBerdir
#117 2512866-117-interdiff.txt13.2 KBBerdir
#117 2512866-117.patch51.46 KBBerdir
#112 2512866-111-interdiff.txt15.11 KBBerdir
#112 2512866-111.patch50.49 KBBerdir
#109 2512866-109-interdiff.txt14.94 KBBerdir
#109 2512866-109.patch40.28 KBBerdir
#106 2512866-107-interdiff.txt14.85 KBBerdir
#106 2512866-107.patch37.41 KBBerdir
#99 2512866-99-interdiff.txt3.89 KBBerdir
#99 2512866-99.patch36.56 KBBerdir
#95 2512866-95-interdiff.txt1.57 KBBerdir
#95 2512866-95.patch36.59 KBBerdir
#93 interdiff.txt2.53 KBlauriii
#93 2512866-93.patch36.59 KBlauriii
#90 interdiff.txt5.12 KBlauriii
#90 2512866-90.patch35.73 KBlauriii
#88 2512866-87.patch37.5 KBlauriii
#77 interdiff.txt11.49 KBlauriii
#77 2512866-77.patch24.83 KBlauriii
#72 interdiff.txt15.32 KBlauriii
#72 2512866-72.patch24.85 KBlauriii
#68 2512866-68.patch10.39 KBlauriii
#66 2512866-66.patch10.41 KBlauriii
#39 interdiff.txt26.39 KBlauriii
#39 follow_up_for_2432837-2512866-36.patch29.57 KBlauriii
#34 follow_up_for_2432837-2512866-34.patch24.11 KBlauriii
#22 follow_up_for_2432837-2512866-22.patch9.61 KBlauriii
#16 follow_up_for_2432837-2512866-16.patch38.49 KBlauriii
#16 follow_up_for_2432837-2512866-test-only-16.patch2.46 KBlauriii
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Filing as major because I'm too tired right now to thoroughly test it. Feel free to remove the "potential" keyword from the title and bump to critical.

Wim Leers’s picture

Discovered this by working on a fix for the failing tests in ToolbarAdminTest at #2429617-130: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!), yet the fix at #2217985-5: Replace the custom menu caching strategy in Toolbar with Core's standard caching. does not solve it, this is AFAICT the explanation. Did not have time yet to dig deeper.

Fabianx’s picture

Title: Follow-up for #2432837: potential security problem with optimizing ['user', 'user.permissions'] cache contexts to ['user'] » Follow-up for #2432837: Security problem with optimizing ['user', 'user.permissions'] cache contexts to ['user']
Priority: Major » Critical

Yes, we should have used 'permissions', because the permissions of a user are not part of the user itself and hence not invalidated, but generated by an external service that is even pluggable.

Proposed resolution:

Internally re-parent user.permissions to just permissions, so we don't need to rename all things ...

- user.permissions
- tags:
- { name: 'cache_context_reparent', 'new_context_name': 'permissions' }

Then push that information to the cache contexts service to reparent contexts before simplifying.

Alternative syntax:

user.permissions:
  alias: permissions

permissions: ...

Then take aliases into account in cache contexts and do the rename as usual ...

Security issues are critical per definition.

Wim Leers’s picture

+1 to permissions instead of user.permissions.

Permissions depend on roles, not on a user. This is why user.roles is correct: roles are a property of a user. But user.permissions is wrong, because permissions are a property of roles. So, there's a level of indirection, and user.permissions pretends there isn't one.

Wim Leers’s picture

IMO keeping user.permissions everywhere is problematic, because:

  1. it is misleading, because we'd rewrite it internally
  2. it means the simple explanation for how the cache context hierarchy works, no longer is true, leading to even more confusion
  3. encourages the wrong cache context (user.permissions instead of permissions) to be used everywhere

Note that in important mitigating factor is that 99% of access checks are using allowedIfHasPermission(s)() and cachePerPermissions(). That means none of those need to be updated!

Therefore I think we should bite the bullet and do a BC break.

Fabianx’s picture

I am okay with breaking BC, just showed a possibility to keep it very easily.

We could keep BC also for a few beta releases, then remove it - not sure how much that CC is used already.

Lets ask berdir.

lauriii’s picture

Issue tags: +Needs tests
Wim Leers’s picture

From IRC:

18:44:26 Fabianx-screen: berdir: How many modules use user.permissions cache context right now?
18:44:48 berdir: Fabianx-screen: uhm.. all of them? :p
18:44:54 Fabianx-screen: berdir: okay
18:45:08 Fabianx-screen: berdir: We need to rename user.permissions to permissions.
18:45:15 Fabianx-screen: berdir: The question now is: Keep BC or not.
18:45:23 Fabianx-screen: Or keep temporarily.
18:45:29 berdir: yep, was following
18:45:50 berdir: that was slightly exaggerated ;)
18:45:52 berdir: but there are a few
18:46:23 berdir: and it's a tricky thing to change, unless you have tests for it, it's quite hard to notice and problematic for security
18:46:49 berdir: changing it would be less of a problem after we've made it a required context in that other issue that we should really do anyway ?
18:47:40 Fabianx-screen: berdir: Well we can transparently keep BC and make it secure.
18:47:55 Fabianx-screen: berdir: It just means we resolve any aliases before checking the things.
18:48:48 Fabianx-screen: So even if you specified user.permissions, because we have a alias definition (for a few version perhaps), you would end up with permissions in all cached data.
18:53:14 Fabianx-screen: berdir: ^^ Thoughts? As you represent 'contrib' the most atm.

My reply:

18:53:20 WimLeers: berdir: no, you would notice VERY strongly
18:53:27 WimLeers: berdir: since 'user.permissions' is an invalid cache context then
18:53:31 WimLeers: berdir: you'd get an exceptoin
18:53:31 WimLeers: :)
18:53:45 berdir: good point
18:53:47 WimLeers: Cache::mergeContexts() calls \Drupal::service('cache_contexts_manager')->validateTokens($cache_contexts);
18:54:09 WimLeers: so any time your cache contexts are merged, you'd notice it
18:54:18 WimLeers: and note that merging does NOT do the optimizing
18:54:22 WimLeers: optimizing is done at the last possible step
18:54:30 WimLeers: and so by that point, it's guaranteed to have been merged 
18:54:44 WimLeers: That's why I don't fear subtle bugs there
Fabianx’s picture

The gist of #8 is:

We will create a BC break patch first, then get core committer feedback.

effulgentsia’s picture

Security issues are critical per definition

I don't know if this one is, but keeping it as such for now. How would you get information disclosure? You cached some rendered content for a user that the user had permission to see at the time you cached it. You then removed a role from the user or a permission from one of the user's roles. Now the user can see the same cached content even though he or she shouldn't based on the new configuration. But, you haven't disclosed any information that the user didn't already see at one time.

Yes, we should have used 'permissions', because the permissions of a user are not part of the user itself and hence not invalidated, but generated by an external service that is even pluggable.

I don't think I follow that logic. The permission hash generation is pluggable, but is there ever a use case for the permissions to depend on any dynamic information (e.g., the requested URL or time of day) other than the user? If so, then I agree with making it top-level, but if not, then I think that's incorrect. Because if all this is about is needing to associate some cache tags when folding a nested context into a parent one, can we then do that instead?

lauriii’s picture

Assigned: Unassigned » lauriii
effulgentsia’s picture

is there ever a use case for the permissions to depend on any dynamic information (e.g., the requested URL or time of day) other than the user?

Well, it's probably an edge case, but since it is theoretically possible, I guess it's correct for permissions to be top-level. However, I'd still like to be able to fold permissions into user when we get #2453835: Allow non-intrinsic (implementation-dependent) cache context services to specify their parents via DynamicParentContextInterface::getParents() done, which means at that time, we'll still need a way to add cache tags as part of that folding. No need for that to block this issue from proceeding as planned though.

We will create a BC break patch first, then get core committer feedback.

That's a nice way to ensure core is following the correct pattern everywhere. Don't know yet if we'll need to add the BC-layer for contrib.

lauriii’s picture

There is some modules which are using Drupal permissions handling to create time based permissions e.g. Tupas Authentication. There it wouldn't make sense to create custom cache context because what they are only depending on is user permissions. So I think that permissions can change on the fly.

Wim Leers’s picture

I don't think I follow that logic.

Me neither, I think the pluggability of permissions is the weakest argument.

Because if all this is about is needing to associate some cache tags when folding a nested context into a parent one, can we then do that instead

catch was strongly against that, but I can't find the issue at the moment. I proposed letting cache contexts specify cache tags at some point, but catch had pretty solid arguments for not doing this.


I think my explanation in #4 is better/clearer:

Permissions depend on roles, not on a user. This is why user.roles is correct: roles are a property of a user. But user.permissions is wrong, because permissions are a property of roles. So, there's a level of indirection, and user.permissions pretends there isn't one.

Fabianx’s picture

#14: Is not correct either, the permissions hash theoretically allows also user based permissions that are only granted for this user and maybe even just for a certain time frame.

With the hash based approach that works perfect obviously - but yes usually permissions are still per user so our original thing made sense in hindsight.

That said another approach would be:

- user should incorporate its children in the return of the 'user' cache context, if you purely need the users ID use [ user.uid ].

Maybe that is a better BC break?

Reasoning:

- [ user ] is similar to route that also has the [ url + request paramters ]
- user.uid is a strict subset of user

while currently

- 'user.roles' is a subset of 'user'

but

- 'user.permissions' is not a subset of 'user' (the output of it)

( which is the problem here)

So either:

- we cannot fold and move over to 'permissions'

OR

- we incorporate permissions hash into 'user' [user.uid:user.permissions] and ensure that code that only needs the users ID uses 'user.id' instead, which could be correctly folded into user as its again a subset.

lauriii’s picture

Status: Active » Needs review
Issue tags: -Needs tests
FileSize
2.46 KB
38.49 KB

Did a search and replace to change user.permissions to permissions. Also added test coverage for this.

lauriii’s picture

Assigned: lauriii » Unassigned

The last submitted patch, 16: follow_up_for_2432837-2512866-test-only-16.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 16: follow_up_for_2432837-2512866-16.patch, failed testing.

catch’s picture

- user should incorporate its children in the return of the 'user' cache context, if you purely need the users ID use [ user.uid ].

This seems sensible to me.

@Wim while I didn't want to associate cache tags with cache contexts in the previous discussion, that was more about a 1-1 relationship between contexts as tags, whereas this is more about invalidating the cache contexts themselves. So not 100% against, but if we do 'user' vs. 'user.uid' that seems happier.

lauriii’s picture

Assigned: Unassigned » lauriii
lauriii’s picture

Assigned: lauriii » Unassigned
Status: Needs work » Needs review
FileSize
9.61 KB

Started implementation for user.uid and user to be uid + permissions. I don't have a interdiff here because it wouldn't make any sense because there is very few things similar for the previous patch and I didn't even use it as a base for this.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Cache/Context/AccountPermissionsCacheContext.php
    @@ -7,35 +7,12 @@
     class AccountPermissionsCacheContext extends UserCacheContext {
    

    Do you mind to just make that implement CacheContextInterface given that there is simply no need for it? It is also semantically wrong now to extend it

  2. +++ b/core/lib/Drupal/Core/Cache/Context/UserCacheContext.php
    @@ -8,34 +8,50 @@
     /**
    - * Defines the UserCacheContext service, for "per user" caching.
    + * Defines the UserCacheContext service, for "per user and permissions" caching.
      */
     class UserCacheContext implements CacheContextInterface {
    
    +++ b/core/lib/Drupal/Core/Cache/Context/UserIdCacheContext.php
    @@ -0,0 +1,48 @@
    +/**
    + * Defines the UserCacheContext service, for "per user" caching.
    + */
    +class UserIdCacheContext implements CacheContextInterface {
    

    Yeah some more docs about when to use what would be nice

  3. +++ b/core/lib/Drupal/Core/Cache/Context/UserCacheContext.php
    @@ -8,34 +8,50 @@
       public function getContext() {
    -    return "u." . $this->user->id();
    +    return sprintf('u.%s.%s', $this->user->id(), $this->permissionsHashGenerator->generate($this->user));
       }
    

    We should explain why its not enough to just use the ID

  4. +++ b/core/lib/Drupal/Core/Cache/Context/UserIdCacheContext.php
    @@ -0,0 +1,48 @@
    +    return "u." . $this->user->id();
    

    Nitpick: Single quotes :P

larowlan’s picture

This looks pretty close

  1. +++ b/core/lib/Drupal/Core/Cache/Context/UserRolesCacheContext.php
    @@ -13,7 +13,7 @@
    +class UserRolesCacheContext extends UserIdCacheContext implements CalculatedCacheContextInterface{
    

    nit: needs a space before {

  2. +++ b/core/modules/system/src/Tests/Render/PermissionsCacheContextTest.php
    @@ -0,0 +1,78 @@
    +    $role = $authenticated_user->getRoles()[1];
    

    [1] looks a bit magic/fragile. Can we document this line or is there a constant we could be using? getRoles has a method to exclude locked built-in (anonymous/authenticated) roles, so could we use that and then reset() instead of hard-coding a 1? Using 1 is relying on an implementation detail, which we could avoid if we exclude the locked roles and use reset.

Wim Leers’s picture

user should incorporate its children in the return of the 'user' cache context, if you purely need the users ID use [ user.uid ].

Eh… so ['user'] should auto-expand to ['user', 'user.is_super_user', 'user.node_grants', 'user.permissions', 'user.roles']?

Logically speaking, that makes sense: "user implies user.*".

That was also the reasoning behind #2432837: Make cache contexts hierarchical (e.g. 'user' is more specific than 'user.roles'). But… that would completely undoes the optimizing part of #2432837. Are we okay with that? Or are you saying we only do the optimizing part in the X-Drupal-Cache-Contexts header?

I'm not yet convinced.

Fabianx’s picture

#25: No, that is not what this is doing:

It includes those things in the result, not expands it out.

e.g.

user => u.[user.uid].[user.permissions] as "result" means that all cases:

user.is_super_user => yes, included
user.roles => yes, included
user.node_grants => yes, included (because node grants are checked per user)
user.permissions => no, not completely included => add it hard-coded into the 'user' cache context.

Your main concern was that there is a legitimate case to only cache something by the users uid (not based on permissions) and user.uid (or user.id) solves that.

=> A [user] is the users UID plus the permissions hash.

Wim Leers’s picture

Oh wow that's even more confusing :(

I think it makes no sense to add a user.uid cache context. That's what user is intended to represent. But it could be argued that that is the best way out. Which doesn't make it "right", but could make it acceptable.

But, a stronger counter argument is: "where does it stop?" — i.e. what if the Organic Groups module in Drupal 8 adds a user.og cache context. That's very similar to the current user.permissions cache context: there's a level of indirection there. Just like a user has roles, a user can be a member of organic groups. But just like permissions exist on the role (user -> role -> permissions), the membership of a user can be determined at the Organic Groups level (user -> organic group -> members). Or, even, group-based permissions (user -> group -> permissions). Hence the same problem will exist there.

Do we just keep expanding the user cache context like this then? Yes, the Organic Groups module could override the user cache context with its own implementation. But… what if multiple modules do something like this? Only one module can override the service — one will win.

dawehner’s picture

Do we just keep expanding the user cache context like this then? Yes, the Organic Groups module could override the user cache context with its own implementation. But… what if multiple modules do something like this? Only one module can override the service — one will win.

Well, in case we would tell people how to use proper decorator services, this would be possible, but I agree its an odd statement.

Just an idea, could we get rid of using the service ID as magic name but rather require a service tag used to specify the cache context name? Once you have that, you could support multiple and take all their information into account.

catch’s picture

So for user.permissions and user.organic_groups.

I think we can fix both of those by attaching the current user's cache tags and those of their roles to those cache contexts.

If a user's organic groups membership changes - invalidate the cache tag for that user (and possibly the organic group itself but that's not an issue here).

if a role is updated with a new permission, then the role cache tag is updated.

This is where I argued against cache tags for cache contexts: https://www.drupal.org/node/2428703#comment-9737035

I think the arguments there (for language, and organic groups not associating the cache tags of all the groups) still apply, and this is really an exception.

However, it would be good to make it not-straightforward to add cache tags to cache contexts, since I think that could massively confuse people who are getting to grips with the concepts in general.

effulgentsia’s picture

Correct me if I'm wrong, but doesn't this problem exist anywhere that CacheContextsManager::optimizeTokens() optimizes away a child context in favor of a parent one, when in fact the child one's getContext() method returns something that depends on some configuration?

So, for example, route.menu_active_trails depends on the configuration of menu links in addition to the route, just as user.permissions depends on the configuration of roles in addition to the user.

So I think what we have here is a concept that cache tags for these configurations isn't needed so long as CacheContextsManager::optimizeTokens() doesn't optimize away the context, because the getContext() method itself returns a different value for a different configuration state. But, we need to add these cache tags whenever the context is optimized away in favor of its parent.

Not sure if such a more limited concept of when to apply the cache tags helps address #29.

Fabianx’s picture

Issue tags: +D8 Accelerate

#30: That is correct.

Lets look at the big picture of the cache context hierarchy:

There is:

  • a) Size (of header, etc.)
  • b) Speed of access
  • c) Varnish / Edge Side Cacheability

The big picture of the idea is that e.g. Varnish can reliably cache things, because all things collapse to either:

[url] or [user]

The idea is also that at least [url] is added before the final cache contexts are processed in a response subscriber of e.g. a varnish module, so that everything collapses to [url] that is dependent on that.

Yes, indeed then all the depends-on-config-using-cache-tags-being-invalidated will fail also in Varnish.

e.g. route.menu_active_trails

=> Having cache tags on contexts is a useful concept to also ensure *SI will work well.

For user on the other hand, the context hierarchy is checked downwards and appropriate values are returned (so that caching just user.roles or just user.permissions is possible).

However that result is returned by a controller handler cached by the users session in Varnish, so we would like to re-auth the user when e.g the permissions change, as then their hash changes. Currently the naive implementation is just time-based.

While the controller could very well hard-code cache tags for known things for now and provide an alter hook, that is a fragile solution when contrib/ comes into play.

=> Without cacheable metadata on contexts, any controller exposing context values to another system will need to hard-code the assumptions the contexts make. With cacheable metadata the merging of values will just work. (e.g. max-age=10 min for permissions hash, tags for config invalidation)

Therefore:

Having a proper API of getCacheTags(), getCacheContexts() (should be [] all the time), getCacheMaxAge() on cache contexts itself would be good, e.g. cacheable dependency interface.

That API is used not generically, but either when values are collapsed or when values are retrieved for caching the context values itself, not something using the context values.

Does that make sense?

pwolanin’s picture

Issue summary: View changes
lauriii’s picture

Assigned: Unassigned » lauriii
lauriii’s picture

Again completely new approach. This is definitely a WIP patch but I would like to hear feedback of different ideas how we could pass parameter for CacheContexts implementing CalculatedCacheContextInterface.

Wim Leers’s picture

  1. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -99,23 +100,28 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
    -   * @return string[]
    +   * @return \Drupal\Core\Cache\Context\CacheContextInterface[]
        *   The array of corresponding cache keys.
        *
        * @throws \InvalidArgumentException
        */
    -  public function convertTokensToKeys(array $context_tokens) {
    +  public function convertTokensToObjects(array $context_tokens) {
         $context_tokens = $this->optimizeTokens($context_tokens);
         sort($context_tokens);
    -    $keys = [];
    +    $cache_contexts = [];
         foreach (static::parseTokens($context_tokens) as $context) {
           list($context_id, $parameter) = $context;
           if (!in_array($context_id, $this->contexts)) {
             throw new \InvalidArgumentException(SafeMarkup::format('"@context" is not a valid cache context ID.', ['@context' => $context_id]));
           }
    -      $keys[] = $this->getService($context_id)->getContext($parameter);
    +      $service = $this->getService($context_id);
    +      // @todo find a better way to do this.
    +      if ($service instanceof CalculatedCacheContextInterface) {
    +        $service->setParameter($parameter);
    +      }
    +      $cache_contexts[] = $service;
         }
    -    return $keys;
    +    return $cache_contexts;
       }
    

    Interesting — this does not return cache context objects, but cache context *services*.

    I think we should keep $keys[] = $this->getService($context_id)->getContext($parameter);, but just have that return a cache context value object. That'd look like this:

    class CacheContextValue {
      protected $value;
      protected $cacheTags;
    }
    
  2. +++ b/core/lib/Drupal/Core/Cache/Context/CalculatedCacheContextInterface.php
    @@ -37,4 +37,9 @@ public static function getLabel();
    +  /**
    +   * @todo find better way to do this.
    +   */
    +  public function setParameter($parameter);
    

    Then this can go away.

  3. +++ b/core/lib/Drupal/Core/Cache/Context/CookiesCacheContext.php
    @@ -12,6 +12,8 @@
    +  use DefaultCacheContextTrait;
    
    +++ b/core/lib/Drupal/Core/Cache/Context/DefaultCacheContextTrait.php
    @@ -0,0 +1,68 @@
    +trait DefaultCacheContextTrait {
    +
    +  /**
    +   * The cache context IDs (to vary a cache item ID based on active contexts).
    +   *
    +   * @see \Drupal\Core\Cache\Context\CacheContextInterface
    +   * @see \Drupal\Core\Cache\Context\CacheContextsManager::convertTokensToObjects()
    +   *
    +   * @var string[]
    +   */
    +  protected $contexts = [];
    +
    +  /**
    +   * The cache tags.
    +   *
    +   * @var array
    +   */
    +  protected $tags = [];
    +
    +  /**
    +   * The maximum caching time in seconds.
    +   *
    +   * @var int
    +   */
    +  protected $maxAge;
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getCacheContexts() {
    +    return $this->contexts;
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getCacheTags() {
    +    return $this->tags;
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getCacheMaxAge() {
    +    return $this->maxAge;
    +  }
    +
    +  /**
    +   * Add cache context to the end of cache context list.
    +   *
    +   * @param string $context
    +   *
    +   * @return $this
    +   */
    +  public function addCacheContext($context) {
    +    $this->contexts[] = $context;
    +    return $this;
    +  }
    

    This architecture cannot actually work for calculated cache contexts, i.e. cases like user.roles:<role name>. Because the cache tags need to vary based on the specified role, and this architecture doesn't allow us to do so.

    Hence my proposal above.

Fabianx’s picture

class CacheContextsValue {
$value;
$cacheableMetadata;
}

we should probably also support max-age, though disallow cache contexts for obvious reasons ...

Status: Needs review » Needs work

The last submitted patch, 34: follow_up_for_2432837-2512866-34.patch, failed testing.

Wim Leers’s picture

we should probably also support max-age

Rationale?

lauriii’s picture

Status: Needs work » Needs review
FileSize
29.57 KB
26.39 KB

Thanks for the review! I was quite crazy with the previous patch. Hopefully I have included a little bit less craziness in this patch :P

Fabianx’s picture

Well, max-age is not critical.

  1. +++ b/core/lib/Drupal/Core/Cache/Context/UserCacheContext.php
    @@ -35,7 +35,17 @@ public static function getLabel() {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getCacheTags() {
    +    foreach ($this->user->getRoles() as $role) {
    +      $tags[] = "config:user.role.$role";
    +    }
    +    return $tags;
       }
    

    There is nothing to inherit from?

    --

    I thought we also have plain config:user.role cache tag, which I think would be better?

    @Wim: Thoughts?

  2. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
    @@ -304,7 +304,17 @@ protected function createCacheID(array $elements) {
    -        $contexts = $this->cacheContextsManager->convertTokensToKeys($elements['#cache']['contexts']);
    +        $context_objects = $this->cacheContextsManager->convertTokensToObjects($elements['#cache']['contexts']);
    ...
    +          if (isset($elements['#cache']['tags'])) {
    +            $elements['#cache']['tags'] = Cache::mergeTags($elements['#cache']['tags'], $context->getCacheTags());
    +          }
    +          else {
    +            $elements['#cache']['tags'] = $context->getCacheTags();
    +          }
    

    I still prefer just passing one cacheable metadata in as second argument that can be modified and is passed to each context.

    This will add 100s of calls for before-cache-case to getCacheTags() even though its always empty ...

    @Wim: Thoughts?

lauriii’s picture

Issue tags: +D8 Accelerate London

Status: Needs review » Needs work

The last submitted patch, 39: follow_up_for_2432837-2512866-36.patch, failed testing.

effulgentsia’s picture

This will add 100s of calls for before-cache-case to getCacheTags() even though its always empty

Right. I don't think we should do it there. Per #30 / #31, we should only call the context service's getCacheTags() method when that context is being optimized away (currently in optimizeTokens(), though currently that function doesn't have a place to attach those tags to, which is maybe the challenge this patch is having).

though disallow cache contexts for obvious reasons

Actually, I think a cache context service implementing getCacheContexts() has some good use cases, such as in the case of a cache context implementation varying by more than its ID implies. For example, core's implementation of user.permissions only varies by user (and configuration for which there are tags). But not by, e.g., the URL or time of day. But, suppose that some crazy contrib module swapped out how permissions were calculated and made them vary by URL or time of day. In this case, that user.permissions implementation could implement a getCacheContexts() method that returned those extra contexts, and optimizeTokens() could be improved to not optimize away contexts that depend on additional contexts unless those additional contexts are already in the token set. This could all be a non-critical followup, just pointing out that I don't think there's any conceptual reason to forbid this part of the interface.

effulgentsia’s picture

Re: the issue summary changes in #32:

Critical it might require a SA if needing to be fixed in a release

Per #10, I continue to be doubtful of this requiring an SA if it were discovered after release or being considered a security vulnerability even if discussed publicly, like it is here, after release. I still think it makes sense to work on until we know for sure whether it can be safely downgraded, and even after, but if we're going to keep it a critical, I'd like to know why it's a security vulnerability.

xjm’s picture

Assigned: lauriii » Unassigned

(Unassigning from @lauriii as he mentioned that more feedback is needed before proceeding.)

Fabianx’s picture

I still feel strongly about:

a) keeping the API change to a minimum
b) not add a performance regression for OOP-nicety

My proposal is:

getContext() => getContext(&$cacheable_metadata = NULL)
getContext($value) => getContext($value, &$cacheable_metadata = NULL)

That makes it optional, though we always pass it in and hopefully is not even an API change.

It also has just one CacheableMetadata instantiation as cost, which is cheap.

Wim Leers’s picture

  1. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextValue.php
    @@ -0,0 +1,60 @@
    + * Provides class for storing cache context with their cacheability metadata.
    

    s/cacheability metadata/cache tags/

  2. +++ b/core/lib/Drupal/Core/Cache/Context/UserCacheContext.php
    @@ -35,7 +35,17 @@ public static function getLabel() {
    +  public function getCacheTags() {
    +    foreach ($this->user->getRoles() as $role) {
    +      $tags[] = "config:user.role.$role";
    +    }
    +    return $tags;
    

    This is wrong. Plus, no need for cache tags here (since it only cares about the specific user, not the values of the user). This can be removed.

    I see the intent here (fix the specific problem described in the IS), but this doesn't solve the generic problem.

    To solve the generic problem ,we'll make sure that the cache contexts that the user cache context (this one) implies (the implied ones being user.permissions, user.roles etc.), when they are optimized, have their cache tags still be retained, if they have any.

  3. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
    @@ -304,7 +304,17 @@ protected function createCacheID(array $elements) {
    -        $contexts = $this->cacheContextsManager->convertTokensToKeys($elements['#cache']['contexts']);
    +        $context_objects = $this->cacheContextsManager->convertTokensToObjects($elements['#cache']['contexts']);
    +        $contexts = [];
    +        foreach ($context_objects as $context) {
    +          $contexts[] = $context->getValue();
    +          if (isset($elements['#cache']['tags'])) {
    +            $elements['#cache']['tags'] = Cache::mergeTags($elements['#cache']['tags'], $context->getCacheTags());
    +          }
    +          else {
    +            $elements['#cache']['tags'] = $context->getCacheTags();
    +          }
    +        }
    

    Fabian said This will add 100s of calls for before-cache-case to getCacheTags() even though its always empty ...
    … but that's an exaggeration, there will only be as many calls as there are things with both #cache[keys] and #cache[contexts]. So, perhaps dozens of cheap calls. This is not at all concerning.

  4. +++ b/core/modules/system/src/Tests/Render/PermissionsCacheContextTest.php
    @@ -0,0 +1,78 @@
    + * Tests the permissions cache context.
    ...
    +class PermissionsCacheContextTest extends KernelTestBase {
    

    This doesn't test the user.permissions cache context specifically, but the case of that cache context being optimized away. So I think the test name should be updated.

  5. +++ b/core/modules/system/src/Tests/Render/PermissionsCacheContextTest.php
    @@ -0,0 +1,78 @@
    +  /**
    +   * Ensures that permission change works with permission and user contexts.
    +   */
    +  public function testPermissionChange() {
    

    Great test :)

  6. I think what's missing here is this:
    +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -99,23 +100,24 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
         $context_tokens = $this->optimizeTokens($context_tokens);
    

    This is where we go from ['user', 'user.permissions'] to ['user'].

    +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -99,23 +100,24 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
    -      $keys[] = $this->getService($context_id)->getContext($parameter);
    +      $cache_contexts[] = $this->getService($context_id)->getContext($parameter);
    

    Hence we need to make sure here that the remaining 'user' cache context gets the cache tags of the 'user.permissions' cache context.

    (In other words: the cache tags of a context that is optimized away are bubbled to the cache context that implies them.)


#43:

Actually, I think a cache context service implementing getCacheContexts() has some good use cases, such as in the case of a cache context implementation varying by more than its ID implies.

-1, but congrats on blowing my mind.

This opens the door to infinite recursion. Besides, alternative implementations of user.permissions could just specify their parent cache context with #2453835: Allow non-intrinsic (implementation-dependent) cache context services to specify their parents via DynamicParentContextInterface::getParents(), so in your example, user.permissions would be implied by url.

Definitely out of scope here.

Wim Leers’s picture

It also has just one CacheableMetadata instantiation as cost, which is cheap.

So then… you're instantiating a CacheableMetadata object whenever you call getContext(). Because it is required to always collect the cache tags. If CacheContextsManager were to call getContext() with $cacheable_metadata === NULL, then we would indeed do less work, but we also wouldn't be solving this issue's problem. (Or am I missing something?)

So how is that any different from having getContext() return a CacheContextValue object, which by the way has fewer properties, so should be marginally faster?

Fabianx’s picture

Well, I was suggesting a reference, so the context itself would instantiate the cacheable metadata.

Given we still have very limited use cases, what about having an interface instead.

If not CacheableDependencyInterface, what about CacheContextDependencyInterface with one getCacheContexts($value = NULL) method?

Then we have one simple instanceof check, have no API change and you can very easily opt-in.

We also open potential extensions for the future.

pwolanin’s picture

Looks like we need to update the issue summary with proposed solutions and any new information

effulgentsia’s picture

What about matching AccessibleInterface::access() and doing getContext([$parameter = NULL,] $return_as_object = FALSE)?

effulgentsia’s picture

Or adding a separate method like getContextMetadata()?

Fabianx’s picture

#51: That was only, because we could not control access, but with cache contexts the only other user that directly looks at cache contexts is probably me.

I would really like a new Interface on this that is specific to this use-case - and avoid introducing any overhead as little as it may be - , but allows extension if needed.

effulgentsia’s picture

Discussed with Fabianx, and here's what we came up with:

CacheableCacheContextInterface extends CacheContextInterface, CacheableDependencyInterface {
}

CacheableCalculatedCacheContextInterface extends CalculatedCacheContextInterface {
  public function getCacheableMetadata($parameter = NULL);
}

For BC, contrib cache context implementations can choose to just implement CacheContextInterface/CalculatedCacheContextInterface, rather than the Cacheable* versions, in which case, they won't be optimized away. Core's should implement the cacheable ones, to set a good example.

Fabianx’s picture

Issue tags: +esi

Tagging ESI, because #54 is exactly what ESI (for 8.1) needs.

I think it is okay to use the same interfaces as everyone else (CacheableDependencyInterface) - even though at the moment we might only care for cache tags.

effulgentsia’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Completely rewrote issue summary.

effulgentsia’s picture

Issue summary: View changes
jibran’s picture

Title: Follow-up for #2432837: Security problem with optimizing ['user', 'user.permissions'] cache contexts to ['user'] » Security problem with optimizing ['user', 'user.permissions'] cache contexts to ['user']

Why we have to put "Follow-up for #2432837" in title? Removing it for now.

effulgentsia’s picture

Core's should implement the cacheable ones, to set a good example.

I changed my mind on that. For example, UrlCacheContext isn't itself cacheable -- you can't optimize it into any other context, which means it's nonsensical to express that its getContext() result can be cached. So, I think that's a good example of a class that should not implement CacheableCacheContextInterface. It could, but then to express itself properly, it would need to implement getCacheMaxAge() as returning 0. But I think not implementing the interface at all is a cleaner expression of its nature.

effulgentsia’s picture

Title: Security problem with optimizing ['user', 'user.permissions'] cache contexts to ['user'] » CacheContextsManager::optimizeTokens() optimizes ['user', 'user.permissions'] to ['user'] without adding cache tags to invalidate that when the user's roles are modified

Retitling to focus on the bug independent of whether it's a security problem (which per issue summary, I don't necessarily think it is). Leaving the security tag though until it's clearer whether that can be removed.

catch’s picture

Fwiw +1 to #54

Wim Leers’s picture

I'm very concerned about one implication of #54: a cache context can return other cache contexts. The recursion potential there is scary.

Fabianx/effulgentsia, could you please also clarify the pseudocode in #54? Because public function getCacheableMetadata($parameter = NULL); does not make sense if it's an implementation of CacheableDependencyInterface.

lauriii’s picture

Assigned: Unassigned » lauriii

Working on this now since we have a clear direction where to go

Fabianx’s picture

#54: If someone misuses that, they will get an error. I don't think mis-use is a reason to babysit things. If contrib does not implement the new interface, someone that wants to ESI their site can:

a) either exchange the service and/or write a patch for the contrib module, which then likely is correct.

The CacheableCalculatedCacheContextInterface does _not_ implement CacheableDependencyInterface, but just has a method:

/**
  * Returns the cacheable metadata associated with this context, based on the parameter value.
  *
  * @param mixed $parameter
  *    The parameter to get context values for.
  *
  * @return \Drupal\Core\Cache\CacheableDependencyInterface
  *   A cacheable dependency interface implementing object, e.g. CacheableMetadata
  */
public function getCacheableMetadata($parameter = NULL);
Wim Leers’s picture

But the non-calculated cache context interface does:

CacheableCacheContextInterface extends CacheContextInterface, CacheableDependencyInterface {
}

… and you're saying that the calculated one does. That seems highly inconsistent.

lauriii’s picture

Status: Needs work » Needs review
FileSize
10.41 KB

Started implementing the idea described in the IS.

Status: Needs review » Needs work

The last submitted patch, 66: 2512866-66.patch, failed testing.

lauriii’s picture

Status: Needs review » Needs work

The last submitted patch, 68: 2512866-68.patch, failed testing.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Cache/Context/AccountPermissionsCacheContext.php
    @@ -51,4 +51,14 @@ public function getContext() {
    +  public function getCacheTags() {
    +    foreach ($this->user->getRoles() as $role) {
    +      $tags[] = "config:user.role.$role";
    +    }
    +    return $tags;
    

    Let's provide $tags = []; to ensure that the result of the function is always an array.

  2. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -99,12 +100,23 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
    +    // Iterate through cache contexts and get their cacheable metadata.
    

    The comment refers to all kind of cacheable metadata, i guess mentioning cache tags is enough. Does someone documenting why just cache tags?

  3. +++ b/core/lib/Drupal/Core/Cache/Context/CacheableCacheContextInterface.php
    @@ -0,0 +1,22 @@
    +interface CacheableCacheContextInterface extends CacheContextInterface, CacheableDependencyInterface {
    
    +++ b/core/lib/Drupal/Core/Cache/Context/CacheableCalculatedCacheContextInterface.php
    @@ -0,0 +1,25 @@
    +interface CacheableCalculatedCacheContextInterface extends CalculatedCacheContextInterface {
    

    Once we document please let us make it clear what is the difference between them or why we need both rather.

Fabianx’s picture

  1. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -99,12 +100,23 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
       public function convertTokensToKeys(array $context_tokens) {
    

    Need to update all callers of this function, e.g. views cache plugins and tests, that should be responsible for many of the failures.

  2. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -99,12 +100,23 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
    +      if ($context instanceof CacheableCacheContextInterface || $context instanceof CacheableCalculatedCacheContextInterface) {
    +        $cacheable_metadata->addCacheTags($context->getCacheableMetadata()->getCacheTags());
    +      }
    

    This should just use:

    $cacheable_metadata = $cacheable_metadate->merge($context->getCacheableMetadata());

  3. +++ b/core/lib/Drupal/Core/Cache/Context/UserCacheContext.php
    @@ -40,4 +41,32 @@ public function getContext() {
    +  public function getCacheableMetadata() {
    +    return $this;
    +  }
    

    I like the idea and simplicity of this.

  4. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
    @@ -305,7 +305,11 @@ protected function createCacheID(array $elements) {
    +        $cid_parts = array_merge($cid_parts, $contexts['contexts']);
    +        if (!isset($elements['#cache']['tags'])) {
    +          $elements['#cache']['tags'] = [];
    +        }
    +        $elements['#cache']['tags'] = Cache::mergeTags($elements['#cache']['tags'], $contexts['cacheable_metadata']->getCacheTags());
    

    The best way is to use the pattern of:

    CacheableMetadata::createFromRenderArray($elements)->merge($contexts['cacheable_metadata'])
    ->applyTo($elements);

lauriii’s picture

Status: Needs work » Needs review
FileSize
24.85 KB
15.32 KB

This should be green patch

Fabianx’s picture

Assigned: lauriii » Wim Leers

Overall this looks wonderful to me!

+++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
@@ -113,7 +113,7 @@
-        $cacheable_metadata->addCacheTags($context->getCacheableMetadata()->getCacheTags());
+        $cacheable_metadata = $cacheable_metadata->merge(CacheableMetadata::createFromObject($context->getCacheableMetadata()));

Unless I miss something the createFromObject is redundant as two cacheable metadata should be able to be merged directly.

---

I could not find more, assigning to Wim for review.

lauriii’s picture

It's not possible because merge requires the parameter to be CacheableMetadata instead of CacheableDependencyInterface

Fabianx’s picture

Yes, right, thanks #74!

Fabianx’s picture

+++ b/core/lib/Drupal/Core/Cache/Context/CacheableCalculatedCacheContextInterface.php
@@ -0,0 +1,25 @@
+   * @return \Drupal\Core\Cache\CacheableDependencyInterface
+   *   A cacheable dependency interface implementing object, e.g. CacheableMetadata

+++ b/core/lib/Drupal/Core/Cache/Context/UserCacheContext.php
@@ -40,4 +41,32 @@ public function getContext() {
+  public function getCacheableMetadata() {
+    return $this;
+  }

I think given #74 it is better to indeed return a CacheableMetadata object here.

And just use the CacheableMetadata::createFromObject($this);

Thoughts?

lauriii’s picture

FileSize
24.83 KB
11.49 KB

Hmm that sounds good to me. Did that change and some doc updates. I also implemented the logic that cacheable metadata will be bubbled up only from the cache contexts that are optimized away

dawehner’s picture

+++ b/core/modules/system/tests/modules/pager_test/src/Controller/PagerTestController.php
index 99a32b3..3641773 100644
--- a/core/tests/Drupal/Tests/Core/Cache/Context/CacheContextsManagerTest.php

--- a/core/tests/Drupal/Tests/Core/Cache/Context/CacheContextsManagerTest.php
+++ b/core/tests/Drupal/Tests/Core/Cache/Context/CacheContextsManagerTest.php

Can we expand the unit test to also test the new inheritance?

Berdir’s picture

Seems OK overall, some feedback below.

Note that I haven't read most comments, just the issue summary and the patch. It will possibly also overlap with the review from Wim that he's working on.

* It might be worth mentioning the issue that introduced user.permissions in the issue summary. That was introduced to solve this problem for user.roles but the optimize use case was forgotten then. Discussed with @WimLeers, having user.permissions is still useful, even though we could make user.roles functionally equivalent by adding those cache tags there too and always using it. Agreed that this is preferable
* The other example mentioned in the issue summary is route.menu_active_trails, that doesn't seem to be addressed yet, would we do that in a follow-up? that one has arguments AFAIK, so it might actually be worth exploring here if my ideas below make any sense about caring about parameters when collecting the metadata.

  1. +++ b/core/lib/Drupal/Core/Cache/Context/AccountPermissionsCacheContext.php
    @@ -51,4 +51,15 @@ public function getContext() {
    +    foreach ($this->user->getRoles() as $role) {
    +      $tags[] = "config:user.role.$role";
    +    }
    

    Should we maybe use $rid to clarify what it is (I know that getRoles() is a bad method name, please don't hit me)

  2. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -99,23 +100,42 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
    -   * @return string[]
    -   *   The array of corresponding cache keys.
    +   * @return mixed[]
    +   *   An array containing two items:
    +   *   - contexts: string[]
    +   *   - cacheable_metadata: \Drupal\Core\Cache\CacheableMetadata.
    

    We have many different ways to do something like this now:

    * BlockRepository now uses a by-reference parameter (there it is actually a list of cacheable metadata).
    * Link generation uses an object (GeneratedLink or so I believe)

    I don't think we're using arrays yet, maybe it would be better to follow the by-reference approach, which has the advantage of not having to enforce an API change if we don't want to (but we can, by making the argument required)

  3. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -99,23 +100,42 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
    +    // Iterate through cache contexts that has been optimized away and get their
    +    // cacheable metadata.
    

    *have* been optimized

  4. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -99,23 +100,42 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
    +    foreach (static::parseTokens(array_diff($context_tokens, $optimized_tokens)) as $context_token) {
    +      list($context_id, $parameter) = $context_token;
    +      $context = $this->getService($context_id);
    +      if ($context instanceof CacheableCacheContextInterface || $context instanceof CacheableCalculatedCacheContextInterface) {
    +        $cacheable_metadata = $cacheable_metadata->merge($context->getCacheableMetadata());
    +      }
    +    }
    

    Is it worth considering the $parameter here somehow and e.g. pass it to getCacheableMetadata()?

    That might lead to less cache tags that need to be verified, for example.

  5. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -129,6 +149,9 @@ public function convertTokensToKeys(array $context_tokens) {
    +   * If cache context is being optimized away, it is able to set cacheable
    +   * metadata for itself which will be bubbled up.
    

    Either *a* cache context or use plural I think?

  6. +++ b/core/lib/Drupal/Core/Cache/Context/CacheableCacheContextInterface.php
    @@ -0,0 +1,28 @@
    + * Provides an interface for cacheable cache contexts.
    + */
    +interface CacheableCacheContextInterface extends CacheContextInterface, CacheableDependencyInterface {
    +
    

    What do we gain by supporting *both* getCacheableMetadata() and CacheableDependencyInterface? We have an easy shortcut to do this with createFromObject() that would save quite a few lines of duplicated code/docs? Except if we want to support that optoinal parameter argument but then we should only have one method..

  7. +++ b/core/lib/Drupal/Core/Cache/Context/UserCacheContext.php
    @@ -14,7 +16,7 @@
      */
    -class UserCacheContext implements CacheContextInterface {
    +class UserCacheContext implements CacheableCacheContextInterface {
     
    

    Why implement the interface, nothing is using it here? It's the permission cache context that actually needs it?

    Ah.. that extends from this so it gets the other methods through this. That doesn't really seem like a good idea to me, since it means that we need to merge int data from more contexts that aren't doing anything?

  8. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTestBase.php
    @@ -124,7 +125,7 @@ protected function setUp() {
             }
    -        return $keys;
    +        return ['contexts' => $keys, 'cacheable_metadata' => new CacheableMetadata()];
    

    Ah, unit tests might be the reason this was done, although I actually can't see any that would return something.

    It is possible though, although a bit convoluted because you need a returnValueCallback(), an example is in BlockPageVariantTest::testBuild()

Wim Leers’s picture

  1. +++ b/core/lib/Drupal/Core/Cache/Context/AccountPermissionsCacheContext.php
    @@ -51,4 +51,15 @@ public function getContext() {
    +  public function getCacheTags() {
    

    Let's add an @see \Drupal\Core\Session\PermissionsHashGenerator::generate.

  2. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -99,23 +100,42 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
    +        $cacheable_metadata = $cacheable_metadata->merge($context->getCacheableMetadata());
    

    This is not receiving the $parameter yet in case of a calculated cache context.

    This points to a gap in the test coverage.

  3. +++ b/core/lib/Drupal/Core/Cache/Context/CacheableCacheContextInterface.php
    @@ -0,0 +1,28 @@
    +interface CacheableCacheContextInterface extends CacheContextInterface, CacheableDependencyInterface {
    ...
    +  public function getCacheableMetadata();
    
    +++ b/core/lib/Drupal/Core/Cache/Context/CacheableCalculatedCacheContextInterface.php
    @@ -0,0 +1,31 @@
    +interface CacheableCalculatedCacheContextInterface extends CalculatedCacheContextInterface {
    ...
    +  public function getCacheableMetadata($parameter = NULL);
    

    This is my biggest concern.

    The first implements CacheableDependencyInterface, the second does not. This is for a good reason: the first doesn't need a parameter, but the second does.

    Therefore, the second must use a different way of specifying cacheability metadata: a getCacheableMetadata() method. But, for consistency reasons, we then add that method to the first as well.

    At that point though, it seems a rather convoluted system. It feels like it'd be better to have Cacheable(Calculated)CacheContextInterface::getContext() change the return value of getContext() from string to a CacheableContextValue (name TBD), which contains the string being returned in HEAD (in CacheContextInterface), plus cacheability metadata.

    IOW: what I said in #48.

    Performance difference of returning a string versus a value object should not be a concern: http://3v4l.org/qKsPF/perf vs http://3v4l.org/AO74i/perf.

  4. +++ b/core/lib/Drupal/Core/Cache/Context/CacheableCacheContextInterface.php
    @@ -0,0 +1,28 @@
    +   * Gets the cacheable metadata for the context based on the parameter value.
    

    There's no parameter in this case.

Fabianx’s picture

#80: Lets not introduce an API change here - if we can do it without.

I am okay with both just defining getCacheableMetadata($value = NULL) and then using in the non-calculated cache contexts as well:

function getCacheableMetadata() {
  $cacheable_metadata = new CacheableMetadata();
  $cacheable_metadata->setCacheTags();

  return $cacheable_metadata;
}

This is likely even faster as before we needed to create the CacheableMetadata anyway from an object.

I don't think we need to implement CacheableDependencyInterface, because we have dedicated Interfaces providing the getCacheableMetadata() method.

Edit:

And yes, I agree we need test coverage for the calculated case.

lauriii’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
lauriii’s picture

Assigned: Wim Leers » lauriii

Assigning away from Wim

Berdir’s picture

Ok, I've been discussing this issue at length here with @WimLeers and @lauriii. We basically made two circles and a looping around every option we could think of to solve this issue and discussing pros and cons.

First, to get this out of the way, we think that taking #2453835: Allow non-intrinsic (implementation-dependent) cache context services to specify their parents via DynamicParentContextInterface::getParents() into account here implies we are solving at least a significant portion of that issue. Which would then pretty much mean solving *that* issue as part of this *critical* issue. I will add a comment there why we should not build on top of this to solve that issue.

Now, to this issue, here's what we suggest:

1. Add *only* getCacheableMetadata() / getCacheableMetadata($parameter = NULL) to the existing two interfaces. This is an API change, but:
a) None of us are aware of any contrib/custom implementations since this is all fairly new, and contrib hasn't really picked this up and there really aren't that many implementations possible. (same argument as the one that moved all the cache context classes to a new namespace)
b) It avoids the complexity of having a total of 4 interfaces to choose from for anyone implementing a cache context. It also gives us a good place to document when and why you need this, since you are basically forced to implement it. (We could allow a null return value to avoid creating empty objects and merging them).

2. We use the CacheableMetadata object even though we only use tags and max-age, we just document on the new methods that any cache contexts set on the object will be ignored, even if set.

3. However, we do add a new value object CacheContextKeys (...Metadata?) that contains keys + tags + max-age, with getKeys(), getTags(), getMaxAge() and applyTo(). CacheContexsManager::convertTokensToKeys() would return it. That is again an API change but so would almost all other options there (except an optional argument) but we think that makes it easy to use and makes it clear that there is something additional that you have to care about if you're using it.

We discussed a number of other things, including interfaces to hint whether a context should be optimized away or not, many different options to return/collect the cacheability metadata but I think those three points above are a good compromise for solving this and keeping it easy to use.

As a last resort, we also discussed whatever-we-give-up-everything-is-stupid option to resolve this issue: Remove context optimization entirely since core itself doesn't need it. But I think that's not needed.

Wim Leers’s picture

berdir++
lauriii++

effulgentsia’s picture

#84 works for me.

We could allow a null return value to avoid creating empty objects and merging them

Per #59, UrlCacheContext (and others) will need to either return NULL or something like CacheableMetadata::createFromObject(NULL). I think both are sufficiently clear, so I don't have a preference on whether we allow the former or not.

we just document on the new methods that any cache contexts set on the object will be ignored, even if set

Would it be ok to add a @todo to this doc indicating that that might change in #2453835: Allow non-intrinsic (implementation-dependent) cache context services to specify their parents via DynamicParentContextInterface::getParents()?

Fabianx’s picture

+1 to #84.

I like the approach and I am onboard with the API change, especially if its just one method added.

Just one question as that did not get clear:

- Why explicit getTags(), getMaxAge(), ... on that new value object instead of just encapsulating the metadata?

Is this to _protect_ from anyone misusing context here?

If that is the reason I would argue an assert() (atm. _trigger_error) would be better here that cache contexts are indeed empty.

If there is another reason, I would love to hear about it.

lauriii’s picture

Status: Needs work » Needs review
FileSize
37.5 KB

Once again a new solution.

In this patch I didn't create getKeys(), getTags(), getMaxAge() and applyTo() methods but instead a getCacheableMetadata method which return CacheableMetadata object.

One question is that do we explicitly want to disallow adding cache contexts inside cache context services?

Status: Needs review » Needs work

The last submitted patch, 88: 2512866-87.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
35.73 KB
5.12 KB

Fixed the fatal and some clean up

Fabianx’s picture

Looks great already!

  1. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -99,23 +100,40 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
    +    if ($cacheable_metadata) {
    +      foreach (static::parseTokens(array_diff($context_tokens, $optimized_tokens)) as $context_token) {
    

    The if looks wrong here as its always true.

  2. +++ b/core/lib/Drupal/Core/Cache/Context/UserCacheContext.php
    @@ -7,6 +7,8 @@
    +use Drupal\Core\Cache\Cache;
    

    Unused in here.

Still needs tests for a FakeCalculatedCacheContext with a dynamic value.

Status: Needs review » Needs work

The last submitted patch, 90: 2512866-90.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
36.59 KB
2.53 KB

Maybe better luck this time. Also fixed #91

Status: Needs review » Needs work

The last submitted patch, 93: 2512866-93.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
36.59 KB
1.57 KB

Fixing the test fails there.

Wim Leers’s picture

Assigned: lauriii » catch
Issue tags: -Needs tests

Nice, green!

Given the new CacheContextOptimizationTest in this patch, we do have tests, so removing the corresponding issue tag. That being said, we do want to expand CacheContextsManagerTest as well I think.

But, what this patch really needs now, is a discussion/review with Alex Pott and catch, to get their take on the design/consequences of this. Assigning to catch to make that clear.

Fabianx’s picture

Issue tags: +Needs tests

#96: You asked for test coverage of a calculated cache context in #80.2 and CacheContextOptimizationTest does only test what is in the issue here, but not with a parameter.

Hence:

"Still needs tests for a FakeCalculatedCacheContext with a dynamic value."

Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextKeys.php
    @@ -0,0 +1,62 @@
    +  /**
    +   * Gets the cache context keys.
    +   *
    +   * @return string[]
    +   */
    +  public function getKeys() {
    +    return $this->keys;
    

    I always found the 'keys' key in #cache to pretty bad (I would have preferred cid_parts or something) and this isn't clear either.

    I'm not saying that we should change it, but we should atleast clarify what we are actually talking about here, possibly also on the convert method.

  2. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextKeys.php
    @@ -0,0 +1,62 @@
    +   * @return \Drupal\Core\Cache\CacheableMetadata
    

    needs a description.

  3. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -99,23 +100,38 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
    -   *   The array of corresponding cache keys.
    +   * @return \Drupal\Core\Cache\Context\CacheContextKeys
    

    Same.

  4. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -99,23 +100,38 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
    +    // Iterate through cache contexts that has been optimized away and get their
    +    // cacheable metadata.
    

    has => have.

  5. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -99,23 +100,38 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
    +      // To make it possible to return NULL instead of CacheableMetadata.
    +      if ($metadata = $context->getCacheableMetadata($parameter)) {
    

    Maybe reword it a bit. Something like "Contexts can return NULL if they have no cacheable metadata that needs to be bubbled up/merged."

  6. +++ b/core/lib/Drupal/Core/Cache/Context/CalculatedCacheContextInterface.php
    @@ -37,4 +37,18 @@ public static function getLabel();
    +   * @param mixed $parameter
    +   *   The parameter to get context values for.
    

    I'd say the parameter is string|null?

    Yes, you could write something like 'whatever:5' but it's still a string when being passed in here.

    Should also clarify what NULL means maybe? not sure how the other methods do that.

  7. +++ b/core/lib/Drupal/Core/Cache/Context/MenuActiveTrailsCacheContext.php
    @@ -33,4 +33,11 @@ public function getContext($menu_name = NULL) {
    +   * {@inheritdoc}
    +   */
    +  public function getCacheableMetadata($menu_name = NULL) {
    +    return NULL;
    

    This is the one that also needs something according to the issue summary. Then we also have something to be used for the parameter-test?

  8. +++ b/core/lib/Drupal/Core/Cache/Context/UserRolesCacheContext.php
    @@ -45,4 +45,11 @@ public function getContext($role = NULL) {
    +   * {@inheritdoc}
    +   */
    +  public function getCacheableMetadata($role = NULL) {
    +    return NULL;
    +  }
    +
    

    This extends UserCacheContext, so it doesn't have to re-implement this...

  9. +++ b/core/modules/comment/src/Tests/CommentRssTest.php
    @@ -52,16 +53,22 @@ function testCommentRss() {
     
    -    $this->assertCacheTags([
    -      'config:views.view.frontpage', 'node:1', 'node_list', 'node_view', 'user:3',
    -    ]);
    -    $this->assertCacheContexts([
    +    $cache_contexts = [
    

    This test seems to move the cache tags assertion around, makes it a bit harder to review?

  10. +++ b/core/modules/node/src/Cache/NodeAccessGrantsCacheContext.php
    @@ -82,4 +83,11 @@ protected function checkNodeGrants($operation) {
    +  public function getCacheableMetadata($operation = NULL) {
    +    return new CacheableMetadata();
    +  }
    

    return NULL?

  11. +++ b/core/modules/system/src/Tests/Entity/EntityCacheTagsTestBase.php
    @@ -653,7 +660,7 @@ protected function createCacheId(array $keys, array $contexts) {
    -    $cid_parts = array_merge($cid_parts, $contexts);
    +    $cid_parts = array_merge($cid_parts, $contexts->getKeys());
    

    As I said, cid_parts is so much clearer ;)

Berdir’s picture

FileSize
36.56 KB
3.89 KB

Ok, partially addressing my own review, but we have a few problems:

* MenuActiveTrailsCacheContext misunderstands what NULL means for getActiveTrailIds(). It does *not* mean all the menus, it means not restricted to a specific menu. So using that cache context without a parameter will not do what you think at all. I think that should just throw an exception that this is isn't supported?

* NodeAccessGrantsCacheContext: As far as I see, this must *not* be below user. There is no cacheable metadata that we can add that would fix this. This returns whatever hook_node_grants() returns and that can be *anything*.

Status: Needs review » Needs work

The last submitted patch, 99: 2512866-99.patch, failed testing.

xjm’s picture

Assigned: catch » Unassigned
Issue tags: +Needs framework manager review

Tagging per #96.

catch’s picture

  1. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextInterface.php
    @@ -31,4 +31,15 @@ public static function getLabel();
     
    +  /**
    +   * Gets the cacheable metadata for the context based on the parameter value.
    +   *
    

    Which parameter value?

  2. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextKeys.php
    @@ -0,0 +1,64 @@
    +
    +/**
    + * A value object to store generated cache keys with its cacheable metadata.
    + */
    +class CacheContextKeys {
    +
    +  /**
    

    Agreed with berdir on this, $cid_parts is much clearer on how this gets used.

    Cache keys sounds like a list of $cids which is not what it is. And are they even keys or really cache context values?

  3. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextKeys.php
    @@ -0,0 +1,64 @@
    +  /**
    +   * The cacheable metadata associated for the optimized cache contexts.
    +   *
    

    Do we care that they're optimized?

    We discussed static caching of cache contexts in other issues, and the cacheability metadata would allow that to happen (i.e. use the cache.static backend from #2501117: Add static caching for PermissionsHashGenerator::generate() the cache tag invalidations work even during the request). The important thing is that the render cache itself doesn't get the extra cacheability metadata unless the context is optimized away, but the information can still be useful for other use cases when it isn't.

  4. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -99,23 +100,38 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
    +    // Iterate through cache contexts that has been optimized away and get their
    

    Nit 'have'.

I'm not sure about this:

1. Add *only* getCacheableMetadata() / getCacheableMetadata($parameter = NULL) to the existing two interfaces. This is an API change, but:
a) None of us are aware of any contrib/custom implementations since this is all fairly new, and contrib hasn't really picked this up and there really aren't that many implementations possible. (same argument as the one that moved all the cache context classes to a new namespace)
b) It avoids the complexity of having a total of 4 interfaces to choose from for anyone implementing a cache context. It also gives us a good place to document when and why you need this, since you are basically forced to implement it. (We could allow a null return value to avoid creating empty objects and merging them).

We have only a single use case in core for a context with cacheable metadata - the user permissions cache context.

Contrib may have use cases - for example "user's organic groups" might get optimized to user, although even then I'm not sure whether that would need to be tagged with the groups, possibly not.

On the other hand, it's very easy to get turned around by this concept, and think that the language cache contexts should have language cache tags as metadata when they don't at all.

So having an extra, esoterically named interface that 99% of cache context implementors can ignore might not be so bad, compared to adding the method that encourages people to put something in there, when they really shouldn't.

However, I don't feel strongly about this at all. The number of people actually implementing cache contexts is going to be tiny anyway, since core provides what's needed for most cases - so the finer points of interface vs. method are not likely to affect many people at all, whereas fixing the user permissions case in core is important to get right.

As effulgentsia has pointed out, the consequences of not getting this right are that things get stale (or for my concerns get invalidated too often) - as opposed to other cacheability issues where different people see the wrong content. It's more of a cache race condition compared to other cache poisoning/information disclosure we're dealing with elsewhere, which is less of a worry if contrib gets it wrong too.

Berdir’s picture

My cid_parts/cache keys feedback was not very helpful. The problem is we *are* using cache keys all over the place, most notably in ['#cache']['keys'] and the cid we return is cache keys + keys from the contexts. So I don't think we should rename it here, it was more a remark a long "I wish we'd have used cid_parts instead of keys 1/2 years ago".

catch’s picture

#103 also makes sense.

Wim Leers’s picture

Issue summary: View changes
Issue tags: -Needs framework manager review

Discussed the proposed solution in this issue with @catch, @alexpott, @effulgentsia, @Gábor Hojtsy, @Berdir and @dawehner. Both @catch and @alexpott approve of this solution, so we can work on finishing this patch, notably the additional test coverage that we want and updating the patch to have getCacheableMetadata() never return NULL, but always return a CacheableMetadata object.

IS completely rewritten.

I hope I captured everything.

Berdir’s picture

Status: Needs work » Needs review
FileSize
37.41 KB
14.85 KB

Started implementing the things we decided, will need to update tests, also reverted my bogus changes in CommentRssTest.

Status: Needs review » Needs work

The last submitted patch, 106: 2512866-107.patch, failed testing.

Wim Leers’s picture

Interdiff review

  1. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -176,9 +173,15 @@ public function optimizeTokens(array $context_tokens) {
    +      elseif ($this->getService($context_token)->getCacheableMetadata()->getCacheMaxAge() == 0) {
    

    Let's use strict equality.

  2. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -176,9 +173,15 @@ public function optimizeTokens(array $context_tokens) {
           else {
    +
             $ancestor_found = FALSE;
    

    Accidental change.

  3. +++ b/core/lib/Drupal/Core/Cache/Context/MenuActiveTrailsCacheContext.php
    @@ -29,6 +29,10 @@ public static function getLabel() {
    +      throw new \InvalidArgumentException('No menu name provided for menu.active_trail cache context.');
    
    @@ -38,6 +42,9 @@ public function getContext($menu_name = NULL) {
    +      throw new \InvalidArgumentException('No menu name provided for menu.active_trail cache context.');
    

    Nit: It's "active trails", plural.

Patch review

  1. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextInterface.php
    @@ -31,4 +31,15 @@ public static function getLabel();
    +   * Gets the cacheable metadata for the context based on the parameter value.
    

    s/based on the parameter value//

    (This is just a copy/paste error; it comes from CalculatedCacheContextInterface, where it is correct.)

  2. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextInterface.php
    @@ -31,4 +31,15 @@ public static function getLabel();
    +   * @return \Drupal\Core\Cache\CacheableMetadata|NULL
    +   *   A cacheable metadata object or NULL.
    

    The "or NULL" part needs to be removed.

  3. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextKeys.php
    @@ -0,0 +1,64 @@
    +class CacheContextKeys {
    ...
    +  protected $cacheableMetadata;
    

    If this just contains CacheableMetadata, why not just change this to class CacheContextKeys extends CacheableMetadata?

  4. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -99,23 +100,35 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
    -   * @return string[]
    -   *   The array of corresponding cache keys.
    +   * @return \Drupal\Core\Cache\Context\CacheContextKeys
        *
    

    Incomplete.

  5. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -99,23 +100,35 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
    +    // Iterate through cache contexts that has been optimized away and get their
    

    s/through/over/
    s/has/have/

  6. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -99,23 +100,35 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
    +      list($context_id, $parameter) = $context_token;
    +      if (!in_array($context_id, $this->contexts)) {
    +        throw new \InvalidArgumentException(SafeMarkup::format('"@context" is not a valid cache context ID.', ['@context' => $context_id]));
    +      }
    +      $context = $this->getService($context_id);
    ...
           list($context_id, $parameter) = $context;
           if (!in_array($context_id, $this->contexts)) {
             throw new \InvalidArgumentException(SafeMarkup::format('"@context" is not a valid cache context ID.', ['@context' => $context_id]));
           }
    

    These bits are identical. Shall we put them in a closure (or protected helper method)?

  7. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -129,6 +142,9 @@ public function convertTokensToKeys(array $context_tokens) {
    +   * If cache context is being optimized away, it is able to set cacheable
    

    s/If c/If a c/

  8. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -157,9 +173,15 @@ public function optimizeTokens(array $context_tokens) {
    +      elseif ($this->getService($context_token)->getCacheableMetadata()->getCacheMaxAge() == 0) {
    

    In case of a calculated cache context, there's a parameter, and that parameter needs to be passed to getCacheableMetadata(). That's missing here.

  9. +++ b/core/lib/Drupal/Core/Cache/Context/CalculatedCacheContextInterface.php
    @@ -37,4 +37,18 @@ public static function getLabel();
    +   * @return \Drupal\Core\Cache\CacheableMetadata|NULL
    +   *   A cacheable metadata object or NULL.
    

    The "or NULL" part is wrong here too.

  10. +++ b/core/lib/Drupal/Core/Cache/Context/CalculatedCacheContextInterface.php
    @@ -37,4 +37,18 @@ public static function getLabel();
    diff --git a/core/lib/Drupal/Core/Cache/Context/CookiesCacheContext.php b/core/lib/Drupal/Core/Cache/Context/CookiesCacheContext.php
    
    +++ b/core/lib/Drupal/Core/Cache/Context/CookiesCacheContext.php
    @@ -7,6 +7,8 @@
    diff --git a/core/lib/Drupal/Core/Cache/Context/IsSuperUserCacheContext.php b/core/lib/Drupal/Core/Cache/Context/IsSuperUserCacheContext.php
    

    The remaining changes in these files can be reverted.

  11. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
    @@ -295,7 +295,7 @@ protected function maxAgeToExpire($max_age) {
    -  protected function createCacheID(array $elements) {
    +  protected function createCacheID(array &$elements) {
    

    Signature change without docblock update.

  12. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
    @@ -305,7 +305,10 @@ protected function createCacheID(array $elements) {
             $contexts = $this->cacheContextsManager->convertTokensToKeys($elements['#cache']['contexts']);
    ...
    +        $cid_parts = array_merge($cid_parts, $contexts->getKeys());
    ...
    +          ->merge($contexts->getCacheableMetadata())
    

    The $contexts variable name feels wrong/misleading now.

  13. +++ b/core/modules/book/src/Cache/BookNavigationCacheContext.php
    @@ -70,4 +70,11 @@ public function getContext() {
    +  public function getCacheableMetadata() {
    +    return NULL;
    +  }
    

    This one was missed in the update in #106.

Berdir’s picture

Status: Needs work » Needs review
FileSize
40.28 KB
14.94 KB

Was a bit early for a nitpick review, I just hacked on this until I was too tired to continue, but addressing it now :)

Interdiff:

3. Then the documentation that was added recently was wrong too, I copied from there ;)

patch:

3. Inheritance vs. Composition. I like it this way, it's a different kind of object.

5. I pointed this out in my reviews twice but forgot to update it too.

6. Not sure about the bets way to do this. What if we check this in parseTokens() and throw an exception there? We could also change the return value to be array($context_id => $parameter) instead of array(array($context_id, $parameter)), then we can avoid the list(). Why is this public?

7. Same as 5.

12. Yes. While doing this, it actually occurred to me that ContextCacheKeys is a much better name than CacheContextKeys. I think. So renamed.

Also fixed the unit tests. Hopefully without fatal errors now.

Status: Needs review » Needs work

The last submitted patch, 109: 2512866-109.patch, failed testing.

Wim Leers’s picture

interdiff.3: #sadpanda :( Sorry about that!

patch.3: Discussed with Berdir, he agrees with #108.patch.3 now, considering that that is then consistent with GeneratedUrl, GeneratedLink and FilterProcessResult. (Calculated)CacheContextInterface can't do that because it needs to accept a parameter; that's why it's okay for that one to diverge from this pattern.

patch.6: Hrm. I'd swear this has already been discussed in the past, but I can't find it. I think it's wrong to throw an exception in parseTokens() because then it's no longer just parsing. However, I realized we can basically do this:

@@ -105,14 +105,12 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
    * @throws \InvalidArgumentException
    */
   public function convertTokensToKeys(array $context_tokens) {
+    $this->validateTokens();
     $context_tokens = $this->optimizeTokens($context_tokens);
     sort($context_tokens);
     $keys = [];
     foreach (static::parseTokens($context_tokens) as $context) {
       list($context_id, $parameter) = $context;
-      if (!in_array($context_id, $this->contexts)) {
-        throw new \InvalidArgumentException(SafeMarkup::format('"@context" is not a valid cache context ID.', ['@context' => $context_id]));
-      }
       $keys[] = $this->getService($context_id)->getContext($parameter);
     }
     return $keys;

+++ b/core/lib/Drupal/Core/Cache/Context/CacheContextInterface.php
@@ -32,13 +32,16 @@ public static function getLabel();
+   * If a max-age of 0 is returned then it means that this context can not
+   * be optimized away.

Nit: 80 cols.

Berdir’s picture

Status: Needs work » Needs review
FileSize
50.49 KB
15.11 KB
Berdir’s picture

Note: The last patch does not yet address the 80 characters nitpick.

Status: Needs review » Needs work

The last submitted patch, 112: 2512866-111.patch, failed testing.

Wim Leers’s picture

  1. +++ b/core/lib/Drupal/Core/Cache/Context/AccountPermissionsCacheContext.php
    @@ -30,7 +30,7 @@ class AccountPermissionsCacheContext extends UserCacheContext {
    -   * @param \Drupal\user\PermissionsHashInterface $permissions_hash_generator
    +   * @param \Drupal\Core\Session\PermissionsHashGeneratorInterface
    

    Nit: This is technically out of scope, but yeah, the current FQCN was wrong. So +1. But this should not have removed the parameter name, let's bring that back :).

  2. +++ b/core/lib/Drupal/Core/Cache/Context/IsSuperUserCacheContext.php
    @@ -28,4 +30,11 @@ public function getContext() {
    +  public function getCacheableMetadata($menu_name = NULL) {
    

    This one should not have a parameter.

  3. +++ b/core/lib/Drupal/Core/Cache/Context/UserCacheContextBase.php
    @@ -0,0 +1,34 @@
    + * Base class for user based cache contexts.
    

    Nit: s/user based/user-based/

  4. +++ b/core/lib/Drupal/Core/Cache/Context/UserCacheContextBase.php
    @@ -0,0 +1,34 @@
    +   * Constructs a new UserCacheContext service.
    

    Nit: UserCacheContextBase
    Nit: s/service/class/

Wim Leers’s picture

12:10:35 WimLeers: catch: ping, Berdir renamed the value object returned by CacheContextsManager::converTokensToKeys() to ContextCacheKeys. Does that name make sense to you?
12:12:06 berdir: catch: it was CacheContextKeys before. this way makes more sense to me because it's cache keys for Context. And not context keys
12:12:19 catch: berdir: yes that's better.

I re-reviewed the entire patch, with a focus on the test coverage, and didn't find any problems.

  1. +++ b/core/lib/Drupal/Core/Cache/Context/ContextCacheKeys.php
    @@ -0,0 +1,64 @@
    +   * Constructs cache context keys value object.
    ...
    +   *   The cache context keys.
    

    Nit: s/cache context keys/cache contexts' keys/

  2. +++ b/core/lib/Drupal/Core/Cache/Context/ContextCacheKeys.php
    @@ -0,0 +1,64 @@
    +   *   The cacheable metadata associated for the optimized cache contexts.
    ...
    +   *   The cache metadata object.
    

    "cacheable" vs "cache" vs "cacheability". Let's be consistent.

  3. +++ b/core/lib/Drupal/Core/Cache/Context/ContextCacheKeys.php
    @@ -0,0 +1,64 @@
    +   * Gets the cacheability metadata associated for the optimized cache contexts.
    

    Nit: s/associated//

  4. +++ b/core/modules/system/src/Tests/Cache/CacheContextOptimizationTest.php
    @@ -0,0 +1,83 @@
    +    // change, which should have bubbled up for the element when it was optimized
    

    s/change/changes/

    80 cols.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
51.46 KB
13.2 KB

Tests fixed, addressed the nitpicks. Changed ContextCacheKeys to extend from CacheableMetadata. Can't say I really like it, but I can live with it ;)

Status: Needs review » Needs work

The last submitted patch, 117: 2512866-117.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
51.33 KB
763 bytes

Weird typo.

Wim Leers’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -125,7 +125,10 @@ public function convertTokensToKeys(array $context_tokens) {
    +    $context_cache_keys = new ContextCacheKeys($keys);
    +    return $context_cache_keys
    +      ->addCacheTags($cacheable_metadata->getCacheTags())
    +      ->setCacheMaxAge($cacheable_metadata->getCacheMaxAge());
    

    Can be simplified to

    return (new ContextCacheKeys($keys))->merge($cacheable_metadata);
    

    Or, if ContextCacheKeys had a setKeys() method — just like FilterProcessResult, GeneratedUrl etc. have setters also — then the initially created object could in fact be a ContextCacheKeys object already, and it'd be as simple as:

    return $context_cache_keys->setKeys($keys);
    
  2. +++ b/core/modules/system/src/Tests/Cache/CacheContextOptimizationTest.php
    @@ -69,7 +69,7 @@ public function testUserPermissionCacheContextOptimization() {
    +    // changes, which should have bubbled up for the element when it was optimized
    

    Still an 80 cols violation.

  3. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextInterface.php
    @@ -31,4 +31,18 @@ public static function getLabel();
    +   * Gets the cacheable metadata for the context.
    ...
    +   *   A cacheable metadata object.
    
    +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -129,6 +142,9 @@ public function convertTokensToKeys(array $context_tokens) {
    +   * If a cache context is being optimized away, it is able to set cacheable
    +   * metadata for itself which will be bubbled up.
    
    +++ b/core/lib/Drupal/Core/Cache/Context/CalculatedCacheContextInterface.php
    @@ -37,4 +37,21 @@ public static function getLabel();
    +   * Gets the cacheable metadata for the context based on the parameter value.
    ...
    +   *   A cacheable metadata object.
    
    +++ b/core/lib/Drupal/Core/Cache/Context/ContextCacheKeys.php
    @@ -0,0 +1,51 @@
    + * A value object to store generated cache keys with its cacheable metadata.
    

    s/cacheable metadata/cacheability metadata/

  4. +++ b/core/lib/Drupal/Core/Cache/Context/ContextCacheKeys.php
    @@ -0,0 +1,51 @@
    +  /**
    +   * The cacheable metadata associated for the optimized cache contexts.
    +   *
    +   * @var \Drupal\Core\Cache\CacheableMetadata
    +   */
    +  protected $cacheableMetadata;
    

    This should be deleted.

  5. +++ b/core/modules/book/src/Cache/BookNavigationCacheContext.php
    @@ -70,4 +71,12 @@ public function getContext() {
    +  public function getCacheableMetadata() {
    +    $cacheable_metadata = new CacheableMetadata();
    +    return $cacheable_metadata->setCacheMaxAge(0);
    +  }
    

    Why does this have max-age = 0? We should document the reason.

Berdir’s picture

Status: Needs work » Needs review
FileSize
52.58 KB
6.52 KB

1. I didn't think that works, using that now. I don't like setKeys() that means it is no longer an immutable object :) (as far as the primary value goes)

5. I guess this was based on #2483181: Make book navigation block cacheable, but but I was just guessing. Looked into it now and documented and implemented it properly. You're welcome to find any flaws in my thinking there ;)

Status: Needs review » Needs work

The last submitted patch, 121: 2512866-121.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
52.58 KB
749 bytes
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Two typos that can be fixed upon commit.

  1. +++ b/core/modules/book/src/Cache/BookNavigationCacheContext.php
    @@ -75,8 +75,20 @@ public function getContext() {
    +      // If the node is part of a book that we can use the cache tag for that
    

    s/that/then/

  2. +++ b/core/modules/book/src/Cache/BookNavigationCacheContext.php
    @@ -75,8 +75,20 @@ public function getContext() {
    +      // book. If not, then we can't be optimized away.
    

    s/we/it/

Hurray! :) Berdir++

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextInterface.php
    @@ -31,4 +31,18 @@ public static function getLabel();
    +  /**
    +   * Gets the cacheability metadata for the context.
    +   *
    +   * If the cache context is being optimized away, cache tags and max-age
    +   * provided by this method will be bubbled up into the cache item.
    +   *
    +   * If a max-age of 0 is returned then it means that this context can not
    +   * be optimized away.
    +   *
    +   * @return \Drupal\Core\Cache\CacheableMetadata
    +   *   A cacheable metadata object.
    +   */
    +  public function getCacheableMetadata();
    

    Let's improve the documentation to mention what it means when this method just returns an empty CacheableMetadata object.

  2. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -99,23 +100,34 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
        *
        * @throws \InvalidArgumentException
        */
    ...
    -      if (!in_array($context_id, $this->contexts)) {
    -        throw new \InvalidArgumentException(SafeMarkup::format('"@context" is not a valid cache context ID.', ['@context' => $context_id]));
    -      }
    

    Is this still true?

  3. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
    @@ -99,23 +100,34 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
    -        throw new \InvalidArgumentException(SafeMarkup::format('"@context" is not a valid cache context ID.', ['@context' => $context_id]));
    

    The SafeMarkup use statement can be removed.

  4. +++ b/core/lib/Drupal/Core/Cache/Context/MenuActiveTrailsCacheContext.php
    @@ -28,9 +29,24 @@ public static function getLabel() {
    +  public function getCacheableMetadata($menu_name = NULL) {
    +    if (!$menu_name) {
    +      throw new \InvalidArgumentException('No menu name provided for menu.active_trails cache context.');
    +    }
    

    Is it worth mentioning in the documentation that if the argument is invalid that the implementation should throw an InvalidArgumentException

  5. +++ b/core/lib/Drupal/Core/Cache/Context/RequestStackCacheContextBase.php
    @@ -12,7 +12,7 @@
     /**
      * Defines a base class for cache contexts depending only on the request stack.
      */
    -abstract class RequestStackCacheContextBase implements CacheContextInterface {
    +abstract class RequestStackCacheContextBase {
    

    I think we should document what interfaces implementations are expected to implement.

  6. +++ b/core/lib/Drupal/Core/Cache/Context/UserCacheContextBase.php
    @@ -0,0 +1,34 @@
    +/**
    + * Base class for user-based cache contexts.
    + */
    +abstract class UserCacheContextBase implements CacheContextInterface {
    

    According to @Berdir this class is not supposed to implement this interface.

Wim Leers’s picture

  1. +1
  2. Good catch. Should be @throws \LogicException, because that's what validateTokens() throws.
  3. Yep, sprintf() should be used instead now. But note that this is a deleted line, so :)
  4. +1
  5. It's only because this is an abstract base class that is used by both CacheContextInterface and CalculatedCacheContextInterface that this change was made. (HEAD was wrong there, but PHP being PHP allowed it.)
  6. Indeed, for same reasons as the prior point.
Berdir’s picture

Status: Needs work » Needs review
FileSize
53.73 KB
7.45 KB

Improved the docs and @throws, fixed the nitpicks from #124.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Looks splendid!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 46e6f72 and pushed to 8.0.x. Thanks!

diff --git a/core/lib/Drupal/Core/Cache/Context/RequestStackCacheContextBase.php b/core/lib/Drupal/Core/Cache/Context/RequestStackCacheContextBase.php
index d6ffd13..846c97d 100644
--- a/core/lib/Drupal/Core/Cache/Context/RequestStackCacheContextBase.php
+++ b/core/lib/Drupal/Core/Cache/Context/RequestStackCacheContextBase.php
@@ -14,7 +14,7 @@
  *
  * Subclasses need to implement either
  * \Drupal\Core\Cache\Context\CacheContextInterface or
- * \Drupal\Core\Cache\Context\CalculatedCacheContextInterface themself.
+ * \Drupal\Core\Cache\Context\CalculatedCacheContextInterface.
  */
 abstract class RequestStackCacheContextBase {
 
diff --git a/core/lib/Drupal/Core/Cache/Context/UserCacheContextBase.php b/core/lib/Drupal/Core/Cache/Context/UserCacheContextBase.php
index 602fde6..060cfb6 100644
--- a/core/lib/Drupal/Core/Cache/Context/UserCacheContextBase.php
+++ b/core/lib/Drupal/Core/Cache/Context/UserCacheContextBase.php
@@ -14,7 +14,7 @@
  *
  * Subclasses need to implement either
  * \Drupal\Core\Cache\Context\CacheContextInterface or
- * \Drupal\Core\Cache\Context\CalculatedCacheContextInterface themself.
+ * \Drupal\Core\Cache\Context\CalculatedCacheContextInterface.
  */
 abstract class UserCacheContextBase {
 

Removed a redundant (and incorrect - should be themselves) word.

  • alexpott committed 46e6f72 on 8.0.x
    Issue #2512866 by lauriii, Berdir, Wim Leers, Fabianx, effulgentsia,...
Wim Leers’s picture

Wim Leers’s picture

I was investigating the failures in #2429617-160: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!), the toolbar failures there were unexpected.

That's how I discovered that AFAICT we forgot about something in this issue all this time: user.permissions continues to work when a user receives *additional* roles. And because we "fixed" the issue here by adding role cache tags when optimizing away user.permissions, that case is *still* broken: it only captures the changes of permissions assigned to roles, it does not capture the changes of roles assigned to the user!
But fortunately we already have the necessary infrastructure in place thanks to this issue, because we solved it generically here. At least one solution is to use the same approach that we used for user.node_grants: consider user.permissions impossible to optimize away.

Thoughts?

catch’s picture

What about adding the user account cache tag to the per-user context?

Fabianx’s picture

Yes, we should add the 'user:[uid]' cache tag in that case as its a property of the user.

I am also okay to make it non-optimize-away-able in addition, but we should add that cache tag anyway.

YesCT’s picture

Do we need a separate issue to do what @catch asked for in #133, or is there an already existing one?

Fabianx’s picture

#135: Yes, we need a follow-up.

YesCT’s picture

Here it is #2533768: Add the user entity cache tag to user.* cache contexts that need it I wasn't sure about the priority or if it should similarly have the security tag or not.

Wim Leers’s picture

Thanks @YesCT for creating that stub, I've helped flesh it out further now.

This issue can now be closed automatically in 2 weeks :)

Status: Fixed » Closed (fixed)

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