BlockContentBlock ignores cache contexts required by the block_content entity

BlockContentBlock belongs to block_content module.

is this in fact active? or postponed on #2099137: Entity/field access and node grants not taken into account with core cache contexts
if it is active, I should be able to look at it on the weekend

Are we sure this is still an issue?
Following steps to reproduce in IS I did the following:

Created a block content block
Placed it in sidebar
Viewed the homepage
Selected distinct cid from cache_render

Here's what I get

The entity_view:block:block_id (fooey in this case) has both timezone and role in it.

Screenshot:

Doh, I had my patch applied - which is attached.

So just needs a test here.

Here's a failing test

I think this covers #5. We should update proposed resolution in IS with current tasks.

Updated IS

Quick review.

1. +++ b/core/modules/block_content/src/BlockContentViewBuilder.php
   @@ -60,4 +62,33 @@ protected function alterBuild(array &$build, EntityInterface $entity, EntityView
   +   * @return array
   
   +++ b/core/modules/block_content/src/Plugin/Block/BlockContentBlock.php
   @@ -37,6 +37,13 @@ class BlockContentBlock extends BlockBase implements ContainerFactoryPluginInter
   +   * @var array.
   
   @@ -153,4 +160,30 @@ public function build() {
   +   * @return array
   

   string[]

2. +++ b/core/modules/block_content/src/Plugin/Block/BlockContentBlock.php
   @@ -37,6 +37,13 @@ class BlockContentBlock extends BlockBase implements ContainerFactoryPluginInter
   +   * Cache contexts. Array with 'tags' and 'keys' keys.
   

   If it's an array with those keys, it's not just cache contexts. I think this comment is wrong?

3. +++ b/core/modules/block_content/src/Plugin/Block/BlockContentBlock.php
   @@ -153,4 +160,30 @@ public function build() {
   +    return parent::getRequiredCacheContexts() + $this->cacheContext();
   

   Must be array_merge(), since keys are numeric.

4. +++ b/core/modules/block_content/src/Plugin/Block/BlockContentBlock.php
   @@ -153,4 +160,30 @@ public function build() {
   +   * Generate the cache context for the block content entity.
   ...
   +  protected function cacheContext() {
   ...
   +    return $this->blockCacheContext;
   

   Must all be plural.

Thanks @WimLeers, all great catches, you can see I changed my mind over time, but didn't go back to rectify.

#14

doh

1. +++ b/core/modules/block_content/src/BlockContentViewBuilder.php
   @@ -60,4 +62,33 @@ protected function alterBuild(array &$build, EntityInterface $entity, EntityView
   +  public function getCacheContexts(EntityInterface $entity, $view_mode, $langcode) {
   

   Should have an @see to \Drupal\Core\Entity\EntityViewBuilder::getBuildDefaults().

   Better yet, I'd argue that the default cache contexts embedded in that method should actually be moved into a \Drupal\Core\Entity\EntityViewBuilder::getCacheContexts() method, so that each entity type's builder can easily override it (that's not an API change!), so that BlockContentBlock can then call that.

   Then there is no duplication of logic (and duplication of the list of cache contexts to use), which always comes with that unavoidable brittleness.

   You can then still create a public getBlockCacheContexts(), which would just return whatever the (inherited) protected getCacheContexts() returns, to allow BlockContentBlock to use it.

2. +++ b/core/modules/block_content/src/Tests/BlockContentFieldTest.php
   @@ -103,6 +109,19 @@ public function testBlockFields() {
   +    $cache_records = db_select('cache_render', 'cr')
   +      ->fields('cr')
   +      ->condition('cid', db_like('entity_view:block:' . $instance['id']) . '%', 'LIKE')
   +      ->execute();
   +    $found_records = FALSE;
   +
   +    foreach ($cache_records as $record) {
   +      $this->assertTrue(strpos($record->cid, 'r.authenticated') !== FALSE, 'Found cache key for role');
   +      $this->assertTrue(strpos($record->cid, date_default_timezone_get()) !== FALSE, 'Found cache key for timezone');
   +      $found_records = TRUE;
   +    }
   +    $this->assertTrue($found_records, 'Found cache records');
   

   All of this can be simplified: it can be turned into a check that checks whether a render cache item with the expected cache ID exists. See \Drupal\system\Tests\Entity\EntityCacheTagsTestBase::verifyRenderCache() and its callers for examples.

fixes warnings, not started on #19 yet

19.1 I think that is what #2099137: Entity/field access and node grants not taken into account with core cache contexts is for - @effulgentsia said we could move ahead here provided the solution was compatible with the approach there. I can tackle it here instead - please advise?

19.2 - done

passes locally, so figure trying something different

@WimLeers or @effulgentsia can you advise if I should be tackling 19.1 here or in the related issue

larowlan--

right, so I'm out of ideas - this passes locally

Sorry, just doing some house-cleaning. Since D8 cacheability is a subset of Performance, adding the more common tag as well.

@larowlan this passes locally for you because you're in Australia/Sydney, but I'm in America/Los_Angeles, and the testbots probably are too.

Thanks Tim
/me feels like a nuffy for not picking that up

Urban Dictionary: nuffy
www.urbandictionary.com/define.php?term=nuffy
If there was a bright center of the universe where intelligence, grace and success dwelled, Nuffy would be the individual that would be farthest from.

Thanks for the vocabulary lesson. :D

We're now solving this generically at #2429257: Bubble cache contexts.

Let's revisit this once that has landed, just to make sure.

#2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance landed, this is now unblocked.

Wanted to do a straight reroll of the patch, but just remove all the hunks that fixed the problem, to see if #2429257: Bubble cache contexts on its own fixed this.

But #2428633: Remove unnecessary BlockContentFieldTest removed BlockContentFieldTest. Not sure what the new best place for a test is.

I believe larowlan wants to take a shot at this.

I'd just bring the test back, possibly as a simplified and renamed version, we don't need the configurable field stuff and so on anymore I think.

yep that's my plan, hoping to work on this in transit to DrupalSouth (in 7 hrs or so)

I don't think you wanted to link to #2284917: In-place editing of custom blocks broken *again* (because attributes and contextual links of custom blocks are lost)?

But, EntityViewBuilder::getBuildDefaults() only adds contexts if isRenderCacheable() is true, which seems wrong, since even if it's not cacheable on its own, we still need the contexts to bubble up.

I noticed that too.

To clarify, that's because we split up #cache[keys] into #cache[keys] + #cache[contexts], but then failed to move #cache[contexts] to the place where #cache[tags] is set, which runs unconditionally. We simply split it up and kept it in the same branch of code.
That's how this happened.

Ah, that makes sense :)

so started on test on plane, but getting "cache_context.timezone" is not a valid cache context ID." exception

+++ b/core/modules/block_content/src/BlockContentViewBuilder.php
@@ -60,4 +62,33 @@ protected function alterBuild(array &$build, EntityInterface $entity, EntityView
+      'cache_context.timezone',

it should be just 'timezone'. Ran into the same problem earlier.

Right but that's in HEAD?

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -169,6 +169,7 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
+        'contexts' => ['theme', 'user.roles'],

Let's put these on separate lines.

Fixed that, and fixed the test failures.

Thanks for the extensive test coverage.

The problem indeed was the conditional vs. non-conditional execution of the code path.

This is RTBC.

It would be great to get an IS update so that it matches the patch.

Thanks, effulgentsia! IS looks great now!

Great how cache context bubbling makes an issue like this straightforward.

Committed/pushed to 8.0.x, thanks!

Thanks all