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.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | 3013125-label_display-for-d8-6.patch | 1.25 KB | bkosborne |
| #13 | 3013125-label_display-12.patch | 1.23 KB | abu-zakham |
| #6 | 3013125-label_display-5-interdiff.txt | 2.38 KB | tim.plunkett |
| #6 | 3013125-label_display-5-PASS.patch | 4.82 KB | tim.plunkett |
| #6 | 3013125-label_display-5-FAIL.patch | 1.48 KB | tim.plunkett |
Comments
Comment #2
tim.plunkettWell spotted! Thankfully we can override this in our existing LayoutSectionItemList
Test included.
Comment #3
tim.plunkettNitpicked my fix a bit :)
Comment #5
johnwebdev commentedShould we test this method separately as well?
This could throw an exception; which is not documented /{@inheritdoc}/ is that something we need to consider?
Comment #6
tim.plunkettFair points, addressed both.
Comment #8
tim.plunkettAdding credit for the great steps to reproduce, and the review.
Comment #9
johnwebdev commentedComment #10
catchCommitted 51ce8b8 and pushed to 8.7.x. Thanks!
Comment #13
abu-zakham commentedPatch for 8.5.8
Comment #14
johnwebdev commentedComment #15
bkosborneHere's the patch for 8.6.x