Problem/Motivation

Repeatable: Always
Steps to repeat:
1. Enable Book and Layout Builder modules
2. Enable Layout Builder for Book content type
3. Manage Layout for Book and add Book navigation block to any available section
4. Check 'Display title', select 'Show block only on book pages', add block and save layout
5. Add new content Book page without adding any book (Book outline -> Book should be -None-) and save
6. View a new created page
patch in #8 works for me.
Thank you !
Expected:
Book navigation block should not be on the page.

Actual:
Book navigation block is on the page.

Proposed resolution

Handle empty block plugins the same way BlockViewBuilder does

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#16 3007439-emptybuild-16.patch8.28 KBWim Leers
#16 interdiff.txt1.88 KBWim Leers
#14 3007439-emptybuild-14.patch8.2 KBtim.plunkett
#10 3007439-emptybuild-10.patch8.26 KBWim Leers
#10 interdiff.txt974 bytesWim Leers
#8 3007439-emptybuild-8-interdiff.txt7.25 KBtim.plunkett
#8 3007439-emptybuild-8.patch8.43 KBtim.plunkett
#5 3007439-emptybuild-4-PASS.patch3.92 KBtim.plunkett
#2 3007439-emptybuild-2-PASS.patch3.92 KBtim.plunkett
#2 3007439-emptybuild-2-FAIL.patch2.05 KBtim.plunkett
Book_settings.png190.43 KBxopoc
Book_display.png90.14 KBxopoc
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xopoc created an issue. See original summary.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Version: 8.6.1 » 8.7.x-dev
Issue summary: View changes
Status: Active » Needs review
Issue tags: +Blocks-Layouts
FileSize
3007439-emptybuild-2-FAIL.patch2.05 KB
3007439-emptybuild-2-PASS.patch3.92 KB

Thanks for the bug report!

Somehow we missed the simplest case when working on BlockComponentRenderArrayTest:
What if the ::build() method returns an empty array?

Status: Needs review » Needs work

The last submitted patch, 2: 3007439-emptybuild-2-PASS.patch, failed testing. View results

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs work » Needs review

It tested against 8.6 by mistake

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
FileSize
3007439-emptybuild-4-PASS.patch3.92 KB

The last submitted patch, 2: 3007439-emptybuild-2-FAIL.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 5: 3007439-emptybuild-4-PASS.patch, failed testing. View results

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs work » Needs review
FileSize
3007439-emptybuild-8.patch8.43 KB
3007439-emptybuild-8-interdiff.txt7.25 KB
xopoc’s picture

xopoc CreditAttribution: xopoc at Princeton University commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

patch in #8 works for me.
Thank you !

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
interdiff.txt974 bytes
3007439-emptybuild-10.patch8.26 KB 
  1. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
@@ -90,6 +90,14 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
+      $is_placeholder_ready = $event->inPreview() && $block instanceof PlaceholderInterface;
...
+

@@ -98,9 +106,9 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
         '#weight' => $event->getComponent()->getWeight(),

    TIL \Drupal\Core\Render\PlaceholderInterface is a thing! :O

    Introduced in #2992410: Provide placeholders for empty blocks (for example, an empty Views listing) apparently.

    This "placeholder" concept does not match the one in \Drupal\Core\Render\Placeholder\PlaceholderStrategyInterface … that's … very confusing :( I'm very sorry to raise this only now, but I hadn't seen that issue yet, and there was no CR for it. I'll leave a comment there. 

  2. +++ b/core/modules/layout_builder/tests/src/Unit/BlockComponentRenderArrayTest.php
@@ -237,6 +238,58 @@ public function testOnBuildRenderInPreview($refinable_dependent_access) {
+    $expected_cache = $expected_build + [
+        '#cache' => [
+          'contexts' => [],
+          'tags' => ['test'],
+          'max-age' => 0,
+        ],
+      ];

    Indentation nit. Fixed.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 3007439-emptybuild-10.patch, failed testing. View results

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community 
00:44:24.199 Error retrieving a new session from the selenium server

Retesting…

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 3007439-emptybuild-10.patch, failed testing. View results

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs work » Needs review
FileSize
3007439-emptybuild-14.patch8.2 KB

Reroll after #3013187: Rename PlaceholderInterface to PreviewFallbackInterface
(that was almost a month ago, how often does RTBC retesting happen these days?!)

Status: Needs review » Needs work

The last submitted patch, 14: 3007439-emptybuild-14.patch, failed testing. View results

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community
FileSize
interdiff.txt1.88 KB
3007439-emptybuild-16.patch8.28 KB

It was late in Tim's timezone. 🤗

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 8bf6ee3870 to 8.7.x and 79b59157c5 to 8.6.x. Thanks!

  • alexpott committed 8bf6ee3 on 8.7.x 
    Issue #3007439 by tim.plunkett, Wim Leers, xopoc: Layout builder renders...

  • alexpott committed 79b5915 on 8.6.x 
    Issue #3007439 by tim.plunkett, Wim Leers, xopoc: Layout builder renders...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

I ran all the changed tests on 8.6.x before backporting. Looks good.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented

I think this may have introduced a bug: #3088198: Layout builder discards cacheability metadata from blocks when they are rendered without content. If any of the followers here can confirm it, that would be great.