Block access results' cacheability metadata is not applied to the render arrays

Here is a patch that would work, but that I am not happy with overall ...

I think better would be if #access would be set in the BlockViewBuilder() and Renderer supported an object for #access.

Lets see what testbot says ...

+++ b/core/modules/block/src/BlockRepository.php
+++ b/core/modules/block/src/BlockRepository.php
@@ -77,9 +77,8 @@ public function getVisibleBlocksPerRegion(array $contexts) {

getVisibleBlocksPerRegion() is now an inaccurate name, since we return all blocks per region and the caller is responsible for checking access.

This effectively reverts #2367579: Move retrieval of visible blocks to a standalone service, which @chx opened for what I can only assume is Mongo:

HEAD runs a config entity query on block entities and then it filters the results. All this is hardwired into the FullPageVariant. More efficient storages would need to replace FullPageVariant

The BlockRepository stuff was meant to significantly reduce the number of blocks that are even possible. A theoretical implementation could read the user from CurrentUserContext, get the roles for that user and run it against visibility.user_role and then remove that visibility rule from the result to avoid a slow PHP check. Of course I have no idea how that would work in actual code because I never actually got around to implement it nor do I know how it fits in the crazy caching world of Drupal 8.

So we can probably continue on with this, just renaming the method and being clear that it does NOT run access checking, and then page_manager/panels will have to update.

Seems then the only speedup possible will be querying per theme. It's way less useful but I will work with what I can (in theory. if someone picks up mongodb)

@chx: That already (and still with this patch) happens, just inside the patch context. It's a loadByProperties(), but that's just a shortcut for an entity query. An entity query that uses a config entity query index now, so it's really fast (one key value lookup to get the config entity ID's).

This should at least fix the fatal error on the tests

Looking at the test fails.

Uhm. I don't think we can do this without #2375695: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed because what it actually means is that block render caching is dead without having fixed the other issue first?

I've verified that at least the BlockCacheTest is green with the patch from #2375695-24: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed. So pretty sure we need that before we can continue here.

Postponing on #2375695: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed, which I had to make critical.

Berdir is right that issue is a hard blocker for this one.

No, it is not only about visibility conditions.

*every* block is set to max-age 0 even if it doesn't have visibility conditions. As soon as we respect that, then blocks are never cached, because max-age prevents that.

See the mentioned BlockCacheTest test. That is very simple, it just tests if a block is cached at all or not. And with this change, the block is no longer cached.

There is no security issue without smartcache because we don't cache the access results.

Combined patch, want to see how many test fails are left then.

Fixing those test fails to better understand what is needed for test coverage. Quite some changes necessary in unit tests because the place where access checks happen moved. Also again annoyed at static methods calling service methods :)

We could possibly extend them for more unit test coverage but this kind of proves already that we are calling those methods.

PageCacheIntegrationTest shows again that we should filter out the single block cache tags when we have the list cache tag around as well. Would save a ton of cache tags that we need to check in the page cache.

Also fixed an annoying and confusing bug in AssertPageCacheContextsAndTagsTrait, the Unwanted/Missing labels are inverted ;)

It's disheartening to see this much effort duplication from #2377757: Expose Block Context mapping in the UI. Whatever the case, it's probably worth looking at in light of this issue. Most of this patch seemed pretty sensible to me and would further simplify what needs doing in 2377757. ++ from that perspective.

Eclipse

+++ b/core/modules/block/src/BlockRepository.php
@@ -77,9 +77,8 @@ public function getVisibleBlocksPerRegion(array $contexts) {
-      if ($block->setContexts($contexts)->access('view')) {
-        $full[$block->getRegion()][$block_id] = $block;
-      }
+      $block->setContexts($contexts);
+      $full[$block->getRegion()][$block_id] = $block;

+++ b/core/modules/block/src/Plugin/DisplayVariant/BlockPageVariant.php
@@ -133,6 +134,15 @@ public function build() {
+        if (!$access->isAllowed()) {
+          $build[$region][$key] = [];
+          CacheableMetadata::createFromObject($access)
+            ->applyTo($build[$region][$key]);
+
+          continue;

These two things together conspire to output render array data that will result in regions rendering when there's nothing in them to display. This is functionally #953034: [meta] Themes improperly check renderable arrays when determining visibility which is why that access check before adding it to the render array is there in the first place. The problem in 953034 persists regardless because even simply adding #access => FALSE into the render arrays somewhere will cause this problem. Still those checks mitigate the problem manifesting with blocks which is the lion share of what most people are likely to see.

Must we apply the cacheable metadata to empty render array elements? If so, we're going to have to rework region rendering in some way.

Eclipse

#45: We have to do that anyway. With placeholders the guarantees that a region is empty or not are not very strong anyway.

What if your block is *SI'd in, etc.

There is a Element::isEmpty() in the works however that could be used for now partially at least.

--

Downgrading to major and merging with the other issue for now per berdir's comment.

@EclipseGc: I'm confused, I don't think it overlaps with that? The combined patch is this on top of #2375695: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed, if anything then that issue overlaps with yours?

And about those two lines, yes that's why the last test there is failing. I don't think we have to do add it inside an otherwise empty block element, we can also add it higher up, but so far, that didn't solve the test fail.

As discussed, will move the merged patch over to #2375695: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed

I'm pretty sure this isn't postponed but duplicate of #2375695: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed. Everything I did here is part of the other issue. We do need a follow-up issue to try and get rid of the max-age 0 in case of missing contexts but that's a different isuse.

As long as that other issue has test coverage, that should be fine.