Use the Layout Builder for EntityViewDisplays

This does not yet update the UI.
And this is already a big patch. Parts of it need to be split out into other blocking issues.

Work is being done here:
https://github.com/timplunkett/d8-layouts/commits/2922033-layout_defaults

f437e373b6 Move the core block plugin schema out of block module.
1ea595139f Use setComponent() within EntityDisplayBase::init() to allow subclasses to intercept all calls.
ff7ba837ee Rewrite Section's data model.
8fb7a0087b Refactor to use Section instead of arrays or LayoutSectionItem.
db0cfd9be5 Add LayoutBuilderEntityViewDisplay
0ea0d114fa Introduce LayoutEntity.
ef0e05d8b0 Use LayoutEntity

Missed a spot, oops. Never make last minute changes *after* running tests.

Also attached is a patch made via format-patch, for easier reviewing.

Filed #2925657: EntityDisplayBase::init() should use ::setComponent() for fields and #2925651: Config schema for \Drupal\Core\Block is wrongly in block.module as the two core blockers

1. +++ b/core/modules/layout_builder/src/Section.php
   @@ -5,26 +5,114 @@
   +    return \Drupal::service('plugin.manager.core.layout')->createInstance($this->getLayoutId(), $this->getLayoutSettings());
   
   +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
   @@ -189,12 +177,12 @@ protected function buildAddSectionLink($entity_type_id, $entity_id, $delta) {
   +    $layout = $section->getLayout();
   
   +++ b/core/modules/layout_builder/src/Form/ConfigureSectionForm.php
   @@ -121,14 +110,15 @@ public function buildForm(array $form, FormStateInterface $form_state, EntityInt
   +    $this->layout = $section->getLayout();
   
   +++ b/core/modules/layout_builder/src/LayoutSectionBuilder.php
   @@ -82,24 +67,21 @@ public function __construct(AccountInterface $account, LayoutPluginManagerInterf
   +    $layout = $section->getLayout();
   

   While I can appreciate that this is a convenience, we're polluting our value object with the container. Given that these seem to be the only calling code for the ::getLayout method, and both ::getLayoutId and ::getLayoutSettings are public *and* this calling code already has/had the layout manager injected, should we leave the ::createInstance call to the calling code *or* should we add a parameter to ::getLayout for the layout plugin manager and make it the calling code's responsibility to either create the instance or pass along the manager. That would keep our value object pure.

2. +++ b/core/modules/layout_builder/layout_builder.install
   @@ -0,0 +1,35 @@
   +function layout_builder_install() {
   

   we need a test for this

3. +++ b/core/modules/layout_builder/layout_builder.install
   @@ -0,0 +1,35 @@
   +    $field_layout_settings = $display->getThirdPartySettings('field_layout');
   

   If there are no field layout settings should we continue; to the next version

4. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -0,0 +1,331 @@
   + * Provides an entity view display entity that has a layout.
   + */
   +class LayoutBuilderEntityViewDisplay extends BaseEntityViewDisplay implements LayoutEntityDisplayInterface {
   
   +++ b/core/modules/layout_builder/src/Entity/LayoutEntityDisplayInterface.php
   @@ -0,0 +1,69 @@
   + * Provides an interface for entity displays that have layout.
   + */
   +interface LayoutEntityDisplayInterface extends EntityDisplayInterface {
   

   should these be @internal?

5. +++ b/core/modules/layout_builder/src/LayoutSectionBuilder.php
   @@ -93,6 +93,35 @@ public function buildSection(Section $section) {
   +    $layout = $section->getLayout();
   

   oh and another call here..

So in terms of breaking this up, I see two chunks

• the LayoutEntity / decorator stuff - should be pretty stand alone
• the change in the section/region data model to drop the nesting - again should be pretty stand alone

Progressing well, nice work

+++ b/core/modules/layout_builder/layout_builder.module
@@ -35,6 +37,10 @@ function layout_builder_entity_type_alter(array &$entity_types) {
+  $entity_types['entity_view_display']->setClass(LayoutBuilderEntityViewDisplay::class);
+  if (\Drupal::moduleHandler()->moduleExists('field_ui')) {
+    $entity_types['entity_view_display']->setFormClass('edit', EntityViewDisplayEditForm::class);
+  }

What if someone has their own improved Field UI module?

#6

1) You're right, of course. It's just so damn convenient. I'll work on this

2) I think we could stand for a more explicit test, but LayoutBuilderFieldLayoutCompatibilityTest does test this.

3) We still need the second half of this loop to run.

4) Not sure but probably

I agree with the chunks. I have them split like that in the branch.

#7
This is temporary. Before this issue is finished we need our own Form class, this was just to stop the Field Layout UI from fataling.

#6.1
Section is not a value object, it's a domain object.
Domain objects contain business logic and are mutable. Amusingly enough, they are sometimes called "entities" in other systems.
I'm still amenable to some method of injecting the plugin manager, but I still didn't do that.
Additionally, we need the plugin manager for another part of the code that this class is absolutely responsible for, so we're going to have it anyway.

What I did do was cut down the usages of getLayoutId/getLayoutSettings to only 2 calls, the times when it is being stored. I also marked the methods @internal and explained when they should be used.
I've also removed setLayoutId/setLayoutSettings entirely. Finally, I moved the logic for switching a layout that was only in the defaults code before.

#6.2
I wrote \Drupal\Tests\layout_builder\Kernel\LayoutBuilderInstallTest

#6.3
While writing the above test and reexamining this code, I realized that we DON'T want to start with a default section. I've removed that from LayoutBuilderEntityViewDisplay and fixed this install code

#6.4
Made all of the classes @internal

#7
Added an explicit @todo.

I still have to split up this issue, working on that next

Split the first portion out:
#2926914: Rewrite \Drupal\layout_builder\Section to represent the entire section, not just the block info

Here's a slimmed down version of what is next after that blocker issue.

This is now further postponed on #2927349: Decouple the Layout Builder UI from entities

I have this split up as 3 commits locally:

1. Adding the entity class override, and defining the API
2. Using it to store Layout defaults
3. Providng a UI for Layout defaults

The complete patch is all 3 of those.

The combined patch is all 3 of those plus #2927349: Decouple the Layout Builder UI from entities and #2671964: ContextHandler cannot validate constraints

one blocker left

1. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -0,0 +1,281 @@
   +    $sections = $this->getSections();
   +    if (isset($sections[$delta])) {
   +      $start = array_slice($sections, 0, $delta);
   +      $end = array_slice($sections, $delta);
   +      $this->setSections(array_merge($start, [$section], $end));
   +    }
   +    else {
   +      $this->appendSection($section);
   +    }
   +    return $this;
   

   could use a trait for this, there was similar code elsewhere?

2. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -0,0 +1,281 @@
   +    if ($this->isOverridable() && !$entity->get('layout_builder__layout')->isEmpty()) {
   +      return $entity->get('layout_builder__layout')->getSections();
   +    }
   

   nice

3. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplayStorage.php
   @@ -0,0 +1,100 @@
   +      $configuration_reflection = new \ReflectionProperty($component, 'configuration');
   +      $configuration_reflection->setAccessible(TRUE);
   ...
   +      $additional_reflection = new \ReflectionProperty($component, 'additional');
   +      $additional_reflection->setAccessible(TRUE);
   ...
   +        'configuration' => $configuration_reflection->getValue($component),
   +        'additional' => $additional_reflection->getValue($component),
   

   ick - anything we can do to avoid this?

4. +++ b/core/modules/layout_builder/src/OverridesSectionStorageInterface.php
   @@ -0,0 +1,18 @@
   +interface OverridesSectionStorageInterface {
   

   internal for now?

5. +++ b/core/modules/layout_builder/src/Routing/LayoutBuilderRoutes.php
   @@ -72,6 +75,55 @@ public function getRoutes() {
   +  public function onAlterRoutes(RouteBuildEvent $event) {
   
   +++ b/core/modules/layout_builder/src/Routing/SectionStorageDefaultsParamConverter.php
   @@ -0,0 +1,35 @@
   +class SectionStorageDefaultsParamConverter extends EntityConverter implements SectionStorageParamConverterInterface {
   

   We should be able to add kernel/unit tests for these?

#18
1)
Because of slight differences between the two, I'd rather avoid trying to overgeneralize. Instead, I've linked to #66183: Add helper methods to inject items into a particular position in associative arrays, which should make this first part a one-liner eventually.

3)
I forgot I left this ugliness in. I've gone ahead and rewritten this.

4)
Yep!

5)
Sure!
The test for LayoutBuilderRoutes asserts the entire structure of each route.
It feels really prescriptive and possibly brittle, but it also revealed a problem in the logic already, so I guess it's worth it.

Wrote the tests for the others too.

Some of these additions need to be moved to #2927349: Decouple the Layout Builder UI from entities, working on that now

32348a8a54 Fix storage to not use reflection
f02d060c53 Point to NestedArray::insert() issue to eventually remove the array_slice/array_merge complexity.
f4aeaafd29 Mark OverridesSectionStorageInterface as @internal
3ed5589b17 Add test for LayoutBuilderRoutes
39e57a8092 Add test for LayoutTempstoreParamConverter.
590249fe4c Add test for SectionStorageDefaultsParamConverter.
a90e9889b5 Add test for SectionStorageOverridesParamConverter.

This is a patch with all 3 parts combined, the individual commits are on github for now

Realized that this will fail tests until #2921626: Add proper context-awareness to Layout Builder happens.
Still technically postponed, but I want to see some test results.

4d9b0e5d37 Add dependency on Field UI
bc8129417e Fix label of entity view display
e5ea4dd23a Redirect to Manage Display when done editing defaults.
229caff328 Handle fields that cannot be rendered when in preview.
05320e15fc Avoid collision on tempstore collection name
fc6e6b73fc Store the sample entity in the tempstore.
25223cb60a Move "Manage Layout" to the front end
702a68e166 Fix hook_install check

Still includes #2921626: Add proper context-awareness to Layout Builder, but that's now RTBC!

Fixing a couple things, and making defaults work for every view mode.

Blocker is in!
Added in the revert functionality.

@tim.plunkett Doing some manual testing - if I enable the standard profile, layout builder, then visit /admin/structure/types/manage/article/display-layout/default and try to configure + save an existing block (like Image, for instance), I get a 500 error. Removing existing field blocks, adding new ones, and configuring them seems to work OK.

There was some weirdness in the add/update flow, specifically around relying on the correct UUID persisting.
Which is precisely the exception you hit in #26.

This fixes that by generating/retrieving and then tracking the UUID throughout the form flow, instead of trying to find it at the end during submit.

Part 1 of review:

1. +++ b/core/modules/layout_builder/layout_builder.install
   @@ -0,0 +1,52 @@
   +    foreach ($display->getComponents() as $name => $component) {
   +      if (isset($field_definitions[$name]) && $field_definitions[$name]->isDisplayConfigurable('view') && isset($component['type'])) {
   +        $uuid = $uuid_generator->generate();
   +        $configuration = [];
   +        $configuration['id'] = 'field_block:' . $display->getTargetEntityTypeId() . ':' . $name;
   +        $configuration['label_display'] = FALSE;
   +        $configuration['formatter']['type'] = $component['type'];
   +        $configuration['formatter']['label'] = $component['label'];
   +        $configuration['formatter']['settings'] = $component['settings'];
   +        $configuration['formatter']['third_party_settings'] = $component['third_party_settings'];
   +        $configuration['context_mapping']['entity'] = 'layout_builder.entity';
   +        $components[] = (new SectionComponent($uuid, $component['region'], $configuration))->setWeight($component['weight']);
   +      }
   

   This kind of logic is good for migrating settings from existing displays, but new displays don't have their default fields (i.e. "Body") added into sections. I also noticed that when I add a new Content Type and go to configure its Entity Display layout, the "Content" category is missing until I rebuild cache! That should be addressed.

2. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -38,17 +35,18 @@ function layout_builder_entity_type_alter(array &$entity_types) {
   +  $entity_types['entity_view_display']
   +    ->setClass(LayoutBuilderEntityViewDisplay::class)
   +    ->setStorageClass(LayoutBuilderEntityViewDisplayStorage::class)
   +    ->setFormClass('edit', LayoutBuilderEntityViewDisplayForm::class);
   

   Are there cases where an Entity Type would want to "opt-out" of Layout Builder? I'm wondering if we should restrict the scope to known Entity Types, to prevent contrib headaches.

3. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -0,0 +1,386 @@
   +  public function removeSection($delta) {
   +    $sections = $this->getSections();
   +    unset($sections[$delta]);
   +    $this->setSections(array_values($sections));
   +    return $this;
   +  }
   

   So $delta in the other insert/append/setSection methods is always numeric (and sequential), right? Just double checking because array_values will discard any existing non-numeric keys.

4. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -0,0 +1,386 @@
   +    $sections = $this->getSections();
   +    if (!isset($sections[$delta])) {
   +      return parent::getDefaultRegion();
   +    }
   

   Maybe we should have a hasSection method, given the number of other related methods that are being added here?

5. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -0,0 +1,386 @@
   +    if ($this->isOverridable() && !$entity->get('layout_builder__layout')->isEmpty()) {
   +      return $entity->get('layout_builder__layout')->getSections();
   

   The key layout_builder__layout is hard-coded in a lot of places in layout_builder, should this be a constant or returned by a method?

6. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -0,0 +1,386 @@
   +    $bundle_parameter_key = $entity_type->getBundleEntityType() ?: 'bundle';
   

   Why provide a default for the bundle key here? If there is not bundle entity type, we shouldn't add the bundle as a parameter.

7. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplayStorage.php
   @@ -0,0 +1,55 @@
   +    if (!empty($record['third_party_settings']['layout_builder']['sections'])) {
   +      $record['third_party_settings']['layout_builder']['sections'] = array_map(function (Section $section) {
   +        return $section->toArray();
   +      }, $record['third_party_settings']['layout_builder']['sections']);
   +    }
   

   We should do this logic for individual entities as well, since we're currently using serialization.

Last bit of review:

1. +++ b/core/modules/layout_builder/src/Routing/SectionStorageDefaultsParamConverter.php
   @@ -0,0 +1,39 @@
   +      // If a bundle is not provided but a value corresponding to the bundle key
   +      // is, use that for the bundle value.
   +      if (empty($defaults['bundle']) && isset($defaults['bundle_key']) && !empty($defaults[$defaults['bundle_key']])) {
   +        $defaults['bundle'] = $defaults[$defaults['bundle_key']];
   +      }
   

   It seems like there's a lot of code in this patch that fills in values when an entity type is not bundle-able, is there any harm in omitting the bundle completely in that case?

2. +++ b/core/modules/layout_builder/src/Section.php
   @@ -73,14 +73,20 @@ public function __construct($layout_id, array $layout_settings = [], array $comp
   +    // @todo Add the regions to the $build in the correct order. This is done
   +    //   for parity with \Drupal\field_layout\FieldLayoutBuilder::buildView(),
   +    //   which incorrectly adds all regions to the build.
   +    $regions = array_fill_keys($layout->getPluginDefinition()->getRegionNames(), []);
   

   Is there an issue for this?

Also, It looks like there's test coverage for the routing changes introduced by this issue, but not much for actually creating an Entity View Display default using Layout Builder and verifying that entities are displayed properly using that default.

#28
1) Ughhhhh we're going to have to intercept calls to `setComponent()` and translate that into... something.

2) Can you open a follow-up to discuss this?

3) As of now, we expect them to be numeric, and sequential. This could be documented/codified/enforced better.

4) Where else would we use this? I'd rather avoid needlessly expanding the interface... Though it could also be on a trait.

5) It's in 3 places: This class, which owns the field. That class's corresponding ParamConverter. And a hook_form_alter.
We could constant-ify this, I did that in previous iterations. Just didn't seem important, since it's not something anyone else should be using.
If you feel like it'd be an improvement, please open a follow-up

6) We need this for the ParamConverter.

7) Is serializing an array any better or worse than serializing the object? I really don't know. Can you open a follow-up for this?

#29

1) Yep, and that's 100% due to the Field UI and EntityViewDisplays, specifically around the field_ui_base_route property on all entity types. We're inheriting that mess, and can't really touch it.

2) We might consider removing this hack, and rewriting the accompanying test that proves that migrating from Field Layout to Layout Builder results in *identical markup*.

Needing code changes:
#28.1
#28.3
#29.2

Needing follow-ups:
#28.2
#28.4
#28.5
#28.7

Follow ups:

#28.2 - #2936358: Layout Builder should be opt-in per display (entity type/bundle/view mode)
#28.4 - After reviewing possible uses for a new method, a follow up is probably overkill.
#28.5 - #2936360: Remove duplicate references to the "layout_builder__layout" field from Layout Builder
#28.7 - This is covered by #2914503: Determine if serializing blobs is safe for use in Layout Builder

Addressed #28.1, both parts.

Turns out I took care of the brittle test already, so #29.2 can be resolved. Thanks, past me!

For #28.3, I moved the array_values() call to the setter itself, and documented the behavior on the interface.
LayoutSectionItemList (overrides) already enforced this behavior.

To the best of my knowledge, all feedback is addressed in the patch or by follow-ups.

Thanks for the review!

1) I think our decision here is appropriate for the first iteration, and yeah we now have that issue to discuss more.

2) This branch has existed locally for me for a loooong time, and this code was needed before the Section/SectionComponent rewrite. It is indeed no longer needed!

3) We currently only support overrides for the default, so this is okay for now.
Opened #2936507: Determine how an override can identify its corresponding default, which is only relevant pending resolution of #2907413: Consider supporting Layout Builder Overrides for other view modes

4) Hah, nice catch.

+++ b/core/modules/layout_builder/layout_builder.module
@@ -38,17 +36,18 @@ function layout_builder_entity_type_alter(array &$entity_types) {
+  $entity_types['entity_view_display']
+    ->setClass(LayoutBuilderEntityViewDisplay::class)

I wonder if the Blocks-Layouts initiative has any plans to work on #1875974: Abstract 'component type' specific code out of EntityDisplay so it can integrate properly with the existing system :)

I've been tracking that issue for about 5 years. I really hope to be able to help push it forward as a part of getting this module to become a full part of core.

1. +++ b/core/modules/layout_builder/layout_builder.install
   @@ -0,0 +1,37 @@
   +function layout_builder_install() {
   

   Do we need an uninstall here or does the third-party settings api automatically remove these for us?

2. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -57,140 +56,54 @@ function _layout_builder_hide_layout_field(array &$form) {
   +function _layout_builder_create_section_component_from_options($entity_type_id, $bundle_id, $name, array $options) {
   

   If we moved this to a protected method on the new LayoutBuilderEntityViewDisplay we could just call ->setComponent in the install hook, and we'd make sure no-one can ever call this code. Yes its prefixed with _, but enforced private API is better than naming conventions. Thoughts?

3. +++ b/core/modules/layout_builder/src/Form/LayoutBuilderEntityViewDisplayForm.php
   @@ -0,0 +1,91 @@
   +    $form['manage_layout'] = [
   +      '#type' => 'link',
   +      '#title' => $this->t('Manage layout'),
   +      '#weight' => -10,
   +      '#attributes' => ['class' => ['button']],
   +      '#url' => $this->entity->getLayoutBuilderUrl(),
   +    ];
   +
   +    // @todo Expand to work for all view modes in
   +    //   https://www.drupal.org/node/2907413.
   +    if ($this->entity->getMode() === 'default') {
   +      $form['layout'] = [
   +        '#type' => 'details',
   +        '#open' => TRUE,
   +        '#title' => $this->t('Layout options'),
   +        '#tree' => TRUE,
   +      ];
   

   Forgive me if I missed it, but I don't see any functional tests for this form?

4. +++ b/core/modules/layout_builder/src/Routing/LayoutBuilderRoutes.php
   @@ -125,6 +172,16 @@ protected function buildRoute($type, $route_name_prefix, $path, array $defaults,
   +    if ($type === 'overrides') {
   

   Is there a missing API here - StorageType::supportsReverting

   duck typing etc

Assigning to @xjm who indicated in #2927349-18: Decouple the Layout Builder UI from entities that she wanted the opportunity to sign-off here.

1) Third party settings API handles it for us

2) I can look into that

3) #2924201: Resolve random failure in LayoutBuilderTest so that it can be added to HEAD has it :(

4) I think LayoutBuilderRoutes needs to become more extensible, it's not sustainable for every type of Layout Builder UI to expect to be included in this single class. Looking into this as well.

When I click 'manage layout' from manage display I'm sent to the layout page, which is fine.
But when I save the layout, I'm put back on the 'manage display' page with no indication that something happened.
Do I need to click save again there?
Did my changes save?
Screenshot:

On the edit layout page, the 'generate sample values' could use some love. The title generated is very long and breaks the layout. I realize that isn't related to the code here, but its definitely something that is a bit off. We really need a general usabililty review here (tagged as such).

Screenshot of that:

Also I think the 'save layout' button needs some visual affordance to make it clear that its the primary action here. At present its a bit lost.

Added messages to the Layout Builder UI for each action.

@EclipseGc opened #2936655: Layout Builder should use the toolbar modes system once it exists. to explicitly codify that our usage of local tasks is not desired nor permanent.
See the comps for the non-beta plan for the UI, which has had product manager and UX review: https://projects.invisionapp.com/boards/KH3EPJMFNWR4Z

I addressed #41.2 and #41.4 as well.

+++ b/core/modules/layout_builder/layout_builder.install
@@ -16,22 +16,20 @@ function layout_builder_install() {
+    foreach ($components as $name => $component) {
+      $display->setComponent($name, $component);

+++ b/core/modules/layout_builder/layout_builder.module
@@ -76,34 +76,3 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
-function _layout_builder_create_section_component_from_options($entity_type_id, $bundle_id, $name, array $options) {

nice!

I introduced two bugs in the last patch.

First, the move to a static method on SectionStorageInterface ran into this Prophecy bug: https://github.com/phpspec/prophecy/issues/119

Second, the inlining of _layout_builder_create_section_component_from_options() failed to update $options after calling parent::setComponent().

Some notes from manual testing:

#1.
After enabling the module, going to Comment Types > Manage Display > Manage layout results in

Fatal error: Call to a member function access() on a non-object in /home/du749/www/core/modules/comment/src/CommentAccessControlHandler.php on line 34

This only occurs if there is no content. Once you create a node, Manage layout works for the default comment type.

#2.
When you enable this module, it modifies the layout of the Article content type, putting the comment field at the top (above the body and image). Screenshots attached of before and after, they aren't great because my screen is very small atm! (Steps to test: Create an article on a fresh install, view the page, then enable "Layout Builder" and view it again.) Not a showstopper since it's an easy fix, but it's definitely unexpected.

Article page before enabling:

Article page after enabling:

After enabling the module, going to Comment Types > Manage Display > Manage layout results in

Fatal error: Call to a member function access() on a non-object in /home/du749/www/core/modules/comment/src/CommentAccessControlHandler.php on line 34

Possible an issue with CommentStorageHandler::createSampleEntity?

Thanks @pameeela

My array_column/array_multisort wasn't working as expected, but thankfully Drupal provides a method for this already: Drupal\Component\Utility\SortArray::sortByWeightElement()

Using that fixes #50.2.

The bug in #50.1 is because of \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::generateSampleValue()
(one step down the chain from CommentStorageHandler::createSampleEntity as suggested by @larowlan)
It tries to load an existing entity (in this case, a node) and can't find any.
But if you were to create and save a node first, this page would work fine (as @pameeela pointed out)
#2936793: EntityReferenceItem::generateSampleValue() should create a sample entity if a referenceable entity is not found is an issue to fix that, leaving it out of the patch

Needed a reroll due to #2935779: Come up with an affordance for what parts of the page are/are not affected by Layout Builder.
Additionally, fixed some PHPCS warnings.
Leaving RTBC as there are no functional changes.

Got Fatal error: Uncaught PDOException: SQLSTATE[HY000]: General error: 13 database or disk is full in /var/www/html/core/lib/Drupal/Core/Database/Connection.php:1207
from the testbot, requeueing

The sample content with the dead fish in #50 amused me.

Just scanning the patch to start; a few nitpicky things and questions I noticed while scanning:

1. +++ b/core/modules/layout_builder/config/schema/layout_builder.schema.yml
   @@ -5,3 +5,42 @@ core.entity_view_display.*.*.*.third_party.layout_builder:
   +  label: 'Component'
   

   I think this should probably be more specific ("Layout component"), since AFAIK it's used for translation?

2. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -0,0 +1,429 @@
   +/**
   + * Provides an entity view display entity that has a layout.
   + *
   + * @internal
   + */
   
   +++ b/core/modules/layout_builder/src/Entity/LayoutEntityDisplayInterface.php
   @@ -0,0 +1,33 @@
   +/**
   + * Provides an interface for entity displays that have layout.
   + *
   + * @internal
   + */
   

   (Here and maybe elsewhere.) Because experimental or because actually internal? Guessing the latter because of the interface but we should document why always.

3. +++ b/core/modules/layout_builder/src/Field/LayoutSectionItemList.php
   @@ -16,7 +18,7 @@
   -class LayoutSectionItemList extends FieldItemList implements SectionStorageInterface {
   +class LayoutSectionItemList extends FieldItemList implements SectionStorageInterface, OverridesSectionStorageInterface {
   

   I do like this better than the approach from last month.

4. +++ b/core/modules/layout_builder/src/Field/LayoutSectionItemList.php
   @@ -91,7 +94,7 @@ public function getContexts() {
   -  public function getStorageType() {
   +  public static function getStorageType() {
   

   Is this in scope?

5. +++ b/core/modules/layout_builder/src/Form/LayoutBuilderEntityViewDisplayForm.php
   @@ -0,0 +1,91 @@
   +    // @todo Expand to work for all view modes in
   +    //   https://www.drupal.org/node/2907413.
   

   Ah interesting.

   Do we know necessarily that the "full" view mode is the default? I think I've had sites before where that was not the case.

   Probably OK to fix after beta though.

6. +++ b/core/modules/layout_builder/src/OverridesSectionStorageInterface.php
   @@ -0,0 +1,26 @@
   + * @internal
   + *   Layout Builder is currently experimental and should only be leveraged by
   + *   experimental modules and development releases of contributed modules.
   + *   See https://www.drupal.org/core/experimental for more information.
   

   Yep that's what I'm looking for. :)

7. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -306,4 +306,23 @@ protected function currentUser() {
   +   * @internal
   +   *   This is intended for use by a storage mechanism for section components.
   

   Interesting.

8. +++ b/core/modules/layout_builder/src/SectionStorageInterface.php
   @@ -61,6 +61,8 @@ public function insertSection($delta, Section $section);
   +   * This will re-key every subsequent section.
   +   *
   

   In what way? What does this mean for calling code?

What happens if the site builder tries to override a layout of not-the-default-display, or configure one for not-the-default-display?

#59
1)
Firstly, these strings are not used anywhere that I know of, but it is a best practice to provide labels anyway.
Secondly, I'm not sold on pigeon-holing these as "Layout components" only. In my mind, this is the first step toward addressing #1875974: Abstract 'component type' specific code out of EntityDisplay (as mentioned by @amateescu in #39)

2) Fixed

3) Thanks

4) That change is needed to address #41.4

5) We no longer have full anywhere in our code. D8 ships with the full view mode turned off, so this is the best we can check.
It will be really interesting to see if we can resolve that disconnect from this side of things. But definitely post-beta material.

8) Tried to reword this. But basically, if you have 4 sections they are stored as 0,1,2,3 and calling ->removeSection(2) will result in deltas of 0,1,2 not 0,1,3.

#60
Configuring the layout for a not-the-default-display works as you'd expect. For example, with nodes you can go and see that the teaser will be configured to use a smaller image style and a trimmed body. This is migrated over upon install. Changing the layout for the teaser will affect their display on the homepage view.

There is no UI, no secret route parameter, nothing you can do to get to a place to override the layout of another display than the default. It is currently impossible, by design. #2907413: Consider supporting Layout Builder Overrides for other view modes would be for discussing allowing this.

I have tested patch from #62 and I experienced few problems. I didn't have time to dig deeper what happened there, just want to list them here since the issue is marked as RTBC and maybe problems are relevant. Also, someone can give additional feedback.

I have used Standard profile for testing and after enabling Layout Builder.

1. Delete Field - I just went to Article -> Manage display -> just click Save for Default and Teaser. Then I removed Comment field from Article. After that I went to create Article and used Preview => exception occurred
2. Taxonomy Term - create new vocabulary -> Manage display -> click "Manage layout" -> I get "Page not found". But if I click just Save on manage display, then "Manage layout" works
3. Media - maybe it's not related to this issue (could be related to media track), but when I go to "Manage layout" for Image media => exception occurs

Well spotted @mtodor!

1)
I had tested this, but with Body field.
If the field is present on any other bundles, then it will not print any output.
But if it is the last instance of the field, it will cause an exception.

This pointed to an issue with config dependencies. All the correct dependencies are in place, but we did not respond to it correctly.

2)
This is due to all code expecting the load-or-create flow of entity_get_display(), which is unfortunate. EntityConverter does not handle this, but now we do.

3)
This was an issue with routing, somehow media_type was being loaded before section_storage (but after when node usually runs?)
Ensured that this won't happen again by using NestedArray::mergeDeep().

I have tested additionally, all problems listed in #64 are fixed. Nice work @tim.plunkett!

I have found one additional problem. You can reproduce it in following way:
1. go to manage display of article (any view mode)
2. add block for Body field
3. change "Formatter" => that will trigger Ajax Request
=> that ajax request adds a new block to the collection of blocks for a section and when you do "Add Block" exception will happen. Additionally, you can just close right tray dialog and Save Layout -> then the new layout state is kinda broken.

I have attached proposal for a solution, it's not nice, but works for what I have tested. Other option could be to react on trigger element in the request. I'm not sure, what way is better, maybe there is some other cleaner solution.

Another nice find!
Unlike the other bugs above, this one is currently untestable.
I will make a note on #2924201: Resolve random failure in LayoutBuilderTest so that it can be added to HEAD to test this scenario.

I've rewritten the fix slightly to use temporary form_state values, interdiff is against #65

I also checked for any other cases of generating new UUIDs, but they are all safe.

I have found two additional problems.

1. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -0,0 +1,451 @@
   +  public function onDependencyRemoval(array $dependencies) {
   +    $return = parent::onDependencyRemoval($dependencies);
   +    foreach ($dependencies['config'] as $field_config) {
   +      $id = 'field_block:' . $this->getTargetEntityTypeId() . ':' . $field_config->getName();
   +      foreach ($this->getSections() as $delta => $section) {
   +        foreach ($section->getComponents() as $uuid => $component) {
   +          if ($component->getPluginId() === $id) {
   +            $section->removeComponent($uuid);
   +            $return = TRUE;
   +          }
   +        }
   +      }
   +    }
   +    return $return;
   +  }
   

   There is problem here when I try to delete full content type, then additionally in list of dependencies is also entity type (fe. NodeType) and node type doesn't have method getName().

   Also in confirmation dialog, wrong information is provided. It's written that configuration for display views will be changed and then if you have 4 display views it will be displayed 4 times same name, for example: "Article content items". I think it should actually display Display View names.

2. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -0,0 +1,451 @@
   +      $new_component = (new SectionComponent(\Drupal::service('uuid')->generate(), $options['region'], $configuration));
   

   When new field is created, then $options['region'] doesn't exist. So PHP is giving Notice. I guess we could use getDefaultRegion() as fallback.

This addresses both points.

First, this was an oversight when I was copying the logic from the parent:: implementation
Fixed it and added test coverage

Second, I cleaned up this code as @mtodor suggested in #layouts slack.
Additionally, I removed places in our tests where we passed 'region' to sidestep this bug. Whoops!

The getDefaultRegion method on entity displays is a relic of the old Field UI that we're replacing, we shouldn't try to use it here.
Fixed!

Adding a confirmation form for the revert link.

For #74, I pinged @tim.plunkett about this problem. I mistakenly thought it was part of #2914484: Prevent the layout field from being removed if overrides exist This is the issue:

Clicking "Revert to defaults" accidentally when editing a layout override nukes your layout override with no override, even if you click "Cancel" afterward.

Testing the confirm form now.

Per discussion with @tim.plunkett, @dyannenova, and @EclipseGc, we should add some basic browser tests for this functionality. Experimental modules don't need to have perfect test coverage, but we do need some for the basics.

Meanwhile, trying to reconstruct things that I posted last night (d.o ate my comment). Will post shortly.

Forgot to mention above: the reason those tests weren't included yet was #2924201: Resolve random failure in LayoutBuilderTest so that it can be added to HEAD. However, @tim.plunkett is going to write browser tests instead to work around that issue, and the things that actually need JS tests (like drag-and-drop) can still have tests added later (mostly not related to this issue).

Alright so:

1. Promoting to major feature, because this functionality is a central part of the Layout Builder, which in turn is a strategic priority for D8's featureset.
    
2. I previously had concerns about the architecture of Layout Builder, specifically around how the API interacts with the storage. The previous blockers addressed a lot of my concerns by decoupling the storage. I asked for the opportunity to sign off on this final issue to make sure that that was still true with the final changes. No concerns here.
    
3. My other concern was about the actual storage mechanism. As I alluded to on an earlier issue in the chain, I originally expected that all layouts would be stored as configuration, independently of what entities, displays, etc. they might be mapped to. I was originally concerned that this issue used a separate storage implementation from the individual entity layouts (a.k.a overrides).

   However, when I spoke to the layouts team two weeks ago, they raised the point that while administrators would likely control and deploy layouts for content types, individual entity layouts would often be created by content authors, and (as such) would need to go through moderation workflows. It would actually create data integrity problems if the layouts were not moderated along with the content, because content authors would be placing blocks and editing content together and submit it for review together.

   So, at a high-level, it totally makes sense for this issue to use a separate storage implementation from the individual entity overrides (rather than replacing their storage implementation entirely).

   I also spoke to @larowlan about this. He indepedently went through almost exactly the same thought process, with the same initial expectations and then the same reasoning for why that would not actually work.
    

Unassigning from myself to clarify that this issue does not need to wait on my further signoff. (@larowlan and I already discussed that last week; it just didn't end up back on the issue. Sorry for the delay!) That said, I am still reviewing and testing this.

Test added!
Also fixed our workaround for #2936464: Remove setComponent() workaround in LayoutBuilderEntityViewDisplay to the actual correct fix, which is a huge relief for me.
Additionally, we should not be checking entity access during preview (aka, a sample entity).

Whoops, yet another comment lost, blah.

But anywho:

It looks a little goofy with the help block, but that's not meant to be permanent anyway, so it does not matter here.

I also confirmed that prior user input (between save and is not lost when the user clicks "Cancel" on the confirm form. Thanks, TempStore!

I went through my miscellaneous screenshots of things I noticed in testing this and confirmed that none of them are introduced here.

Fixing broken image.

Reviewed with @webchick and @effulgentsia, most of these points will get follow-ups, but one of them (I think it will be #4?) needs to be resolved in this issue.

1. +++ b/core/modules/layout_builder/config/schema/layout_builder.schema.yml
   @@ -5,3 +5,42 @@ core.entity_view_display.*.*.*.third_party.layout_builder:
   +    additional:
   +      type: ignore
   +      label: 'Additional data'
   

   @todo Rename to third_party_settings?

2. +++ b/core/modules/layout_builder/layout_builder.install
   @@ -0,0 +1,35 @@
   +  Cache::invalidateTags(['rendered']);
   

   @todo explain this!

3. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -0,0 +1,443 @@
   +  public function label() {
   +    $bundle_info = \Drupal::service('entity_type.bundle.info')->getBundleInfo($this->getTargetEntityTypeId());
   

   @todo Move this upstream

4. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -0,0 +1,443 @@
   +  public function onDependencyRemoval(array $dependencies) {
   +    $return = parent::onDependencyRemoval($dependencies);
   +    foreach ($dependencies['config'] as $config_entity) {
   +      if (!$config_entity instanceof FieldDefinitionInterface) {
   +        continue;
   +      }
   +
   +      $id = 'field_block:' . $this->getTargetEntityTypeId() . ':' . $config_entity->getName();
   +      foreach ($this->getSections() as $delta => $section) {
   

   @todo #2579743: Config entities implementing EntityWithPluginCollectionInterface should ask the plugins to react when their dependencies are removed

5. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -0,0 +1,443 @@
   +      $configuration['id'] = 'field_block:' . $this->getTargetEntityTypeId() . ':' . $name;
   

   @see #1875974: Abstract 'component type' specific code out of EntityDisplay
   @todo Open an issue to consider moving the data model improvements upstream
   @todo Open an issue about final destination for this code (Drupal\Core\Layout and Field UI)

6. +++ b/core/modules/layout_builder/src/Plugin/Block/FieldBlock.php
   @@ -141,8 +147,8 @@ public function build() {
   -    // First consult the entity.
   -    $access = $entity->access('view', $account, TRUE);
   +    // First consult the entity, except not while the entity is being previewed.
   +    $access = empty($entity->in_preview) ? $entity->access('view', $account, TRUE) : AccessResult::allowed();
   

   @todo Is this okay? :D

7. +++ b/core/modules/layout_builder/src/Plugin/Derivative/LayoutBuilderLocalTaskDeriver.php
   @@ -72,6 +75,35 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   +      $this->derivatives["entity.$entity_type_id.layout_builder_revert"] = $base_plugin_definition + [
   +        'route_name' => "entity.$entity_type_id.layout_builder_revert",
   +        'title' => $this->t('Revert to defaults'),
   +        'parent_id' => "layout_builder_ui:entity.$entity_type_id.layout_builder",
   +        'entity_type_id' => $entity_type_id,
   +        'weight' => 10,
   +        'cache_contexts' => ['layout_builder_is_active:' . $entity_type_id],
   

   @todo Revert should only be presented when overrides exist

Okay translating those notes into issues and changes:

#84
1) Opened #2939928: Discuss renaming SectionComponent's additional to third_party_settings for consistency

2) Added a comment

3) Opened #2939931: Add an implementation of EntityDisplayBase::label() and linked in the code

4) This... needed a big change.
First, removed the old code that hardcoded a fix for FieldBlock
Then added dependency calculation to FieldBlock itself
Also we need to work around #2579743: Config entities implementing EntityWithPluginCollectionInterface should ask the plugins to react when their dependencies are removed.
Additionally we need a temporary version of #2939925: Add a getPluginDependencies() method to \Drupal\Core\Plugin\PluginDependencyTrait
This change has a whole new test method.

5) Opened #2939932: Integrating Layout Builder directly into Standard profile

6) This is not ideal. And as needed for #2937799: Allow greater flexibility within SectionComponent::toRenderArray(), we might as well pass around the $in_preview value now

7) Added an @todo pointing to #2917777: Improvements to the styling, grouping, etc. of the Layout Builder UI actions form and updated that issue to note the needed change.

Hmmm the error, and what is described in #87, is because we can't rely on the contexts during dependency calculation.
The only reason it was needed was for the current bundle.
But due to #2671964: ContextHandler cannot validate constraints being committed, we can properly have a derivative per-bundle without exploding the list of blocks in the UI.

The interdiff is made to ignore whitespace changes, to make it slightly easier to understand.

Stupid mistakes. Forgot to run phantomjs locally when testing, so the failing test was skipped...

Switched from dynamic dependencies to declared dependencies, since we can.
Also adding an explicit test for this to FieldBlockTest, in addition to the other implicit tests in Layout Builder.

Attaching an interdiff for this change, as well as a larger one for all changes since the last RTBC patch in #80.

I rushed that last one and didn't have all my changes staged. This half is important too! :)

1. +++ b/core/modules/layout_builder/src/Form/ConfigureBlockFormBase.php
   @@ -109,7 +116,7 @@ public function __construct(LayoutTempstoreRepositoryInterface $layout_tempstore
        $this->contextRepository = $context_repository;
   

   There doesn't look to be a contextRepository field on this class (existing issue, can be follow up)

2. +++ b/core/modules/layout_builder/src/Form/ConfigureBlockFormBase.php
   @@ -109,7 +116,7 @@ public function __construct(LayoutTempstoreRepositoryInterface $layout_tempstore
        $this->classResolver = $class_resolver;
   

   same for classResolver

1 I believe is used by a subclass, that indeed could be moved.
2 is used by a trait

#2940212: [meta] Miscellaneous UI issues for the Layout Builder module lists out various UI issues that are not in scope here (as a pile of screenshots). Hopefully this is also helpful to other reviewers. :)

Thankfully a one-line fix, and it fixes the theme issue, the contextual links issue, and the settings tray styling issue :)
Added a test for it too.

Per #44 and #97

Adding review credits

Fixing stale method name in a comment from before a refactor. Leaving at RTBC.

Posted a CR https://www.drupal.org/node/2940777

Gave this another code review and the only issues I have with it are addressed by #2937483: Defining a new type of section storage relies on magic strings and hidden assumptions

@webchick and @effulgentsia have gone over the code with @tim.plunkett

@EclipseGc has given it another extensive review and manual test in #100

I gave another manual test this morning, screenshot attached. Worked well - found a UX issue that I added to the existing meta

Committed 02f9325 and pushed to 8.6.x

Published the change record

:tada:

Committed #105 as fbf13ed and pushed to 8.6.x