#Needs change record

I've been working on "saving" the persistent cache and reducing the BC impact of this. There are a few interesting advantages of keeping the persistent cache, for example the optimization with loadRevisionUnchanged() that allows to load a revision from static cache and only bypass the static cache, which is very useful when saving an entity, and also just loading the current revision which is what's going on with getActive().

* I don't see a use case for invalidating a specific revision as an API. The cache should be self-contained, basically the only reason for external calls to resetCache() are tests and we've mostly removed that too thanks to automatically invalidating the memory cache.
* This allows to remove the default_revision_for cache tag, we should always have the entity ID available and then we can delete that from the cache directly.

The remaining cache tag is now revision_for.., that still shows up a lot in the performance tests, because of the optimization to also put the default revision with its revision ID into the cache, but that means we need to check the state of those cache tags on cache set.

I plan to look into either not doing that anymore for now or alternatively remove the cache tag. The number of revisions for an entity is a known quantity. So in cases where we need to invalidate them, we can query the ids and then invalidate the cache. I think that's not necessary that often, basically just when an entity is deleted?

Looks like the most recent changes broke something, will look into that, off for now.

Started with a rebase.

📌 Add assertions to OpenTelemetryNodePagePerformanceTest::testNodePageWarmCache() Active got in, which shows that on demo_umami on node pages, we have ~80 node revision queries due to the local tasks block, so that shows how valuable this would be:

     $expected = [
-      'QueryCount' => 172,
-      'CacheGetCount' => 247,
+      'QueryCount' => 94,
+      'CacheGetCount' => 250,

The reason for this is the active routes, which query and load the most recent revision for edting, instead of the default. I don't think we really understood (I certainly didn't) the cost of introducing that standardization around the getActive() and using it directly on routes. And then with umami you have edit + delete + translations + layout builder ...

Note that this happens even for anonymous users which do not have access to those routes. But upcasting happens before access checks.

But, this comes at a price with the current implementation, a massive increase of cache tag lookups:

-      'CacheTagLookupQueryCount' => 28,
+      'CacheTagLookupQueryCount' => 57,

I haven't looked at this in many years, but I do believe that this is doing too much and that held it back for that long. I will need to have a better understanding of the current implementation, but I strongly support #132 back from 2018 right now and to split this up and only do static caching first. That should essentially give us the same benefit in this specific scenario without the cost. Does that risk that we might never get persistent caching for revisions? Yes. But it also means that we might get static caching soon as opposed to never.

I think this should be postponed until 🐛 Node Type / Entity bundle conditions evaluation is wrong when context is not provided Needs work is fixed.
If merged without the other fix, the behavior would be confusing (see #23).

In the meantime, the block_visibility_conditions contrib module seems to be a correct workaround.

catch → closed merge request !10060

Committed/pushed to 11.x, thanks!