content_moderation_entity_access() wrongly adds uncacheable dependencies

+++ b/core/modules/content_moderation/content_moderation.module
@@ -199,12 +199,12 @@ function content_moderation_entity_access(EntityInterface $entity, $operation, A
+
+    //  \Drupal\content_moderation\StateTransitionValidation::getValidTransitions()
+    // returns a list of transitions based on the user's permission to use them.
+    $access_result->cachePerPermissions();

The tricky part is that someone could override that service and actually *do* something that varies on more than just permissions.

So to handle it correctly, we would need to allow that service to return cacheability metadata I guess..

While we're here, I'm not sure what this check is meant to reflect, but no issues with that being a follow-up:

$moderation_info->isModeratedEntity($entity) && $entity->moderation_state

Some comments/concerns on #6:

• Why are we returning NULL in some cases? The hook docs look like they expect neutral.
• If $moderation_info->isModeratedEntity($entity) is FALSE, we return the NULL case, however wouldn't we also require the cacheability metadata of ::isModeratedEntity to be returned with that NULL/neutral case? It would need to be invalidated if the conditions of isModeratedEntity changed right? Ie, the cacheability of all 'update' entity access results for revisionable entities should depend on the workflow_list tag. I'm not sure if this makes sense, but something feels off.
• From an API perspective, making the service itself implement CacheableDependencyInterface isn't that useful if the cacheability depends on the arguments passed to getValidTransitions. You could argue, since our default implementation is what uses $entity, $workflow and user.permissions to make those decisions, all of those should be added in the body of that method, and optional for other implementations.

This cacheability stuff is far from a strong area for me, so happy to be corrected on the finer details here.

Also, not sure why the version was changed with my comment. Guessing this was meant for 8.6.x anyway.

Let's add some tests.

Rerolled #6 and added the test coverage as suggested in #11.

+++ b/core/modules/content_moderation/content_moderation.module
@@ -234,12 +234,9 @@ function content_moderation_entity_access(EntityInterface $entity, $operation, A
 
     $access_result->addCacheableDependency($entity);
-    $access_result->addCacheableDependency($account);
     $workflow = $moderation_info->getWorkflowForEntity($entity);
     $access_result->addCacheableDependency($workflow);
-    foreach ($valid_transition_targets as $valid_transition_target) {
-      $access_result->addCacheableDependency($valid_transition_target);
-    }
+    $access_result->addCacheableDependency($transition_validation);
   }

Lets add a check that $transition_validation implements CacheableDependencyInterface?

Or we on purpose don't do that, because then services that don't override it would be uncacheable? Not sure, but then we should probably document that somehow?

And I'm not sure if just unit tests are enough here. What I'd like to to see is a functional test that makes sure that a page isn't marked as uncacheable due to this.

I added a comment for #15 and a functional test (#16) that creates a node and asserts dynamic cache headers for the created node.

In my manual testing with content moderation and node entities of content_moderation_entity_access() (without the patch max-age: 0) I always ended up with max-age: -1 due to its result being merged with node_node_access() access result which defaults to permanent max-age. That said, I could not upload a failing Test-Only patch.

I am not sure if the current test is enough or whether we need another one (with non-node entities?) that would make a page uncacheable?

I see, this is a bit trickier than I remembered, because the problematic code is "only" in the $op = update case. That means we need to have something that checks that. For example the local tasks block. But that is a block and is then auto-placeholdered.

Lets go with a kernel test then, then we can assert the exact cacheability and make sure it's cacheable.

I've reviewed this again, I'm still not quite sure about the service implementing CacheableDependencyInterface directly. I think primarily my issue is, it doesn't actually provide all the appropriate cacheability information about decisions made within the service, the correct cacheability is a combination of:

Code in the service:

  public function getCacheContexts() {
    return ['user.permissions'];
  }

and dependencies added in the access hook:

    $access_result->addCacheableDependency($entity);
    $workflow = $moderation_info->getWorkflowForEntity($entity);
    $access_result->addCacheableDependency($workflow);

I think that's a bit misleading and leads me to believe that getValidTransitions should probably just optionally accept a RefinableCacheableDependencyInterface parameter.

Yeah, that's a fair point, I suppose it would be possible although pretty weird to build up an internal cacheablity metadata and return it.

I also thought about returning a value object as ArrayObject, but BC is impossible, because what we specifically do here in this case is if ($valid_transitions) and you can't simulate that.

It's much tricker with BC though because unfortunately, we have an interface. I'm a bit concerned on whether that will delay getting it committed. Attached is a patch that does that, but didn't update documentation BC stuff yet.

I think the quickest path to fixing the bug would be to keep the current approach and update content_moderation_entity_access with the correct metadata and file a follow up for creating an implementation where the content_moderation.state_transition_validation service can specify the metadata.

NW given the issues you already identified in #23. I'm not actually sure how to add methods to an interface with BC.

So basically going back to #2 with the test coverage? Fine with me :)

Created a combined patch of #2 and functional tests from #20.

Created #3095685: Allow content_moderation.state_transition_validation service to specify the cacheability metadata as a followup as per #24.

+++ b/core/modules/content_moderation/content_moderation.module
@@ -234,12 +234,12 @@ function content_moderation_entity_access(EntityInterface $entity, $operation, A
+    //  \Drupal\content_moderation\StateTransitionValidation::getValidTransitions()
+    // returns a list of transitions based on the user's permission to use them.

Extra space at the start of this line. Can we perhaps describe the service instead of refreshing the fully qualified method name, something like "The transition validation service returns ... ".

Beyond that, fix and test looks good to me.

Addressed #29.

Fixed #31.

Committed and pushed 84b9809a0b to 9.0.x and 56719db926 to 8.9.x. Thanks!

Is there an actual bug here apart from this not being cacheable? Is there any advantage to having this fixed in 8.8.0?

I guess the answer to that is, it depends. For our project, this was an important improvement. Depending on how/where access is checked, the max-age 0 can "bleed" into the main content and make that the whole page uncacheable for dynamic page cache. And also in the local tasks block, which is then converted to a placeholder, that can bubble up into the internal/anonymous page cache if you use https://www.drupal.org/project/cache_control_override or a similar custom solution.

Also, a placeholdered local tasks block can result in it being inserted later on into the page and moving things around with big_pipe.

On the other side, the current implementation might be hiding incorrect custom access checks that don't consider cacheability correctly and would get cached incorrectly once this is fixed.

I'm OK with it being in 8.9 only, happy it is now committed :)

On the other side, the current implementation might be hiding incorrect custom access checks that don't consider cacheability correctly and would get cached incorrectly once this is fixed.

Yeah this is why I'm tempted to let this stew in 8.9.x and not backport to 8.8.x. That gives us 6 months for any of this to come out of the woodwork.