Layout Builder should be opt-in per display (entity type/bundle/view mode)

Here's the hard part.

replacing every Entity Type's entity_view_display class with its own

That is not true.

It is replacing the entity class of the entity_view_display entity type.
It is global, across the whole site.

In order to accomplish this, we'd have to track which entity types we wanted this functionality for, and split the code path in every method based on that, either doing our own logic or calling parent::

> by replacing every Entity Type's entity_view_display class with its own
I'd like to keep it configurable, maybe there's a way to extend default display with some method like a killswitch?
or add extra plugin collection of classes with fallback to core implementation?

This needs some cache clearing, and some test coverage. But it's a start.

Tested the patch, and it works very fine. The code itself looks good as well.

However, we probably also need to think about the following issue: If you opt-out for a entity display, the form will still display "manage layout" for other view modes than "default". Because of #2907413: Consider supporting Layout Builder Overrides for other view modes and the following lines of code:

// @todo Expand to work for all view modes in
//   https://www.drupal.org/node/2907413.
if ($this->entity->getMode() === 'default') {

I do not have time right now to see what the implications would be the move the disable_defaults part out of there, but pretty sure that would be confusing for users at least.

Patch tested and somewhat works.

Manage Fields UI is still hidden on other view modes except default, and as previously mentioned the manage layout button still appears, it does appear on all view modes including default.

I'm wondering if the text of the checkbox could be a little confusing: "Disable the Layout Builder for this display.", is "display" the right word here? Technically it's being disabled for the current entity/bundle's display, using "display" ambiguously could easily be interpreted to imply that it will disable the layout builder for the current view mode. I suppose this will depend on the outcome of the issue tagged in #6.

More generally thought, would it make more sense for Layout Builder to be Opt-in instead of Opt-Out? I don't think it makes sense for Layout Builder to replace the Field UI across all Entities since if you consider the majority of Content on a typical site, Layout Builder is a bit overkill.

Closed #2950083: Make it possible to opt out to layout builder taking over field ui as a duplicate.

I want to bring up another use case for allowing specific opt-outs: a stupid simple block or media type.

In testing #2948064: Layout Builder should support inline creation of Custom Blocks using entity serialization for my uses, I wanted a simple custom block type which I could use to embed Media entities. I just need a simple configuration and not the full LB experience for this custom block bundle.

In this case LB hurts the site builder / editorial experience because it makes tweaking the media output a more lengthy process.

More generally thought, would it make more sense for Layout Builder to be Opt-in instead of Opt-Out? I

Originally I felt the opposite. But, now, I feel it should be opt-in. Perhaps on installation, it can enable itself for Nodes and Terms. It'd be useful for some custom block content types, but not all. We have to use a lot of content entities in Drupal Commerce (stores, shipping methods, shipments, etc.) A lot of these are embedded in various ways. Layout Builder might hinder the site builder/configuration experience where the simplistic Field UI is enough to swap formatters used when it is embedded elsewhere.

Another reason that opt-in may be preferable (in addition @mglaman's point about simplistic fields) goes back to other two original scenarios described above:

1. A contrib module that enhances Field UI, like Field Group, is used
2. An Entity Type uses a custom entity_view_display class with logic specific to itself

That is: for existing sites that already have a display solution in place, configuration will immediately appear overridden (even if the functionality is preserved); the DX of opting-out for each of these solutions may create a barrier to adoption.

Yep, I also think opt-in is better.

That is: for existing sites that already have a display solution in place, configuration will immediately appear overridden (even if the functionality is preserved); the DX of opting-out for each of these solutions may create a barrier to adoption.

100% agree on this.

Edit:

This types of issues (https://www.drupal.org/project/drupal/issues/2963012) are also a good indication that LB should be opt-in imo.

Here is a rerolled patch from #5 that opts-in if its decided to go that road.

Also want to chime in and say +1 on opt in.

For experienced site builders, I would think that having layout builder "suddenly" enabled would be confusing and probably (for some, in the beginning at least) unwanted.

Just a slightly rewritten patch from #13 with a helper function to reduce duplication.

There are a LOT of problems that will be raised by making this toggleable.

Right now, when you install the module, all of your existing field formatter settings are migrated over.
If you uninstall, you destroy all the data. And you're left with whatever field formatter settings you had before you installed.

If we continue migrating the data over on install, but don't opt-in at first, then if you make any changes to your entity display BEFORE checking the checkbox, those settings are "lost".

And if we make this something that can be turned off, what do we do with the data?
Keep it around, letting it get out of sync?
Remove it, reverting you back to where you were before you started with LB?
And what happens when you turn it back on again?

What is the real reason people would want to opt-out?
There are a couple listed in the issue summary.
If it's about maintaining the existing Field UI for modules like Field Group, then we can keep the runtime but try to retain the Field UI
If it's about maintaining the existing output (aka not nesting anything at all so that complex preprocess/templates continue to work) we can add a layout specifically for that, and use it by default

+1 on opt in as well. Think about existing sites that would like to use this. Upon enabling the module, they would have to edit all entity types that use any contrib modules like display suite, field group, or any module that uses third party settings. An opt-in would allow all existing types to be retained as they were configured without effecting. Now, perhaps NEW entity types/bundles that are created might have an opt-out option. That would have 1 less step for users to configure LB after enabling the module.

Why couldn't the data be migrated at the time the user chooses to "opt-in". This would solve the issue you mention about settings being lost. In my mind I think it would help to migrate the data only if no existing data exists and then retain if the user opts-out. so if a user opt-in, data is migrated. then the user opts out but they could always return to opting in and the last configured state would be visible.

- My 2 cents

As I previously stated I am in favour of opt-in over opt-out, and it seems almost everyone who commented is also in favour so updated the issue summary to replace references to "opt-out" with "opt-in".

Regarding #15 and #17 I am also in favour of a solution where instead of migrating all data on install data is migrated for individual displays when Layout Builder is enabled on them, as mentioend in #17 this would solve any issues menti9oned in #15.

I've also added a couple more issues with not having any opt-in/opt-out that could come up, I won't bother repeating them here so just see my changes linked to this comment above if you're interested.

At the current Layout Builder sprint, we discussed this in-depth.
For existing sites, LB should be turned off for all existing bundles, and will be opt-in.
However! For any new bundles, LB will be turned on by default (aka opt-out).

This gives us the ability to prevent any negative effects for existing sites, while preserving the functionality for new sites.

Additionally, if LB has been turned off, changes to the Field UI can be synced back to it so that it has the most recent info when re-enabling the LB UI.

Still needs tests.

Fixed some tests. Need to check all the checkboxes.

However! For any new bundles, LB will be turned on by default (aka opt-out).

Could you maybe explain more about why this conclusion was reached?

From my perspective this just doesn't make sense, if I look at any site that I build, Layout Builder is too over the top of the majority of entities and bundles, and so I'd end up disabling it for almost everything.

Trying the patch from #21 and i created a new node type. The LB was not enabled by default for the display. I see the third_party_settings on the entity, but I think its missing the `enable_defaults: true` to be turned on.

@pookmish indeed! I noticed the same. This should fix it, with some more tests.
This isn't ready yet, I still need to fix some of the syncing issues.

Fixing the test by reverting changes I made here :)
Still working on this.

Not happy with the implementation yet, but it is finally passing tests!

Bug whack-a-mole.
Removing the reflection, I feel better about this.

Self review time!

Leaving at NR

1. +++ b/core/modules/layout_builder/layout_builder.install
   @@ -21,13 +21,10 @@ function layout_builder_install() {
   +      $display->setEnabled(TRUE, TRUE);
   

   I forgot about this weird TRUE,TRUE. I dislike that.

   Also this method needs to be renamed, too close to enable()

2. +++ b/core/modules/layout_builder/src/Access/LayoutDefaultsEnabledAccessCheck.php
   @@ -0,0 +1,45 @@
   +    $defaults_section_storage = NULL;
   +    if ($section_storage instanceof DefaultsSectionStorageInterface) {
   +      $defaults_section_storage = $section_storage;
   +    }
   +    elseif ($section_storage instanceof OverridesSectionStorageInterface) {
   +      $defaults_section_storage = $section_storage->getDefaultSectionStorage();
   +    }
   

   This is frustrating logic.

3. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -37,10 +38,37 @@ public function isOverridable() {
   +    if ($enabled && !$this->isEnabled() && ($force_sync || !$this->hasSection(0))) {
   

   This needs better docs. And $force_sync is the TRUE,TRUE from above.

4. +++ b/core/modules/layout_builder/src/Form/LayoutBuilderEntityViewDisplayForm.php
   @@ -70,15 +74,31 @@ public function form(array $form, FormStateInterface $form_state) {
   +        '#default_value' => is_bool($is_enabled) ? $is_enabled : TRUE,
   

   This is very odd and either needs to change or be documented (or both)

5. +++ b/core/modules/layout_builder/src/Form/UpdateBlockForm.php
   @@ -50,4 +52,18 @@ protected function submitLabel() {
   +    if ($this->sectionStorage instanceof LegacyDefaultsSectionListInterface && $field_name = $component->get('field_name')) {
   +      $this->sectionStorage->setLegacyComponent($field_name, $configuration['formatter']);
   

   Probably should also check isset($configuration['formatter'])

6. +++ b/core/modules/layout_builder/tests/src/Kernel/LayoutBuilderInstallTest.php
   @@ -33,6 +33,8 @@ public function testCompatibility() {
   +    $this->entity = $this->reloadEntity($this->entity);
   

   This can be removed

1. +++ b/core/modules/layout_builder/config/schema/layout_builder.schema.yml
   @@ -2,6 +2,9 @@ core.entity_view_display.*.*.*.third_party.layout_builder:
   +    enable_defaults:
   +      type: boolean
   +      label: 'Enable the Layout Builder for this display'
   

   enable_defaults key doesn't make sense to me here. Since we added overrides first it would make sense but from the functionality after this patch this setting actually determines if Layout Builder functionality is actually enabled at all for the bundle.

   For instance we get this value via \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::isEnabled() and show the setting the the user as Use Layout Builder.

   So I think this setting should be enabled

2. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -265,14 +304,38 @@ public function setComponent($name, array $options = []) {
   -      $section = $this->getDefaultSection();
   -      $region = isset($options['region']) ? $options['region'] : $section->getDefaultRegion();
   -      $new_component = (new SectionComponent(\Drupal::service('uuid')->generate(), $region, $configuration));
   -      $section->appendComponent($new_component);
   +      if ($existing_component = $this->findComponentByName($name)) {
   +        $existing_component->setConfiguration($configuration);
   +      }
   +      else {
   +        $section = $this->getDefaultSection();
   +        $region = isset($options['region']) ? $options['region'] : $section->getDefaultRegion();
   +        $new_component = (new SectionComponent(\Drupal::service('uuid')->generate(), $region, $configuration, ['field_name' => $name]));
   +        $section->appendComponent($new_component);
   +      }
   

   I understand what this block of code does but not sure why it is in this patch.

   Why didn't we have to check for existing component before this patch?

3. +++ b/core/modules/layout_builder/src/LegacyDefaultsSectionListInterface.php
   @@ -0,0 +1,31 @@
   + * Defines an interface for an object that stores legacy data for defaults.
   

   Should we document why we need this? Explain what "legacy" means in this context?

   Is this as \Drupal\Core\Entity\Entity\EntityViewDisplay expects?

I've been thinking about it and I'm still sceptical that Layout Builder needs to be opt-in by default for all Entities.

See this idea was starting to grow on me, but then I realised after looking through the Core Module List, we have another Core Experimental Module called Field Layout, which is basically a less advanced version of Layout Builder. Looking at that it let's you move fields around and it makes minimal changes to the Field UI so the interface is very clean and simple to use. Layout Builder does accomplish the same task but has a lot more features and the workflow is more advanced and because you jump in and out of the Admin interface feels less clean.

So looking at these two Experimental Modules, one has to ask, why do we have both of them, it feels a bit like they are two competing modules trying to do the same thing.

With that in mind, let's look at common use cases:

1. You want to rearrange some fields on the User Entity Type and make them layout a bit nicer, well Field Layout does the job nicely the UI is minimal and straightforward.
2. You have a Content Type with a few extra fields and want them to display in a grid above the Body field, well again Field Layout is a simpler way to accomplish this.
3. You have a specific Content Type like a front page where you want the layout of each Node to be customisable with the option to add custom blocks, well Layout Builder is an obvious choice here.

Looking at these I think it's clear that the use case of Layout Builder being opt-in by default isn't really needed, it's simply too overkill for most use cases, especially when Field Layout is enabled. Really the only time I can think someone would use Layout Builder instead of Field Layout is when they want to customise the display on a per-entity basis, inserting custom blocks, views and other objects that aren't fields.

I believe it would be more suitable to take the approach of having Field Layout be enabled by default and on for every Display, then have Layout Builder be opt-in, as that approach will satisfy the majority of use cases out there. It will mean you get the flexibility of being able to customise layout the display for every Entity Type with a very clean and easy to use UI, but then for that rare occasion when you need something more you can opt-in to using Layout Builder to add custom blocks, Views and other objects on a per Entity basis. Correct me if I'm wrong but doesn't that sound like an approach which satisfies the majority of real world cases?

Additionally we need to have a clearly defined line between these two modules, Layout Builder is and needs to be the corner case not the de facto standard for managing Field layouts. We need to remember that Layout builder isn't trying to be a replacement for the Field UI, it isn't a module for managing the layout of fields, it's a module for managing the layout of content and pages, and that is what the use case of it is and will be. Layout Builder is trying to replace Panalizer and similar modules, but I'm worried the approach here failos to do that and will just turn people off of it.

My other concern for having Layout Builder manage the layout for every Entity Type Display is that the workflow isn't very good compared with Field Layout, Field Layout just modifies the Admin interface for managing Displays, whereas Layout Builder takes you out of the Admin interface and it's not easy to just jump between the various tabs for managing fields, this makes the workflow feel a bit broken.

It simply doesn't make sense to have two modules in Core that do essentially the same thing but in different ways, so we need to recognise what the different use cases for each module are and not try and force Layout Builder when it is simply overkill, when the workflow and UI is too complicated and confusing for most Entities.

Field Layout is great if you want to arrange the layout of fields, Layout Builder is great when you want to organise the layout of your content. And so that is why Layout Builder shouldn't be opt-in by default, because it's trying to do far more than it should.

With regard to controlling the rendering of an entity, Field Layout can be chalked up as a failed experiment that shouldn't have made it into core.
It's true utility lies in the ability to rearrange the entity form, which should be preserved.

There is more feedback of yours to address, and I will get to it soon.

@tim.plunkett from what I can tell that seems to be just your opinion, as looking at it objectively the reality seems to be that it works well and has active engagement and development on the issue queue.

So Drupal always stands for flexibility ... perhaps ist the right way to find some common default here and make it configurable so that the user can decide for every entity-type on his own if it should be field ui, field layout, layout builder or something else ..

I've just started to explore these two modules and I agree with Aaron. Field layout is very straightforward, easy to use, and isn't much different from the field UI we're already used to using. I like it. Layout Builder is a lot more confusing, although it probably doesn't help that the UX is pretty rough right now (assuming there is work ongoing on this which I haven't looked into...)

The biggest problem for me with Layout Builder is that it's taking over the display of all the entity types and all the view modes I already set up. I have several smaller view modes (e.g. card, teaser) that do not need a layout. But now if I want to rearrange the fields I have to use layout builder. Most of these are actually managed using custom template files, and being forced to use the layout builder to manage field ordering for these view modes is a big problem.

I just want to be able to turn this on for one particularly node type and one particular view mode. That's it. not on by default. And actually I would be happier if I could put a block in using field layout but it doesn't look like I can do that.

#19 tim.plunkett,

Additionally, if LB has been turned off, changes to the Field UI can be synced back to it so that it has the most recent info when re-enabling the LB UI.

This could be very useful. Would it be possible for the traditional field UI to reflect the layout builder regions? This would be similar to how the block layout screen works now.

This would allow the user to select a layout within "Manage Display" then switch to the Field UI where fields can be added and dragged into the correct region and order.

I'd like to +1 #39 concerning view modes.
In a lot of scenario's our front-enders just want data for a given view mode and they will lay-out in code (teasers, cards, etc).
Having layout builder is overkill and an extra barrier.
Also using layout builder for such view modes is a very strange experience since the view mode is presented as 'full' page via the front-end theme during configuration. Most often that is not the way an entity will be shown for such view modes.

An opt-in/out option per display per view mode would be great.

Putting this on needs work, there are some valid outstanding comments on the last patch (#34). The scope of this issue is still not clear from the IS. From what I quickly saw from the patch it seems that the idea is to make it configurable per display per bundle per entity type right?

Could the IS provide screenshots of the current UI of the two systems in question please?

Working on this

I personally much prefer consistency of the Field Layout vs this and having radically different UIs for view and form displays (dated as they are).
What I had hope Layout Builder would become is a tool for building custom/reusable layouts (and just that), and the assigning of content part be handled through Quick Edit (Place Blocks style), instead both things are conflated in one UI (I haven't been following the development since I was content with Field Layout so sorry if I missed the rationale).

@tim.plunkett, this is a blocker on a project we are working on.

Did you make any progress since #44? Otherwise we will try to dedicate some time on it in our next sprint.

This is a version or #32 for 8.5.

Sorry for the lack of updates here.

I hit a roadblock with the last approach, and was exploring a new approach, but that one failed as well...

My new proposal is that we switch LB to opt-in, and warn that any changes made while LB is on will be lost if you opt back out. And then put the opt-out behind a confirm form.
This will mean that while we can migrate the data from the standard field config into LB, we will make no attempt at writing it back to fields when turning off LB. You will get your old field config exactly as it was before you opted in.

I really really personally wanted to make LB opt-out, because I find it annoying when you install a new module and then nothing happens until you find the magic config switch. Except in this case, it wouldn't have changed anything out of the box anyway. You'd still need to have found the right page anyway.

Just to clarify a bit more.

Still needs the confirm form, but here's a lot of the ugly bi-directional tracking code ripped out.

That approach seems to make sense, tested patch seems to work.

NW for the confirmation form.

I fully support #48. In general I agree that installing modules without seeing any changes is confusing, though for this module I think it is fine. Sites might have all kind of entities and taking over layout for all of them might not be something that is wanted, or even safe. We might just end up breaking their existing layout and styles. I also think a more careful opt-in approach might encourage existing sites to start using it on new type of content they might have, giving us a quicker adoption and more early feedback and bug reports.

If anyone would like to they could write a example/default-LB-config-module that enable LB on all entities. BTW, thanks for all you hard work on this module!

Hit too many weird spots where our "enabled" clashed with the ConfigEntity concept of "enabled". So, renamed our new one to "active" (methods activate()/deactivate())

This adds the confirmation form!

Ready for reviews.

Fixed recently added test.

Didn't read the tests yet, but this patch seems to make a great deal of overarching sense. I have some questions, but they're probably all just the lunatic ravings of a paranoid mind :)

1. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -24,3 +27,17 @@ function layout_builder_post_update_rebuild_plugin_dependencies(&$sandbox = NULL
   +function layout_builder_post_update_activate_existing(&$sandbox = NULL) {
   

   Nit: $sandbox is not used.

2. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -24,3 +27,17 @@ function layout_builder_post_update_rebuild_plugin_dependencies(&$sandbox = NULL
   +    if ($display->getThirdPartySettings('layout_builder')) {
   

   Not a big deal, but if (in_array('layout_builder', $display->getThirdPartyProviders(), TRUE)) might be more "correct" here.

3. +++ b/core/modules/layout_builder/src/Access/LayoutDefaultsActiveAccessCheck.php
   @@ -0,0 +1,45 @@
   +    $defaults_section_storage = NULL;
   +    if ($section_storage instanceof DefaultsSectionStorageInterface) {
   +      $defaults_section_storage = $section_storage;
   +    }
   +    elseif ($section_storage instanceof OverridesSectionStorageInterface) {
   +      $defaults_section_storage = $section_storage->getDefaultSectionStorage();
   +    }
   +    if ($defaults_section_storage) {
   +      $access = AccessResult::allowedIf($defaults_section_storage->isActive());
   +      $access->addCacheableDependency($defaults_section_storage);
   +    }
   

   Doesn't this logic essentially force all section storages to use either DefaultsSectionStorageInterface or OverridesSectionStorageInterface? What if a storage plugin wants to use neither? Is that even allowed? This seems like an API limitation; is it documented anywhere?

4. +++ b/core/modules/layout_builder/src/DefaultsSectionStorageInterface.php
   @@ -32,4 +32,26 @@ public function isOverridable();
   +  /**
   +   * Determines if Layout Builder is active for the defaults.
   +   *
   +   * @return bool
   +   *   TRUE if Layout Builder is active, FALSE otherwise.
   +   */
   +  public function isActive();
   +
   +  /**
   +   * Activates the Layout Builder for the defaults.
   +   *
   +   * @return $this
   +   */
   +  public function activate();
   +
   +  /**
   +   * Deactivates the Layout Builder for the defaults.
   +   *
   +   * @return $this
   +   */
   +  public function deactivate();
   

   Not sure if this has already been bike-shedded, but activate() and deactivate() might be a bit ambiguous if this interface is composited with other interfaces that provide similar functionality. How about renaming these to isLayoutActive(), activateLayout(), and deactivateLayout(), to avoid confusion?

5. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -37,10 +37,52 @@ public function isOverridable() {
   +      foreach ($components as $name => $component) {
   +        $this->setComponent($name, $component);
   +      }
   

   Can we get a comment as to why this is being done, and what it will accomplish?

6. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -37,10 +37,52 @@ public function isOverridable() {
   +      while ($this->count()) {
   

   Nit: Maybe use while (count($this) > 0) to keep the abstraction sealed, and make the condition more explicit?

7. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -37,10 +37,52 @@ public function isOverridable() {
   +    $this->setThirdPartySetting('layout_builder', 'is_active', FALSE);
   +    return $this;
   

   Nit: You can return from $this->setThirdPartySetting(), which already returns $this.

8. +++ b/core/modules/layout_builder/src/Entity/LayoutEntityDisplayInterface.php
   @@ -33,4 +33,26 @@ public function isOverridable();
   +  /**
   +   * Determines if Layout Builder is active for this display.
   +   *
   +   * @return bool
   +   *   TRUE if Layout Builder is active, FALSE otherwise.
   +   */
   +  public function isActive();
   +
   +  /**
   +   * Activates the Layout Builder for this display.
   +   *
   +   * @return $this
   +   */
   +  public function activate();
   +
   +  /**
   +   * Deactivates the Layout Builder for this display.
   +   *
   +   * @return $this
   +   */
   +  public function deactivate();
   

   This is a repeat of code in DefaultsSectionStorageInterface, which suggests that it should be its own internal interface.

9. +++ b/core/modules/layout_builder/src/Form/LayoutBuilderDeactivateForm.php
   @@ -0,0 +1,113 @@
   +  /**
   +   * The messenger service.
   +   *
   +   * @var \Drupal\Core\Messenger\MessengerInterface
   +   */
   +  protected $messenger;
   

   FormBase uses MessengerTrait, so we don't need to define this member.

10. +++ b/core/modules/layout_builder/src/Form/LayoutBuilderDeactivateForm.php
    @@ -0,0 +1,113 @@
    +    $this->messenger = $messenger;
    

    Use $this->setMessenger().

11. +++ b/core/modules/layout_builder/src/Form/LayoutBuilderEntityViewDisplayForm.php
    @@ -46,10 +46,19 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
    +      $key = array_search('layout_builder__layout', $form['#fields'], TRUE);
    +      if (is_int($key)) {
    +        unset($form['#fields'][$key]);
    +      }
    

    Why not just use array_diff()?

12. +++ b/core/modules/layout_builder/src/Form/LayoutBuilderEntityViewDisplayForm.php
    @@ -57,18 +66,26 @@ public function form(array $form, FormStateInterface $form_state) {
    +    $form['#entity_builders']['layout_builder'] = '::entityFormEntityBuild';
    

    Clever!!

13. +++ b/core/modules/layout_builder/src/SectionStorageInterface.php
    @@ -88,10 +88,13 @@ public function getRedirectUrl();
    +   * @param string $rel
    +   *   The link relationship type, for example: view or deactivate.
    

    Needs to mention that the parameter is optional and defaults to 'view'.

1) $sandbox is absolutely used, it's the first argument passed to \Drupal\Core\Config\Entity\ConfigEntityUpdater::update().

2) The current code will return TRUE only if there are any settings. The suggested code will return TRUE if the LB settings are empty, which is not desired.

3) You're completely right. This should delegate directly to the section storage. Made SSI implement AccessibleInterface

4) Agreed, renamed. And switched back to enable per discussion in Slack

5) Added a comment

6) Good call!

7) Technically true, but I'm going to leave it as-is to match the other method

8) Done as part of 4

9/10) Fair enough

11) Good idea. Added a comment as well

13) Fixed

+++ b/core/modules/layout_builder/layout_builder.install
@@ -20,21 +22,19 @@ function layout_builder_install() {
+      $display
+        ->appendSection(new Section($field_layout['id'], $field_layout['settings']))
+        ->enableLayoutBuilder()
+        ->save();

+++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
@@ -37,10 +37,65 @@ public function isOverridable() {
+  public function enableLayoutBuilder() {
+    // Initialize Layout Builder if it is being enabled for the first time.
+    if (!$this->isLayoutBuilderEnabled()) {
+      $this->initializeLayoutBuilder();
+    }

This is the only part that gives me pause.

When you enable Layout Builder, it loops through all of the existing fields and makes corresponding section components for each.

It only does this when enabling LB if LB wasn't already enabled.

But should this instead be a manual step?

Then the install code would read

      $display
        ->enableLayoutBuilder()
        ->appendSection(new Section($field_layout['id'], $field_layout['settings']))
        ->copyExistingFields() // or something
        ->save();

Or, we just leave it as-is.

Agreed in Slack that making things explicit as suggested in #60 was a good approach.

Looks good to me!

Tested with LB already enabled:

• Applied patch
• Ran db updates
• Confirmed no change to layouts on view modes
• Option to opt-out per view mode available and working
• Disabling LB in "default" view mode does not remove enabled LB on other view modes, and no layout settings lost

+++ b/core/modules/layout_builder/layout_builder.install
@@ -23,8 +23,9 @@ function layout_builder_install() {
       $display
-        ->appendSection(new Section($field_layout['id'], $field_layout['settings']))
         ->enableLayoutBuilder()
+        ->appendSection(new Section($field_layout['id'], $field_layout['settings']))
+        ->addComponentsForExistingFields()
         ->save();

This change was the right idea, but done wrong.
When you call enableLayoutBuilder should not be so magic.
Because enableLayoutBuilder should not be magic, it should be a clean setter.
In order to achieve that, it should work the same as setOverridable, which has no logic...

Because the logic lives in preSave()! Which is correct.

Moving to that cleans things up a bunch.
For reference, here's the diff against the patch from #59

     if (isset($field_layout['id'])) {
       $field_layout += ['settings' => []];
       $display
-        ->appendSection(new Section($field_layout['id'], $field_layout['settings']))
         ->enableLayoutBuilder()
+        ->appendSection(new Section($field_layout['id'], $field_layout['settings']))
         ->save();
       $display_changed = TRUE;
     }

Also fixing a bug around the fields being added multiple times, and added corresponding test coverage.

Reroll because of #2932462: Add EntityContextDefinition for the 80% use case

Found a small bug.

1. Apply patch.
2. Enable layout builder
3. Goto admin/structure/types/manage/page/display/default
4. Click "Use Layout Builder"
5. The option "Allow each content item to have its layout customized." shows up
6. Click this option too
7. Click Save
8. "Your settings have been saved." shows
9. "Allow each content item to have its layout customized." is no longer checked.

Not sure if that option shouldn't show up until you have save once or if you should be able select both options in the single step.

Sounds like test coverage is missing.

We can't test that correctly until Nightwatch is up to speed more.
Fixed.

Okay, @phenaproxima insisted on a test, and I'm glad he did. Found a bug that I'd fixed before but accidentally reverted locally.

1. +++ b/core/modules/layout_builder/layout_builder.routing.yml
   @@ -68,6 +73,7 @@ layout_builder.add_block:
   @@ -80,6 +86,7 @@ layout_builder.update_block:
   
   @@ -80,6 +86,7 @@ layout_builder.update_block:
        _form: '\Drupal\layout_builder\Form\UpdateBlockForm'
      requirements:
        _permission: 'configure any layout'
   +    _layout_builder_access: 'view'
   

   This route will actually do an edit operation, right? Therefore, should the operation given to _layout_builder_access be 'edit' or 'update' or something, rather than 'view' for everything? Probably something we can address in a follow-up, but I thought I'd mention it here.

2. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -74,6 +98,27 @@ public function preSave(EntityStorageInterface $storage) {
   +    $original_enabled_value = isset($this->original) ? $this->original->isLayoutBuilderEnabled() : FALSE;
   +    $new_enabled_value = $this->isLayoutBuilderEnabled();
   

   For clarity, can we rename these values to $was_enabled and $is_enabled?

3. +++ b/core/modules/layout_builder/src/Form/LayoutBuilderEntityViewDisplayForm.php
   @@ -46,10 +47,17 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
   +      $form['#fields'] = array_diff($form['#fields'], ['layout_builder__layout']);
   

   This leaks knowledge of the name of the Layout Builder field. Can we maybe set a constant on the Overrides plugin class which we can refer to instead? (Follow-up is fine for that if needed.)

4. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderOptInTest.php
   @@ -0,0 +1,184 @@
   +  public static $modules = [
   

   I think this can be protected.

5. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderOptInTest.php
   @@ -0,0 +1,184 @@
   +    $page->checkField('layout[is_enabled]');
   

   Nit: I like $assert_session->fieldExists('layout[is_enabled]')->check(), since it actually asserts that the field exists before trying to check it, but that's NBD.

6. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderOptInTest.php
   @@ -0,0 +1,184 @@
   +    $page->pressButton('Save');
   

   Same here: we could use the $assert_session->buttonExists('Save')->press() form.

1) Follow-up exists already #2914486: Add granular permissions to the Layout Builder

2) Switching to $already_enabled and $set_enabled, as in the form

3) Already exists in this class, and already has a follow-up #2936360: Remove duplicate references to the "layout_builder__layout" field from Layout Builder

4) Fixed

5/6) Those will fail with their own intelligent failures. I think clickLink() is the only non-helpful one I check for first.

I think we're good here. Thanks, @tim.plunkett!

@alexpott in Slack asked why we are relying on 403s for routes when LB isn't enabled for a certain display.
This is because the routes are built per-entity_type, and the access is per-display.

Just calling it out here, so this doesn't get bounced back.

I had seen this discussion in Slack, and got to thinking today. If in two years from now, some contrib module author creates a new "page-builderish template builder with a UI," does this issue create a useful pattern for choosing which one of three or five manage-display systems the site-builder wants to use for a given view mode of a given bundle?

In other words, I'm advocating for scalability in assigning a display-controlling module in the future if it doesn't add too much complexity now. And maybe we have that already. I don't know; that's why I'm asking the question.

I have manually tested the patch. I already had Layout Builder enabled and have done changes to different displays already. After applying patch and running updates I can notice a lot of displays has been changed when exporting configuration (kinda to be expected since we're adding is_enabled), however it has also added components to displays that I have already configured as I'd like them.

The reason is:

+++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
@@ -74,6 +98,27 @@ public function preSave(EntityStorageInterface $storage) {
+    $already_enabled = isset($this->original) ? $this->original->isLayoutBuilderEnabled() : FALSE;
+    $set_enabled = $this->isLayoutBuilderEnabled();
+    if ($already_enabled !== $set_enabled) {
+      if ($set_enabled) {
+        // Loop through all existing field-based components and add them as
+        // section-based components.
+        $components = $this->getComponents();
+        // Sort the components by weight.

I am wondering if we can by bypass this during the post update, since if the display is already enabled, you might have configured it already. It also looks like it assumes all displays are to be enabled which I'm fine with in the post hook.

Good point!
Switched from using the entity API to the config API to bypass that.
And wrote a full update path test to prove it fixes what you described.

With this patch the diff on the exported config looks accurate. Interdiff also looks good!

Rerolled to avoid clashing with the update path committed in #2953656: No ability to control "extra fields" with Layout Builder

Looks like #82 fails to apply on 8.6.0-alpha1. #79 however applies cleanly... is there any concern with that?

I had to reroll because of an issue committed to 8.6.x that was not in 8.6.0-alpha1
This is fine.

Hush, RTBC runner...

1. +++ b/core/modules/layout_builder/config/schema/layout_builder.schema.yml
   @@ -2,6 +2,9 @@ core.entity_view_display.*.*.*.third_party.layout_builder:
   +    is_enabled:
   

   Let's change this to enabled - this would introduce the first is_enabled config key in core. We have view displays, *.links.menu.yml as things with an enabled key already.

2. +++ b/core/modules/layout_builder/layout_builder.install
   @@ -20,21 +22,43 @@ function layout_builder_install() {
   +/**
   + * Enable Layout Builder for existing entity displays.
   + */
   +function layout_builder_update_8601(&$sandbox) {
   +  $config_factory = \Drupal::configFactory();
   

   Nice to see a beta stability experimental module get an upgrade path.

3. +++ b/core/modules/layout_builder/src/Form/LayoutBuilderEntityViewDisplayForm.php
   @@ -112,26 +140,58 @@ protected function hasOverrides(LayoutEntityDisplayInterface $display) {
      /**
       * {@inheritdoc}
       */
      protected function buildFieldRow(FieldDefinitionInterface $field_definition, array $form, FormStateInterface $form_state) {
   -    // Intentionally empty.
   +    if (!$this->entity->isLayoutBuilderEnabled() && $field_definition->getType() !== 'layout_section') {
   +      return parent::buildFieldRow($field_definition, $form, $form_state);
   +    }
      }
    
      /**
       * {@inheritdoc}
       */
      protected function buildExtraFieldRow($field_id, $extra_field) {
   -    // Intentionally empty.
   +    if (!$this->entity->isLayoutBuilderEnabled()) {
   +      return parent::buildExtraFieldRow($field_id, $extra_field);
   +    }
      }
    
   

   There methods are documented to return an array. In HEAD they already return NULL - can we get a follow-up to either fix the docs or the implementation.

4. Can we get an issue to fix \Drupal\Tests\layout_builder\Kernel\LayoutBuilderCompatibilityTestBase::assertFieldAttributes() - it claims to return something but it doesn't.

1) Changed

2) 🙌🏻

3) Fixed this here and now

4) Not touched by this issue. But opened #2987893: Remove @return docs on \Drupal\Tests\layout_builder\Kernel\LayoutBuilderCompatibilityTestBase::assertFieldAttributes()

Additionally, had to fix the test added by #2986033: [regression] The BC layer for EntityContextDefinition in ContextDefinition is incomplete to account for enabling LB

Leaving at RTBC

+++ b/core/modules/layout_builder/src/Form/LayoutBuilderEntityViewDisplayForm.php
@@ -180,18 +180,22 @@ public function entityFormEntityBuild($entity_type_id, LayoutEntityDisplayInterf
-    if (!$this->entity->isLayoutBuilderEnabled() && $field_definition->getType() !== 'layout_section') {
-      return parent::buildFieldRow($field_definition, $form, $form_state);
+    if ($this->entity->isLayoutBuilderEnabled() || $field_definition->getType() === 'layout_section') {
+      return [];
     }
...
-    if (!$this->entity->isLayoutBuilderEnabled()) {
-      return parent::buildExtraFieldRow($field_id, $extra_field);
+    if ($this->entity->isLayoutBuilderEnabled()) {
+      return [];
     }

Swapping the logic around here makes this easy to reason about - nice! I.e. now it's easy to see - if layout builder is enabled these thing do nothing. Thanks for addressing that here.

Changes look good to me; +1 from me on still-RTBC.

Adding credit

Thank you for assigning credit @tim.plunkett - makes it much easier for a committer!

Committed c6cdc43 and pushed to 8.7.x. Thanks!
Committed 8514539 and pushed to 8.6.x. Thanks!

Great job, thanks. This is huge from a practical standpoint.

Thanks @alexpott!

Hello, I'm trying to apply this patch on 8.5.5, I've noticed a lot of changes between branch 8.5.5 and 8.6.x, so I tried old patches, patch #64 fixed this issue for 8.5.5 branch, I have re-rolled patch #64 we can use it temporarily until brunch 8.6.x released. Thanks

Increased the priority of #2955065: Customized layout does not display for node with customizations enabled when using full content view mode because this got in.