Defining a new type of section storage relies on magic strings and hidden assumptions

We'd want to do this before beta

To me, a StorageInterface in core should always define CRUD methods that take in an ID where appropriate. This is consistent with almost every *StorageInterface.php file I've found. Following that, I would expect that a SectionStorageInterface is something that can at least load, save, and delete a Section or list of Sections based on an ID. Instead, as #4 outlines, StorageInterface does quite a few things that are mostly unrelated to normal *StorageInterface operations.

With that, I would suggest renaming the interface to something like SectionListInterface, which is consistent with the getSections/getSection/appendSection/insertSection methods. The other methods are tricky, here are my thoughts on them:

1. getContexts - Shouldn't a section be provided contexts by the thing using them? I wouldn't expect that the thing that stores/lists sections to also be capable of building its own contexts.
2. getStorageId/getStorageType - Could I ever use there IDs to generically load any section storage? For example, could I get the section storage for an entity with only this data (i.e. $something->load('entity:node:1')->getSections()? From what I understand, these are only used to determine a unique tempstore ID, but can't be used to perform CRUD operations on the storage from another place.
3. label/save - These make sense, but seeing the save method being implemented in LayoutSectionItemList is a little scary. What about creating a new revision, forward revisions, validation, etc.? Should a field ever have a method that updates the parent entity?
4. getCanonicalUrl/getLayoutBuilderUrl - I would expect a method like this to be provided by a handler, not a storage. Should a storage be aware of its user interface?

Splitting these methods up is hard, but here's a hypothetical setup:

1. The LayoutSectionItemList field should keep the getSections/getSection/appendSection/insertSection methods.
2. Create a new service that deals with section storage generically, which can perform CRUD operations on any storage based on a type and ID.
   User interfaces can then be written generically and invoke the service like $service->load('entity:node:1')->insertSection(...)->save(). The service would know to load Node 1 and pass relevant data to the LayoutSectionItemList field.
3. There should be a new service or an addition to the above that knows how to provide contexts to sections based on the current entity.

I find the naming between LayoutBuilderRoutes and LayoutBuilderRoutesTrait to be confusing. One is an event subscriber and the other is a trait the plugins (which the event subscriber delegates to) happen to us. And that extra layer of abstraction between the two makes drawing a line between them sort of weird. Maybe I'm being overly nitpicky here.

1. +++ b/core/modules/layout_builder/src/SectionStorage/SectionStorageManager.php
   @@ -0,0 +1,64 @@
   +  public function loadEmpty($id) {
   +    return $this->createInstance($id);
   +  }
   

   ?? is this like... necessary rather than just calling createInstance()?

2. +++ b/core/modules/layout_builder/src/SectionStorage/SectionStorageManager.php
   @@ -0,0 +1,64 @@
   +  public function loadFromRoute($id, $value, $definition, $name, array $defaults) {
   +    $plugin = $this->createInstance($id);
   +    if ($section_list = $plugin->convert($value, $definition, $name, $defaults)) {
   +      return $plugin->setSectionList($section_list);
   +    }
   +  }
   

   ok, so forgive my complete craziness, but when I see this method I expect to see loadFromRoute(Route $route). I know the ID is from the route defaults. Feels weird to parse it out before it gets to this method. Is that nuts to say?

I'll review for strings and more detail shortly.

Eclipse

1. +++ b/core/modules/layout_builder/src/SectionStorageInterface.php
   @@ -95,7 +31,71 @@ public function getStorageId();
   +  public function convert($value, $definition, $name, array $defaults);
   

   Not sure of the naming of this method, I realise its a param converter in disguise, but perhaps we can find a name that means more in the domain of layout builder.

2. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutSectionTest.php
   @@ -169,10 +169,11 @@ public function providerTestLayoutSectionFormatter() {
   -    $this->drupalGet($node->toUrl('layout-builder'));
   +    $this->drupalGet($canonical_url->toString() . '/layout');
   

   Can you elaborate a bit on why this has changed? I assume it is because of
   

   (why should the field or config entity care how the URL parameters for the Layout Builder UI are structured?)

   but its not obvious

We'd want to do this before beta

and then some!

?? is this like... necessary rather than just calling createInstance()?

I originally was going to say the same thing, but then realised that `loadEmpty` is the correct naming in the context of a layout domain, while `createInstance` is a plugin-api naming.

I think it is fine to name the methods in the context of the domain.

I think it is fine to name the methods in the context of the domain.

FWIW, I completely agree, I just think a pure passthrough method is sort of weird. Not a hill I care to go die on, and Tim's reasoning seems sound.

Eclipse

So thinking about a use case for another section type I came up with the following scenario.

• A module called 'layout library'
• Module defines a config entity which defines a layout
• Admin/site builder builds up a suite of N approved layouts with each layout being associated with a given content entity type and bundle
• Admin/site builder configure M node-types to have a ER field allowing a reference to a layout library config entity
• Content editors can pick layout from edit form
• Special form mode for content editors that shows live preview as they change layout (like the change view mode form in the header on node preview) - saving sets the value of that field.

How would that look here?

• the config entity would implement SectionListInterface
• getStorageId would return the ID of the config entity
• getStorageType would return 'layout_library'
• setSectionList would defer to the base handler after checking the passed argument was an instance of the config entity
• alterRoutes would provide a route to add a new layout library entry for each enabled content entity type + bundle
• not sure what getCanonicalUrl would do
• getLayoutBuilderUrl would redirect to editing in the library for the given content entity type + bundle
• convert would do the job of upcasting as per the others (loading the config entity). Although this may be possible with existing entity converters
• getContexts would do the job of creating a sample entity context based on the given content entity type + bundle
• label would return the name defined in the library
• save would save the config entity

So I think this is a pretty good fit, just wondering how/where getCanonicalUrl fits

PS, when this goes beta, I might actually build such a solution.

Whilst I like the idea of content-editors being able to customise the layout of their content entity, I also like the structure this provides to keep them on the rails.

1. +++ b/core/modules/layout_builder/src/SectionStorage/SectionStorageManager.php
   @@ -0,0 +1,64 @@
   +    $plugin = $this->createInstance($id);
   +    if ($section_list = $plugin->convert($value, $definition, $name, $defaults)) {
   +      return $plugin->setSectionList($section_list);
   +    }
   

   missing return point? also means missing a test for when no section list

2. +++ b/core/modules/layout_builder/tests/src/Unit/DefaultsSectionStorageTest.php
   @@ -50,15 +51,15 @@ public function testConvert($success, $expected_entity_id, $value, array $defaul
   +    $result = $this->plugin->convert($value, [], 'the_parameter_name', $defaults);
   
   @@ -124,10 +125,10 @@ public function testConvertCreate() {
   +    $result = $this->plugin->convert($value, [], 'the_parameter_name', [], 'the_parameter_name', []);
   

   One of these has 6 args, the other has 4. I think 4 is the right amount.

This is a review of 21. I'll post subsequent reviews of interdiffs. I was fairly picky with this and even gave the regular text strings a pretty good looking at during the review just to make sure we didn't have obvious typos or other english-weirdness.

1. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/SectionStorageBase.php
   @@ -0,0 +1,90 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getSections() {
   +    return $this->sectionList->getSections();
   +  }
   

   Ok, so I mentioned this to Tim on chat already, but both of the actual plugin classes have proxy methods they use instead of going directly to $this->sectionList. That's beneficial for a number of reasons but especially since we can throw a sane exception there if there is no SectionList object present (since setSectionList() is sort of like a views plugin's init() method). Subsequent calls to methods of the SectionListInterface could in theory be called on a null value, which throws an error to the effect of "called getSections() method on NULL object". That would be especially confusing in the case of this method since it proxies to a method of the exact same name. I suspect Tim will have a fix for this before I even finish this review, but I'm including it for completeness on this patch.

2. +++ b/core/modules/layout_builder/src/SectionStorage/SectionStorageManager.php
   @@ -0,0 +1,64 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function loadEmpty($id) {
   +    return $this->createInstance($id);
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function loadFromSectionList($id, SectionListInterface $section_list) {
   +    return $this->createInstance($id)->setSectionList($section_list);
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function loadFromRoute($id, $value, $definition, $name, array $defaults) {
   +    $plugin = $this->createInstance($id);
   +    if ($section_list = $plugin->convert($value, $definition, $name, $defaults)) {
   +      return $plugin->setSectionList($section_list);
   +    }
   +  }
   

   ok, I still think this is totally weird. That being said, there's a certain degree of "I like this" going on too. In a perfect world, we'd overwrite createInstance() with some additional logic that could tell if the createInstance method were called internally or externally and would throw an exception for externally sourced calls that gave some reasoning for using these methods instead. Still that is likely a huge degree of overkill and I DO really like how clean and utilitarian these methods are.

3. +++ b/core/modules/layout_builder/src/SectionStorage/SectionStorageTrait.php
   @@ -0,0 +1,114 @@
   +  /**
   +   * Indicates if there is a section at the specified delta.
   +   *
   +   * @param int $delta
   +   *   The delta of the section.
   +   *
   +   * @return bool
   +   *   TRUE if there is a section for this delta, FALSE otherwise.
   +   */
   +  protected function hasSection($delta) {
   +    return isset($this->getSections()[$delta]);
   +  }
   

   Tim and I discussed this at length. We're both generally in favor of exposing "hasThingy" methods, but Tim chose not to in this case because hasSection(7) seems sort of ambiguous and unhelpful. I agree completely, and I'm just documenting it for future reviewers.

Overall, this patch moves a lot of things around to organize them better, introduces a new plugin system and puts almost all the domain knowledge for introducing a new layout builder use case into that plugin. What is left is fairly standard sorts of things (like local tasks) which are considered temporary UI solutions by the layout builder team for our UI. All in all, I'm very happy with the state of this patch and it moves the LB code base in a really good direction without sacrificing any flexibility and instead really improves our DX.

Eclipse

+++ b/core/modules/layout_builder/src/Routing/LayoutBuilderRoutesTrait.php
@@ -50,6 +50,13 @@ protected function buildRoute(RouteCollection $collection, SectionStorageDefinit
+    if ($route_name_prefix) {
+      $route_name_prefix = "layout_builder.$type.$route_name_prefix";
+    }
+    else {
+      $route_name_prefix = "layout_builder.$type";
+    }
+

I think this makes a lot of sense. There are a few places where we just might not need an additional prefix, so ++ to this change.

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/DefaultsSectionStorage.php
@@ -0,0 +1,260 @@
+  /**
+   * Gets the entity storing the overrides.
+   *
+   * @return \Drupal\layout_builder\Entity\LayoutEntityDisplayInterface
+   *   The entity storing the defaults.
+   */
+  protected function getDisplay() {
+    return $this->sectionList;
+  }

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -0,0 +1,234 @@
+  /**
+   * Gets the entity storing the overrides.
+   *
+   * @return \Drupal\Core\Entity\FieldableEntityInterface
+   *   The entity storing the overrides.
+   */
+  protected function getEntity() {
+    return $this->sectionList->getEntity();
+  }

Would it make sense for both of these methods to call to $this->getSectionList() so that they and their methods also benefit from the exception that method can throw?

Eclipse

So I've spent a bit of time looking at #2924058: Discuss using Layout Builder to control full site layout and replace Block UI which has an implementation for both before and after this patch and I think it's telling. Tim's gone the extra mile, so there's some non-essential-to-this-review stuff in that patch, so I'm going to highlight what is essential for the sake of this patch.

Routing

I'm comparing the patches provided in posts 2 and 13. The first real section of note is the LayoutBuilderRoutes class. The patch in 2 actually edits this class, which is something core modules can get away with, but a contrib module wishing to implement a Layout Builder solution would have to provide a whole new event subscriber, so that's an entry in the services.yml file and a php class, just to get at the route alterations which need to happen.

The patch in this issue removes that need and is illustrated by the patch on comment 13 of #2924058: Discuss using Layout Builder to control full site layout and replace Block UI because there is an alterRoutes() method on the plugin type which was implemented.

Parameter conversion/upcasting

In the patch on 2, Tim had to provide a custom class to do parameter upcasting. This means another entry in the ole services.yml file and a corresponding php class. After this issue lands, the patch in 13 illustrates again that this is delegated to a method of the plugin which again simplifies the dx of building one of these implementation.

Section Storage

The section storage system as it stands in core currently revolves around a magic naming convention in the services.yml file. This puts us up to three services.yml entries and 3 corresponding php files just to setup your own implementation of layout builder. Again, after this patch, we're still just creating the plugin class.

There are of course a few things this doesn't resolve (like the local task deriver) but I've explained all of those in previous reviews. This patch radically improves the dx of using layout builder, and moves the whole of this code base in a clearly better and cleaner direction. I've looked over this code pretty carefully and fell good about rtbcing it pending tests passing.

Eclipse

re-affirming testbot is weird and my rtbc.

Patch introduces more changes that described, also queued tests for other php versions

1. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/DefaultsSectionStorage.php
   @@ -196,25 +196,34 @@ protected function getEntityTypes() {
   +      list($entity_type_id, $bundle, $view_mode) = explode('.', $id);
   

   nit, should we use the third arg here, passing 3?

2. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -108,22 +108,30 @@ public function getStorageId() {
   +      list($entity_type_id, $entity_id) = explode(':', $id);
   

   same deal here

changes look good in my book

Assuming this passes tests, I am completely on board with this further detangling of the dubiously named convert() method into two new methods that actually say what they do. RTBC pending passing tests.

Eclipse

re-affirming rtbc pending passing tests.

Eclipse

Needed re-roll after #2914484: Prevent the layout field from being removed if overrides exist, testing locally

Took for a manual test, no issues. Tested on block content and node.

Couple of things I picked up so far, going through the process of building the module described in #16 to really put this through its paces

1. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -28,6 +28,15 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getTargetEntityType() {
   +    return $this->entityTypeManager()->getDefinition($this->getTargetEntityTypeId());
   +  }
   

   I'm not sure we need this to be public. The base class already has \Drupal\Core\Entity\EntityDisplayBase::getTargetEntityTypeId

   There are three calls:

   - one from default storage - which is a plugin that can inject the entity type manager (and already does)
   - one from the form, which already has the entity type manager injected
   - one from self, which obviously can still be called if it is made protected

   Feels like unnecessary API surface that can be removed

2. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -258,29 +176,7 @@ public function buildMultiple(array $entities) {
   -  protected function getSampleEntity($entity_type_id, $bundle_id) {
   +  public function getSampleEntity($entity_type_id, $bundle_id) {
   

   I don't think this API belongs on this object. If another module needs it, it can't call it without instantiating the display, but there is no use of $this, so the display is not needed.

   This whole method can be removed from the interface and moved to a standalone service in my opinion. Again, smaller API surface for the view display (albeit moved elsewhere)

   Then the only usage (in the section storage) can have the service injected via CFPI.

   And other modules can use it.

+++ b/core/modules/layout_builder/src/SectionStorageInterface.php
@@ -2,100 +2,118 @@
+  public function getCanonicalUrl();

When implementing this, it took me a while to work out what this method is for.

It is used to redirect the user back to somewhere after saving the layout.

So I think perhaps getRedirectUrl might be a more intuitive name?

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/DefaultsSectionStorage.php
@@ -0,0 +1,281 @@
+  /**
+   * {@inheritdoc}
+   */
+  protected function getRouteParameters() {

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -0,0 +1,242 @@
+  /**
+   * {@inheritdoc}
+   */
+  protected function getRouteParameters() {

we need real docs here, there is no parent method for the {@inheritdoc}

OK, I think I've completed the process of implementing one of these plugins, my observations are as above comments.

I think it works well, the alterRoutes still feels a bit light on for docs, that was probably the most confusing/difficult bit to implement/understand - this is what we have

/**
   * Alters the route collection during route building.
   *
   * @param \Symfony\Component\Routing\RouteCollection $collection
   *   The route collection.
   */
  public function alterRoutes(RouteCollection $collection);

If it could go into detail about what routes to alter, and how to work with the buildRoutes method, that'd go a long way to making this a largely smooth process.

The project WIP is https://www.drupal.org/project/layout_library, @tim.plunkett, @samuel.mortenson, @EclipseGC you all have push access.

The plan is I'll flesh it out further to the point of being useful in coming months.

For #44 to #47

The interdiff looks really good to me. RTBC pending passing tests.

Eclipse

Validated the API of a new layout section storage plugin with Layout library module

Screenshots of editing layouts from the library

Winning!

The only extension point/API missing is in this code on \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::getRuntimeSections

  /**
   * Gets the runtime sections for a given entity.
   *
   * @param \Drupal\Core\Entity\FieldableEntityInterface $entity
   *   The entity.
   *
   * @return \Drupal\layout_builder\Section[]
   *   The sections.
   */
  protected function getRuntimeSections(FieldableEntityInterface $entity) {
    if ($this->isOverridable() && !$entity->get('layout_builder__layout')->isEmpty()) {
      return $entity->get('layout_builder__layout')->getSections();
    }

    return $this->getSections();
  }

It effectively embeds knowledge of the override storage handler in the defaults, but doesn't allow other modules to interact, limiting us to two plugin implementations without needing to do some hacky stuff like #2941808: Subclass \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::getRuntimeSections.

I'm not sure how to unpick that though.

This is an entity, so using module handlers or plugin managers is even more hacky.

Open to suggestions but I don't really see any better way of doing it

But I feel that it is out of scope for this issue, as we are currently not changing either buildMultiple() or getRuntimeSections() here

Agree

Regardless, I like the current code more than that in the gist that uses hook_entity_view_alter()

And #2941808: Subclass \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::getRuntimeSections can just use hook_entity_view_alter() if it has to.

saving review credits

This has been reviewed extensively, and I even took the extraordinary step of writing a contrib module to verify the API.

Committed as 10eb692 and pushed to 8.6.x

🦅🍒