Remove the ability to configure a block's cache contexts

Just like #2099137: Entity/field access and node grants not taken into account with core cache contexts must be aware of the fields they contain and appropriately incorporate the cache contexts defined by the field formatters into the cache ID of the rendered entity, the same must be done for other things, like Views.

In any case: it'd be better to not force the "select cache contexts" UI on every block; it'd be a big step forward to only do it for e.g. Views blocks. Because surely, getting there (to what I describe above), that is a lot of work.

I don't see how we are ever going to be able to explain option 2 clearly to the end user. That would be an interesting contrib project (block_max_cache_contexts or something like that), but it doesn't seem appropriate for Drupal core.

Thanks, I didn't know about that one! We could simply close that issue if we indeed remove this (indeed too hard to use) UI.

Uh, where?

Must be misremembering :)

So far, what I've seen is that there are use cases where being able to set this manually are useful, but the bubbling might actually break that.

Yes, and no:

• Yes, bubbling would cause more cache contexts to be associated than the ones you selected.
• No, because bubbling only adds cache contexts that are actually necessary.

It is very common to have static expanded menu trees in site footers. Because they are always expanded, there's no need for them to be cached separately for each page. So this allows me to cache them globally.
[…]
Same for menus where you know that they don't change for different users […]

True! The real problems here are:

1. We currently associate the user.roles cache context with all menu blocks. This is patently wrong. (And it's my fault, BTW.) As you say: some menus don't vary per role. Yet others have menu links that are actually per-user, in which case per-role is not even sufficient.
   NOTE: you cannot choose to not vary per-role anyway. So you currently aren't able to override this via the UI anyway
2. #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees will ensure that the cache contexts associated with each rendered link (the access result + the generated URL) are bubbled. Once that is the case, we can remove that silly per-role cache context mentioned in point 1, and we'll automatically only vary by the cache contexts that we need to vary by.

A way to keep supporting those use cases would be great.

The way to keep supporting that is to do it automatically, thanks to cache context bubbling :) Cache context bubbling ensures we vary by the minimal set of cache contexts necessary, hence minimizing the number of cache item variations, and thus maximizing the cache hit ratio.

Also, the fact that we have more and more cache contexts is problematic and making that list longer.

Exactly; it's becoming overwhelming. And it'll only get worse in contrib.

Views blocks were mentioned, not sure what will happen with those? Will they really be intelligent enough to figure out the right contexts with bubbling or do we still need it for them? So we'd need a system for certain plugins to opt-in to showing them?

They will be smart enough. Each Views plugin/handler specifies its cache contexts. See #231837: Just white screen after activating it; and see all \Drupal\views\Plugin\CacheablePluginInterface implementations specifically. This is not yet 100% done, but we're actively working towards this, see #2451679: Validate cache contexts (+ cache contexts in some views plugins wrong), #2381277: Make Views use render caching and remove Views' own "output caching" and #2452317: Let views result cache use cache contexts, for example.

Or we could have something like a cache context advanced mode that you would have to explicitly enable, so that the UI by default would just show a checkbox?

There shouldn't be a need for this anymore. Render arrays must say which context they vary by. Cache context bubbling ensures that the render cached blocks are varied by those contexts. The bubbled cache contexts are by definition the cache contexts that must be varied by.

Initial patch. I've tried to update all affected tests too, but I probably will have missed some.

Updated IS.

To encourage developers even more to make their blocks cacheable, the above patch changed BlockBase::getCacheMaxAge() to return a non-zero value.

This change caused the test failures above. The solution is simple: the blocks that aren't cacheable should indicate that by returning a max-age of zero. I did this for all blocks that you can see in the interdiff, and added todos linking to the issues that allow those blocks to be made cacheable.
I did make the necessary changes to make form blocks cacheable (I ensured the right cache tags are present on it), because that was only a few LoC.

This will be green.

But I think there are also valid examples where cache tag based invalidations are either not possible or are doing too much.

Agreed. But in those cases, it's up to the code providing the block to set the appropriate max-age. The main reason we even had the cache contexts & max-age in the UI, was for custom blocks. But custom blocks now inherit the cache contexts of the fields they contain, and the cache contexts and max-age of the filters that are applied to the body text. So that use case is completely gone.

So then:

• I have a views block that's displaying the most read nodes, based on statistics.module. — it's the responsibility of the Statistics + Views integration to set the appropriate max-age.
• A similar example is a list of most commented nodes. […] on a highly frequented website, you might have a lot of new comments and don't want to invalidate your cached frontpage every time someone writes a new comment — indeed, but this sort of thing would be up to Views (or a Views plugin) to provide
• In general, I agree that it's useful to override cacheability metadata for some kinds of blocks. But it's only necessary on relatively advanced/high-traffic websites. And it's definitely not something that's useful for all blocks. So IMO this belongs in contrib, or specific implementations. (For example in the Statistics example, it IMO belongs in the Statistics module, since that is so very clearly related to time and hence to max-age).

That example actually gave me an interesting idea. Both blocks would also be invalidated when nothing changed of course. What would be really interesting would be a cache tag invalidation with a delay mechanism. For example, invalidating a cache tag would actually only be triggered 1h later, any additional invalidation within that timeframe would be ignored. But that's not something that we have right now and I don't think we could build that in core.

This has also crossed my mind a long time ago, but like you, I dismissed it for now because it doesn't belong in core, and because we have more urgent matters to deal with first. But, yes, absolutely: very fascinating :)

[…] Which currently is required, because max age defaults to never. I totally agree that's bad. What if we change the *default* max age to permanent (with a way for plugins to override/change that)?

This patch already does exactly that :)

#2458349: Route's access result's cacheability not applied to the response's cacheability is now also blocked on this.

From IRC:

(12:04:03) berdir: WimLeers: (block cacheability UI) No the patch doesn't do that. my suggestion is to keep the UI for max age and default *that* to permanent
(12:05:31) berdir: WimLeers: "it's up to the code providing the block to set the appropriate max-age" The statistics example is a generic view block, not something from statistics.module

• First line: Aha. I indeed intend to remove the max-age UI.
• Second line: but the statistics Views integration comes from \Drupal\comment\Plugin\views\field\StatisticsLastCommentName . To communicate the cacheability of a View, we already have \Drupal\views\Plugin\CacheablePluginInterface. It'd just be a matter of extending that plugin with a getCacheMaxAge() method (or rather, replacing ::isCacheable() with that) and then we'd have the necessary data.

Another reason we should remove the max-age support: because it's not something that's part of blocks themselves; it's part of BlockBase! Hence the config schema and config are actually written against BlockBase instead of BlockPluginInterface, which is very, very wrong.

If that still doesn't convince you, then let's remove blocks' configurable max-age in a separate issue then, since we have agreement on removing block cache contexts.

(Attached patch is just chasing HEAD.)

Discussed this with @Berdir and @catch in IRC.

We all think that we can and should remove a configurable max-age from blocks. But not in this issue, because to do it, would require users to be able to configure the max-age in the Views UI (which would then apply to a Views block, but also other displays of a view). I.e. it belongs on the block itself. That'd also require extending Views' CacheablePluginInterface to specify a max-age (::getCacheMaxAge()), probably replacing ::isCacheable() in the process.

So, assigning this to myself to reroll accordingly.

Done.

Per @Berdir in IRC, he was +1 to changing the default max-age value for BlockBase-based blocks to Cache::PERMANENT (which he also said at the bottom of #16). This allowed me to keep the interdiff smaller.

This function should have died in fire a long time ago :(

:D

That said, shouldn't we also add the list cache tag, for now?

Yes. (The only reason I'm touching it, is because I broke the forum blocks along the way; I already added node_list to ForumBlockBase::getCacheTags().)

IS updated.

And for #20 & #22: created #2458763: Remove the ability to configure a block's cache max-age to remove the configurable max-age from BlockBase.

#2458413: BlockViewBuilder should specify cache contexts even for uncacheable blocks landed, reroll to fix a small conflict.

• #26: correct.
• #27: good out-of-scope remark — let's fix that as part of #2458763: Remove the ability to configure a block's cache max-age — see #24.

Yay!

This unblocked:

1. #2458763: Remove the ability to configure a block's cache max-age
2. #2444231: Fix CacheableInterface so it makes sense (was: "Make Config objects & Entities implement CacheableInterface + add BubbleableMetadata::createFromCacheableObject()")
3. #2458349: Route's access result's cacheability not applied to the response's cacheability

And it allowed me to close #2231551: Clarify block cache settings configuration form.