Move the Layout Builder UI into an entity form for better integration with other content authoring modules and core features

I strongly disagree with the suggestions in this issue.

First, the Layout Builder UI is not a form itself. The primary interactions are not form-based, and only in some cases do certain actions result in a form being presented.

Second, the Layout Builder UI only currently happens right now to be presented in Drupal render arrays. There has been discussion of a decoupled UI that would use JSON API underneath for manipulating the data.

Third, the fact that there is any magic done only at the entity form level is exactly why we have all of these bugs in the first place.

Which brings us back to #2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing as the true underlying issue.

This to me is very reminiscent of the move from form-level validation to data-level validation.
Coupling anything to the form representation of this is a mistake, and one that we made in the past and are still struggling to undo.

Here's a first pass at converting from that sandbox to an actual patch.

This exposed #3009344: Reloading the Layout Builder while JS is available differs from when JS is not available, because of the page refresh caused by the form submit. Adding that patch into here for now.

This breaks horribly when comment module is installed (and comments are enabled for the bundle)

Here is failing test

+++ b/core/modules/layout_builder/src/Plugin/Field/FieldWidget/LayoutBuilderWidget.php
@@ -0,0 +1,122 @@
+    return $layout_controller->layout($section_storage);

This fails because the comments fields uses #lazy_builder which I think is in compatible with forms. Of course other fields also will use this. Are there other things fields use when rendering that will be incompatible in a form context?

Here is patch that just renders the layout right here.

For some reason this patch makes viewing the user profile fail.
Here is:

1. A patch with just a test addition to test viewing the user. This passes without this patch
2. Same patch as #11 with user view test. It should fail.
3. I change drupalci.yml to only run this 1 test in both patches

No API for how to handle revisions means no way of knowing if we got all the magical edgecases. Here it is that while even if the entity storage supports revisions, the entity type may not 🤔🤔🤔

Reroll for #2936360: Remove duplicate references to the "layout_builder__layout" field from Layout Builder, no other changes

Manual functional testing of this all checks out, as delineated below. I'm wondering if it would be good to add content_moderation + layout_builder test integration that validates the following:

0. With layout_builder & content_moderation configured for a revisionable entity type...
1. Save a node with no fielded content
2. Save a new layout, with a single 'basic block' inline block, in "draft" state.
3. View the node. Verify the basic block is not present
4. Publish the draft of the node
5. Verify the basic block is present, in the layout section chosen.
6. Revert to the previous revision
7. View the node. Verify the basic block is not present

Great idea @mark_fullmer!

Interdiff is the actual test added to the patch, I had to manipulate it for the FAIL patch to not assume it's a form.

I didn't do #17.6 and #17.7, whoops. Missing some permission, cause there's no revision tab available.

There are still a couple things to be resolved.
First, the local tasks are still there, so there is a duplicate "Save Layout" task that functions differently from the "Save" button (which should likely be renamed).
Second, the form is only for overrides, not for defaults. I think that will be confusing, and make resolving #2917777: Improvements to the styling, grouping, etc. of the Layout Builder UI actions form particularly difficult.
Third, I'm not 100% sold on the workaround for comment forms, but it might be okay...

All in all, it's still ready for a review I think

Switching between users in the test is only testing CM functionality, not anything special here, so I removed it.
Also simplified by using the Powered by Drupal block instead of the block content module.

I tried to switch from node to entity_test_rev but it seems that the Revisions UI and the revert links only exist for nodes?!

Updated IS for remaining tasks

1) Fixed

2) Fair point, but just to be safe I changed the name from layout_form to layout_builder__form, I feel a little better about overwriting it blindly if it's namespaced

3) This is being altered because the initial creation of the route is in the trait which is also used for the defaults (Manage Display).
This should be fixed here as part of #21.2, added an inline comment so we don't forget :)

I think it's more about the divergence of the two UIs being confusing.
In HEAD both have local tasks in the same place for Save/Cancel.
With this patch, defaults will retain those but overrides will instead have form buttons.

I'm going to give this a shot this week, if it's horrible we can figure out a follow-up

I did not get a chance to work on this yet. Unassigning for now, but this is still a priority.
Here's a reroll.

I'm super biased, but I really like this interdiff :D

Okay, not quite as nice.

Going to work on this again, rerolling for now.

Reroll after #3027938: Abstract the contents of LayoutBuilderController into a render element

This makes saving actually use the new buttons, removing the local tasks.

+++ b/core/modules/layout_builder/src/Plugin/Field/FieldWidget/LayoutBuilderWidget.php
@@ -0,0 +1,124 @@
+      $entity->{OverridesSectionStorage::FIELD_NAME} = $temporary_section_storage->getSections();

This was bothering me, so I refactored that whole bit.

Note that this currently incorporates the patch from #3009344: Reloading the Layout Builder while JS is available differs from when JS is not available

Fixing #40, and rerolled

As seen in #2938182-7: Design intuitive affordances for Layout Builder (for illustrating which parts of the page are editable in a given context), and as discussed in #2917777: Improvements to the styling, grouping, etc. of the Layout Builder UI actions form, the buttons should be at the top of the form.

I cleaned up the instantiation of the one-off form display, and documented the new hook.
Also I cleaned up the formatting of the @todo, renamed the forms now that we have two of them, and added trailing commas to the new widget annotation where applicable.

I think this is ready for review.

1. +++ b/core/modules/layout_builder/layout_builder.api.php
   @@ -0,0 +1,29 @@
   +function hook_layout_builder_entity_form_display_alter(\Drupal\Core\Entity\Display\EntityFormDisplayInterface $display) {
   

   Should hook also have "override" in the title some how? \Drupal\layout_builder\Form\DefaultsEntityForm is also an entity form but the alter doesn't happen there.

2. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -54,7 +59,15 @@ function layout_builder_entity_type_alter(array &$entity_types) {
   +  // Ensure every fieldable entity type has a layout form.
   +  foreach ($entity_types as $entity_type) {
   +    if ($entity_type->entityClassImplements(FieldableEntityInterface::class)) {
   +      $entity_type->setFormClass('layout_builder__form', OverridesEntityForm::class);
   +    }
   +  }
   

   Should we only be changing the form class if at least 1 bundle for the entity type has the layout builder enabled and overrides enabled?

   Maybe it doesn't matter since this is new form class key.

   Also why is the $operation parameter here 'layout_builder__form' instead of just 'layout_builder'. Looking at other entity classes in the annotations their form handler keys don't include 'form'.

   Right now this would create form ids like node_article_layout_builder__form_form which if you wanted to alter you would have to use mymodule_form_node_article_layout_builder__form_form_alter.

   Also should override getBaseFormId() others the base form id for all node layout overrides would be just node_form which is the same as all other node operations. So if a module is currently implementing hook_form_BASE_FORM_ID_alter for node_form then they would now be altering this new operation.

   While they probably should be checking operation they may have only been expecting 'edit' and 'add'

(treating each "Also" as new bulleted item)

1) Agreed
2) We can't rebuild entity types when enabling overrides, it's safer to do it here
3) In #25 I switched it from layout_form to layout_builder__form, but I agree layout_builder is enough
4) Fair enough!

Because of the way forms rebuild their entities, we need to resync them as contexts.
Also expanding that part of the test to make it clearer why it was failing.

Here's a version _without_ the revision changes.

I had the right idea but the wrong fix. Also restoring the @todo for the revert link.

#54
1) Ah, yes
2) Similar to #3003666: Provide access to a Section or SectionComponent from within a $form_state, we need this for code outside of LB
3) 🙌🏻
4) Agreed, removed.

+1

#2924201: Resolve random failure in LayoutBuilderTest so that it can be added to HEAD introduced more tests looking for the link that is now a button.

The interdiff looks fine. I'll take it for a quick spin.

1. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -11,6 +11,7 @@
   +use Drupal\Core\Entity\Display\EntityFormDisplayInterface;
   

   unused use. shouldn't need any changes to this file now

2. +++ b/core/modules/layout_builder/src/Form/DefaultsEntityForm.php
   @@ -0,0 +1,129 @@
   +class DefaultsEntityForm extends EntityForm {
   

   Should we have LayoutEntityFormTrait?

   actions() is almost the same

   getSectionStorage() is the same

   buildEntity() could be the same if there was protect field for contextKey

   save() has chunk the same

3. +++ b/core/modules/layout_builder/src/Form/DefaultsEntityForm.php
   @@ -0,0 +1,129 @@
   +   * @var \Drupal\layout_builder\SectionStorageInterface
   +   */
   +  protected $sectionStorage;
   

   Should this actually be DefaultsSectionStorageInterface since the entity forms is made to work with defaults not generic storage?

4. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -0,0 +1,163 @@
   +   * @var \Drupal\layout_builder\SectionStorageInterface
   

   Should this actually be OverridesSectionStorageInterface since the entity forms is made to work with overrides not generic storage?

5. +++ b/core/modules/layout_builder/src/Routing/LayoutBuilderRoutesTrait.php
   @@ -60,22 +62,20 @@ protected function buildLayoutRoutes(RouteCollection $collection, SectionStorage
   +    if ($entity_type_id) {
   +      $main_defaults['_entity_form'] = "$entity_type_id.layout_builder";
   +    }
   +    else {
   +      $main_defaults['_controller'] = '\Drupal\layout_builder\Controller\LayoutBuilderController::layout';
   +    }
   

   We should check here that the entity type has form handler for 'layout_builder' looking at layout_library it passes and entity_type_id but not the entity form. So I think this would fail.

6. The issue summary should mention the new hook under API changes.

7. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutDisplayTest.php
   @@ -38,6 +38,16 @@ protected function setUp() {
   +   * Tests viewing a user.
   +   */
   +  public function testViewUser() {
   

   Why are adding this?

FYI: It errors out on simplytest.me so I didn't test it yet. I have found some patches don't spin up there even though they pass tests. I'm not sure why. I'm checking with @nerdstein on this.

#60
1) Fixed
2) I think they are different enough that a trait right now would be too early of an abstraction
3/4) We don't call any particularly specific methods for either interface, I don't see a reason to get overly specific with it
5) I regret writing this trait. I had to add the logic higher up.
6) Okay
7) Same as #54.4, so I removed it here. It was a test exposing a flaw in a much earlier approach that makes no sense now (had to do with #lazy_builder I think)

Given the architectural impacts of this change, the fact that this impacts how LB will integrate with other modules, and the concerns raised in #3, I think this issue should probably have a framework manager review.

What happened to the comment regression test from back in #11?

Also, those needed screenshots would be helpful for reviewing this. Thanks!

Rebuttal to #3, which was written by a particularly wonderful but misinformed individual:

First, while the primary interactions of Layout Builder UI are indeed not form-based, the benefit of having certain form widgets present (revision log, moderation state) outweighs any of these theoretical concerns.

Second, a decoupled version of this UI would also need to take into account the other fields presented by this new form, again referring to the revision log or moderation state.

Third, my original resistance to this issue was because it was coupling the move to an entity form with the changes needed for translation and revision support, and I was worried that the work would not be done in the "correct" place, aka #2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing
Now that all of the custom translation/revision code has been removed from this issue and that other issue has been progressing, I no longer have this concern.

Finally, the work done in #3027938: Abstract the contents of LayoutBuilderController into a render element made this issue so much cleaner and not nearly as worrisome as it was before.

As far as the comment regression test, I removed that per #54.4
It was only added by me to prove a problem with the initial early code that has since been resolved by multiple other issues, mostly in FieldBlock itself.
It was out of scope here anyway, and no part of this issue could generate a patch to purposefully make that test fail.

What about abandoning the Widget approach and just printing the LB UI directly in the form?
The rest of the changes would stay the same.

I retested the patch and it's failing, which is a relief.
Here's a patch for #66 while I look into #67 more.

On second thought, I think keeping the widget is fine and we can use this workaround with an explicit @todo since that will change eventually.

Also fixed another link-to-button change in a new test.

I think this makes a lot of sense. Only found some random nitpicks.

1. +++ b/core/modules/layout_builder/layout_builder.api.php
   @@ -0,0 +1,29 @@
   +/**
   + * Allows customization of the Layout Builder UI for per-entity overrides.
   + *
   + * @see hook_entity_form_display_alter()
   + * @see \Drupal\layout_builder\Form\OverridesEntityForm::init()
   + */
   +function hook_layout_builder_overrides_entity_form_display_alter(\Drupal\Core\Entity\Display\EntityFormDisplayInterface $display) {
   +  $display->setComponent('moderation_state', [
   +    'type' => 'moderation_state_default',
   +    'weight' => 2,
   +    'settings' => [],
   +  ]);
   +}
   

   Shouldn't $display be LayoutEntityDisplayInterface?

2. +++ b/core/modules/layout_builder/src/Form/DefaultsEntityForm.php
   @@ -0,0 +1,129 @@
   +class DefaultsEntityForm extends EntityForm {
   

   Should this be @internal?

3. +++ b/core/modules/layout_builder/src/Form/DefaultsEntityForm.php
   @@ -0,0 +1,129 @@
   +    return $this->entity->getEntityTypeId() . '_layout_builder_form';
   

   Not really a big deal for this patch, but IMHO we should always use $this->getEntity() in entity forms. Classes should consume their own APIs. But, that said, core is already littered with examples of this anti-pattern so I'll consider it a nitpick and not mention it again in this issue. :)

4. +++ b/core/modules/layout_builder/src/Form/DefaultsEntityForm.php
   @@ -0,0 +1,129 @@
   +    $this->entity = $this->sectionStorage->getContextValue('display');
   

   Can we use $this->setEntity() here?

5. +++ b/core/modules/layout_builder/src/Form/DefaultsEntityForm.php
   @@ -0,0 +1,129 @@
   +    return EntityViewDisplay::load($route_parameters['entity_type_id'] . '.' . $route_parameters['bundle'] . '.' . $route_parameters['view_mode_name']);
   

   Nit: We should probably use $this->entityTypeManager->getStorage()->load() here.

6. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -0,0 +1,169 @@
   +/**
   + * An entity form for serving Layout Builder overrides.
   + */
   

   "Serving"?

7. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -0,0 +1,169 @@
   +class OverridesEntityForm extends ContentEntityForm {
   

   Should be marked explicitly @internal?

8. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -0,0 +1,169 @@
   +    $display = EntityFormDisplay::create([
   

   This should probably use $this->entityTypeManager->getStorage()->create().

9. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -0,0 +1,169 @@
   +    $this->entity = $this->sectionStorage->getContextValue('entity');
   

   Let's use $this->setEntity().

10. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
    @@ -0,0 +1,169 @@
    +    $this->messenger()->addStatus($this->t('The layout override has been saved.'));
    

    I don't know if words "layout override" should be exposed to users. A friendlier message here might simply be "The layout has been saved."

11. +++ b/core/modules/layout_builder/src/Plugin/Field/FieldWidget/LayoutBuilderWidget.php
    @@ -0,0 +1,61 @@
    +class LayoutBuilderWidget extends WidgetBase {
    

    Should this be @internal too?

1) No, LEDI is for entity view. This is a vanilla entity form display.

2) Fixed

3) This is copied straight from the parent class, but I'll humor you :)

4) Same

5) Changed. Could have been worse, the upstream code used entity_get_display() :)

6) Changed to match the other form.

7) Fixed

9) Fixed

10) This string isn't changed in the patch:

-    if ($section_storage instanceof OverridesSectionStorageInterface) {
-      $this->messenger->addMessage($this->t('The layout override has been saved.'));
-    }
-    else {
-      $this->messenger->addMessage($this->t('The layout has been saved.'));
-    }

11) Fixed with the full internal message (the forms don't need that)

Also per discussion with @phenaproxima I added a test implementation of hook_layout_builder_overrides_entity_form_display_alter().
For the FAIL patch I commented out the invocation of that alter.

The changes look excellent. One teensy thing I found in the interdiff, tho:

+++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
@@ -14,7 +14,9 @@
+ * Provides a form containing the Layout Builder UI for overrides/.

That slash probably shouldn't be there. :)

Hah, I'm also missing a `return `.

It's Christmas-ey! RTBC for me once framework managers sign off.

1. +++ b/core/modules/layout_builder/src/Form/DefaultsEntityForm.php
   @@ -0,0 +1,130 @@
   +    $actions = parent::actions($form, $form_state);
   

   Nice, this is a lot better UX than the local tasks were.

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockTestBase.php
   @@ -79,13 +79,11 @@ protected function setUp() {
   +    // Reload the page to prevent random failures.
   +    $this->drupalGet($this->getUrl());
   

   $this->getSession()->reload() (and also ick)

Regarding #3 I assume that @tim.plunkett has had a change of heart on that front as he's been working on this patch?

@tim.plunkett can you confirm?

Leaving the needs FM tag until that point.

Nevermind, @Sam152 pointed me to #65

I think the new hook will probably need a change record. Tagging for that.

Looks good to me!

I know the visual changes aren't specifically the point of the issue, but they're also a UX improvement for several reasons.

Crediting @mark_fullmer and @phenaproxima for reviews that shaped the patch
Crediting @Sam152 for the issue summary and manual testing screenshots

Committed a9a5382 and pushed to 8.7.x. Thanks!

Published change record

After updating the core, existing node layout form which contains a contact form can not save the changes. Nested contact form validation prevents submission. Sorry, if this is a lack of my knowledge.

Can you open a new issue with full steps to reproduce, and link it here? Thanks!

@tim.plunkett
I was able to reproduce #93 and created an issue here: https://www.drupal.org/project/drupal/issues/3046667

I installed Drupal 8.8.0 and it seems that this feature is absent.
I don't see the 'Layout (entity form)' tab which is present on the video.

@Sam152, tell me please, is it possible now to use the 'Layout builder' for customizing layouts of entity forms?
I looked into
/core/modules/layout_builder/layout_builder.api.php
and I don't see the hook from the patch in it.
Thanks.