Updated: Comment #83

Problem/Motivation

Blocks having their own caching system is obsolete as of #918808: Standardize block cache as a drupal_render() #cache: they effectively are using render caching (#cache).

Proposed resolution

Therefore:

  • We should remove the Block Cache system, since it's just a shorthand/abstraction for render caching.
  • We should make blocks set the appropriate cache tags, so that they can use the same render cache invalidation mechanism as everything else.
  • (#6) We should not force block implementers to deal with the implementation details of leveraging the render cache maximally: setting #cache is easy, putting the expensive computations in #pre_render is not. In other words: keep the Block API simple, and make it use render cache automatically (block plugins implement a build() method to build a render array of what they represent, and that's basically it).
  • Ideally (in the spirit of "Fast by default"), we will eventually be able to make as many blocks as possible in Drupal core cacheable forevery by default. Thanks to cache tags, that's feasible: cache a rendered block forever (Cache::PERMANENT), until one of its cache tags is invalidated. That's it!
    Note that in case of complex needs for sites with e.g. highly dynamic (e.g. time-dependent) hook_entity_access() implementations, it's trivial to disable caching: just change the default "Cache: Maximum age" setting on a block from "Forever" to "zero seconds"! I.e.: opt-out as a default, therefore fast by default.
    To be able to do that for a given block plugin, it must have test coverage to ensure that its cache tags are indeed invalidated in all necessary cases. The first examples for which we are able to do this, are the SystemMenuBlock (#62) (thanks to the test coverage added in #2179083: Rendered menus (e.g. menu blocks) should set cache tags to inform the page cache), CustomBlockBlock (#106) blocks (thanks to #2188565: Test coverage for Comment, Custom Block, Node, Taxonomy Term and User entity cache tags), and SystemHelpBlock (#114).
  • (#85) Furthermore, admin users should be able to specify the cache contexts that a cached block should be varied by. This empowers them to cache most blocks.
  • While the above ("make blocks cacheable forever") may seem like it's out of scope, it's really part of the necessary API validation.

For more screenshots, see #96. This is what it looks like overall:

For profiling, see #124. The short version:


Remaining tasks

None!

Optional follow-ups:

  1. #2020387: Convert "Active forum topics" block to a View
  2. #2158419: BookNavigationBlock should have better cache granularity and cache tags
  3. Make as many blocks as possible cached forever by default.

User interface changes

  1. Blocks now have a "Maximum age" setting, which is a <select> with the same options as the "Page cache maximum age" setting.
  2. Blocks now have a "Vary by context" setting, which is a set of checkboxes of available contexts.
  3. Block caching is removed from the Views UI. Views keeps its time and tag based caching.

API changes

  1. Blocks no longer declare a cache strategy. Instead, they implement CacheableInterface, for which BlockBase sets sane defaults, but which each block plugin may override. E.g. by default, there is no cache granularity, but a menu block may declare itself to be cacheable per page and per role.
  2. There is a new @cache_contexts service that looks for tagged "cache context" services and makes those cache contexts available to the entire Drupal caching system. Drupal core will ship with:
    1. cache_context.url (UrlCacheContext), cfr. DRUPAL_CACHE_PER_PAGE)
    2. cache_context.language (LanguageCacheContext), cfr. the old implicit per-language-if-multiple-available caching of blocks)
    3. cache_context.theme (ThemeCacheContext), cfr. the old implicit per-theme caching of blocks)
    4. cache_context.user (UserCacheContext), cfr. DRUPAL_CACHE_PER_USER)
    5. cache_context.user.roles (UserRolesCacheContext), cfr. DRUPAL_CACHE_PER_ROLE)
  3. Query-dependent cache keys can now be generated using Cache::keyFromQuery().
  4. Consequently, the old block cache API and related functions are now removed: the old constants, and the drupal_render_cache_by_query() and drupal_render_cid_parts() functions.
  5. A block's "cache" property is no longer set to to "enabled" (1) or "disabled" (-1), but has a max_age property now, that defaults to 0 (as in "this block may be cached for zero seconds"). It also has a contexts property, which allows the user to specify cache contexts the block should be varied by.
  6. Significantly simplified block rendering logic.
  7. (#58) Cache::PERMANENT === -1 instead of Cache::PERMANENT === 0

Original report by @moshe weitzman

Problem/Motivation

As of #918808: Standardize block cache as a drupal_render() #cache, The Block Cache system is a shorthand that blocks can optionally use to get #cache set on their render arrays. That #cache array gets the proper keys on it for each block constant (e.g. DRUPAL_CACHE_PER_ROLE). The #cache array can't be further customized though, so it has no custom expire time, no custom bin, and now no custom cache tags. Especially this last limitation is the final straw.The value of the shorthand is no longer there.

Proposed resolution

Blocks should simply return render arrays, with #cache on them if they want to be cached. Only one block in all of core uses the block cache constants (book module). That block is converted to the new system without logic changes

Files: 
CommentFileSizeAuthor
#132 blockcache-2158003-132.patch126.35 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,636 pass(es).
[ View ]
#124 d8_blockcache_diff.png34.49 KBmsonnabaum
#80 interdiff.txt745 bytesWim Leers
#80 blockcache-2158003-80.patch77.83 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,098 pass(es).
[ View ]
#67 blockcache-2158003-67.patch81.11 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,096 pass(es).
[ View ]
#6 blockcache_interdiff.txt15 KBmsonnabaum
#6 blockcache-2158003-6.patch45.74 KBmsonnabaum
FAILED: [[SimpleTest]]: [MySQL] 59,574 pass(es), 16 fail(s), and 0 exception(s).
[ View ]
#1 block_cache.patch34.86 KBmoshe weitzman
PASSED: [[SimpleTest]]: [MySQL] 59,412 pass(es).
[ View ]

Comments

moshe weitzman’s picture

Status:Active» Needs review
Related issues:+#918808: Standardize block cache as a drupal_render() #cache
StatusFileSize
new34.86 KB
PASSED: [[SimpleTest]]: [MySQL] 59,412 pass(es).
[ View ]
moshe weitzman’s picture

Title:Remove Block Cache APi in favor of blocks returning #cache with cache tags» Remove Block Cache API in favor of blocks returning #cache with cache tags
Issue summary:View changes
moshe weitzman’s picture

Issue summary:View changes
Issue tags:+D8 cacheability, +Performance
moshe weitzman’s picture

Issue summary:View changes
benjy’s picture

I tested the patch and every seems to work OK. The block caching stuff has gone from the UI and overall it's a nice clean-up.

Few things,

/**
* Block cache is always enabled in 8.x.
*
* @ingroup config_upgrade
*/
function block_update_8000() {
  update_variable_del('block_cache');
}

Should we remove this as well? Or maybe that serves a purpose for the migration people.

Is the book module the only one that provides a block that's cached in core?

+1 for RTBC from me but I think it would be worth having another set of eyes look at this as well.

msonnabaum’s picture

StatusFileSize
new45.74 KB
FAILED: [[SimpleTest]]: [MySQL] 59,574 pass(es), 16 fail(s), and 0 exception(s).
[ View ]
new15 KB

I like the general idea here of not having block cache be its own thing, but I think this approach has some downsides. Primarily, it exposes the guts of render caching to the block implementor. While the #cache part is rather simple, putting the expensive bits in #pre_render is not. I think this will result in some very long, complex Block::build methods, as well as confusion around how #pre_render works. Some might argue that this is knowledge that most Drupal developers should have, but considering that it was introduced in D7 and there are still relatively few that are familiar with it, I'd say that's unrealistic.

Instead of throwing out all of block cache, I think we can just fix what's not working, and expose what render cache needs at a higher level.

This patch builds on #1, but adds a "CacheableInterface," which contains methods for each part of #cache. This allows us to assemble a render array for a block using the cache data from the CacheableInterface methods, and assigning it's "build" method as a #pre_render callback. This is actually similar to how block cache works in core now, we just dont expose anything other than "granularity" in the block configuration.

Some notes about the patch:

- Adds support for passing "cache contexts" as keys. This is essentially what we do now with granularity, but I think it makes sense to just treat them as keys and not their own special thing. I'm using the existing constants in the patch, but I could see us adding something like Cache::CURRENT_USER, or "%current_user". There is also a clear overlap between CacheHelper::addCacheContextsToKeys and drupal_render_cid_parts. Ideally we'd be deprecate granularity and drupal_render_cid_parts would go away.
- To demonstrate how this could work in practice, I added a "Cache: max age" field to the default block form. This seems to work fine, even for views blocks (views could actually remove their blockcache support if we did this). Although this implementation is a bit naive, we should have *some* basic cache config on the block form.
- CacheHelper is obviously not a great name, but I just used it as a temporarily place to throw new things. Stuff like keyFromQuery should probably be in a Cacheable trait, whereas addCacheContextsToKeys should be in some type of CacheContext service (it appears that we still dont have a consistent method of accessing global contexts).
- I reworked the forum blocks a bit since they use drupal_render_cache_by_query, which does quite a bit on its own. I extracted the logic for getting a key from the query, so it's just returning that for it's cacheKeys() method, and the base class handles the execution of the query rather than having to stash it in #query for a function to execute later on. This feels like a way cleaner separation of responsibilities to me.

Another thing to note here, is that most of this isnt specific to block cache or render arrays, so we reuse this interface elsewhere, regardless of the caching mechanism.

The last submitted patch, 6: blockcache-2158003-6.patch, failed testing.

moshe weitzman’s picture

The code and the justification look good to me. Thanks.

  1. +++ b/core/modules/block/lib/Drupal/block/BlockBase.php
    @@ -185,4 +196,25 @@ public function getMachineNameSuggestion() {
    +    return (int)$this->configuration['cache']['max_age'] > 0;

    Should be boolean based on method name

  2. +++ b/core/modules/block/lib/Drupal/block/BlockViewBuilder.php
    @@ -40,21 +40,32 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
    +            '#pre_render' => array(array($plugin, 'build')),

    Why is #pre_render nested under #content (and #cache is not)? Thats not how current code does it.

  3. Looks like we are losing some test coverage. If so, thats no good.
  4. Need doxygen on new methods/classes.
msonnabaum’s picture

StatusFileSize
new38.11 KB
FAILED: [[SimpleTest]]: [MySQL] Fetch test patch: failed to retrieve [blockcache-2158003-9.patch] from [drupal.org].
[ View ]

@moshe -

1. That method does return a bool.

2. That was the only way I could get it to work. I believe the difference is the result of handling it in the view builder as opposed to _block_get_renderable_region. Moving this wasn't strictly necessary, but it just seemed wrong to me to have the knowledge of how to construct a render array from a block spread across two different places. Who is responsible for this knowledge is debatable, but it should at least be in one place.

This patch adds the block cache tests back and adds more docs.

Status:Needs review» Needs work

The last submitted patch, 9: blockcache-2158003-9.patch, failed testing.

The last submitted patch, 9: blockcache-2158003-9.patch, failed testing.

msonnabaum’s picture

Status:Needs work» Needs review
StatusFileSize
new39.7 KB
FAILED: [[SimpleTest]]: [MySQL] 59,719 pass(es), 18 fail(s), and 0 exception(s).
[ View ]
new2.38 KB

Updated the blockcache tests to use the new interface instead of the constants.

The last submitted patch, 12: blockcache-2158003-12.patch, failed testing.

dawehner’s picture

Awesome work!

  1. +++ b/core/lib/Drupal/Core/Cache/CacheableHelper.php
    @@ -0,0 +1,89 @@
    +   */
    +  protected function currentPath() {
    +    global $base_root;
    +    return $base_root . request_uri();
    +  }

    Let's pull that from the request object instead.

  2. +++ b/core/modules/block/lib/Drupal/block/BlockBase.php
    @@ -20,7 +21,8 @@
    +abstract class BlockBase extends PluginBase implements BlockPluginInterface,
    ...

    ...
    +                                                       CacheableInterface {

    WTF to this line-wrapping.

  3. +++ b/core/modules/block/lib/Drupal/block/BlockBase.php
    @@ -109,6 +113,12 @@ public function buildConfigurationForm(array $form, array &$form_state) {
    +    $form['cache']['max_age'] = array(
    +      '#type' => 'select',
    +      '#title' => t('Cache: Max age'),
    +      '#default_value' => $this->configuration['cache']['max_age'],
    +      '#options' => drupal_map_assoc(array(0, 60, 300, 1800, 3600, 21600, 518400), 'format_interval'),
    +    );

    Should we really expose this in a UI? If we do we should explain this option better.

  4. +++ b/core/modules/block/lib/Drupal/block/BlockViewBuilder.php
    @@ -40,21 +40,32 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
    +            'expire' => REQUEST_TIME + $plugin->cacheMaxAge(),

    Do we need expire as well as max_age?

  5. +++ b/core/modules/book/lib/Drupal/book/Plugin/Block/BookNavigationBlock.php
    @@ -116,4 +116,12 @@ public function build() {
    +  }
    ...
    +  public function cacheKeys() {
    +    return array('book_navigation', 'book', DRUPAL_CACHE_PER_PAGE, DRUPAL_CACHE_PER_ROLE);
    +  }
    +
    +  public function cacheTags() {
    +    // @todo Save tags for each entity in the book.
    +    return array('content' => TRUE);

    It is really great that this this information can be provided by blocks now! I could imagine that even a view could implement that interface and used both on pages and blocks.

Wim Leers’s picture

Status:Needs review» Needs work

Since #2167039-85: Regression: page cache tags broken, also: some routes incorrectly use _controller -> No page object in template variables -> Fatal in seven_preprocess_page(), the CacheableInterface has been committed, so this patch no longer needs to introduce it. Note that methods have been prefixed with "get", though.

msonnabaum’s picture

Status:Needs work» Needs review
StatusFileSize
new2.31 KB
new38.64 KB
FAILED: [[SimpleTest]]: [MySQL] 63,227 pass(es), 72 fail(s), and 16 exception(s).
[ View ]

Here's a re-roll to use the cache interface that's in core now.

msonnabaum’s picture

@dawehner

1. Agreed. I was just hesitant to do much with this helper since the whole thing feels kinda temporary, it should be changed in this patch either way.

2. I do that style of line wrapping instead of letting it go over 80 chars because I think it's a lot more readable and looks ok when you add more. I'm sure I'm breaking some doc I've never read by doing it, so happy to change it if it avoids a bike shed.

3. Yes, we definitely need to have this in the UI. Any block can disable it easily by returning FALSE for isCacheable. I think what's there is the bare minimum, but there's certainly room for improvement. This patch should improve the docs as you suggest, but I'd like to leave anything more advanced there for a separate issue.

4. Expire is just in render api. I think we should only ever expose max age, since it better describes what is wanted. Max age is already used for page cache, so just doing the same thing here.

Status:Needs review» Needs work

The last submitted patch, 16: blockcache-2158003-15.patch, failed testing.

sun’s picture

Nice work!

  1. +++ b/core/modules/block/lib/Drupal/block/BlockBase.php
    @@ -185,4 +196,51 @@ public function getMachineNameSuggestion() {
    +   * Uses the "content" tag by default. Implementations are encouraged to
    +   * overide this method to provide more granular tags.
    ...
    +  public function getCacheTags() {
    +    return array('content' => TRUE);
    +  }

    Would it make sense to add the block's ID as a tag in this base implementation?

    So if a particular block is updated/deleted, we'd be able to invalidate all the caches that contain it?

  2. +++ b/core/modules/forum/lib/Drupal/forum/Plugin/Block/NewTopicsBlock.php
    @@ -21,17 +21,12 @@ class NewTopicsBlock extends ForumBlockBase {
    -  public function build() {
    -    $query = db_select('forum_index', 'f')
    +  protected function forumQuery() {
    +    return db_select('forum_index', 'f')

    The new method name is not really clear to me.

    Also, the name makes it sound as if the query would be executed, but the method actually just builds a Select query object only.

    Can we rename this into buildForumQuery()?

  3. +++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.php
    @@ -101,4 +101,8 @@ public function getMachineNameSuggestion() {
    +  public function cacheKeys() {
    +    return array($this->getMachineNameSuggestion());
    +  }

    Shouldn't all of these block-specific implementations call into parent::getCacheKeys() + add their own, instead of replacing the base tags?

Wim Leers’s picture

Priority:Normal» Critical
Status:Needs work» Needs review
Issue tags:+Spark, +sprint
Related issues:+#2124957: Replace 'content' cache tag with 'rendered' and use it sparingly
StatusFileSize
new66.11 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch blockcache-2158003-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new39.06 KB

#19.1: YES! Blocks lack cache tags. The only question is whether it's appropriate to do it in this issue or not. However, it turns out to be rather trivial:

  public function getCacheTags() {
    $block_cache_tag = str_replace(':', '__', $this->getPluginID());
    return array('content' => TRUE, 'block' => $block_cache_tag);
  }

Therefor I propose to add it here.

In this reroll

  1. The reroll at #15 didn't update invocations of the non-get-prefixed methods on CacheableInterface to the get-prefixed ones. Fixed.
  2. Some blocks, like the "powered by" block is by nature static. That's why I added "1 month", "1 year" and "10 years" as well. I replaced "6 days" with "1 week" and I also added "1 day". The end result is that all the typical units of time us humans use are available. I applied "10 years" to the "powered by" block by default.
  3. Block cache tags added. Comes with test coverage.
  4. (After I added block cache tags, I kept finding problem after problem after problem. So I had to change *a lot*. High-level points below, see the patch for details.)
  5. Removed _block_get_renderable_region() and _block_get_renderable_block().
  6. Fixed ordering of blocks: now uses the same sort function as elsewhere.
  7. Views blocks inappropriately exploited an implementation detail: build() is supposed to return just a render array, but Views used the fact that this ran before getConfiguration() to dynamically change the label configuration. This is wrong. And it breaks now that the calling order has changed. Fixed.
  8. CacheableInterface doesn't belong on BlockBase, but on BlockPluginInterface.
  9. Cache keys should be generated by BlockViewBuilder, not by BlockBase. Block plugins can *add* more cache keys though (only "cache context" keys are necessary: per role, per page, query). The default cache keys now are analogous to those used for other entities.
  10. Contextual link handling on blocks simplified. The special casing for the "help" and "main content" blocks is removed from Block module.
  11. hook_block_view_alter() is NOT being invoked when building blocks that will be cached in D8 HEAD. Fixed.
  12. Use the same helper method for both non-cacheable blocks and cacheable blocks (in the latter case this method is used as a #pre_render callback).
  13. Added test coverage for contextual links on cacheable blocks.
  14. Ensured that the "main content" and "help" blocks are never cacheable, and that the "Cache: Max age" form item doesn't show up for them.
  15. The default "Powered by Drupal" block in Bartik has been changed to be cached for 10 years by default.
  16. sun's remarks implemented.

TODO

  1. In BlockTest, one assertion fails, and I can't seem to fix it. I hope somebody else can.
  2. Test coverage for CacheableHelper missing.
  3. … let's hope not too many tests fail :)

Marking critical because this blocks #2124957: Replace 'content' cache tag with 'rendered' and use it sparingly.

Status:Needs review» Needs work

The last submitted patch, 20: blockcache-2158003-20.patch, failed testing.

msonnabaum’s picture

StatusFileSize
new63.09 KB
FAILED: [[SimpleTest]]: [MySQL] 63,641 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

Here's a re-roll of #20 that hopefully applies.

msonnabaum’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 22: blockcache-2158003-22.patch, failed testing.

Wim Leers’s picture

At least *less* fails now :)

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new66.41 KB
FAILED: [[SimpleTest]]: [MySQL] 63,628 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new3.96 KB

Now there should be only one fail.

Status:Needs review» Needs work

The last submitted patch, 26: blockcache-2158003-26.patch, failed testing.

moshe weitzman’s picture

  1. isCacheable() should cast to boolean, not int
  2. '#title' => t('Cache: Max age'). This needs a description. I doubt most folks know what this means.
  3. Unrelated: menu block and book block have DRUPAL_CACHE_PER_PAGE. Do we really want that?

I'll take a look at the test fail now.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new66.53 KB
FAILED: [[SimpleTest]]: [MySQL] 63,686 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new1.25 KB
  1. Fixed.
  2. Added.
  3. Book block already had that. I think it makes sense for the menu block to have it too, because menu_tree() is used for rendering menus and its docs say:
    /**
    * Renders a menu tree based on the current path.
    *
    * The tree is expanded based on the current path and dynamic paths are also
    * changed according to the defined to_arg functions (for example the 'My
    * account' link is changed from user/% to a link with the current user's uid).
Wim Leers’s picture

I just found a massive bug introduced by CacheableHelper: because DRUPAL_CACHE_PER_(PAGE|ROLE|USER) are integers, that means that any cache key that is integer 1, 2 or 3 will also be upcasted to the corresponding cache key that CacheableHelper generates.

So, the render cache entry for node/term/user/… 1, 2 or 3 will have a wrong cache key like
entity_view:taxonomy_term:r.anonymous:full:bartik:r.anonymous

instead of

entity_view:taxonomy_term:1:full:bartik:r.anonymous

The simplest solution would be to change DRUPAL_CACHE_PER_(PAGE|ROLE|USER) to strings. But even in that case, unwanted replacements are possible (though improbable).

Keeping cache granularities, having them affect cache keys & cache tags

Overall, it looks like it might be better to not merge "cache granularity" with "cache keys"? When caching, "cache granularity" should indeed be converted to "cache keys". But there's an important distinction:

  1. Cache tags need to be bubbled up, because they indicate "what the contained things are", so if a parent gets cached, then it must be tagged with the union of all nested cache tags.
  2. Cache keys, on the other hand, don't need to be bubbled up — they couldn't be, it'd be nonsensical. Only the current thing knows *what* it is and *how* to generate a unique key.
  3. Cache granularity is something in between cache tags and cache keys. Like cache tags, they need to be bubbled up, because if something grandchild in the current render array needs to be per-user, then the grandparent needs to be per-user too! And because it needs to be per-user, it must also affect the cache key (if not, then something cached for user A would be served to user B). However … and we haven't addressed this yet … it should in fact also affect the cache tags: if the granularity includes CACHE_PER_ROLE, then changing anything about the role might cause this cache entry to be stale. The mechanism for that is cache tags.

So, in short:

  1. Cache tags: for cache invalidation.
  2. Cache keys: for cache lookup.
  3. Cache granularity: for request contexts, which affects both cache invalidation and cache lookup.

Finally, at the very least some cache granularities are missing, such as DRUPAL_CACHE_PER_LANGUAGE (currently, entity render caching is language-agnostic… which means that it will unfortunately serve the same cache entry regardless of current language — easily fixable, but what if UI translations change? Since there's no cache tags for languages, you'll have to clear all the caches. Keeping cache granularities and have them affect cache keys & tags would solve that.) But more likely, we're missing the ability to *add* cache granularities, such as DRUPAL_CACHE_PER_COUNTRY.

Status:Needs review» Needs work

The last submitted patch, 29: blockcache-2158003-29.patch, failed testing.

catch’s picture

However … and we haven't addressed this yet … it should in fact also affect the cache tags: if the granularity includes CACHE_PER_ROLE, then changing anything about the role might cause this cache entry to be stale. The mechanism for that is cache tags

This isn't necessarily true, CACHE_PER_PAGE/per language etc. are only about ensuring the right version of content is served, not necessarily at all about invalidation. If invalidation is also needed (for example a role name being rendered), then tags should be added separately.

It's also not feasible to bubble up cache granularity. There's #2099137: Entity/field access and node grants not taken into account with core cache contexts for allowing modules to affect cache granularity higher than the level they actually get invoked. Then there's #post_render_cache for allowing content to avoid adding additional granularity at all, that's the best we can do I think.

Wim Leers’s picture

This isn't necessarily true, CACHE_PER_PAGE/per language etc. are only about ensuring the right version of content is served, not necessarily at all about invalidation. If invalidation is also needed (for example a role name being rendered), then tags should be added separately.

I agree that currently it's not at all about invalidation. But I'm questioning whether that's correct. If it needs to be cached on a per-X basis… doesn't that always imply that if X changes, the cache entries should be invalidated too?
Can you come up with an example of where my statement doesn't hold?

It's also not feasible to bubble up cache granularity.

Why not?

It'd be just the same as bubbling up cache tags:

cache_granularity(element) = union(cache_granularity(child of element))
catch’s picture

Cache tags only need to be calculated on a cache miss, the cache ID needs to be calculated on cache hits as well.

But I'm questioning whether that's correct. If it needs to be cached on a per-X basis… doesn't that always imply that if X changes, the cache entries should be invalidated too?

Per-page caching doesn't relate to invalidation at all - it just means you want something recalculated on every path uniquely.

Per-language caching - possibly you might want to update the HTML cache of some user interface text if a translation gets updated, but I don't think you'd want an entity to be invalidated. Updating a translation of the entity means the entity itself gets updated anyway.

Wim Leers’s picture

but I don't think you'd want an entity to be invalidated

But many (most?) rendered entities contain at least some labels, and labels are translated using UI translation, not entity translation.

catch’s picture

This is true. In that case I'd expect the field system to add that tag though - it knows it's adding interface translated labels that might need invalidation per-language, and it could do so only when one instance has the label set to not hidden even.

Wim Leers’s picture

If the field system is responsible for that, then the menu system will have to add it for every menu block, the search block will have to add it for itself, and so on. Does that really make sense?

The current UI language affects everything with a UI string, which is… a lot, if not almost everything. That's why I argue it should be a cache granularity.

catch’s picture

Since block titles are translatable/translated, wouldn't the block system just add it for all blocks?

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new69.02 KB
FAILED: [[SimpleTest]]: [MySQL] 64,767 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new6.37 KB

Reroll; patch didn't apply anymore.

Additional changes (see interdiff):

  1. #2179083: Rendered menus (e.g. menu blocks) should set cache tags to inform the page cache went in and added test coverage that this patch would break, so updated those tests.
  2. In doing so, I noticed that cache tags were only being set if blocks were cacheable; this is wrong. Cache tags should always be added, because they inform container elements, in this case the page cache. Fixed.
  3. The default caching of the "Powered by" block was added in #20 but got lost in the #22 reroll. Now it's added again, but this time it's set on the block plugin itself, with Cache::Permanent.
  4. The hiding of the "Cache: Max age" form item when a block is not cacheable was said to be added in #20 but was actually not in the patch. Restored.

Detailed test coverage for block's cache tags should wait until #2188565: Test coverage for Comment, Custom Block, Node, Taxonomy Term and User entity cache tags gets in, which adds helpful base classes to do exactly that.

Status:Needs review» Needs work

The last submitted patch, 39: blockcache-2158003-39.patch, failed testing.

dawehner’s picture

Adding a reference to an issue which touches A LOT of similar code.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Cache/CacheableHelper.php
    @@ -0,0 +1,89 @@
    +/**
    + * A helper class that contains methods that could be useful to any object using
    + * the Drupal Cache API.
    ...
    +class CacheableHelper {
    ...
    +  protected function getContext($context) {
    +    switch ($context) {
    +    case DRUPAL_CACHE_PER_PAGE:
    +      // @todo: Make this use the request properly.
    +      return $this->currentPath();
    +    case DRUPAL_CACHE_PER_USER:
    +      return "u." . $this->currentUser()->id();
    +    case DRUPAL_CACHE_PER_ROLE:
    +      return 'r.' . implode(',', $this->currentUser()->getRoles());
    +    default:
    +      return FALSE;
    +    }

    If we use it like that we have to document why this cannot be a service

  2. +++ b/core/modules/block/block.module
    @@ -479,6 +417,11 @@ function template_preprocess_block(&$variables) {
    +  // A block's label is configuration: it is static. Allow dynamic labels to be
    +  // set in the render array.
    +  if (isset($variables['elements']['content']['#block_label']) && !empty($variables['configuration']['label_display'])) {
    +    $variables['label'] = $variables['elements']['content']['#block_label'];
    +  }

    Is there any reason we don't name it #label instead or even just #title?

  3. +++ b/core/modules/block/lib/Drupal/block/BlockBase.php
    @@ -32,7 +34,9 @@ public function __construct(array $configuration, $plugin_id, array $plugin_defi
    +        'max_age' => 0,

    @@ -108,7 +112,19 @@ public function buildConfigurationForm(array $form, array &$form_state) {
    +      '#options' => drupal_map_assoc(array(0, 60, 300, 1800, 3600, 21600, 86400, 604800, 2592000, 31536000, 315360000), 'format_interval'),

    See below

  4. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockInterfaceTest.php
    @@ -83,6 +85,14 @@ public function testBlockInterface() {
    +          '#options' => drupal_map_assoc(array(0, 60, 300, 1800, 3600, 21600, 86400, 604800, 2592000, 31536000, 315360000), 'format_interval'),

    We do have alternatives for both drupal_map_assoc and format_interval.

  5. +++ b/core/modules/forum/lib/Drupal/forum/Plugin/Block/ForumBlockBase.php
    @@ -55,4 +79,12 @@ public function blockSubmit($form, &$form_state) {
    +    $cacheable_helper = new CacheableHelper;
    +    return array($this->cacheKeyFromQuery($this->buildForumQuery()));

    Is it just me that the cacheable_helper is not used here?

  6. +++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
    @@ -358,6 +358,7 @@ protected function drupalCreateContentType(array $values = array()) {
    +   *   - cache: array('max_age' => 0).

    @@ -374,6 +375,9 @@ protected function drupalPlaceBlock($plugin_id, array $settings = array()) {
    +        'max_age' => 0,

    Should we use CacheBackendInterface::CACHE_PERMANENT instead?

  7. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemHelpBlock.php
    @@ -117,4 +117,24 @@ public function build() {
    +    // The help block is never cacheable.
    +    $form['cache']['#access'] = FALSE;
    ...
    +    // The help block is never cacheable.
    +    return FALSE;

    +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemMainBlock.php
    @@ -28,4 +28,24 @@ public function build() {
    +    // The main content block is never cacheable.
    +    $form['cache']['#access'] = FALSE;
    ...
    +    // The main content block is never cacheable.
    +    return FALSE;

    We should maybe also explain why the block is not cacheable.

  8. +++ b/core/modules/system/lib/Drupal/system/Tests/Cache/PageCacheTagsIntegrationTest.php
    @@ -64,9 +64,28 @@ function testPageCacheTags() {
         $this->verifyPageCacheTags($node_1_path, array(

    @@ -76,6 +95,17 @@ function testPageCacheTags() {
         $this->verifyPageCacheTags($node_2_path, array(

    Note: this method uses assertIdentical which caused problems in the linked issue above. I guess you had to manually rearrange all those tags?

  9. +++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.php
    @@ -29,13 +29,11 @@ public function build() {
         $this->view->display_handler->preBlockBuild($this);

         if ($output = $this->view->executeDisplay($this->displayID)) {
    -      // Set the label to the title configured in the view.
    -      if (empty($this->configuration['views_label'])) {
    -        $this->configuration['label'] = Xss::filterAdmin($this->view->getTitle());
    -      }
    -      else {
    -        $this->configuration['label'] = $this->configuration['views_label'];
    +      // Override the label to the dynamic title configured in the view.
    +      if (empty($this->configuration['views_label']) && $this->view->getTitle()) {
    +        $output['#block_label'] = Xss::filterAdmin($this->view->getTitle());
           }

    I really like that we have support for it now!

  10. +++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.php
    @@ -48,6 +46,20 @@ public function build() {
        */
    +  public function getConfiguration() {
    +    $configuration = parent::getConfiguration();
    +
    +    // Set the label to the static title configured in the view.
    +    if (!empty($configuration['views_label'])) {
    +      $configuration['label'] = $configuration['views_label'];
    +    }
    +
    +    return $configuration;
    +  }
    +
    +  /**
    +   * {@inheritdoc}

    Are you sure this change is needed? This will potentially change the behavior before saving, while the old code did just ran during building() so the stored data never changes.

  11. +++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.php
    @@ -101,4 +113,8 @@ public function getMachineNameSuggestion() {

    +  public function cacheKeys() {
    +    return array($this->getMachineNameSuggestion());
    +  }

    Given that the configuration of the block might change the output of the block (like change the total displayed items) we should rely on the actual configured block ID instead.

dawehner’s picture

Great work btw.!

dawehner’s picture

Great work btw.!

effulgentsia’s picture

Status:Needs work» Needs review
StatusFileSize
new65.91 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

Just a reroll. Does not address any feedback. CNR for bot.

Status:Needs review» Needs work

The last submitted patch, 45: blockcache-2158003-45.patch, failed testing.

Wim Leers’s picture

Status:Needs work» Needs review

45: blockcache-2158003-45.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 45: blockcache-2158003-45.patch, failed testing.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new69.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,831 pass(es), 6 fail(s), and 1 exception(s).
[ View ]

Straight reroll from #39, hopefully this one will allow Drupal to install again…

Status:Needs review» Needs work

The last submitted patch, 49: blockcache-2158003-49.patch, failed testing.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new70.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,835 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
new7.94 KB

Fixed test failures, except the one we've been trying to debug for weeks. Debugging that one is up next.


#42:

  1. I'll leave this for msonnabaum.
  2. Nope, no reason, except for clarity/explicitness. I went with #title.
  3. Fixed.
  4. Fixed.
  5. Fixed.
  6. No, 0 means that something may not be cached (i.e. cache for zero seconds), Cache::Permanent means it can be cached infinitely.
    However, that's a great point, because Cache::Permanent === 0! That's a pretty big problem right there… @msonnabaum, any suggestions on how we fix that?
  7. Done.
  8. The tags are in a specific order, yes, but I didn't have to rearrange anything. The order is always the same because they are set and collected in a deterministic manner. If you prefer, we can change those assertions to check equality instead of identicalness.
  9. :)
  10. Interesting point. I'm sure this is needed — from #20, point 7:

    Views blocks inappropriately exploited an implementation detail: build() is supposed to return just a render array, but Views used the fact that this ran before getConfiguration() to dynamically change the label configuration. This is wrong. And it breaks now that the calling order has changed. Fixed.

    getConfiguration() MUST return the effective configuration. Otherwise you break assumptions in the block render layer.
    Finally, Views already was doing the exact same thing: it was modifying $this->configuration before the patch, that hasn't changed. It'd be better if $configuration['views_label'] didn't exist at all, but that was probably done for good reason (i.e. for some reason you cannot use $configuration['label']?).

  11. That's already the case. From BlockViewBuilder:
          if ($plugin->isCacheable()) {
            $build[$entity_id]['#pre_render'][] = array($this, 'buildBlock');
            // Generic cache keys, with the block plugin's custom keys appended
            // (usually cache context keys like DRUPAL_CACHE_PER_ROLE).
            $default_cache_keys = array('entity_view', 'block', $entity->id(), $entity->langcode);
            $build[$entity_id]['#cache'] += array(
              'keys' => array_merge($default_cache_keys, $plugin->getCacheKeys()),
              'bin' => $plugin->getCacheBin(),
              'expire' => REQUEST_TIME + $plugin->getCacheMaxAge(),
            );
          }

    In there, $entity->id() ensures the configured block ID ends up in the cache key. The funny thing is that this is *identical* to the code you were commenting on… so it's actually completely useless. Removed it. Well-spotted :)

Status:Needs review» Needs work

The last submitted patch, 51: blockcache-2158003-51.patch, failed testing.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new72.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,875 pass(es), 8 fail(s), and 43 exception(s).
[ View ]
new9 KB

Fixed one test failure in this reroll. The problem was this hunk that I added for usability reasons:

    // Don't allow the user to configure the max age for this block if the block
    // plugin declares itself not cacheable or if it declares itself permanently
    // cacheable.
    if (!$this->isCacheable() || $this->getCacheMaxAge() === Cache::PERMANENT) {
      $form['cache']['#access'] = FALSE;
    }

The problem is that by default, every block is not cacheable. So you wouldn't be able to configure them. So very silly. The solution is to not do this automatically, but let blocks with hardcoded cacheability settings slightly alter the form to communicate their hard-coded defaults to the end user. We can worry about an abstraction to do that later, for now this is sufficiently clean.

Now this should be down to 3 test failures again.

Status:Needs review» Needs work

The last submitted patch, 53: blockcache-2158003-53.patch, failed testing.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new74.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,054 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new2.54 KB

Some unexpected failures. Fixed and locally verified that this should now be down to 3 failures again.

Status:Needs review» Needs work

The last submitted patch, 55: blockcache-2158003-55.patch, failed testing.

The last submitted patch, 55: blockcache-2158003-55.patch, failed testing.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new74.91 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,059 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new3.33 KB

@dawehner/#42.6: Regarding Cache::PERMANENT === 0: I discussed this with msonnabaum. The short version is: "Cache::PERMANENT === 0 doesn't make sense".

The longer version:

  • Cache::PERMANENT is intended to be used in a cache bin's expire column. That indicates a UNIX timestamp at which a given cache entry should expire.
  • The expire column is really a materialized/absolute TTL. A TTL is relative, and with only a TTL, you don't know when to expire a cache entry. Therefor, we convert a TTL into a UNIX timestamp.
  • In Drupal 7, we had CACHE_PERMANENT === 0 and CACHE_TEMPORARY === -1. Now we only have Cache::PERMANENT anymore.
  • 0 is a valid UNIX timestamp. Probably because -1 already had been assigned a different meaning in Drupal 7, we opted for 0 as the value representing "cache permanently".
  • Now we can make things more consistent and simple: every valid UNIX timestamp is just a UNIX timestamp, without special meaning. We only assign special meaning to an invalid UNIX timestamp: Cache::PERMANENT === -1.
  • The side benefit is that we can now use Cache::PERMANENT both for TTL (i.e. when indicating how long something should be cacheable, e.g. a block's max age) and actual cache entry expiration (i.e. how long something is cacheable, e.g. a cached block).

Status:Needs review» Needs work

The last submitted patch, 58: blockcache-2158003-58.patch, failed testing.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new75.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,059 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new934 bytes

I finally found the root cause for the two BookTest failures since #51:

  1. #51 renamed #block_label to #title.
  2. The two failing tests occurred in testNavigationBlockOnAccessModuleEnabled(), which specifically sets 'block_mode' => 'book pages'. This causes BookNavigationBlock::build() to execute the code in the elseif ($current_bid) {} scope. That scope contains this code:
            if (!empty($below)) {
              $book_title_link = array('#theme' => 'book_title_link', '#link' => $data['link']);
              return array(
                '#title' => drupal_render($book_title_link),
                $below,
              );

    Yep, that's #title right there.

  3. Therefore the assigned block label doesn't get printed, but whatever is assigned to #title is. And apparently, that's empty.
  4. So $book_title_link is broken. But what *should* it generate? Taking a look at where it comes from… it comes from the massive blocks-as-plugins commit/patch.
  5. If we go back to that version of Drupal 8, then theme_book_title_link() also didn't exist. And there was no book title or title link visible in the output. So this has been broken for well over a year. Furthermore, the test coverage is unchanged: testNavigationBlockOnAccessModuleEnabled() is functionally equivalent with what we had back then.
    Most interesting: it explicitly tests that the block label is the randomly generated one, not the book title!
  6. Therefore, that whole #title thing is just dead code and can be removed!
$ git show
commit 6e7b1219feccc456297876d73092d42d93f56a30
Author: Dries <dries@buytaert.net>
Date:   Fri Jan 4 12:05:13 2013 -0500

    Issue #1535868 by EclipseGc, tim.plunkett, xjm, Jody Lynn, sdboyer, naxoc, tizzo, effulgentsia, dawehner, disasm, beejeebus: Convert all blocks into plugins.


Now this should be down to 1 test failure again.

Status:Needs review» Needs work

The last submitted patch, 60: blockcache-2158003-60.patch, failed testing.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new81.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,101 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new10.87 KB

Clean-up:

  • Fixed: all block plugins that implement defaultConfiguration() should call the parent method.
  • Fixed: made the "max age" setting for page cache and blocks consistent: similar wording, identical options, etc.
  • Fixed: block.schema.yml was outdated.
  • Improved: SystemMenuBlock now defaults to caching forever. We can do this now (and enjoy the performance improvement that this brings), because #2179083: Rendered menus (e.g. menu blocks) should set cache tags to inform the page cache ensures menu cache tags are cleared correctly in all cases. In the future, we should be able to make most blocks cached forever by default. Note that for those who have advanced/very dynamic menu blocks: it's trivial to disable caching. We just want Drupal 8 to be fast by default.
tim.plunkett’s picture

+++ b/core/modules/book/lib/Drupal/book/Plugin/Block/BookNavigationBlock.php
@@ -64,7 +64,8 @@ public static function create(ContainerInterface $container, array $configuratio
+    $default_configuration = parent::defaultConfiguration();
+    return $default_configuration + array(

This could have just been return parent::defaultConfiguration() + array(... Oh well.

Also, the + of two arrays won't work if its nested, but that's probably overkill.

Status:Needs review» Needs work

The last submitted patch, 62: blockcache-2158003-62.patch, failed testing.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new80.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,099 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new5.77 KB

Addressed #63.

Status:Needs review» Needs work

The last submitted patch, 65: blockcache-2158003-65.patch, failed testing.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new81.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,096 pass(es).
[ View ]
new844 bytes

And this should fix the last test failure! Finally figured it out :)

Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Cache/CacheableHelper.php
    @@ -0,0 +1,89 @@
    +   *
    +   * @param $query
    +   *   A select query object.
    +   *
    +   * @return string
    +   *   A hash of the query arguments.
    +   */
    +  function keyFromQuery($query) {

    We should type hint this as a SelectQueryInterface... ?

  2. +++ b/core/lib/Drupal/Core/Cache/CacheableHelper.php
    @@ -0,0 +1,89 @@
    +   *
    +   * @todo Document this properly once the input arguments are decided on,
    +   * assuming the reuse of existing cache constants is temporary.

    Nitpick, multi-line @todo's should be indented two spaces.

msonnabaum’s picture

StatusFileSize
new74.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch blockcache-2158003-69.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new9.22 KB

Wasn't happy with how block defaults were being handled. Here's a different approach that simplifies it a bit. The block subclasses no longer have to call parent:: and merging of defaults is now enforced whenever it is set (this seems like an existing bug).

Also took care of Berdir's feedback.

Status:Needs review» Needs work

The last submitted patch, 69: blockcache-2158003-69.patch, failed testing.

tim.plunkett’s picture

+++ b/core/modules/block/lib/Drupal/block/BlockBase.php
@@ -30,11 +30,7 @@
-    $this->configuration += $this->defaultConfiguration() + array(
-      'label' => '',
-      'module' => $plugin_definition['module'],
-      'label_display' => BlockInterface::BLOCK_LABEL_VISIBLE,
-    );

@@ -48,14 +44,14 @@ public function getConfiguration() {
+    $this->configuration = $configuration + $this->baseConfigurationDefaults() + $this->defaultConfiguration();

This used to be $configuration + default + base defaults, you've switched the order.

+++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.php
@@ -48,8 +46,22 @@ public function build() {
+  public function getConfiguration() {
+    $configuration = parent::getConfiguration();
+
+    // Set the label to the static title configured in the view.
+    if (!empty($configuration['views_label'])) {
+      $configuration['label'] = $configuration['views_label'];
+    }
+
+    return $configuration;
+  }

Shouldn't this now be in setConfiguration?

msonnabaum’s picture

StatusFileSize
new74.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch blockcache-2158003-72.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Takes care of the first part of #71. Good call.

msonnabaum’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 72: blockcache-2158003-72.patch, failed testing.

msonnabaum’s picture

Status:Needs work» Needs review
StatusFileSize
new74.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,080 pass(es), 3 fail(s), and 11 exception(s).
[ View ]

Try that again.

Status:Needs review» Needs work

The last submitted patch, 75: blockcache-2158003-75.patch, failed testing.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Cache/CacheableHelper.php
@@ -0,0 +1,91 @@
+ * @todo Consider converting to a trait once traits are available.

We have traits now

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new77.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,114 pass(es), 0 fail(s), and 11 exception(s).
[ View ]
new2.53 KB

Wasn't happy with how block defaults were being handled. Here's a different approach […]

Much better! :)

Making patch green again.

Status:Needs review» Needs work

The last submitted patch, 78: blockcache-2158003-78.patch, failed testing.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new77.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,098 pass(es).
[ View ]
new745 bytes

The exceptions are because we removed one parent::defaultConfiguration() too much: ViewsBlock was explicitly getting the defaults from ViewsBlockBase, so we want to keep that.

Now it should be green again.

effulgentsia’s picture

I tried reading through the patch, but it was a lot to digest, so I haven't grokked a lot of it yet. An updated issue summary explaining everything the patch changes and why, along with what is still being debated (if anything), would be extremely helpful.

I do want to chime in though with a +1 to CacheableInterface (already in HEAD), and to this issue's decision to make BlockBase implement it. I think doing the same to FormatterBase will be the key to the way we solve #2099137: Entity/field access and node grants not taken into account with core cache contexts. That's not this issue's concern, but just adding that for context.

Cache granularity is something in between cache tags and cache keys. Like cache tags, they need to be bubbled up

I agree with catch in #32 that it's not feasible to have render api auto bubble up cache granularity, since that would require you to build out the entire render array tree prior to calculating a cache hit/miss, so on cache hits, you'd lose most of the benefit of the cache. In fact, I think this is the fundamental problem with #cache as a public portion of render api, because if it can be set on any arbitrary element, then descendant elements get no chance to affect the granularity, and instead the module responsible for the descendant element must also know every possible cacheable ancestor and implement an alter for it. However, where D8 gives us a way out of this conundrum is that we now have a manageable set of render cacheable objects: blocks, views, menus, entity displays, and formatters. So, because we know the architectural relationships between these objects, we can implement optimized approaches for bubbling up what's needed via these objects, and for that, having these objects implement CacheableInterface is very handy.

Therefore, I think we can remove 'granularity' entirely from #cache, since with CacheHelper, granularities can be set in 'keys'. That could be a follow up though.

msonnabaum’s picture

Agreed. I'm working on refactoring CacheHelper into a CacheContext service that uses it's own constants, which I should hopefully have posted tomorrow. My plan was to introduce that now, then in a followup we can move stuff like drupal_render_cid_parts, drupal_render_cid_create, etc to classes, and then get rid of granularity.

Wim Leers’s picture

Issue summary updated. I think it should now be possible to only read the issue summary now (+ linked select comments) and get a good sense of what this issue is doing.

Wim Leers’s picture

Issue summary:View changes
msonnabaum’s picture

StatusFileSize
new37.76 KB
new85.2 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,289 pass(es), 20 fail(s), and 257 exception(s).
[ View ]
new23.36 KB

At first I started creating new constants so we weren't abusing the old ones, like Cache::CURRENT_USER. That works, but it is no more capable or flexible than how we used granularity in the past. I feel like if we're going to do this, we should improve it, so that's what this patch is attempting.

It introduces a new CacheContexts service, that holds all the available contexts, and takes over the resolving responsibilities from CacheableHelper. The available contexts are now extendible via tagged services. The existing ones (per user, per role, per page) are converted in this patch and split between core and user module.

To provide a cache context, you simply need a class that declares the contexts you provide and which method to call for each.

I also added a form element to the block configuration form for picking contexts on a per-block basis. This is the feature I've always wanted for blocks/views/panes, but couldn't easily be implemented because granularity was fixed. Here's what it looks like:

block configuration with cache context field

CacheableHelper is now gone. I was going to make a trait, but once I moved all the context stuff out, there was only one method, so I moved keyFromQuery to the main Cache class.

I tried to make declaring cache contexts as simple as possible for contrib, and this is the best I could come up with. I'm not a fan of tagged services, but these should be rare enough that they don't warrant an additional yaml file. It's pretty nuts how complex it is now to just map a string to a callable. I'm open to ideas to simplify it.

Status:Needs review» Needs work

The last submitted patch, 85: blockcache-2158003-85.patch, failed testing.

msonnabaum’s picture

Status:Needs work» Needs review
StatusFileSize
new85.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,314 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
new913 bytes

Hoping this fixes many of those failures…

Status:Needs review» Needs work

The last submitted patch, 87: blockcache-2158003-87.patch, failed testing.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new88.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,304 pass(es).
[ View ]
new6.88 KB

Only making the patch green, only essential changes, no review of #85.

Wim Leers’s picture

StatusFileSize
new93.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,351 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new33.97 KB
new60.35 KB
new59.45 KB
new60.84 KB

#85:

I like the changes you made a lot.

Superficial clean-up changes in this reroll

Almost all changes can be considered "superficial clean-up".

  • The "cache maximum age" setting is the same concept as HTTP's Cache-Control: max-age=<X seconds> header. I think that similarly, the "cache contexts" setting can be considered the same concept as HTTP's Vary: <some request header>.
  • We shouldn't use #type = fieldset, but #type = details.
  • The "cache contexts" setting only makes sense to configure if the "cache maximum age" setting is >0, use #states to ensure it's disabled otherwise.
  • I think it should be called "Cache settings", to be consistent with "Visibility settings". Either both should have the "settings" prefix, or neither.
  • CacheContexts::addContextsToKeys() was poorly named IMHO: it implies cache contexts are being added to cache keys, whereas cache contexts already *are* in the cache keys, but they're placeholders, and this method replaces the placeholders. Therefore, renamed to CacheContexts::convertTokensToKeys()
  • Fixed a lot of docs and typehints.
  • Renamed "Current user id" context to "User". This analogous to what we had before: "cache per user", not "cache per current user". "Current user" could be misinterpreted as being the admin user modifying the form.
  • Renamed "Current user's roles" to "User's roles".
  • The existing blocks that used cache contexts (DRUPAL_CACHE_PER_PAGE and friends) had not yet been updated to use the system introduced in #85. Fixed.
  • Fixed: BlockBase::isCacheable() was returning FALSE when the max age was set to "Forever" (Cache::PERMANENT).

With the above changes, a block using BlockBase now looks like this by default:

Once a max age is configured, the user can configure contexts:

And some blocks come with required contexts, which then looks like this:


Bigger changes in this reroll

  • The "Request path" context was problematic in a few regards: 1) it's referring to "path", but we're getting rid of paths in favor of routes, 2) it's in fact the full URL, so more than only the path (or the route). Therefor, I've renamed it to just "URL". Plus, it's now using the injected Request object. Please verify that what I did is okay.
  • I tried to think of ways to simplify the way you can declare cache contexts. After careful consideration by msonnabaum and myself, we decided to have one service for every cache context, that way no "how to generate X" metadata is necessary anymore. It's very rare to declare new cache contexts at all, let alone *multiple* cache contexts within one module, so it's better DX: a class that can be named after the specific cache context it provides, with only two methods, and nothing else to worry about. We now also have a CacheContextInterface that such services should implement.
  • Status:Needs review» Needs work

    The last submitted patch, 90: blockcache-2158003-90.patch, failed testing.

    Wim Leers’s picture

    Issue summary:View changes
    Status:Needs work» Needs review
    Issue tags:-Needs tests
    Related issues:+#2099137: Entity/field access and node grants not taken into account with core cache contexts
    StatusFileSize
    new105.67 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,321 pass(es), 2 fail(s), and 0 exception(s).
    [ View ]
    new19.14 KB

    This reroll is about removing dead code. And updating the state of the overall issue (updated issue summary + tags). I didn't fix the 2 simple test failures, but I hope it stays at 2 :)

    As of #85, we're removing the "cache granularity" concept entirely as a stand-alone concept: cache keys can now be either static strings or special token/placeholder strings, that are replaced by the corresponding Cache Context service.

    In this reroll

    Removed
    All block cache constants: DRUPAL_NO_CACHE, DRUPAL_CACHE_CUSTOM, DRUPAL_CACHE_PER_ROLE, DRUPAL_CACHE_PER_USER, DRUPAL_CACHE_PER_PAGE, DRUPAL_CACHE_GLOBAL.
    drupal_render_cache_by_query() — its successor was added in #85: Cache::keyFromQuery().
    drupal_render_cid_parts()
    Added
    LanguageCacheContext: functionally identical to the default granularity that drupal_render_cid_parts() used to add
    ThemeCacheContext: functionally identical to the default granularity that drupal_render_cid_parts() used to add
    Updated
    EntityViewBuilder was the only code besides block caching that was using cache granularity — now it's setting the cache_context.user.role cache key instead
    BlockViewBuilder used to depend on drupal_render_cid_parts() to always set the "per theme" cache granularity, now it's setting cache_context.theme for every block. Note that it's not setting the "per language" cache granularity, since that should be a deliberate choice by the admin user or the block plugin developer.
    msonnabaum’s picture

    Awesome work. I agree with nearly all those changes, and the form stuff you changed is exactly what I wanted, but I didn't know how to do with form api.

    Renamed "Current user id" context to "User". This analogous to what we had before: "cache per user", not "cache per current user". "Current user" could be misinterpreted as being the admin user modifying the form.

    That's interesting, I'd never considered that that would be interpreted as the user editing the form. I'm not 100% convinced, but I don't feel strongly either way, I'm fine with the change if others agree.

    Status:Needs review» Needs work

    The last submitted patch, 92: blockcache-2158003-92.patch, failed testing.

    Wim Leers’s picture

    I found the root cause for the two test failures since #90.

    I enabled "cache forever" for SystemMenuBlock since #62. But due to a bug, it was not actually being cached yet, and I fixed that in #90:

    Fixed: BlockBase::isCacheable() was returning FALSE when the max age was set to "Forever" (Cache::PERMANENT).

    With caching of menu blocks actually working, the blocks were now being built in a #pre_render callback. And apparently, cache tags set by #pre_render callbacks are actually broken. Patch to fix that at #2215719: Cache tags set by #pre_render callbacks are lost in page cache. Once that's in, this patch will become green again.

    Wim Leers’s picture

    Status:Needs work» Needs review
    StatusFileSize
    new61.12 KB
    new60.65 KB
    new62.71 KB
    new105.77 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,283 pass(es), 2 fail(s), and 146 exception(s).
    [ View ]
    new6.41 KB

    I went over the patch once more, and this is the final clean-up I found. Notable:

    • Updated block.schema.yml again, for cache contexts.
    • I accidentally didn't re-enable the tests that Mark added after I disabled them for debugging :P
    • The <select multiple></select> form element to select cache contexts doesn't work: many users don't know how to select multiple things and it's too easy to not notice you can scroll to list more things once there are several. So I replaced them with checkboxes.

    So, redid the screenshots from #90:

    With the above changes, a block using BlockBase now looks like this by default:


    Once a max age is configured, the user can configure contexts:


    And some blocks come with required contexts, which then looks like this:



    There should still be 2 failures, see the previous comment.

    Status:Needs review» Needs work

    The last submitted patch, 96: blockcache-2158003-96.patch, failed testing.

    Wim Leers’s picture

    Issue summary:View changes
    Wim Leers’s picture

    Issue summary:View changes
    Gábor Hojtsy’s picture

    I think this is a great generalization and akin to what I imagine EclipseGC envisioned way back to provide contexts to feed into blocks, so they know what they are dependent on. Did not find major issues, found the following though:

    1. +++ b/core/includes/bootstrap.inc
      @@ -974,6 +974,10 @@ function drupal_serve_page_from_cache(stdClass $cache, Response $response, Reque
      +  // Never set an expiration date further than one year into the future.
      +  if ($max_age > 31536000 || $max_age === \Drupal\Core\Cache\Cache::PERMANENT) {
      +    $max_age = 31536000;
      +  }

      Without further explanation why this looks a bit puzzling...

    2. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
      @@ -152,8 +152,13 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
      -        'keys' => array('entity_view', $this->entityTypeId, $entity->id(), $view_mode),
      -        'granularity' => DRUPAL_CACHE_PER_ROLE,
      +        'keys' => array(
      +          'entity_view',
      +          $this->entityTypeId,
      +          $entity->id(),
      +          $view_mode,
      +          'cache_context.user.roles',
      +        ),

      I know language was not here but entity views would depend on language a great deal also.

    3. +++ b/core/modules/block/lib/Drupal/block/BlockBase.php
      @@ -108,11 +128,39 @@ public function buildConfigurationForm(array $form, array &$form_state) {
           $form['cache'] = array(
      -      '#type' => 'value',
      -      '#value' => $this->configuration['cache'],
      +      '#type' => 'details',
      +      '#title' => t('Cache settings'),
      +    );

      Based on the screenshot it was not evident if this details element is collapsed by default. I think the settings here looks pretty technical and scary for an innocent eye. At least I would move it to the bottom of the form but also probably just collapse it by default. I think we are confident in our defaults?

    Wim Leers’s picture

    Status:Needs work» Needs review
    StatusFileSize
    new106.42 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch blockcache-2158003-101.patch. Unable to apply patch. See the log in the details link for more information.
    [ View ]
    new1.87 KB

    #100: thanks for the review!

    1. Comment updated.
    2. The key for the entity's language is actually being added further down in the code:
            if ($entity instanceof TranslatableInterface && count($entity->getTranslationLanguages()) > 1) {
              $return['#cache']['keys'][] = $langcode;
            }

      As you can see, it's only added if the entity exists in >1 languages.

    3. By default, these settings would indeed be inside a collapsed <details> element, precisely for the reasons you cite. Of course, showing a screenshot of a closed <details> element isn't very helpful, so I opened them just for the screenshots :) We are indeed confident in our defaults.

    This reroll also fixes the 146 exceptions introduced by the previous patch, which apparently were caused by #type = checkboxes… WTF!

    Status:Needs review» Needs work

    The last submitted patch, 101: blockcache-2158003-101.patch, failed testing.

    Wim Leers’s picture

    Status:Needs work» Needs review
    StatusFileSize
    new106.53 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,395 pass(es), 2 fail(s), and 0 exception(s).
    [ View ]

    Chasing HEAD. #2134857: PHPUnit test the entity base classes broke several hunks of this patch.

    Status:Needs review» Needs work

    The last submitted patch, 103: blockcache-2158003-103.patch, failed testing.

    Wim Leers’s picture

    #2188565: Test coverage for Comment, Custom Block, Node, Taxonomy Term and User entity cache tags landed, so in the next reroll of this patch (which is blocked on #2215719: Cache tags set by #pre_render callbacks are lost in page cache) can also enable "cache forever" for custom blocks :)

    Wim Leers’s picture

    Status:Needs work» Needs review
    StatusFileSize
    new107.48 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,367 pass(es), 3 fail(s), and 0 exception(s).
    [ View ]
    new2.61 KB

    #2215719: Cache tags set by #pre_render callbacks are lost in page cache landed — now we can continue here! :)

    In the mean time, there have been quite significant changes to HEAD that require changes to this patch:

    Wim Leers’s picture

    Issue summary:View changes

    Status:Needs review» Needs work

    The last submitted patch, 106: blockcache-2158003-106.patch, failed testing.

    catch’s picture

    diff --git a/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.php b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.php
    index 657b635..94bea95 100644
    --- a/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.php
    +++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.php
    @@ -46,7 +46,6 @@ protected function defineOptions() {

         $options['block_description'] = array('default' => '', 'translatable' => TRUE);
         $options['block_category'] = array('default' => 'Lists (Views)', 'translatable' => TRUE);
    -    $options['block_caching'] = array('default' => DRUPAL_NO_CACHE);
         $options['block_hide_empty'] = array('default' => FALSE);

         $options['allow'] = array(
    @@ -131,13 +130,6 @@ public function optionsSummary(&$categories, &$options) {
           'value' => empty($filtered_allow) ? t('None') : t('Items per page'),
         );

    -    $types = $this->blockCachingModes();
    -    $options['block_caching'] = array(
    -      'category' => 'other',
    -      'title' => t('Block caching'),
    -      'value' => $types[$this->getCacheType()],
    -    );
    -

    Enabling block caching for Views blocks in 7.x allows for cache hits to avoid initialising the View object at all - this can save anything up to 100ms compared to using a views cache plugin on the same block. Feels like we'd still want a way for views to set #cache on its block plugins so they only have to initialize the view object itself on misses, otherwise that's quite an easy optimization on views-block-heavy sites gone.

    Wim Leers’s picture

    #109: I'm not sure what you're saying exactly. Block caching is now available for every block by default, including Views' blocks. That means we're just able to remove Views' specific support for it, since Views' blocks — like all others — get it for free :)
    So: yes, absolutely, we definitely still want a way for Views to set #cache. It's still configurable, and still in the same place, just with a UI that's consistent with all other blocks, and with more refined options.

    Wim Leers’s picture

    Status:Needs work» Needs review
    Issue tags:+Needs tests
    StatusFileSize
    new109.94 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,437 pass(es).
    [ View ]
    new5.42 KB

    Greenification.


    I've jotted down a bunch of additional test cases I want to add. I'll do that tomorrow. In the mean time, this should be good to review :)

    msonnabaum’s picture

    @catch - What wim said. You can now set block cache settings on the block config for the view, now that those view blocks show up in the block UI.

    catch’s picture

    I completely missed that the Views blocks now have cache settings in the blocks UI :( That makes complete sense though!

    Wim Leers’s picture

    Issue summary:View changes
    StatusFileSize
    new112.33 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,105 pass(es), 739 fail(s), and 8 exception(s).
    [ View ]
    new11.45 KB

    Five different block plugins are enabled in a default install of Drupal:

    1. SystemHelpBlock
    2. SystemMainContent — this is impossible to cache (at least in the foreseeable future)
    3. SystemMenuBlock — already cached forever since #62
    4. SearchBlockForm
    5. UserLoginBlock

    Three of those are very feasible to cache forever by default: the two form blocks (search + user login) are easily cacheable (they already are cached for anonymous users when page caching is enabled, which demonstrates they work just fine) and the help block (which is cacheable per URL).
    So now all four block plugins for which caching is feasible ship with caching enabled by default.

    This helps validating what we're doing in this issue, and should help simplify profiling.


    Tests coming next.

    catch’s picture

    This is only true if we disable $form['#cache'] and $form['#token'], and additionally assume that no custom module is going to AJAX-enable those forms per #1694574: drupal_process_form() deletes cached form + form_state despite still needed for later POSTs with enabled page caching. That might be fine to do for those blocks just pointing out. Also I doubt we have tests for those forms where one user visits the form then another submits which is where the CSRF protection will kick in/break if the HTML is cached.

    Wim Leers’s picture

    #115: I considered that too, but fact is that those forms already work with page caching enabled today. It's trivial for other modules that AJAX-enable those forms to disable render caching. In fact, that's one of the test cases I'm working on :)

    catch’s picture

    I considered that too, but fact is that those forms already work with page caching enabled today.

    That's because the form token is based on the session ID. Cached pages are only built for/served to users with no session, so the token is always the same.

    As soon as you get a session ID, the form token will be different, but you'll also not see anything from the page cache so it continues to work (mostly by accident).

    Wim Leers’s picture

    Issue summary:View changes
    StatusFileSize
    new111.66 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,379 pass(es), 352 fail(s), and 1 exception(s).
    [ View ]
    new799 bytes

    I see! So I'll remove the "cache by default" for SearchBlockForm. UserLoginBlock is fine because it only allows itself to be rendered for the anonymous user.

    The last submitted patch, 114: blockcache-2158003-114.patch, failed testing.

    Wim Leers’s picture

    Issue summary:View changes
    StatusFileSize
    new109.18 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,373 pass(es), 352 fail(s), and 1 exception(s).
    [ View ]
    new2.69 KB
    new7.49 KB

    From IRC:

    catch: WimLeers: so the login form won't work if anonymous user gets a session.
    WimLeers: catch: HAH!
    catch: WimLeers: I am trying to think whether it's OK to remove #token from these entirely.
    WimLeers: catch: That's what I was thinking/hoping but didn't want to bikeshed here
    catch: Possible drawbacks are flood control.
    catch: And for search maybe some kind of trackig like 'recent searches', that is more far-fetched.
    catch: What do you prefer? Spin off doing those things to a separate issue? Or do it right here?
    catch: WimLeers: I think separate issue, it is easy to break and easy to add subtle security issues either way.

    So I'll have to remove the "cache by default" for UserLoginBlock also.

    Follow-up created: #2221391: Cacheable form blocks: Remove #token from the search and user login forms.

    By now I've undone almost everything in #114 :( Attached is also interdiff-111.txt, which shows the interdiff relative to #111, to make things easier to track.

    The last submitted patch, 118: blockcache-2158003-118.patch, failed testing.

    Status:Needs review» Needs work

    The last submitted patch, 120: blockcache-2158003-120.patch, failed testing.

    Wim Leers’s picture

    Status:Needs work» Needs review
    Related issues:
    StatusFileSize
    new108.91 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,459 pass(es).
    [ View ]
    new1016 bytes

    Greenification. Also: I'm stupid.

    msonnabaum’s picture

    Issue tags:-Needs profiling
    StatusFileSize
    new34.49 KB

    Here's some quick profiling on this patch, using the front page as an authenticated user, with a few blocks added to the sidebar. The following blocks are either cached or uncached in each run:

    block:bartik_footer
    block:bartik_tools
    block:views_block__comments_recent_block_1
    block:views_block__content_recent_block_1

    Granted, this isn't that surprising, and not an entirely realistic scenario, but a useful reminder that block cache is still the most important place we can add caching.

    And although the improvement is significant, I was slightly disappointed that it wasn't more. I realized that much of this is due to the fact that the main content of the page is a view, and since it's SystemMainBlock, we can't use block caching there. That seems like something we should try to tackle after this since it's pretty limiting.

    Wim Leers’s picture

    Issue tags:-Needs tests
    StatusFileSize
    new126.54 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch blockcache-2158003-125.patch. Unable to apply patch. See the log in the details link for more information.
    [ View ]
    new21.27 KB

    And here's the promised extra test coverage!

    This reroll introduces BlockViewBuilderTest — the block-specific equivalent of EntityViewBuilderTest. It comes with extensive tests for all possible kinds of alters, which unveiled problems in the previous patches (that I was aware of, but also more subtle problems with the solution I had in mind).

    Note: BlockStorageUnitTest supposedly did CRUD tests, but it also did render tests. So I moved those over to BlockViewBuilderTest.

    Wim Leers’s picture

    Issue summary:View changes

    This is now ready for final reviews — all remaining tasks have been addressed.

    beejeebus’s picture

    spent some time looking through this patch - i like it! looks ready to go to me.

    jessebeach’s picture

    Draft change notice: https://drupal.org/node/2222293

    moshe weitzman’s picture

    Status:Needs review» Reviewed & tested by the community

    This solves the problem at hand regarding cache tags, and also embraces our improved render cache API by implementing CacheableInterface

    webchick’s picture

    Assigned:moshe weitzman» catch

    Zoooom!

    Status:Reviewed & tested by the community» Needs work

    The last submitted patch, 125: blockcache-2158003-125.patch, failed testing.

    Wim Leers’s picture

    Status:Needs work» Reviewed & tested by the community
    StatusFileSize
    new126.35 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,636 pass(es).
    [ View ]

    • Commit a6ffb28 on 8.x by catch:
      Issue #2158003 by Wim Leers, msonnabaum, effulgentsia, moshe weitzman:...
    Berdir’s picture

    Status:Reviewed & tested by the community» Fixed
    Issue tags:-sprint

    This has been committed by @catch. Published change record.

    superspring’s picture

    Status:Fixed» Needs work

    This patch causes an issue when adding fields to a content type.

    Drupal now throws an error when rendering the successfully created page. The field is still created.

    The error thrown is:
    Twig_Error_Runtime: An exception has been thrown during the rendering of a template ("SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'cid' at row 1: INSERT INTO {cache_block} (cid, serialized, created, expire, tags, checksum_invalidations, checksum_deletions, data) VALUES [...] in "core/themes/bartik/templates/page.html.twig" at line 173. in Twig_Template->displayWithErrorHandling() (line 291 of core/vendor/twig/twig/lib/Twig/Template.php).

    Berdir’s picture

    What is the cid that it is trying to insert? The field is limited to 255 characters, sounds like you end up with a very long cid. Not sure why tests don't catch this?

    The only thing that I can think of is to check the length and hash it when it's over the limit. Probably in a new issue, as it's not really the fault of this one but a more generic problem in the render cache API?

    tim.plunkett’s picture

    Status:Needs work» Fixed

    Regardless of the bug, this is fixed. If you can reproduce reliably, please open a new issue and link it here.

    Status:Fixed» Closed (fixed)

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

    znerol’s picture