Problem/Motivation
In \Drupal\block\BlockViewBuilder::preRender(), the block entity gets removed from the render array and replaced with a copy that doesn't depend on the block config entity.
If the block content is empty, then the cacheability metadata gets copied correctly using the \Drupal\Core\Cache\CacheableMetadata::merge() function. However, if the block content is not empty, the cacheability metadata isn't copied at all.
I noticed this, with @dawehner's help and guidance, after hours of debugging a view's display of type block that was missing a 'url' cache context, causing an empty block to be cached in the render cache if another page where one of the view's arguments could not be determined from the URL and resulted in an empty block.
Proposed resolution
Move the code that properly merges the cacheability metadata from the codepath where the block's content is empty to the main codepath for \Drupal\block\BlockViewBuilder::preRender()
Remaining tasks
Write a patch- Find exact steps to reproduce.
- Add test(s) to patch.
- Review, feedback.
- RTBC, commit.
User interface changes
None.
API changes
I honestly don't know enough about Drupal 8's render API to know if there are any, but I think no (?)
Data model changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | block_view_cachability-2632828-17.patch | 1.31 KB | mrinalini9 |
| #8 | block_view_cachability-2632828-8.patch | 1.3 KB | lammensj |
| #2 | block_view_cachability-2632828-2.patch | 1.63 KB | mparker17 |
Comments
Comment #2
mparker17Patch attached. Reviews welcome.
I (or someone else) should add steps to reproduce and tests, but I'm trying to track down why this block isn't displaying for a client site and thus might not get around to doing these things for a while.
Comment #4
Gravypower commentedI can confirm this bug when using the easy breadcrumbs module. Applied to 8.1.3 and it fixes my issue.
Comment #5
andypostLooks that just needs tests
Comment #8
lammensj commentedSomething changed in the BlockViewBuilder in Drupal 8.3, therefor the patch was not applicable anymore.
Comment #11
borisson_We should probably add tests for this, so we can be sure that the problem is resolved with this patch.
Comment #13
lukasss commentedComment #16
mrinalini9 commentedComment #17
mrinalini9 commentedRerolled patch for 9.1.x.
Comment #22
smustgrave commentedSending back to needs work for the tests.
Also needs steps for how to test. Probably should be added first to help whoever does the tests.