Problem/Motivation

EntityViewBuilder::getSingleFieldDisplay() relies on creating a fake non-sectioned display and setting values on it for rendering.
EntityViewDisplay::setComponent() is called during rendering, while the Layout Builder BC layer for ::setComponent() calls assumes it is called during configuration changes only (in install/update, or when adding new fields).

To avoid recursion, a check for the magic view mode \Drupal\Core\Entity\EntityDisplayBase::CUSTOM_MODE was added.

However, after #2936358: Layout Builder should be opt-in per display (entity type/bundle/view mode) there is now a check for whether LB is enabled for that display.
Since it can never be enabled for that magic view mode, the extra check is redundant.

Proposed resolution

Remove the @todo and the check for CUSTOM_MODE

Remaining tasks

N/A

User interface changes

N/A

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#9 2936464-9.patch894 bytesphenaproxima
#7 interdiff-2936464-4-7.txt1.06 KBphenaproxima
#7 2936464-7.patch10.51 KBphenaproxima
#4 2936464-4.patch10.33 KBphenaproxima
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett 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
Component: layout.module » entity system
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented

As described in #2976148-21: Layout-based entity rendering should delegate to the correct section storage instead of hardcoding to either defaults or overrides, fixing this issue may further mitigate the possibility of infinite recursive rendering.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Active » Needs review
FileSize
2936464-4.patch10.33 KB

Here's one possibility: move the business logic of rendering field items into EntityViewBuilder, rather than EntityViewDisplay, then have EntityViewDisplay delegate to that. This allows us to completely remove getSingleFieldDisplay(). No tests yet -- I'd like this idea validated first, and I want to see how many tests this breaks.

Status: Needs review » Needs work

The last submitted patch, 4: 2936464-4.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

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

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs work » Needs review
FileSize
2936464-7.patch10.51 KB
interdiff-2936464-4-7.txt1.06 KB

This oughta fix at least one failure.

Status: Needs review » Needs work

The last submitted patch, 7: 2936464-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Title: Discuss rewriting EntityViewBuilder::getSingleFieldDisplay() to adjust for Layout Builder » Remove setComponent() workaround in LayoutBuilderEntityViewDisplay
Component: entity system » layout_builder.module
Status: Needs work » Needs review
FileSize
2936464-9.patch894 bytes

Discussed with @tim.plunkett in Slack. We realized that this approach will not work, because:

  1. viewField() will not fire hook_entity_display_build_alter(), which breaks things. Even if we fixed it in core, this would be a BC-breaking change for contrib and I don't think it can be realistically done until the rest of the rendering system is untangled, presumably in Drupal 9.
  2. Layout Builder is opt-in now, which means LayoutBuilderEntityViewDisplay has an isLayoutBuilderEnabled() method that short-circuits setComponent() under very clear conditions. Therefore, recursive rendering is not a thing we need to worry about much. You'd need to try to cause it.
  3. It's a whole hell of a lot easier to just remove the existing workaround which was preventing recursive rendering.
  4. Trying to fix the rendering system now is not worth the large number of migraines it will cause. Let's wait until we can completely abandon BC (i.e., Drupal 9) to do that.

All that said, here's a new patch which removes the workaround. Another todo bites the dust!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs review » Reviewed & tested by the community

I concur!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs issue summary update

I think the issue summary needs an update to add why #9 is the correct approach. I might be missing something but the comment in #9 doesn't really explain it to me. It would be good to know what test was originally breaking that caused us to add the work around and if it is possible by clicking around in the UI to cause the recursive rendering.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Rewrote the IS. A longer explanation is below.

EntityViewDisplay::setComponent() is used to configure the field formatter for a given field.
Layout Builder swaps out the EntityViewDisplay entity class for it's own, and overrides setComponent().
It does this to add a new field block corresponding to the given field.
For example, when you add a new body field to a content type, node_add_body_field() calls setComponent() which adds a field block for the body field. This way newly added fields are immediately visible when using LB.

However, when when something like Views field row style wanted to render a single field in isolation, a temporary EntityViewDisplay is created but not saved, and the requisite formatter settings are set on it for that single field.

\Drupal\Core\Field\FieldItemListInterface::view($display_options)
\Drupal\Core\Entity\EntityViewBuilderInterface::viewField($items, $display_options)
\Drupal\Core\Entity\EntityViewBuilder::getSingleFieldDisplay($entity, $field_name, $display_options)
\Drupal\Core\Entity\Entity\EntityViewDisplay::create($values)
\Drupal\Core\Entity\EntityDisplayBase::setComponent($field_name, $display_options)

Crucially, the fake/temporary display is created without a specified view mode, leaving it with the default of \Drupal\Core\Entity\EntityDisplayBase::CUSTOM_MODE, which is documented as being for this runtime case.

Without the existing check, any runtime/fake/temporary display will have a field block added to the display.
The new flow is

\Drupal\layout_builder\Plugin\Block\FieldBlock::build()
\Drupal\Core\Field\FieldItemListInterface::view($display_options)
\Drupal\Core\Entity\EntityViewBuilderInterface::viewField($items, $display_options)
\Drupal\Core\Entity\EntityViewBuilder::getSingleFieldDisplay($entity, $field_name, $display_options)
\Drupal\Core\Entity\Entity\EntityViewDisplay::create($values)
\Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::setComponent($field_name, $display_options)
... add new block, repeat

Hence the check for $this->getMode() === static::CUSTOM_MODE.

However, with the change made in the opt-in patch, we now additionally prevent this BC layer from running when LB is disabled for that view mode.
Since it will never be enabled for CUSTOM (it defaults to disabled), we no longer need this extra check.

alexpott’s picture

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

Thanks for updating the issue summary. Makes sense and looking at \Drupal\Core\Entity\EntityViewBuilder::getSingleFieldDisplay I can see how these are never going to have layout builder enabled via third party settings as the view mode is created there and then.

Backporting to 8.6.x since layout builder is experimental.

Committed and pushed a878a80d6c to 8.7.x and ee4efcfb77 to 8.6.x. Thanks!

  • alexpott committed a878a80 on 8.7.x 
    Issue #2936464 by phenaproxima, tim.plunkett: Remove setComponent()...

  • alexpott committed ee4efcf on 8.6.x 
    Issue #2936464 by phenaproxima, tim.plunkett: Remove setComponent()...

Status: Fixed » Closed (fixed)

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