Remove Block Cache API in favor of blocks returning #cache with cache tags

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.

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.

@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.

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

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.

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.

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

@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.

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?

#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.

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

At least *less* fails now :)

Now there should be only one fail.

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).
   

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.

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.

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))

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.

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.

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.

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.

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

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.

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

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.

Great work btw.!

Great work btw.!

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

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

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

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 :)

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.

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

@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).

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.

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.

+++ 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.

Addressed #63.

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

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.

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.

+++ 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?

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

Try that again.

+++ 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

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

Much better! :)

Making patch green again.

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.

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.

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.

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.

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:

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.

Hoping this fixes many of those failures…

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

#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

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.

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.

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.

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.

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?

#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!

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

#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 :)

#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:

• #2134857: PHPUnit test the entity base classes got reverted (for which we had to do a reroll in #103),
• #2188565: Test coverage for Comment, Custom Block, Node, Taxonomy Term and User entity cache tags got committed, which necessitates updating the tests added there and removed some tests patches so far were updating
• Another side effect of that patch is that we can now enable "cache forever" by default for custom blocks (as already said in #105)

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.

#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.

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 :)

@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.

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

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.

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.

#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 :)

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).

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.

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 user login form.

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.

Greenification. Also: I'm stupid.

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.

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.

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

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

Zoooom!

Chasing HEAD! #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall broke a few hunks.

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

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).

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?

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

To address #135: #2224861: Cache SystemMenuBlock and BookNavigationBlock per active trail (currently cached per URL, this breaks on very long URLs).

Follow-up: #2236167: Use request stack in cache contexts