Add the user entity cache tag to user.* cache contexts that need it

AFAIK the cacheable metadata would not get loaded unless the context is optimized away

Yeah, we need an IS update with the bug described.

Yes, we either need to change it so that it's always added or we need to add it to the subcontexts.. (because that is unexpected if it changes) and if code depends on something from the user then it also needs to add relevent cache tags.

IS updated.

Basically, this is a new issue created from #2512866-132: CacheContextsManager::optimizeTokens() optimizes ['user', 'user.permissions'] to ['user'] without adding cache tags to invalidate that when the user's roles are modified, with two relevant discussion comments: the subsequent comments #133 and #134 there.

A recap!

So what @catch proposed in #133 of that issue (What about adding the user account cache tag to the per-user context?) doesn't really make sense, for the reason @lauriii mentions in #1: we only add cacheability metadata of a cache context if it is optimized away.

A solution to that problem would be what @Berdir says in #3: always add the cacheability metadata of a cache context, even when it is not optimized away. But that'd be a significant behavior change, and it took us many days and many discussions to arrive at the solution we found in #2512866: CacheContextsManager::optimizeTokens() optimizes ['user', 'user.permissions'] to ['user'] without adding cache tags to invalidate that when the user's roles are modified. So I'm kinda reluctant to just do it. (Even though I was advocating for that in #2512866 originally.)

@Berdir also briefly mentions or we need to add it to the subcontexts.., i.e. have a parent cache context provide cacheability metadata for its children in case it is optimized away. I like the idea at first sight, but I don't think that actually makes sense. Not only would the implementation be messy, but if we look at all user.* cache contexts (copy/pasted from https://www.drupal.org/developing/api/8/cache/contexts):

user
  .is_super_user
  .node_grants
    :operation
  .permissions
  .roles
    :role

Then we see that it doesn't apply universally: it does not apply to the user.is_super_user cache context.

So after the recap in #5 of everything that was discussed, here's my proposal: just have those child cache contexts for which this does matter just specify the user cache tag.

That'd look simply like this.

Whether this is good enough comes down to one question:

What is the expectation when adding the the user cache context and you do something that depends on e.g. a field of the user. Now that field changes. Are you responsible for adding the user cache tag in that case yourself or would you expect that "vary by user" takes care of that?

If the first (you have to explicitly say "is invalidated when the user changes) then it's fine. If not, then we still have a problem.

I'm not sure :)

+++ b/core/modules/node/src/Cache/NodeAccessGrantsCacheContext.php
@@ -89,6 +89,8 @@ protected function checkNodeGrants($operation) {
+    $cacheable_metadata->setCacheTags(['user:' . $this->user->id()]);
+
     if (!\Drupal::moduleHandler()->getImplementations('node_grants')) {

Not sure this makes sense. Per the documentation, cache tags are ignored if max-age is zero, so there's no point in defining it.

And if we don't return max-age then that just means that there are no node grant implementations, which doesn't need to be invalidated. unless code or the module list changes (I still think we should delete all caches if a module is installed, but that's another topic).

Also, AFAIK the "security" tag should only be used for actual security issues.. the other one should be security enhancements or something like that. So, security shouldn't be used on non-critical issues...

Note: That is different from user.roles or user.permissions. If you say it varies by the permissions of the user, then any action that can affect the permissions (or roles) of a user should be taken care of for you, definitely. And that's what we're doing with this patch.

#7: I agree, it makes sense for user.permissions and user.roles, huge +1, I think we should leave user.node_grants alone ...

I just changed my mind.

Even if there are no node grants active, when the user gets added some node grants then ... we should invalidate, but we do indeed not invalidate the user then ...

So probably node_grants should be another follow-up and keep this focused to the issue at hand.

Major as it soft blocks smart-cache.

What is the expectation when adding the the user cache context and you do something that depends on e.g. a field of the user. Now that field changes. Are you responsible for adding the user cache tag in that case yourself or would you expect that "vary by user" takes care of that?

Yes, if you render something that not just varies by user, but contains some property of the user, then you must add User::getCacheTags() to that rendered thing's cacheability. This has always been the case.

Not sure this makes sense. Per the documentation, cache tags are ignored if max-age is zero, so there's no point in defining it.

Oh heh :P They'd indeed be ignored. But the cache tag also does no harm. I'm thinking: what if contrib comes in and just subclasses this class, and overrides the max-age to a non-zero value?

And if we don't return max-age then that just means that there are no node grant implementations, which doesn't need to be invalidated

Good point. So what if I just move the user:X cache tag just a little bit, like this?

RTBC - if tests pass :).

Tested the previously failing test manually as well, couldn't find anything wrong, so RTBC +1

Updated IS, added beta eval.

Looks good! I'm happy to see that the solution on the parent issue actually created API which solves this kind of issues. I think what we still need is better documentation about how this works to make people actually understand. Now there is way larger difference between applying user and user.roles cache contexts than it was before and people should understand that.

Just as a last note, shouldn't we have tests for this since this is a bug?

I'm happy to see that the solution on the parent issue actually created API which solves this kind of issues.

Indeed :)

I think what we still need is better documentation about how this works to make people actually understand. Now there is way larger difference between applying user and user.roles cache contexts than it was before and people should understand that.

That's already done: https://www.drupal.org/node/2459039/revisions/view/8630634/8652850

1. +++ b/core/lib/Drupal/Core/Cache/Context/AccountPermissionsCacheContext.php
   @@ -57,7 +57,7 @@ public function getContext() {
   +    $tags = ['user:' . $this->user->id()];
   

   Change looks fine, but we should add inline docs to explain why this needs both the user ID cache tag and the role cache tag - just 'Permissions may change when either a role is updated to have different permissions, or a user is updated to have different roles' should do it?

2. +++ b/core/modules/node/src/Cache/NodeAccessGrantsCacheContext.php
   @@ -93,6 +93,8 @@ public function getCacheableMetadata($operation = NULL) {
          return $cacheable_metadata;
        }
    
   +    $cacheable_metadata->setCacheTags(['user:' . $this->user->id()]);
   +
        // If the site is using node grants, this cache context can not be
        // optimized.
        return $cacheable_metadata->setCacheMaxAge(0);
    
   

   If max age is 0 what does the cache tag do here?

2) This has been discussed a bit, see second part of #7 and the response in #10. If you disagree with that then it can be removed, but that's the reason.

#7/#10 is OK just needs to be reflected in the inline comment that it's done for correctness, that the max-age might change etc.

I'll reroll this in 1-2 hours.

Ok, got sidetracked in #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) for a while. So, with a small delay, here's an updated patch.

RTBC + 1 again

Just as a last note, shouldn't we have tests for this since this is a bug?

Yes.

I will write the tests so we can get this in from blocking the Smart Cache

Woot! Thanks @lauriii :)

This was a little simpler than I thought in the beginning. No interdiff since test-only patch works as interdiff.

Looking great :)

1. +++ b/core/modules/system/src/Tests/Cache/CacheContextOptimizationTest.php
   @@ -80,4 +80,45 @@ public function testUserPermissionCacheContextOptimization() {
   +   * Ensures that 'user.roles' cache context is defining cacheable metadata.
   

   s/is defining cacheable metadata/still works when it is optimized away/

2. +++ b/core/modules/system/src/Tests/Cache/CacheContextOptimizationTest.php
   @@ -80,4 +80,45 @@ public function testUserPermissionCacheContextOptimization() {
   +    $user1 = $this->createUser();
   

   s/$user1/$root_user/

3. +++ b/core/modules/system/src/Tests/Cache/CacheContextOptimizationTest.php
   @@ -80,4 +80,45 @@ public function testUserPermissionCacheContextOptimization() {
   +    $role = $authenticated_user->getRoles()[1];
   

   You're trying to get the role other than the 'authenticated' one, I think?

   I think this only works because usually, the randomly generated role name will not alphabetically be something like abXXXX. But it could. So I think this introduces a random fail.

4. +++ b/core/modules/system/src/Tests/Cache/CacheContextOptimizationTest.php
   @@ -80,4 +80,45 @@ public function testUserPermissionCacheContextOptimization() {
   +    // 'user.roles' cache context defined a cacheable metadata for permission
   

   s/cacheable metadata/cache tag/

5. +++ b/core/modules/system/src/Tests/Cache/CacheContextOptimizationTest.php
   @@ -80,4 +80,45 @@ public function testUserPermissionCacheContextOptimization() {
   +  }
    }
   

   Nit: needs \n in between.

Thanks for the review!

#30.3: 'authenticated' is a special case. User::getRoles() is always setting the 'authenticated' user role as first manually.

Interdiff for the previous patch

Hello I am a RTBC fairy, I saw this nice little patch and just could not go further without RTBC'ing :p.

Seriously:

Great work, back to RTBC.

Committed/pushed to 8.0.x, thanks!

Woohoo!

This was the final blocker to #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) :)

+++ b/core/modules/system/src/Tests/Cache/CacheContextOptimizationTest.php
@@ -80,4 +80,45 @@ public function testUserPermissionCacheContextOptimization() {
+    // cache context defined a cache tag for user entity changes23, which should

I think there is a typo in the code comments. What is "user entity changes23"?

That typo snuck in in the #31/#32 reroll. Can you file a follow-up issue to fix that?

Follow up issue added here: https://www.drupal.org/node/2540764

Sorry that I comment so old issue, but I'm creating custom cache context similar to "UserRolesCacheContext", and I don't understand why "user:ID" cache tag is needed exactly here. (Or I understand cache context implementations wrong)

As far as I understand "::getContext()" value is always calculated, and "::getCacheableMetadata()" is merged with cache data, that uses the context.

In this way lets get users 2 and 3 that have the same role "viewer", and some abstract block with "user.roles" cache context.

Case:

• User 2 views it first, cache is created with "user:2" tag.
• User 3 views it later, he see cached data, with "user:2" tag.
• When user 2 roles are changed - the cache is invalidated.

But for user 3 the block is still valid, looks like cache invalidation isn't needed.

Other case:

• User 2 views it first, cache is created with "user:2" tag.
• User 3 views it later, he see cached data, with "user:2" tag.
• When user 3 roles are changed - the cache isn't invalidated.
• User 2 views cached data, as "['viewer']" roles context matches for him
• User 3 sees other content - because his context is ["other_role"] now, but not "['viewer']"

Looks like everything works without "user:ID" tag.

Reading comments I see that issues take place when lower "user.roles" context is optimized to use with "user" context. And "user:ID" is needed to invalidate caches when target user is changed. But why the cache metadata implemented not on "UserCacheContext" level?

Looks like it leads to the same result and looks more correct:

Buggy case as I see it for user 4:

• Admin menu cached with "user.roles" context
• Cache was optimized with "user" context, and equal to "user=4"
• No "user:4" tag is set
• When "admin" role is removed from user no menu cache invalidated
• User still has ID 4 and context "user=4" is valid for him

Current case:

• Admin menu cached with "user.roles" context
• Cache was optimized with "user" context, and equal to "user=4"
• "user:4" tag is set from "user.roles" context, where it is not needed itself
• When "admin" role is removed from user cache invalidated
• Everything works

Correct case:

• Admin menu cached with "user.roles" context
• Cache was optimized with "user" context, and equal to "user=4"
• "user:4" tag is set from "user" context, which looks logical because this cache belongs to this user only with full "user" context
• When "admin" role is removed from user cache invalidated
• Everything works