Problem/Motivation

I applied the patch in #2952967: When allowing content items to have customized layouts, clicking 'Layout' on full view of a node takes you to the configuration for the 'default' view mode to my 8.5.0 project. Even with the patch applied, I was not able to view customized layouts on a per node basis.

Steps to reproduce:
- Enable the "full content" view mode for basic page
- Check "Allow each content item to have its layout customized."
- Create a new basic page
- Click "Layout" and add a block to the layout (I used the powered by Drupal block)
- Save the layout and view the node

I expected to see the customized layout with the new block added, but only the default layout appeared.

It appears that the allow_custom setting applies on a per-view mode basis. The third party settings config object for the view mode's layout_builder does not include that flag for any view mode configuration other than the default. LayoutBuilderEntityViewDisplay checks this value before it decides whether to return the section configuration from the view mode config or the customized layout stored with the node.

Proposed resolution

I updated the form method in LayoutBuilderEntityViewDisplayForm to include the layout options form elements for both "default" and "full" view modes and updated my view mode configuration. The customized layout was then used when viewing the node.

I am not sure this is the best solution as it appears that the layout stored with the node is not associated with a particular view mode so the updates would show for any view mode which can be customized. I think this can cause unexpected behavior as it is.

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pixlkat created an issue. See original summary.

pixlkat’s picture

Issue summary: View changes
tim.plunkett’s picture

Issue tags: +Needs tests

Thanks for the super detailed bug report!

Ughhhhhhh I hate hate hate the default/full split. So many strange interactions.

tim.plunkett’s picture

If you enable overrides on default before you enable the Full view mode, everything worked fine. Which is why our test didn't catch it.

This should better mimic the situation where you have the Full view mode enabled before you enable overrides.

The last submitted patch, 4: 2955065-layout-4-FAIL.patch, failed testing. View results

andypost’s picture

+++ b/core/modules/layout_builder/src/Form/LayoutBuilderEntityViewDisplayForm.php
@@ -89,6 +89,35 @@ public function form(array $form, FormStateInterface $form_state) {
+      $query = $this->entityTypeManager->getStorage($this->entity->getEntityTypeId())->getQuery()
+        ->condition('status', TRUE)
+        ->condition('mode', $canonical_mode);

Maybe instead of uncached query use entity repository? Like \Drupal\Core\Entity\EntityDisplayRepositoryInterface::getViewModeOptionsByBundle()

tim.plunkett’s picture

Except we don't need to load the entities at all, just checking for their existence.

tim.plunkett’s picture

Issue tags: +sprint
mtodor’s picture

Nice work with provided steps to reproduce and patch.

I have tested this patch a bit, what is strange for me is when you enable "Allow each content item to have its layout customized." for default view and then you enable also Full display -> option is suddenly on Full display tab. That's strange and it could lead to some problems and bad UX.

What about keeping that option on Default display? Would that be possible?

Some strange things that occurred to me:

  1. Enable "Allow each content item to have its layout customized." and "Full content" at same time on Default view -> Save -> go on Full content tab "Allow each content item to have its layout customized." is not enabled.
  2. Enable "Allow each content item to have its layout customized." on Default view -> Save -> Enable "Full content" -> Save -> go on Full content tab "Allow each content item to have its layout customized." is enabled.

And one additional with following steps to reproduce (I tested on default Article display):

  1. on Default view -> enable "Allow each content item to have its layout customized." -> Save
  2. on Default view -> enable "Full content" -> Save
  3. on "Full content" view tab -> disable "Allow each content item to have its layout customized." -> Save
  4. on Default view -> disable "Full content" -> Save
  5. => Exception will thrown
    The website encountered an unexpected error. Please try again later.Drupal\Core\Entity\Query\QueryException: 'layout_builder__layout' not found in Drupal\Core\Entity\Query\Sql\Tables->ensureEntityTable() (line 343 of core/lib/Drupal/Core/Entity/Query/Sql/Tables.php).
    Drupal\Core\Entity\Query\Sql\Tables->addField('layout_builder__layout', 'INNER', NULL) (Line: 44)
    Drupal\Core\Entity\Query\Sql\Condition->compile(Object) (Line: 163)
    Drupal\Core\Entity\Query\Sql\Query->compile() (Line: 74)
    Drupal\Core\Entity\Query\Sql\Query->execute() (Line: 141)
    Drupal\layout_builder\Form\LayoutBuilderEntityViewDisplayForm->hasOverrides(Object) (Line: 81)
    Drupal\layout_builder\Form\LayoutBuilderEntityViewDisplayForm->form(Array, Object) (Line: 117)
    Drupal\Core\Entity\EntityForm->buildForm(Array, Object) (Line: 40)
    Drupal\layout_builder\Form\LayoutBuilderEntityViewDisplayForm->buildForm(Array, Object, Object)
    ...
    
tim.plunkett’s picture

Issue tags: -sprint

Removing from sprint tag as I'm not actively working on this. Please feel free to retag if someone picks this up!

tim.plunkett’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
phenaproxima’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll +Blocks-Layouts
FileSize
6.3 KB

Rerolled.

phenaproxima’s picture

Title: Customized layout does not display for node with customizations enabled when using full content view mode » [PP-1] Customized layout does not display for node with customizations enabled when using full content view mode
Status: Needs review » Postponed
Related issues: +#2936358: Layout Builder should be opt-in per display (entity type/bundle/view mode)

This intersects with the work being done in #2936358: Layout Builder should be opt-in per display (entity type/bundle/view mode), so I'm postponing on that.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

phenaproxima’s picture

Status: Postponed » Needs work

Blocker is in.

phenaproxima’s picture

Title: [PP-1] Customized layout does not display for node with customizations enabled when using full content view mode » Customized layout does not display for node with customizations enabled when using full content view mode
phenaproxima’s picture

Status: Needs work » Needs review
FileSize
6.49 KB

Rerolled again.

Status: Needs review » Needs work

The last submitted patch, 17: 2955065-17.patch, failed testing. View results

dmsmidt’s picture

Priority: Normal » Major

Since #2936358: Layout Builder should be opt-in per display (entity type/bundle/view mode) got in, this issue became more relevant.
It is now possible to NOT enable Layout Builder on the Default view mode and DO enable it on Full view mode.
But in this case it is impossible to enable customization per content item.

Since LayoutBuilder is now set up per view mode, it makes sense to repeat this setting aswell across all view modes.
Although there is no UI to change, for example a teaser view mode for a certain item (afaik), this could be a use case in the future.

tim.plunkett’s picture

Agreed with the major priority.
#2907413: Consider supporting Layout Builder Overrides for other view modes is the issue to expand this to all view modes.

yareckon’s picture

Just ran into this while test driving 8.6 for the first time. A less experienced drupaller wouldn't have had the tickle at the back of their mind that said "I wonder if this is a full / default bug?". And it was...

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
6.55 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch, 22: 2955065-layout-22.patch, failed testing. View results

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
6.54 KB

Rerolled again!

Status: Needs review » Needs work

The last submitted patch, 25: 2955065-25.patch, failed testing. View results

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
10.16 KB
6.6 KB

Fixed another bug and added a lot more test coverage

tim.plunkett’s picture

Let's future-proof this one addition. Added a comment.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

I don't really have any feedback here. The change is very tightly scoped and makes sense, and the test coverage is super extensive. I feel good about this patch, overall.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: 2955065-layout-28.patch, failed testing. View results

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community

Random media fails :\

alexpott’s picture

Version: 8.8.x-dev » 8.7.x-dev
Status: Reviewed & tested by the community » Fixed

Credited @pixlkat for opening the issue and @andypost, @dmsmidt and @mtodor for contributing to the discussion and patch review.

Committed and pushed 518e8fff0c to 8.8.x and 1d0a02ed7d to 8.7.x. Thanks!

Backported to 8.7.x as a major bugfix + it is great to have layout builder with less bugs in its first stable release - great work.

I ran the tests locally to ensure we had test coverage of the else added in #27 and we do!

  • alexpott committed 518e8ff on 8.8.x
    Issue #2955065 by tim.plunkett, phenaproxima, pixlkat, andypost, mtodor...

  • alexpott committed 1d0a02e on 8.7.x
    Issue #2955065 by tim.plunkett, phenaproxima, pixlkat, andypost, mtodor...

Status: Fixed » Closed (fixed)

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

bkosborne’s picture

With this change, I wondered what would happen to default entity view displays that had layout builder enabled with overrides, since this change prevents you from enabling overrides on default displays.

I performed this test:

Drupal 8.6.15:

  • Content type with LB enabled with overrides on default entity view display. "Full content" entity view display not configured/added at all.
  • Added a node and created a layout override, adding a custom inline block.

Upgraded to 8.7.0:

  • When viewing the entity view display config for the content type, observed that the "overrides" checkbox is no longer there at all.
  • Viewed the individual node, the overwritten display is still loaded, and the custom inline block is displayed.
  • I enabled the entity view display for "Full content". I left the LB checkboxes unchecked.
  • Viewed the node again, and observed that the node is no longer displaying the override. The inline block is not shown.
  • Go back to entity view settings for "Full content" and enable LB with overrides.
  • View node once more, and observe that the node is once again displaying the override and the inline block is shown.

So, for anyone, like me, that has been using overrides with the "Default" entity view displays, nothing seems to break, but this behavior is a little funky. To get things back to the way they "should be", I will follow these steps on any entity that has overrides enabled for default entity view display:

  1. Enable the "Full" entity view display, and enable LB on it with overrides.
  2. Configure the default layout for "Full" entity view display, and make it match the layout for the "Default" entity view display.
  3. Disable layout builder on the "Default" entity view display.