Problem/Motivation

We added this in [#2222293]. But it really makes very little sense, because each block should automatically get the max age it can be cached for. Back then, that seemed an impossibility. But a few weeks ago, #2429257: Bubble cache contexts landed, which made it a reality.
(Bubbling also is sufficient for custom blocks — thanks to fields' and filters' ability to associate cache tags, contexts and max-age.)

If we're asking the user to indicate how long a block can be cached, we've already failed: the user shouldn't even have to make that choice. It should happen automatically, based on the contents of the block.

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.

It does make sense to be able to configure a max-age in very advanced use cases, where you want invalidation not to happen via tags, but you want a block to be cached for at least X seconds. But that's up to the block plugin itself.

Proposed resolution

Remove the UI.

Remaining tasks

TBD

User interface changes

No more max-age configuration when configuring BlockBase-based blocks.

API changes

  • The config schema is changed.
  • BlockBase::getCacheMaxAge() returns Cache::PERMANENT instead of the configured max-age (which defaulted to zero), thus strongly encouraging all blocks extending from BlockBase to be cacheable.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue priority Major because 1) despite being horrible for usability in the first place, 2) it also makes caching less efficient than it can be, and 3) it ends up being a lie to the user: we present the configured cache max-ages as the only cache max-ages being varied by, but the contents of the block may (and almost always do) bubble additional max-ages (the lowest max-age wins), thus making the UI not only unusable, but also a lie that we cannot possibly explain clearly.
Prioritized changes The main goal of this issue is usability + performance + DX
Disruption Disruptive for custom/contributed modules in a minor way: if they provide blocks, and they extend the BlockBase class to do so, they won't have to alter the block's configuration form anymore to enforce a certain max-age.
CommentFileSizeAuthor
#57 interdiff.txt2.79 KBWim Leers
#56 block-remove-max-cache-ui-2458763-56.patch17.02 KBWim Leers
#48 interdiff.txt855 bytesWim Leers
#48 block-remove-max-cache-ui-2458763-48.patch19.18 KBWim Leers
#45 interdiff.txt805 bytesWim Leers
#45 block-remove-max-cache-ui-2458763-45.patch18.73 KBWim Leers
#42 interdiff.txt542 bytesWim Leers
#42 block-remove-max-cache-ui-2458763-42.patch18.06 KBWim Leers
#39 interdiff.txt5.2 KBWim Leers
#38 block-remove-max-cache-ui-2458763-38.patch17.6 KBWim Leers
#35 block-remove-max-cache-ui-2458763-35.patch15.06 KBWim Leers
#25 interdiff.txt519 bytesm1r1k
#25 block-remove-max-cache-ui-2458763-25.patch14.89 KBm1r1k
#23 interdiff.txt11.31 KBm1r1k
#23 block-remove-max-cache-ui-2458763-23.patch14.89 KBm1r1k
#21 block-remove-max-cache-ui-2458763-21.patch9.08 KBm1r1k
#21 interdiff.txt674 bytesm1r1k
#19 block-remove-max-cache-ui-2458763-19.patch9.16 KBm1r1k
#19 interdiff.txt1.17 KBm1r1k
#17 block-remove-max-cache-ui-2458763-16.patch9.26 KBm1r1k
#17 interdiff.txt2.43 KBm1r1k
#13 block-remove-max-cache-ui-2458763-13.patch6.83 KBm1r1k
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: Remove the ability to configure a block's cache max-age » [PP-1] Remove the ability to configure a block's cache max-age
Status: Active » Postponed

Blocked on #2428805: Remove the ability to configure a block's cache contexts.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Blocked on #2428805: Remove the ability to configure a block's cache contexts.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

The max-age typically applies to all displays of a view anyway, precisely because it's a different display of the same query (the same results, same data, hence same max-age).

This is not correct and rather irrelevant for this issue :) There's almost nothing that is not by-display in views, you can't change the table/entity type you're querying (not the base table, at least), but other than that, there are no limits, including cache configuration.

=> Remove that from the issue summary or rewrite it (but I would remove it, because doesn't matter for this issue).

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue summary: View changes

Done.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: [PP-1] Remove the ability to configure a block's cache max-age » Remove the ability to configure a block's cache max-age
Status: Postponed » Active

#2428805: Remove the ability to configure a block's cache contexts landed, now unblocked.

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented

Removing UI's++

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

@Berdir: thinking about this some more, isn't what you actually need for your use cases not the ability to force a certain max-age, but rather the ability to set a min-age?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Yes, I was thinking about that too, that would be an interesting way to solve that problem.

Not quite sure how to support that, though. We could change Renderer to always allow invalidated items too, and then check if they set a min-age and keep using that if it hasn't expired yet?

We had that for the page cache removed it, I don't remember anymore why exactly.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Drupal Association commented

The relevant comment by Berdir was:

#2428805-16: Remove the ability to configure a block's cache contexts

I think the examples of something that changes time based on the contents of the view make sense, where invalidation is too costly (cache tag invalidation - while precise costs a lot of write DB performance).

I also wondered about alteration possibilities of bubbled data.

Would #post_render work? I assume not.

In the worst case a contrib/ render_cache 8.x could provide a Renderer that either

a) accepts additional #render_cache_pre_bubbling and #render_cache_post_bubbling callbacks - not to be mixed up with #post_render_cache and #pre_render ...
b) an optional alter hook that is called per cache item, because I do not care about the overhead on my own sites

However I think we should do something in core to allow more customizability on the API level, too.

That is - as I just realized - another issue though.

amateescu’s picture

amateescu CreditAttribution: amateescu for Drupal Association commented
Issue tags: +D8 upgrade path

Adding a tag that will help the beta-to-beta effort later :)

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Drupal Association commented

The question is:

Do we really need a UI feature for that except for views?

And I think we don't ...

m1r1k’s picture

m1r1k CreditAttribution: m1r1k commented
Assigned: Unassigned » m1r1k

I will try during devdays:)

m1r1k’s picture

m1r1k CreditAttribution: m1r1k commented
Assigned: m1r1k » Unassigned
Status: Active » Needs review
Issue tags: +drupaldevdays
FileSize
block-remove-max-cache-ui-2458763-13.patch6.83 KB
Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Drupal Association commented
Issue tags: -drupaldevdays +D8 Accelerate Dev Days

Tagging

Status: Needs review » Needs work

The last submitted patch, 13: block-remove-max-cache-ui-2458763-13.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

The block config entity's config schema, as well as the UI test have not yet been updated.

m1r1k’s picture

m1r1k CreditAttribution: m1r1k commented
Status: Needs work » Needs review
FileSize
interdiff.txt2.43 KB
block-remove-max-cache-ui-2458763-16.patch9.26 KB

Status: Needs review » Needs work

The last submitted patch, 17: block-remove-max-cache-ui-2458763-16.patch, failed testing.

m1r1k’s picture

m1r1k CreditAttribution: m1r1k commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.17 KB
block-remove-max-cache-ui-2458763-19.patch9.16 KB

meh, wrong place for form item.
@wim-leers Why do we need update schema? We only remove UI for choosing cache life-time (but keep it for views)?

Status: Needs review » Needs work

The last submitted patch, 19: block-remove-max-cache-ui-2458763-19.patch, failed testing.

m1r1k’s picture

m1r1k CreditAttribution: m1r1k commented
Status: Needs work » Needs review
FileSize
interdiff.txt674 bytes
block-remove-max-cache-ui-2458763-21.patch9.08 KB

Fix BlockInterfaceTest

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Needs work 
  1. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
@@ -186,22 +186,10 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
     $form['cache']['max_age'] = array(
...
+      '#type' => 'value',
+      '#default_value' => Cache::PERMANENT,
     );

    This also needs to be removed. 

  2. +++ b/core/lib/Drupal/Core/Block/BlockBase.php
--- a/core/modules/block/src/BlockForm.php
+++ b/core/modules/block/src/BlockForm.php

    Two accidental changes in this file, can be removed :) 

  3. +++ b/core/modules/system/src/Plugin/Block/SystemPoweredByBlock.php
--- a/core/modules/views/src/Plugin/Block/ViewsBlockBase.php
+++ b/core/modules/views/src/Plugin/Block/ViewsBlockBase.php

+++ b/core/modules/views/src/Plugin/Block/ViewsBlockBase.php
@@ -158,6 +159,24 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
+    $form['cache'] = array(
+      '#type' => 'details',
+      '#title' => $this->t('Cache settings'),
+    );
+    $form['cache']['max_age'] = array(
+      '#type' => 'select',
+      '#title' => $this->t('Maximum age'),
+      '#description' => $this->t('The maximum time this block may be cached.'),
+      '#default_value' => $this->configuration['cache']['max_age'],
+      '#options' => $period,
+    );

    Interesting :) You're keeping this around just for Views.

    Fabianx said in #11:

    Do we really need a UI feature for that except for views?

    And I think we don't ...

    Let's hear what Berdir thinks.

    If we want to keep this, then we still want to change the block config entity's config schema. Because as of this patch, it's no longer something that applies to all blocks, but only to the Views blocks. So it's up to Views to provide a custom schema for views blocks.

    If you need help with that, please ping me in IRC tomorrow, and I'll find somebody who can help you :) That's definitely not the simplest task. See the cheat sheet at http://hojtsy.hu/blog/2014-dec-12/drupal-8-configuration-schema-cheat-sheet

m1r1k’s picture

m1r1k CreditAttribution: m1r1k commented
Status: Needs work » Needs review
FileSize
block-remove-max-cache-ui-2458763-23.patch14.89 KB
interdiff.txt11.31 KB
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Needs work

Just one super stupid typo, then this is RTBC :)

+++ b/core/lib/Drupal/Core/Block/BlockBase.php
@@ -311,7 +301,9 @@ public function getCacheTags() {
+    // only through cache tg invalidation.

s/tg/tag/

m1r1k’s picture

m1r1k CreditAttribution: m1r1k commented
Status: Needs work » Needs review
FileSize
block-remove-max-cache-ui-2458763-25.patch14.89 KB
interdiff.txt519 bytes
Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Drupal Association commented
Assigned: Unassigned » Berdir

Assigning to Berdir for review.

@Berdir: Are we sure we really need a UI in the blocks to set the max-age? Especially if views time based caching would / should set max-age automatically, too ...

And a hook_block_view_BLOCK_ID_alter is simple to do from the outside and the inside ...

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Assigned: Berdir » Unassigned

Ok, ok ;)

I just didn't want it to be removed together with the other issue, so that we can evaluate this properly and separately.

As discussed with Wim, I think the views block should expose max age properly, so that for example a block with a random sort isn't cached. We need #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface for that. Not actually for that example (0 or permanent would work for that), but for time based caching.

(Trick question: if you have a randomly sorted view that is explicitly cached for 1h, what should the max-age of that block be?)

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Acquia commented

I don't think it is the view block that needs to expose that but the time based views cache plugin.

As that is broken in views right now, are we sure we cannot go ahead here and fix views afterwards?

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Acquia commented
Title: Remove the ability to configure a block's cache max-age » [PP-1] Remove the ability to configure a block's cache max-age
Status: Needs review » Postponed

Postponing on #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface after discussion.

While it is views that is broken right now of not setting max-age properly, this can be workaround-ed partially by configuring a block max-age.

Therefore views supporting max-age is a dependency for this to go in ...

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Related issues: +#2504139: Blocks containing a form include the form action in the cache, so they always submit to the first URL the form was viewed at
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Possible way to move forward with this without waiting on #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface, which is somehow behaving weirdly.

Move the cache settings UI to a helper method that's not called by default, keep it on views blocks. Then we can at least remove it from all the other blocks in core..

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Sounds good. But AFAICT the other issue is pretty much ready, so :)

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented

Lets dooooo it! :)

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman at Acquia commented
Issue tags: +sprint
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: [PP-1] Remove the ability to configure a block's cache max-age » Remove the ability to configure a block's cache max-age
Status: Postponed » Needs review
FileSize
block-remove-max-cache-ui-2458763-35.patch15.06 KB

But AFAICT the other issue is pretty much ready, so :)

… and then it went to uncover like half of all the critical upgrade path criticals…

But now it should really be ready to go in.

This is a straight reroll of #25, just with a bunch of conflicts resolved.

Status: Needs review » Needs work

The last submitted patch, 35: block-remove-max-cache-ui-2458763-35.patch, failed testing.

The last submitted patch, 35: block-remove-max-cache-ui-2458763-35.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review
FileSize
block-remove-max-cache-ui-2458763-38.patch17.6 KB

In this reroll:

  • upgrade path added
  • LocalActionsBlock and LocalTasksBlock updated also
  • fixed nits
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
interdiff.txt5.2 KB

Forgot the interdiff.

Status: Needs review » Needs work

The last submitted patch, 38: block-remove-max-cache-ui-2458763-38.patch, failed testing.

The last submitted patch, 38: block-remove-max-cache-ui-2458763-38.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review
FileSize
block-remove-max-cache-ui-2458763-42.patch18.06 KB
interdiff.txt542 bytes

#2005546: Use branding block in place of page template branding variables (site name, slogan, site logo) just landed and broke this.

Status: Needs review » Needs work

The last submitted patch, 42: block-remove-max-cache-ui-2458763-42.patch, failed testing.

The last submitted patch, 42: block-remove-max-cache-ui-2458763-42.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review
FileSize
block-remove-max-cache-ui-2458763-45.patch18.73 KB
interdiff.txt805 bytes

Status: Needs review » Needs work

The last submitted patch, 45: block-remove-max-cache-ui-2458763-45.patch, failed testing.

The last submitted patch, 45: block-remove-max-cache-ui-2458763-45.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review
FileSize
block-remove-max-cache-ui-2458763-48.patch19.18 KB
interdiff.txt855 bytes

HAH!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting for Drupal Association commented

Are we really sure we want to change behaviour on existing sites? I mean this basically what happens.
A much less disruptive change would be provide a BC and remove the UI support for it.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue summary: View changes

I'd wager that 99% have not used this manual override functionality.

Also, see point 3 in the beta evaluation:

3) it ends up being a lie to the user: we present the configured cache max-ages as the only cache max-ages being varied by, but the contents of the block may (and almost always do) bubble additional max-ages (the lowest max-age wins), thus making the UI not only unusable, but also a lie that we cannot possibly explain clearly.

This UI and configurability stems from the days that block plugins did not provide this metadata themselves. If something is uncacheable in a block, then that uncacheable thing will bubble it itself (e.g. a form).

Shipping with this capability — even if it's not in the UI — is IMO a super misleading thing; it will almost always end up confusing the user.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Acquia commented

I agree with #50. Especially - that this functionality should only be used by advanced users and for those it is just an alter hook away.

Compared to D7, where e.g. the block_cache_alter module (which I maintain) provided this additional functionality and in which it was necessary to use time based expiration, the cache tags based expiration is in 90% of the cases the better choice.

Hence I agree with removing that UI and functionality as it is severely misleading.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman at Acquia commented
Status: Needs review » Reviewed & tested by the community

This is a nice UI and code cleanup. Only deletions.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

See #27. Nothing has really changed since then. The other issue has not yet been committed and IMHO, we really should have an integration test that ensures that max-age of a views block is bubbled up correctly. AFAIK, that still doesn't work or does it now?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

The other issue is not yet in, but it has been held up time and time again on other issues. So I wanted to make sure that this issue was at least ready.

Once the other issue lands, Views blocks indeed will bubble max-age from Views.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work

Needs reroll.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review
FileSize
block-remove-max-cache-ui-2458763-56.patch17.02 KB

Rebased. #2511516: Make local tasks and actions hooks provide cacheability metadata, to make the blocks showing them cacheable conflicted with this, and reduces the amount of work this patch has to do. :)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community
FileSize
interdiff.txt2.79 KB

Re-RTBC'ing given that this patch interdiff clearly shows that the only difference with #48 is the removal of a few hunks of changes. The rest is identical.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 56: block-remove-max-cache-ui-2458763-56.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community 
GET http://ec2-52-88-182-40.us-west-2.compute.amazonaws.com/checkout/update.php/start?id=4&op=do_nojs returned 0 (0 bytes).

Testbot again. Sigh.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 56: block-remove-max-cache-ui-2458763-56.patch, failed testing.

Xano queued 56: block-remove-max-cache-ui-2458763-56.patch for re-testing.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano at Druid commented
Status: Needs work » Reviewed & tested by the community

Seems like random test failures.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Agreed with this, the feature is very confusing/misleading as-is, let alone unnecessary.

Committed/pushed to 8.0.x, thanks!

  • catch committed e2d424d on 8.0.x 
    Issue #2458763 by Wim Leers, m1r1k: Remove the ability to configure a...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Version: 8.0.x-dev » 9.x-dev
Issue tags: -sprint

Thanks, removing from UX sprint now.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Version: 9.x-dev » 8.0.0