NodeController::revisionOverview is uncacheable

LGTM

+++ b/core/modules/node/tests/src/Functional/NodeRevisionsUiTest.php
@@ -167,6 +167,7 @@ public function testNodeRevisionsTabWithDefaultRevision() {
 
     $this->drupalGet('node/' . $node_id . '/revisions');
+    $this->assertSession()->responseHeaderNotEquals('x-drupal-dynamic-cache', 'UNCACHEABLE');
 

Is there a positive assertion we can make here?

Thank you @larowlan for addressing #8. I left a comment on the MR. Do you think we should address it?

Hey there.

Took some time to setup testing for the assertions for state of cache, so here you go :)

Thank you for updating the patch @fubarhouse.

+++ b/core/modules/node/tests/src/Functional/NodeRevisionsUiTest.php
@@ -167,6 +167,9 @@ public function testNodeRevisionsTabWithDefaultRevision() {
+    $this->assertSession()->responseHeaderEquals('x-drupal-dynamic-cache', 'MISS');
+    $this->assertSession()->responseHeaderEquals('x-drupal-dynamic-cache', 'HIT');

I think we have to revisit the page between these two.

I've added a drupalGet request between the MISS and HIT checks in the NodeRevisionsUITest.php file. Patch and interdiff attached. Needs review and testing. (I was unsure how to test.)

Security question: Since a username is a user-entered value, does it need any kind of filtering before being used in a cache tag? Or does the initial username validation suffice?

Removed the fix from the patch, to see if the added testing tests the fix.

The fix looks good to me.
The patch without the fix fails, therefor the added test is testing the fix.
The added test also looks good to me.
For me it is RTBC.

Reuploading the full patch to be committed.

Thanks for the feedback. Here's some more:

+++ b/core/modules/node/tests/src/Functional/NodeRevisionsUiTest.php
@@ -167,6 +167,10 @@ public function testNodeRevisionsTabWithDefaultRevision() {
+    $this->assertSession()->responseHeaderEquals('x-drupal-dynamic-cache', 'MISS');
+    $this->drupalGet('node/' . $node_id . '/revisions');
+    $this->assertSession()->responseHeaderEquals('x-drupal-dynamic-cache', 'HIT');
+    $this->assertSession()->responseHeaderNotEquals('x-drupal-dynamic-cache', 'UNCACHEABLE');

1. IMO it would be helpful to add a comment to explain why this is being tested
2. In all other code, X-Drupal-Dynamic-Cache is used instead of x-drupal-dynamic-cache
3. I agree with the previous comment that username caching itself probably needs a test... I didn't see that tested anywhere when I looked

For reference, there are a few examples of explicitly checking the dynamic cache in an assert:

• core/modules/layout_builder/tests/src/Functional/LayoutSectionTest.php
• core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
• core/modules/toolbar/tests/src/Functional/ToolbarCacheContextsTest.php
• core/modules/node/tests/src/Functional/NodeBlockFunctionalTest.php
• core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
• core/modules/rest/tests/src/Functional/ResourceTestBase.php

Tagging for manual testing of the response headers before and after the patch. If you haven't checked headers before, here are instructions for Chrome:

https://stackoverflow.com/questions/4423061/how-can-i-view-http-headers-...

and one for Firefox:

https://www.websparrow.org/misc/how-to-view-http-headers-in-mozilla-firefox

Going to attempt a new patch today.

Looking at this in Firefox's Network tab, with the patch from comment #17 applied and the cache cleared, I don't see a response header x-drupal-dynamic-cache being set on node/%/revisions. (I do see it on node/%.)

Can we verify that the patch to set the x-drupal-dynamic-cache on node/%/revisions is working before moving on to getting the test coverage (of which I am in progress, but am fine with someone else taking it on as I am a novice at tests)?

I also updated the issue summary with steps to test and remaining tasks (please verify that I have the right idea about this).

+++ b/core/modules/node/tests/src/Functional/NodeRevisionsUiTest.php
@@ -167,6 +167,10 @@ public function testNodeRevisionsTabWithDefaultRevision() {
+    $this->assertSession()->responseHeaderEquals('x-drupal-dynamic-cache', 'HIT');
+    $this->assertSession()->responseHeaderNotEquals('x-drupal-dynamic-cache', 'UNCACHEABLE');

We only need the first assertion here - no? Having both seems unnecessary.

Agree with @alexpott. working on it.

Addressed #24 & uploaded updated patch.

Hi @yogeshmpawar,
Verified and tested patch #26.
Patch applied successfully but not working as expected.

Testing Steps:
# Goto: Appearance -> Apply Seven theme
# Create a node (can be published or unpublished).
# Edit the node you created.
# Open the web developer tools Inspect pane and select the Network tab.
# View the node and click on its Revisions tab.
# On the Network tab in your browser's Inspect tab, scroll up to the request for /revisions and select that row. In the adjacent tab, look under Response Headers for the `x-drupal-dynamic-cache` row and note the value (should be MISS). Refresh the page and repeat, noting the new value (should be HIT).

Expected Results:
After applying patch #26,
# User should see x-drupal-dynamic-cache: UNCACHEABLE (currently it's not appearing) and it gets removed
# User should see x-cache: HIT
# User should see x-drupal-cache: MISS

Actual Results:
# Currently user is able to see x-drupal-dynamic-cache: UNCACHEABLE
# x-cache: HIT is not appearing
# x-drupal-cache: MISS is not appearing

Please refer attached screenshots for the same.
Not working as expected.
Can be a move to Needs Work.

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 after i added this code for closing the issue by reviewing it. 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 visible on response headers.

Might need some more insights here from the community / reporter/ maintainer, 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!

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 #3525111: NodeRevisionsAllTest::testRevisions() fails in 11.1.x

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 #3525111: NodeRevisionsAllTest::testRevisions() fails in 11.1.x.

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 #3347015: Consider using the administration theme when editing or creating content by default, 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 #3525111: NodeRevisionsAllTest::testRevisions() fails in 11.1.x 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 #2334319: {% trans %} does not support render array and MarkupInterface valued expressions is merged before fixing up the conflicts here too since that changes the rendering of $username as well.

Fix merge conflict MR !1111.

This adds back code that was removed. Instaed, the @todo above needs to be dealt with now.

In combination with #2334319: {% trans %} does not support render array and MarkupInterface valued expressions, is likely enough to just remove that.