Use a single per-theme cache tag for block config entities

ok, test showed an edge case which is when there are no blocks yet on the page. For now restoring the generic list cache tag, which removes some of the benefit of this, possibly we can add the theme cache tag to the page instead. The list cache tag might also be needed for other lists (REST, JSON:API), if we don't add it to the page it won't hurt to invalidate it.

BlockSystemBrandingTest shows off another change, if we no longer tie the cache tag to the underlying config, we don't see changes that are done directly to config, but I think we don't support that on other entity types that customize this as well.

The last submitted patch, 3: block-3341042-cache-tags-4.patch, failed testing. View results →

I updated the issue summary a bit, as I made some changes. The changes to the performance tests look nice, but unlike some other improvements, this comes with some drawbacks, and it's really hard to estimate how sites will be affected by the possible drawbacks. See issue summary for details.

However, there is a tradeoff here. If there are sites out there that frequently update specific block configurations (not content blocks, not menus, just block config entities) then this will now invalidate every single block. I *think* that's not very common, but I have no data on that.

I've come across some horrific situations with block content entities before, like breaking the layout builder optimisation to mark them as not re-usable, so that thousands of block plugins were being loaded on every request. But don't remember ever seeing serious problems with frequent updates of block config entities or thousands of them etc. Doesn't mean it's out there but it would be a new horror, not a familiar one.

* There might also be possible API changes from this. Several tests had to be updated because they were directly updating the underlying block configs instead of going through the config entity, but that was always discourages as it bypasses hooks and other things such as the list cache tag. But maybe people invalidated cache tags directly or something.

This all sounds OK with a change record to me. The worst case is a site needs a manual cache clear.

Strangely, there's also a a Block::postSave() that invalidates the block configs cache tag on saving a new one, explaining that it contains the theme, but as far as I see, that's not actually the case, leftover?

Can confirm this is not the case.

Got added here: https://git.drupalcode.org/project/drupal/-/commit/0189add1e36ab40283172...

Got removed here: https://git.drupalcode.org/project/drupal/-/commit/5859ca2cf238b4741b122...

Both these issues involved you :D Anyway, the explanation seems to be in #2040135-38: Caches dependent on simple config are only invalidated on form submissions → . So that comment is indeed an artifact that people forgot to remove in the latter issue.

The gains here are impressive. I generally would not like removing individual cache tags, but for blocks it makes more sense as they're more of a page building tool than an actual content tool. The content that is shown inside of them usually has its own cache tags.

I do feel like the comments could use a bit more love. If I were not aware of this issue and stumbled upon the comments as they are in the MR I would rightfully be annoyed at the lack of documentation as to why we're only using a list cache tag and are omitting a view cache tag.

Setting to NW for that, but +1 on the issue in general from me.

There might also be possible API changes from this. Several tests had to be updated because they were directly updating the underlying block configs instead of going through the config entity, but that was always discourages as it bypasses hooks and other things such as the list cache tag.

Only scenario where I can see this being a pain is deployments. But those usually run a cache rebuild anyways.

Pushed some updates, some doc related points still open.

Note that standard profile is a somewhat unrealistic scenario. As I've written initially in the issue summary, the gains on umami for example are less impressive, as most blocks do in fact have other cache tags and I think will be true for many real sites too. Specifically, combining this with 📌 Add assertions to OpenTelemetryNodePagePerformanceTest::testNodePageWarmCache() Active gives me this diff on the assertions:

-      'CacheTagLookupQueryCount' => 28,
+      'CacheTagLookupQueryCount' => 24,
       'CacheTagGroupedLookups' => [
-        ['entity_types', 'route_match', 'access_policies', 'routes', 'router', 'entity_field_info', 'entity_bundles', 'local_task', 'library_info'],
+        ['entity_types', 'route_match', 'access_policies', 'routes', 'router', 'entity_field_info', 'entity_bundles', 'local_task', 'library_info', 'http_response'],
         ['config:views.view.related_recipes'],
         ['config:core.extension', 'views_data'],
         ['node:10', 'node:3', 'node:6', 'node:7', 'node_list'],
@@ -318,20 +318,16 @@ protected function testNodePageWarmCache(): void {
         ['media:3'],
         ['config:core.entity_view_display.node.recipe.full', 'config:filter.format.basic_html', 'node:1', 'taxonomy_term:13', 'taxonomy_term:22', 'taxonomy_term:31'],
         ['config:views.view.recipe_collections'],
-        ['CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form', 'block_view', 'config:block.block.umami_search'],
-        ['config:block.block.umami_account_menu', 'config:system.menu.account'],
-        ['config:block.block.umami_branding', 'config:system.site'],
-        ['config:block.block.umami_main_menu', 'config:system.menu.main'],
-        ['config:block.block.umami_messages'],
-        ['config:block.block.umami_help'],
-        ['config:block.block.umami_local_tasks', 'config:workflows.workflow.editorial'],
+        ['CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form', 'config:block_list'],
+        ['config:system.menu.account'],
+        ['config:system.site'],
+        ['config:system.menu.main'],
+        ['config:workflows.workflow.editorial'],
         ['config:views.view.recipes'],
-        ['config:block.block.umami_breadcrumbs'],
-        ['config:block.block.umami_views_block__recipe_collections_block', 'taxonomy_term:1', 'taxonomy_term:10', 'taxonomy_term:11', 'taxonomy_term:12', 'taxonomy_term:14', 'taxonomy_term:15', 'taxonomy_term:16', 'taxonomy_term:2', 'taxonomy_term:3', 'taxonomy_term:4', 'taxonomy_term:5', 'taxonomy_term:6', 'taxonomy_term:7', 'taxonomy_term:8', 'taxonomy_term:9', 'taxonomy_term_list'],
-        ['block_content:2', 'block_content_view', 'config:block.block.umami_footer_promo', 'config:core.entity_view_display.block_content.footer_promo_block.default', 'config:core.entity_view_display.media.image.medium_8_7', 'config:image.style.medium_8_7', 'file:37', 'media:19'],
-        ['config:block.block.umami_footer', 'config:system.menu.footer'],
-        ['config:block.block.umami_disclaimer'],
-        ['block_content:1', 'config:block.block.umami_banner_home', 'config:block.block.umami_banner_recipes', 'config:block.block.umami_content', 'config:block.block.umami_languageswitcher', 'config:block.block.umami_page_title', 'config:block.block.umami_views_block__articles_aside_block_1', 'config:block.block.umami_views_block__promoted_items_block_1', 'config:block_list', 'config:configurable_language_list', 'http_response'],
+        ['taxonomy_term:1', 'taxonomy_term:10', 'taxonomy_term:11', 'taxonomy_term:12', 'taxonomy_term:14', 'taxonomy_term:15', 'taxonomy_term:16', 'taxonomy_term:2', 'taxonomy_term:3', 'taxonomy_term:4', 'taxonomy_term:5', 'taxonomy_term:6', 'taxonomy_term:7', 'taxonomy_term:8', 'taxonomy_term:9', 'taxonomy_term_list'],
+        ['block_content:2', 'block_content_view', 'config:core.entity_view_display.block_content.footer_promo_block.default', 'config:core.entity_view_display.media.image.medium_8_7', 'config:image.style.medium_8_7', 'file:37', 'media:19'],
+        ['config:system.menu.footer'],
+        ['block_content:1', 'config:configurable_language_list'],
         ['config:user.role.anonymous'],
       ],
       'ScriptCount' => 1,

So, "only" 4 queries less, compared to the ~10 in standard profile.

Looks good to me, can RTBC if we explain in more detail why Block and BlockViewBuilder behave differently. Doesn't have to be a whole novel, but the current docs are a little light.

I have a feeling that 📌 Create placeholders for more things Active might help with the more realistic umami scenario, because it's combining the cache tag lookups for all placeholdered blocks. Would need this issue and 📌 Add assertions to OpenTelemetryNodePagePerformanceTest::testNodePageWarmCache() Active to fully demonstrate it probably.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.