#Needs manual testing

public function addSavepoint($savepoint_name = 'mimic_implicit_commit')

Very naively (having just jumped into this issue for the first time myself), could we leverage the $savepoint_name method argument as a solution instead?

It sounds like the issue is primarily a conflict in the mimic_implicit_commit name itself. Presumably changing $savepoint_name = 'mimic_implicit_commit' to $savepoint_name = 'some_other_name' would also solve this problem? (I'm not suggesting that... just posing the thought.)

I don't know all of the code that calls addSavepoint(), but would it be possible to override that in the downstream modules that are affected by this issue, such that the default argument can be changes to avoid the conflict?

Just throwing out alternative ideas here to stimulate the conversation a bit, while I wrap my head around this issue myself. Apologies if these have already been explored. :-)

Can someone please update the issue summary to include an explanation of what the proposed change is to the logic, and why it is necessary? There are plenty of comments in this thread that say "the patch works!" but very little about what the broader implications of the change might be. Reading the code diff doesn't provide any of this, and it raises further questions.

Changing if ($this->inTransaction()) { to if ($this->inTransaction() && !$this->transactionManager()->has($savepoint_name)) means that the addSavepoint() logic will NOT run in certain scenarios anymore. Is this OK? Has thought been given to how this might behave in other contexts? If so, I don't see any of that documented here (unless I missed it?) and if this causes issues later, future developers will have a hard time looking back at this issue and understanding why the decision was made.

I'm not critiquing the solution itself - it might be the right answer! But I'd like to see more of an explanation so that it's easier to understand at a glance.

Setting back to "Needs work" for the issue summary update, per @smustgrave's comment #62 above.

Another 3.5 years on from the last comment. The patches we have here are optionally adding a class and providing styles in classy. We no longer have classy, and the remaining themes have a mix of including the icon and not including it.

• claro - icon
• olivero - no icon
• starterkit_theme - icon
• umami - icon

Not sure what's the next step, but it doesn't seem to be related to system.module so I'm changing the component to CSS.

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.

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.

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

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.

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...)

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)

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?

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)',

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 .

Still a few unsresolved comments on the MR

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

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..

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.