We've run into a bit of a weird problem and I'm not really sure what the best way to solve it is.

What are the steps required to reproduce the bug?

1. Set up a content type with an overrideable layout and create an instance of that content type - this doesn't happen with config entity layouts, just content ones
2. Edit your content's layout
3. Add a generic site-level block (e.g. the help block) with "Display title" checked
4. Save your layout
5. Edit your layout again
6. Edit the block you added
7. Untick "Display title"
8. Update the block
9. Save the layout
10. View the content or edit the layout and block again - you'll see that "Display title" is still ticked

What behavior were you expecting?

The title should no longer be displayed/"Display title" should no longer be ticked.

What happened instead?

The title stayed visible and "Display title" was ticked again after saving and reloading.

Why is this happening?

This comes down to PHP evaluating non-numeric strings as being equal to zero. When the section component block configuration is saved for content it's saved as a field, and therefore goes through Drupal\Core\Field\FieldItemList::equals(). Then when this happens at the end of that method:

return $value1 == $value2;

and PHP checks the array it ends up comparing, e.g.:

// Evaluates to zero.
$value1[0]['section']->getComponents()['e420722e-929f-4818-b4b9-318864a9bf3d']->toArray()['configuration']['label_display']
// Evaluates to 'visible' which - as a non-numeric string - also evaluates to zero.
$value2[0]['section']->getComponents()['e420722e-929f-4818-b4b9-318864a9bf3d']->toArray()['configuration']['label_display']

(not literal code, this is just how I checked it while debugging).

If something else has been changed this doesn't matter since it's all one big field. So the whole thing gets updated. However, if the *only* thing that changed was that "Display title" was unticked then all parts of the field are evaluated as equal so Drupal doesn't save the field.

Now what?

So we know what's wrong, but I'm not really sure where to go from here. Do we:

  • Override/rewrite FieldItemList::equals? Seems like a bit of an extreme solution, though it might catch other issues like this.
  • Add some validation on the section component save to catch this specific issue? Seems a little hacky and means the block configuration ends up being saved in a slightly different way than the main block system ('invisible' vs zero/false I guess). Could cause who knows what problems.
  • Alter the field before it hits FieldItemList::equals() somehow *without* changing what's saved? I suppose I'd like to do this but I'm not sure there's a simple way to do so.

Comments

Dylan Donkersgoed created an issue. See original summary.

tim.plunkett’s picture

Status: Active » Needs review
Issue tags: +Blocks-Layouts
StatusFileSize
new2.15 KB
new3.29 KB

Well spotted! Thankfully we can override this in our existing LayoutSectionItemList

Test included.

tim.plunkett’s picture

Nitpicked my fix a bit :)

The last submitted patch, 2: 3013125-label_display-2-FAIL.patch, failed testing. View results

johnwebdev’s picture

  1. +++ b/core/modules/layout_builder/src/Field/LayoutSectionItemList.php
    @@ -55,4 +57,18 @@ public function getEntity() {
    +  public function equals(FieldItemListInterface $list_to_compare) {
    

    Should we test this method separately as well?

  2. +++ b/core/modules/layout_builder/src/Field/LayoutSectionItemList.php
    @@ -55,4 +57,18 @@ public function getEntity() {
    +      assert($list instanceof LayoutSectionItemList);
    

    This could throw an exception; which is not documented /{@inheritdoc}/ is that something we need to consider?

tim.plunkett’s picture

Fair points, addressed both.

The last submitted patch, 6: 3013125-label_display-5-FAIL.patch, failed testing. View results

tim.plunkett’s picture

Adding credit for the great steps to reproduce, and the review.

johnwebdev’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 51ce8b8 and pushed to 8.7.x. Thanks!

  • catch committed d553a65 on 8.7.x
    Issue #3013125 by tim.plunkett, johndevman, Dylan Donkersgoed: Content...

Status: Fixed » Closed (fixed)

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

abu-zakham’s picture

StatusFileSize
new1.23 KB

Patch for 8.5.8

johnwebdev’s picture

bkosborne’s picture

StatusFileSize
new1.25 KB

Here's the patch for 8.6.x