Remove CacheableInterface (and no longer let block plugins implement it)

FWIW, The interdiff looks great!

Re-rolled and fixed a couple of nitpicks.

One use case that I came up with for cacheability of a block is the system_powered_by block.

Caching does not make sense as a cache_get is potentially more costly then executing the block.

max-age = 0 also does not make any sense.

What might make sense is to omit the 'keys' in that case and just pass any lower level information through to upper levels.

Another possibility is to use downstream-ttl for that, e.g. system_powered_by_block would set:

max-age = 0
downstream-ttl = CACHE_PERMANENT

indicating that while this block should not be cached, its fine to cache the data in upper levels.

If we remove isCacheable() we will need to / should find a solution for such blocks. (Concerns also by berdir)

What about not doing that via an interface method or cache property at all, and just implement hook_block_view_BASE_BLOCK_ID_alter() to unset #cache['keys']?

1. +++ b/core/modules/block/src/BlockViewBuilder.php
   @@ -72,25 +73,14 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
   -      if ($plugin->isCacheable()) {
   -        $build[$entity_id]['#pre_render'][] = array($this, 'buildBlock');
   -        // Generic cache keys, with the block plugin's custom keys appended.
   ...
   -        $build[$entity_id]['#cache']['keys'] = array_merge($default_cache_keys, $plugin->getCacheKeys());
   

   which is exactly what this did ...

2. +++ b/core/modules/node/src/Plugin/Block/SyndicateBlock.php
   @@ -65,10 +65,10 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   -  public function isCacheable() {
   +  public function getCacheMaxAge() {
        // The 'Syndicate' block is never cacheable, because it is cheaper to just
        // render it rather than to cache it and incur I/O.
   -    return FALSE;
   +    return 0;
      }
   

   This is also wrong as it will bubble ...

3. +++ b/core/modules/system/src/Plugin/Block/SystemMainBlock.php
   @@ -59,9 +59,9 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   -  public function isCacheable() {
   +  public function getCacheMaxAge() {
        // The main content block is never cacheable, because it may be dynamic.
   -    return FALSE;
   +    return 0;
   

   This is wrong as that will bubble, as effulgentsia said, do not set the cache keys in that case.

I am not sure the hook_*_alter() is the solution here however.

Maybe leaving isCacheable() on the block plugins is simpler (they do exactly that) and just remove the getCacheKeys()?

And again we should avoid dealing with #cache tags, contexts and max-age directly.

Even if that is out of scope for this issue, we need to use high level functions on that based on interfaces now that we can.

Maybe leaving isCacheable() on the block plugins is simpler (they do exactly that)

The current API docs of CacheableInterface explicitly say otherwise:

+++ /dev/null
@@ -1,43 +0,0 @@
- * ... And if a child object is not
- * cacheable, then neither is any parent object.

And, most implementations are based on that interpretation, both for blocks, such as SystemMainBlock, and for non-blocks, such as menu links and Views plugins. So I don't think we should keep the name isCacheable() while changing its meaning to be different for blocks than for everything else. If we really want "too cheap to bother caching" as a method, I think it's fine to add a new method for blocks, such as isCheap(). But I'm not convinced that it's worth it: most blocks aren't cheap, so seems like a lot of bother just to save on a cache_get() of a few silly blocks, especially if we can have some other issue to do all block cache gets as a multiple operation, and when sites that really want to squeeze out those extra cache gets can do so via the alter hook.

I am fine with that, so what about the main content block?

Currently that one would screw up the cache headers ...

#14:

+++ b/core/modules/node/src/Plugin/Block/SyndicateBlock.php
@@ -65,10 +65,10 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
-  public function isCacheable() {
+  public function getCacheMaxAge() {
     // The 'Syndicate' block is never cacheable, because it is cheaper to just
     // render it rather than to cache it and incur I/O.
-    return FALSE;
+    return 0;

This should be removed too, then ...

I am okay to cache the main content block in that fashion. I ran into the exact same problem in render_cache for Drupal 7 :-D.

I think setting the route context and max-age is set automatically anyway, does not hurt and is actually the correct thing to do and we can still leave the unsetting of keys for now as in #15 for the BlockDisplayVariant with a @todo to remove when SmartCache lands.

And for controllers returning a #pre_render(_cache) pattern themselves, this will also work with maximum effect for caching.

So +1 to the POC, but another +1 to having that code path not yet active.

Not yet sure I want the block display variant to do it though.

I think I even would prefer an alter hook (wow!) in system module for now.

Just writing this down before I forget it : an idea came from reading this discussion (and others) about caching. It seems we are discussing something a bit different from "cacheability = ability to be cached", which would be "worthy of caching" instead. To take the "power by" block example, it is indeed cacheable, but not cache-worthy. This will probably happen a lot on contrib and site-specific blocks bringing content from static markup (ads, stats, various embeds) : the markup generation will be cheaper than the associated cache_get, and will not need to bubble up to higher-level components.

#20:

the markup generation will be cheaper than the associated cache_get, and will not need to bubble up to higher-level components.

Use #post_render_cache (or #cache placeholders when we implement those) if this really should always be generated newly.

However most of those blocks can be easily cached in upper levels (i.e. as part of the whole page).

For now the solution is to unset #keys to avoid caching at the blocks level - maybe we add an AlterableBlockPluginInterface to be able to not have to add a .module and a hook just to remove the #keys or set the visibility of the block (more to that later).

... if this really should always be generated newly.

Only in cases where the content is so dynamic that it _must_ be generated newly.

I agree that unsetting #keys is the better solution for all generic cases.

+++ b/core/modules/system/src/Plugin/Block/SystemMainBlock.php
@@ -42,26 +41,4 @@ public function build() {
-  public function buildConfigurationForm(array $form, FormStateInterface $form_state) {
-    $form = parent::buildConfigurationForm($form, $form_state);
-
-    // The main content block is never cacheable, because it may be dynamic.
-    $form['cache']['#disabled'] = TRUE;
-    $form['cache']['#description'] = $this->t('This block is never cacheable, it is not configurable.');
-    $form['cache']['max_age']['#value'] = 0;
-
-    return $form;
-  }

Although this UI will be removed entirely in #2458763: Remove the ability to configure a block's cache max-age, it still exists for now, so let's have this patch not introduce a regression here (i.e., being able to configure this block's max-age, even though that makes no sense). Perhaps we should just change the #value to permanent and the text to something better like "This block's maximum age cannot be configured, because it depends on the contents."

This also needs an issue summary update for API changes and beta evaluation, and possibly some CRs need to be updated.

Other than that, I think this is RTBC.

1. +++ b/core/modules/block/src/Plugin/DisplayVariant/BlockPageVariant.php
   @@ -142,6 +142,13 @@ public function build() {
   +        if ($block_plugin instanceof MainContentBlockPluginInterface) {
   +          unset($build[$region][$key]['#cache']['keys']);
   +        }
   

   I still feel uncomfortable about that.

   Can we add an alterSomething() method to the blocks itself that is called by BlockViewBuilder?

   Or lets say it like that to keep this in scope:

   Can we discuss adding an AlterableBlockPluginInterface in a follow-up?

   Implementing a hook to change my own final rendered data feels like a hack and hardcoding such things does not help - even if this is a really special case.

2. +++ b/core/modules/node/src/Plugin/Block/SyndicateBlock.php
   @@ -48,27 +48,4 @@ public function build() {
   -    // @see ::isCacheable()
   -    $form['cache']['#disabled'] = TRUE;
   -    $form['cache']['#description'] = $this->t('This block is never cacheable, it is not configurable.');
   -    $form['cache']['max_age']['#value'] = 0;
   

   I believe we need the same here as for the main content block.

CNW, for 2.

I opened #2467097: Decide if and how block plugins should alter the BlockViewBuilder generated render array for #26.1. In case that's not sufficient, leaving to Fabianx to RTBC.

I agree with #27.1 that even independent of that issue, the code in this patch is the right place for that unsetting, because the necessity to do it is not due to the plugin per se, but due to a particular strategy of how/when to aggregate it onto the page (see #16), and BlockPageVariant seems like a fine home for the consequence of that strategy.

Yep, lets open a follow-up for that.

So we make the max-age now again configurable for the syndicate block? I don't care, because we remove it anyway ...

RTBC - if that is not an issue.

Actually, we should make SyndicateBlock consistent with SystemPoweredBy and SystemMessages, so nonconfigurable as permanent. True about the UI being temporary anyway, but good to have consistency in the interim. NW for that, but otherwise, RTBC+1.

Yep, RTBC it is. #30 is what I meant in #26.2 :).

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 3e275d6 and pushed to 8.0.x. Thanks!