NodeController::revisionOverview is uncacheable

Came here from Bug smash initiative. I am still able to replicate this issue on D11 and tried initial level of debugging.

Wondering why CacheableMetadata::createFromRenderArray($username)->addCacheableDependency($node) or CacheableMetadata::createFromRenderArray($username) are not working as expected. Checked type of argument that i am passing and also looked into multiple other ways of defining it, still x-drupal-dynamic-cache is not coming on response headers.

Might need some more insights here from the community.

Might need some more insights here from the community, thats why moving it to Needs review status.

Is it designed that way or needs fix ? note i am aware of related issue https://www.drupal.org/project/drupal/issues/3232018 → and from my POV possibly we need to take some action on this issue, since it is affecting in many more pages with x-drupal-dynamic-cache: UNCACHEABLE in response headers.

This doesn't need more info, the bug has been reproduced in #32 and also previously, it does need a conversion to an MR though at least.

By default, x-drupal-dynamic-cache header is present when revision oveview page is accessed and its value is 'UNCACHEABLE'.

Now, If revision overview page is accessed after applying patch #17, x-drupal-dynamic-cache header wont be there. This is due tho the following condition in onResponse() method of DynamicPageCacheSubscriber.

    // Don't cache the response if the Dynamic Page Cache request & response
    // policies are not met.
    // @see onRequest()
    if ($this->requestPolicyResults[$request] === RequestPolicyInterface::DENY || $this->responsePolicy->check($response, $request) === ResponsePolicyInterface::DENY) {
      return;
    }

$this->responsePolicy->check() calls several policy rules and Drupal\dynamic_page_cache\PageCache\ResponsePolicy\DenyAdminRoutes rule returns 'deny' for revison overview route as its an admin route. (_admin_route: true). So x-drupal-dynamic-cache header is omitted.

Rebased against 11.x, removed the ->addCacheableDependency($node) addition (out of scope), and moved testing to a dedicated test case since we need to disable the use_admin_theme setting.

1) Drupal\Tests\node\Functional\NodeRevisionsUiTest::testNodeRevisionsCacheability
Behat\Mink\Exception\ExpectationException: Current response header "X-Drupal-Dynamic-Cache" is "UNCACHEABLE (poor cacheability)", but "MISS" expected.
/builds/issue/drupal-3227637/vendor/behat/mink/src/WebAssert.php:888
/builds/issue/drupal-3227637/vendor/behat/mink/src/WebAssert.php:180
/builds/issue/drupal-3227637/core/tests/Drupal/Tests/WebAssert.php:969
/builds/issue/drupal-3227637/core/modules/node/tests/src/Functional/NodeRevisionsUiTest.php:229
FAILURES!
Tests: 5, Assertions: 52, Failures: 1.

Shows test coverage

Change seems straight forward and gone through a number of reviews. Believe feedback has been addressed.

Committed and pushed 44f56c2c1b8 to 11.x and 371936c49aa to 11.1.x. Thanks!

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

We have a failing test in 11.1.x:
Drupal\Tests\node\Functional\NodeRevisionsAllTest::testRevisions()

git bisect reveals the failure was introduced here.

For some reason, it was not failing in the MR when it was merged.

If a page was not cached before, it is not enough to just fix the reasons why it was not cached.
We also need to check that all the cache metadata added to the page is complete!
I suspect we have missing cache tags..

To be analysed in 🐛 NodeRevisionsAllTest::testRevisions() fails in 11.1.x Active

The fix for the regression in 11.1.x isn't straightforward and it's been in 11.1.x for over a month, it's also not clear why this is passing on 11.x, given both sides of that are confusing, I'm going to go ahead and revert here. There is the start of a fix in 🐛 NodeRevisionsAllTest::testRevisions() fails in 11.1.x Active .

It looks like the difference is to do with some other unrelated cache metadata making this cacheable on 11.1.x when it's not on 11.x (in the context of NodeRevisionsAllTest)

11.x cache headers:

  'Cache-Control' => 'must-revalidate, no-cache, private',
  'X-Drupal-Dynamic-Cache' => 'UNCACHEABLE (response policy)',
  'X-Drupal-Cache-Tags' => 'http_response rendered',
  'X-Drupal-Cache-Contexts' => 'languages:language_interface theme url.query_args user.permissions user.roles:authenticated',
  'X-Drupal-Cache-Max-Age' => '-1 (Permanent)',

11.1.x cache headers:

  'Cache-Control' => 'must-revalidate, no-cache, private',
  'X-Drupal-Dynamic-Cache' => 'HIT',
  'X-Drupal-Cache-Tags' => 'http_response rendered',
  'X-Drupal-Cache-Contexts' => 'languages:language_interface theme url.query_args.pagers:0 url.query_args:_wrapper_format user.permissions user.roles:authenticated',
  'X-Drupal-Cache-Max-Age' => '-1 (Permanent)',

Here's the crucial difference:

11.x has node.settings:use_admin_theme defaulting to true https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/node/...

11.1.x has it defaulting to false https://git.drupalcode.org/project/drupal/-/blob/11.1.x/core/modules/nod...

DPC has a response policy to blanket deny on admin pages via Drupal\dynamic_page_cache\PageCache\ResponsePolicy\DenyAdminRoutes

This default setting was changed in 📌 Consider using the administration theme when editing or creating content by default Active , does that just need to be committed to 11.1.x?

This default setting was changed in #3347015: Consider using the administration theme when editing or creating content by default, does that just need to be committed to 11.1.x?

I would say this is a significant functional change that we would not want to have in 11.1.x.
If all we care is to make a test work, we can change that setting in the test itself.
Or rather, we change that setting in the test to use the front-end theme instead of the back-end theme, if we are interested in the cache behavior.

Then, the more important question is:
Can and should we cache a revision list?
If so, which cache tags make sure that this cache is invalidated when revisions other than the published revision are added, modified or deleted? I don't think such a cache tag exists currently.
Also, what is the use case where caching the revision list actually makes sense? That is, where the revision list is visited significantly more often than it is changed? (I suppose working with revisions in workflow)

Can and should we cache a revision list?

I think in a real world scenario we'd almost never hit the bug that this test is hitting. To reproduce it via a browser I had to:

1. Disable the admin theme setting for node.settings
2. Disable the breadcrumb and primary tab blocks (these add the node cache tag)

Without doing this the node cache tag gets added to the page and when editing a node via the UI to create a new revision, the revisions page is purged.

I think it's perfectly reasonable to have a cacheable revisions page, especially since the reason why it isn't is because of a bugged call to addCacheableDependency (which I thought we deprecated in #3232018: Trigger a deprecation when using \Drupal\Core\Cache\RefinableCacheableDependencyTrait::addCacheableDependency with a non CacheableDependencyInterface object → but that didn't cover the renderer...)

I don't think we differentiate between default and non-default revisions when invalidating cache tags so it should actually be invalidated correctly. Didn't test this but couldn't find any differentiation.

Whether that's in itself a caching bug (or lack of feature) is a different question though. We could add an explicit test that it gets invalidated, and then if we stop invalidating tags for non-default revisions it would cover that case.

If so, which cache tags make sure that this cache is invalidated when revisions other than the published revision are added, modified or deleted?

One thing I noticed from MR 12158 for 🐛 NodeRevisionsAllTest::testRevisions() fails in 11.1.x Active is this:

    // Delete older revisions.
    foreach ($older_revision_ids as $older_revision_id) {
      $node_storage->deleteRevision($older_revision_id);
    }

    // At first, the old list is still in the cache.
    // @todo This is bad, the list should be updated.
    $this->drupalGet('node/' . $node->id() . '/revisions');
    $this->assertSession()->responseContains('page=1');
    $this->assertSession()->pageTextContains(end($logs));

Drupal\Core\Entity\ContentEntityStorageBase::deleteRevision() does not invalidate cache tags, afaict. Looks like all the cache tag invalidation for entity CRUD operations occurs in the entity class and not the storage class, and delete() is never called on the loaded revision in deleteRevision(). I'm not sure whether this is a bug or as intended.

The question is which cache tags should be invalidated here.
I suspect we would have to invent a new cache tag that does not exist yet.

(here is what I remember, please correct me)

There is one cache tag for the published / active version of an entity.
This gets invalidated when that published / active version is updated.
There are good reasons why we would not want to invalidate this when other revisions are deleted or updated, because we want the published display to be served from cache as often as possible.

Instead, we could introduce a cache tag for "revision list for entity ", which gets invalidated when any revision of that entity is deleted, created or updated, or there is a workflow change.

We would have to properly test this.
And we need to determine if we really want to fill the cache with rendered entity revisions.
It could be really nasty on storage, for possibly little benefit.

We might as well wait until 📌 {% trans %} does not support render array and MarkupInterface valued placeholders Needs work is merged before fixing up the conflicts here too since that changes the rendering of $username as well.