Block access is checked even if the block's region is never rendered

I just noticed that BlockRepository::getVisibleBlocksPerRegion() checks the access on every block from every region and then only keeps blocks from regions returned by ActiveTheme::getRegions(), so some of these access checks are called for nothing.

I opened a MR that makes it check only blocks from regions returned by ActiveTheme::getRegions().
This might fix 🐛 Block access is checked even if block is inside a group that is not displayed Active but not the issue with theme regions that are not rendered.

Added some unit tests to show the problem.

The issue summary says:

> Don't render this region in page.html.twig.

But then the implementation uses $activeTheme->getRegions(). That's not the same thing. getRegions() isn't dynamic. I'm not clear how it would even be possible to have a block defined for a region that's not in there?

@prudloff mind updating the summary some please.

I originally opened the issue because we noticed that access to some blocks was checked even if they were in a region that's never really displayed. (In our case regions created by the blockgroup module.)
My first approach was to try to check access only if the region is rendered.

Then I noticed in #2 that regions created by blockgroup were not return by $active_theme->getRegions().
So I thought that would be a simpler way to exclude them.

But I just checked again and I think I was misled because when you create a new region in blockgroup, at first it appears on /admin/structure/block but not in $active_theme->getRegions().
But if you clear caches, it starts appearing in $active_theme->getRegions().
So my solution is not useful.

prudloff → changed the visibility of the branch 3374446-block-access-is to hidden.

Thank you for taking a look! Still an issue though?

Yes, the issue summary is still correct.

It may be correct, but I'm not sure it's fixable. We can't know if there's going to be some dynamic preprocess/twig logic deciding not to print regions. There is no concept for regions being dynamic. And checking access likely still faster than preparing the block that then won't be used, especially with all the stuff around placeholdering and so on.

There are open issues around improving block access caching, might make more sense to focus on that and start caching accessible/non-accessible information if it's well cacheable.

I agree this would hard to fix without changing the way regions work.

Our main concern is this scenario :
- We have a region that is not displayed on every page.
- This region contains a block and the block's access method returns an access result with a low max age.
- Every page inherits this low max age even if the region is not displayed.

Our current workaround is to add a condition on the block itself that duplicates the logic that displays the region. (I think block access methods are not called if their condition is false.)
But this can be tedious if the region contains a lot of blocks.

Should we close this as wontfix since we don't really have a solution with the current region system?

Did you consider doing the thing that depends on age/time while building the block instead of access? so you allow access, but you return an empty array, that should result in displaying nothing, see \Drupal\block\BlockViewBuilder::preRender.

And you could further combine that into forcing a lazy builder, see 📌 Create placeholders for more things Active and combined with recent lazy builder/render cache optimizations, that should then be pretty efficiently cache your block separately from dynamic page cache. We have auto-placeholder cache contexts, we could possibly also have auto-placeholder max-age, so if things are for example cacheable for less than 600s, we automatically placeholder them if possible.

But yes, I'd say this is a won't fix, see #2479459: BlockRepository::getVisibleBlocksPerRegion() does an uncached entity query on every request → for a related discussion although that one is more about the entity query than access.

Did you consider doing the thing that depends on age/time while building the block instead of access?

Yes, we do that for some custom blocks but it is not always possible for more complex contrib blocks, for example 🐛 Facet block adds view max age on every page Active

Marking as wontfix then.