Rewrite \Drupal\layout_builder\Section to represent the entire section, not just the block info

Just looking at Section.php so far

1. +++ b/core/modules/layout_builder/src/Section.php
   @@ -5,89 +5,232 @@
   +  protected function validateBlockInfo($uuid, array $block_info) {
   +    if (!isset($block_info['region'])) {
   +      throw new \InvalidArgumentException(sprintf('The block with UUID of "%s" is missing a region', $uuid));
   +    }
   +    if (!isset($block_info['configuration'])) {
   +      throw new \InvalidArgumentException(sprintf('The block with UUID of "%s" is missing configuration', $uuid));
   

   It seems that to get to this function the block should always have a weight. Should validate that here?

   For instance in setBlockInfo() we call validateBlockInfo() and then do a sort assuming all blocks have a weight.

2. +++ b/core/modules/layout_builder/src/Section.php
   @@ -5,89 +5,232 @@
   -  public function updateBlock($region, $uuid, array $configuration) {
   -    if (!isset($this->section[$region])) {
   -      throw new \InvalidArgumentException('Invalid region');
   -    }
   

   Ok this is what you use to update a block not setBlockInfo() except you could update an existing block with setBlockInfo() and I don't think you would get error. Should error be thrown in setBlockInfo() if the uuid is already set. Right now I think it would just override the settings.

3. +++ b/core/modules/layout_builder/src/Section.php
   @@ -5,89 +5,232 @@
   +      if (isset($info['weight'])) {
   +        $this->setBlockInfo($uuid, $info);
   +      }
   +      else {
   +        $this->addBlock($uuid, $info);
   +      }
   

   Looking at this it seems that you can add a block to the region with either addBlock() or setBlockInfo() but if I want to specify the weight I should use setBlockInfo().

   The doc for setBlockInfo() doesn't specify this.

   Also addBlock() just calls setBlockInfo() after getting the weight. Could we just get rid of addBlock() and have setBlockInfo() handle find the weight if none is provided.

   Or at least update the doc to so it is obvious when to call which?

+++ b/core/modules/layout_builder/src/Field/LayoutSectionItemList.php
@@ -16,17 +17,74 @@ class LayoutSectionItemList extends FieldItemList implements LayoutSectionItemLi
+  public function addSection($index, Section $section) {

\Drupal\layout_builder\SectionStorageInterface::addSection() uses $delta not $index in it's signature. Would be clearer if the they matched.

Otherwise looks good!

Duh! Missed that setBlockInfo() is protected.

I like the method name changes in #6. Confirming #5 is fixed.

Some nice test coverage here, ++

1. +++ b/core/modules/layout_builder/src/Field/LayoutSectionItemListInterface.php
   @@ -24,23 +25,4 @@
   -  public function addItem($index, $value);
   

   Without this the interface only has ::get - which is defined in the parent. Do we need it?

2. +++ b/core/modules/layout_builder/src/Section.php
   @@ -5,89 +5,232 @@
   + * - A multi-dimensional array keyed by block UUID, containing information like
   + *   region, weight, and block configuration.
   

   Devil's advocate: could/should we model this as a value object instead of a magic array?

3. +++ b/core/modules/layout_builder/src/Section.php
   @@ -5,89 +5,232 @@
   +      // @todo Devise a mechanism for mapping old regions to new ones in
   +      //   https://www.drupal.org/node/2796877.
   +      $new_region = $layout_definition->getDefaultRegion();
   

   thinking about the implementation, we could support a third argument here 'LayoutChangeStrategyInterface', defaulting to null.

   If null, we could use this logic - which could be moved to new DefaultLayoutChangeStrategy(), then later we'd just pass in something nicer if we had a UI to support something better, but follow up/todo etc

4. +++ b/core/modules/layout_builder/src/Section.php
   @@ -5,89 +5,232 @@
   +  protected function validateBlockInfo($uuid, array $block_info) {
   

   If we did go down the value object to model the block info, this would sit well on that

5. +++ b/core/modules/layout_builder/src/SectionStorageInterface.php
   @@ -0,0 +1,84 @@
   +interface SectionStorageInterface {
   

   extends \Countable?

6. +++ b/core/modules/layout_builder/tests/src/Kernel/LayoutSectionItemListTest.php
   @@ -0,0 +1,121 @@
   +    $expected = [
   

   could just $expected[] = ['layout_id' => 'layout_twocol_bricks'...] here to save repeating the existing bits (the third test case)

+++ b/core/modules/layout_builder/src/Section.php
@@ -121,26 +121,19 @@ public function getBlocksInfo() {
+    $blocks_info = $strategy->changeLayout($layout_id, $layout_settings, $this);

nice

Yeah, I see your point on constructor and generic get/set

Let's see where we end up before we introduce the wrong abstraction?

this looks close to me

I think it looks a lot cleaner, I was surprised (pleasantly). And it would use less memory because of the php 5.4 object optimisations. But a third opinion would be good though - what say you @tedbow?

Review of 11, I'll do 13 interdiff next. I didn't read other's comments, so if I repeated stuff, I apologize. Also, I may contradict myself in the comments a little, so read all the way through before responding.

1. +++ b/core/modules/layout_builder/src/Controller/MoveBlockController.php
   @@ -69,31 +69,31 @@ public static function create(ContainerInterface $container) {
   -    /** @var \Drupal\layout_builder\Field\LayoutSectionItemInterface $field */
   -    $field = $entity->layout_builder__layout->get($delta_from);
   -    $section = $field->getSection();
   +    /** @var \Drupal\layout_builder\SectionStorageInterface $field_list */
   +    $field_list = $entity->layout_builder__layout;
   +    $section = $field_list->getSection($delta_from);
    
   -    $block = $section->getBlock($region_from, $block_uuid);
   -    $section->removeBlock($region_from, $block_uuid);
   +    $block = $section->getBlockInfo($block_uuid);
   +    $section->removeBlock($block_uuid);
   

   I really like this whole change block. It simplifies the parameters and makes it easier to read.

2. +++ b/core/modules/layout_builder/src/Field/LayoutSectionItemList.php
   @@ -11,22 +13,79 @@
   +  public function appendSection(Section $section) {
   +    /** @var \Drupal\layout_builder\Field\LayoutSectionItemInterface $item */
   +    $item = $this->appendItem();
   +    $item->updateFromSection($section);
   +    return $this;
   +  }
   

   I remember seeing an addSection(0, $section) earlier and am wondering why that wasn't just a call to this method.

   Also, this and addSection... I'm not terribly stoked with the naming convention. I'm saying that out loud because it's true, but I see that you're following the methods already on the class from other interfaces, so I won't complain too badly, I just find the create vs append vs add naming abstruse.

3. +++ b/core/modules/layout_builder/src/Field/LayoutSectionItemList.php
   @@ -11,22 +13,79 @@
   +    foreach ($this->list as $delta => $item) {
   +      $values[$delta] = $this->getSection($delta);
   +    }
   +    return $values;
   

   So... crazy suggestion... but you could do a yield $this->getSection($delta); here I think. Maybe the returned generator would mess stuff up, but you might play with it. We should look into yielding where possible as we continue developing.

4. +++ b/core/modules/layout_builder/src/Field/LayoutSectionItemList.php
   @@ -11,22 +13,79 @@
   +    if (!$field = $this->get($delta)) {
   +      throw new \OutOfBoundsException('Invalid delta');
   +    }
   +
   +    return $field->getSection();
   

   cute... debating if I like assigning the variable in the condition for use AFTER the condition. I think I don't, but this is certainly the most concise way to write the method.

5. +++ b/core/modules/layout_builder/src/Form/ConfigureSectionForm.php
   @@ -166,19 +156,15 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +      $section->changeLayout($plugin_id, $configuration);
   

   So I'm going to propose we NOT allow users to change layouts. We're using more than one layout on a page, and allowing them to just add a new layout, drag blocks around and then kill layouts (and blocks) they no longer want or need seem totally optimal to me. If we support layout changing, we have to do stuff like block mapping, and I really want to never do that again because we cannot do it well and it will never work the way we want (at least not until computers can read our minds).

6. +++ b/core/modules/layout_builder/src/Form/UpdateBlockForm.php
   @@ -42,12 +42,12 @@ public function getFormId() {
   -    $block = $field->getSection()->getBlock($region, $uuid);
   ...
   +    $block_info = $field->getSection()->getBlockInfo($uuid);
   

   I know I was initially against moving to putting region on the block info, but this clearly demonstrates the dx benefits and I'm glad you convinced me otherwise. :-)

7. +++ b/core/modules/layout_builder/src/LayoutSectionBuilder.php
   @@ -82,33 +67,24 @@ public function __construct(AccountInterface $account, LayoutPluginManagerInterf
   +        $block_output['#weight'] = $weight++;
   

   ok, so this is actually a thing which gives me some pause here. Despite my praise above about removing regions from the array keys and adding them to block info, doesn't that complicate this process? Don't we need to also be saving relative block weights during the administrative layout process and then using them here? maybe $block_info['weight']??

8. +++ b/core/modules/layout_builder/src/Section.php
   @@ -5,89 +5,225 @@
   +    if (!is_string($layout_id)) {
   +      throw new \InvalidArgumentException(sprintf('Layout plugin ID of type "%s" must be a string', gettype($layout_id)));
   +    }
   

   Wouldn't it be so nice if we could just use a scalar typehint in the function signature :-D *cough*PHP7*cough*

9. +++ b/core/modules/layout_builder/src/Section.php
   @@ -5,89 +5,225 @@
   +  public function changeLayout($layout_id, array $layout_settings = [], LayoutChangeStrategyInterface $strategy = NULL) {
   +    if (!$strategy) {
   +      $strategy = new DefaultLayoutChangeStrategy();
   +    }
   +
   +    $blocks_info = $strategy->changeLayout($layout_id, $layout_settings, $this);
   +
   +    $this->layoutId = $layout_id;
   +    $this->layoutSettings = $layout_settings;
   +    $this->blocksInfo = $blocks_info;
   +    return $this;
   +  }
   

   Let's remove this bit.

10. +++ b/core/modules/layout_builder/src/Section.php
    @@ -5,89 +5,225 @@
    +    uasort($this->blocksInfo, function ($a, $b) {
    +      return $a['weight'] > $b['weight'] ? 1 : -1;
    +    });
     
    

    Ok, I see you're doing some sorting during setBlockInfo()... I'd not considered this. Does this mean your blocks are always sorted in some specific order? I guess I was envisioning some process by which you track order individually across each region.

Eclipse

Reviewing the interdiff of 13

1. +++ b/core/modules/layout_builder/src/Controller/MoveBlockController.php
   @@ -85,12 +85,12 @@ public function build(EntityInterface $entity, $delta_from, $delta_to, $region_f
   +      $section->appendBlock($block_uuid, $block_info);
   

   We don't need block_uuid here do we? because $block_info already knows it? Or maybe this is just wrong? I see other examples later that are not passing the uuid individually.

2. +++ b/core/modules/layout_builder/src/LayoutSectionBuilder.php
   @@ -131,24 +127,14 @@ protected function buildBlock($uuid, array $configuration, CacheableMetadata $ca
   +    $block = $this->blockManager->createInstance($block_info->getBlockId(), $block_info->getConfiguration());
   

   Might as well put a protected method on BlockInfo() that gets the block manager too (like Section does with layout) then you could $block = $block_info->getBlock(); I dunno, maybe too much magic and service location at that point, but it's along the same lines as what you did in Section, and that made a lot of stuff easier to read.

3. +++ b/core/modules/layout_builder/src/Section.php
   @@ -237,23 +195,41 @@ protected function setBlockInfo($uuid, array $block_info) {
   +    $block_info = $this->getBlockInfo($uuid);
   +
   +    $blocks = $this->getBlocksInfoByRegion($block_info->getRegion());
   +    $slice_id = array_search($uuid, array_keys($blocks), TRUE);
   +    // Only process the blocks that need new weights.
   +    $blocks_to_set = array_slice($blocks, $slice_id + 1, NULL, TRUE);
   +    $new_weight = $block_info->getWeight();
   +
        unset($this->blocksInfo[$uuid]);
   +    $this->adjustBlockWeights($blocks_to_set, $new_weight);
   

   whoa, that got way more complicated... :-(

4. +++ b/core/modules/layout_builder/src/Section.php
   @@ -302,21 +278,20 @@ protected function getBlocksInfoByRegion($region) {
   +  public function insertBlockAfter(BlockInfo $block_info, $preceding_uuid) {
   +    $region = $block_info instanceof BlockInfo ? $block_info->getRegion() : $block_info['region'];
   

   this looks like BC, but you're typehinting BlockInfo in the signature, so I think it's unnecessary.

So, all said and done, I'm fine with 11 or 13, so really either way. 13 is certainly easier to read. As long as it's an implementation detail of layout_builder and not a requirement of blocks in general, I'm on board... but it reminds me a lot of the Block entity, which is where much of my "meh" comes from. That might actually be a good thing though because it might indicate that we're adopting an architecture that will be a fairly easy drop in replacement for block layout when the time comes.

All in all, this patch is coming along pretty nicely and cleans up a bunch of stuff, so generally speaking ++.

Eclipse

Just scanning so far... The diff is a bit goofy which makes it a bit hard to read. Thinking also of how we could parcel this into more manageable chunks:

• An initial patch that adds the new APIs.
• A followup that does some of the renaming of methods and the like.
• Another followup to remove the now-unneeded interfaces (we can just stick an @todo on them at first).
• Etc.

I think we'll also eventually want a CR for this, but no point in writing it just yet as we're still finalizing the API.

+++ b/core/modules/layout_builder/src/Section.php
@@ -5,158 +5,280 @@
+ * @internal
  */

Is it internal because it's experimental, or will it remain internal? Let's add text explaining the internal.

1. +++ b/core/modules/layout_builder/src/Field/LayoutSectionItemInterface.php
   @@ -16,6 +16,8 @@
   + * @deprecated
   
   +++ b/core/modules/layout_builder/src/Field/LayoutSectionItemListInterface.php
   @@ -13,8 +14,10 @@
   + * @deprecated
   

   Presumably we'll fill these out later on the issue.

2. +++ b/core/modules/layout_builder/src/Field/LayoutSectionItemList.php
   @@ -17,16 +19,123 @@ class LayoutSectionItemList extends FieldItemList implements LayoutSectionItemLi
   +   * Converts an array-based section value to an object.
   +   *
   +   * @param \Drupal\layout_builder\Section|array $value
   +   *   A section value.
   

   What kinds of arrays are these array-based section values? Is the structure defined somewhere that we can link?

3. +++ b/core/modules/layout_builder/src/Field/LayoutSectionItemList.php
   @@ -17,16 +19,123 @@ class LayoutSectionItemList extends FieldItemList implements LayoutSectionItemLi
   +      throw new \OutOfBoundsException('Invalid delta');
   ...
   +      throw new \OutOfBoundsException('Invalid delta');
   

   This is one of those exeptions that could be hard to debug; probably better to do something like "No section exists for delta $delta in $do_I_know_my_layout_name_or_something.

4. +++ b/core/modules/layout_builder/src/Form/UpdateBlockForm.php
   @@ -61,7 +58,7 @@ protected function submitLabel() {
   -    $section->updateBlock($region, $uuid, $configuration);
   +    $section->getComponent($uuid)->setConfiguration($configuration);
   
   +++ b/core/modules/layout_builder/src/LayoutSectionBuilder.php
   @@ -84,31 +83,21 @@ public function __construct(AccountInterface $account, LayoutPluginManagerInterf
   -   * @param array $section
   -   *   An array of configuration, keyed first by region and then by block UUID.
   +   * @param \Drupal\layout_builder\SectionComponent[] $components
   +   *   An array of components.
   
   @@ -124,33 +113,30 @@ public function buildSectionFromLayout(LayoutInterface $layout, array $section)
   +   * @param \Drupal\layout_builder\SectionComponent[] $components
   +   *   An array of components.
   

   I appple-Fed through the patch and didn't find anything anywhere that defines what a "component" is. Based on how it's used, it's a "block or blocklike thing"? Presumably the SectionComponent docblocks could explain what it is.

   Also, naming things is hard. Is there a more meaningful and less overloaded word that we could use than "component"?

+++ b/core/modules/layout_builder/src/Plugin/Field/FieldType/LayoutSectionItem.php
@@ -36,15 +33,7 @@ class LayoutSectionItem extends FieldItemBase implements LayoutSectionItemInterf
+    $properties['section'] = DataDefinition::create('layout_section')

nice

1. +++ b/core/modules/layout_builder/src/LayoutSectionBuilder.php
   @@ -84,32 +82,23 @@ public function __construct(AccountInterface $account, LayoutPluginManagerInterf
   +      if ($block_output = $component->build()) {
   

   If we are going to through all the trouble of abstracting components away from blocks this should be renamed $component_output

2. +++ b/core/modules/layout_builder/src/LayoutSectionBuilder.php
   @@ -124,78 +113,15 @@ public function buildSectionFromLayout(LayoutInterface $layout, array $section)
   -    $block = $this->blockManager->createInstance($configuration['id'], $configuration);
   

   All uses of $this->blockManager have been removed except in the constructor. It can be removed.

   Also $account, $contextRepository, and $contextHandler

3. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -0,0 +1,308 @@
   + * A component represents the smallest part of a layout. Components wrap a
   + * renderable plugin, currently using \Drupal\Core\Block\BlockPluginInterface.
   

   Why not explicitly only support blocks in the code with exceptions that say "only Blocks are currently supported.

   Just seems weird to say blocks are only supported but only place that is checking if the plugin is Block in build() and still there is no error, just not output. A section component without output is useless.

   Maybe throw exception in __construct then you can remove the if statement from build().

4. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -0,0 +1,308 @@
   +class SectionComponent implements RefinableCacheableDependencyInterface {
   

   Not a fan of the SectionComponent name either. What about SectionRenderable?
   UPDATE: @tim.plunkett pointed out the sections themselves are are renderable so this would be confusing.
   I think SectionComponent is probably as good as anything unless we are willing to go for ThingThatIsRenderedInsideASectionButNotNecessarlyABlock 😜

ThingThatIsRenderedInsideASectionButNotNecessarlyABlock

+1.

@tim.plunkett pointed out that since this is experimental a CR isn't required, which is totally right. Overall API docs can wait for the actual API once it's finalized/stable, and live in the codebase, and a CR can just reference those docs.

Just a few morsels of docs; please confirm accuracy.

+++ b/core/modules/layout_builder/src/Section.php
@@ -5,158 +5,285 @@
+  public function insertComponent($delta, SectionComponent $new_component) {
+    $components = $this->getComponentsByRegion($new_component->getRegion());
+    $count = count($components);
+    if ($delta > $count) {
+      throw new \OutOfBoundsException('Invalid delta');
+    }

Missed one.

+++ b/core/modules/layout_builder/src/Plugin/DataType/SectionData.php
@@ -0,0 +1,36 @@
+    if ($value && !$value instanceof Section) {
+      throw new \InvalidArgumentException('Value is not a valid section');
+    }

What value?

Looking good, only a couple of minor things and some observations

1. +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
   @@ -189,12 +189,12 @@ protected function buildAddSectionLink($entity_type_id, $entity_id, $delta) {
   -    $build = $this->builder->buildSectionFromLayout($layout, $item->section);
   ...
   +    $build = $section->toRenderArray();
   

   this is nice

2. +++ b/core/modules/layout_builder/src/Plugin/Field/FieldType/LayoutSectionItem.php
   @@ -73,17 +65,6 @@ public static function mainPropertyName() {
   -          // @todo Address in https://www.drupal.org/node/2914503.
   

   this needs to be added back for the section blob?

3. +++ b/core/modules/layout_builder/src/Section.php
   @@ -5,158 +5,302 @@
   +    // If the delta is the end of the list, append the component instead.
   +    elseif ($delta === $count) {
   

   can just be an if, the previous if throws an exception

4. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -0,0 +1,313 @@
   +    if ($plugin instanceof BlockPluginInterface) {
   ...
   +    $plugin = $this->pluginManager()->createInstance($this->getPluginId(), $this->getConfiguration());
   ...
   +    return \Drupal::service('plugin.manager.block');
   

   In what circumstances would this not be an instance of BlockPluginInterface?

   In that follow up we should switch this {duck}->toRenderArray()

   instead of putting this knowledge here

5. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -0,0 +1,313 @@
   +  public function get($property) {
   ...
   +  public function set($property, $value) {
   

   we should have a trait for this, the concept is used in a few places (unrelated)

   i've reviewed a couple of patches (#2909472: Add value objects to represent the return of hook_requirements, #2908735: [meta] Add value objects to represent hook_schema) that could use it

#52.4 right that makes sense now its the right link :) I commented on the ::toRenderArray() about making it more generic

Let's get this in and move to the next steps.

Adding review credits whilst here.

Just to say I read through this and it all looks like a good improvement and pretty clear, makes sense to me to get it in and continue with follow-ups.

@catch did you commit this? I tried to just now but it does not apply.

Edit: nm it applied now; no idea what I was doing wrong. :)

I didn't commit it :)

Erg, just lost a big dreditor review, Automatic Updates--. Trying to resummarize:

The part of this patch that I get hung up on every time is the storage. Configured layout/section data being stored as field content data seems weird and wrong to me. I guess the reason it was done this way is that HEAD currently only supports layout overrides for individual content entities, and since those individual content entities are saved as content, the sections are too. In #2922033: Use the Layout Builder for EntityViewDisplays that will change; layouts/sections will be attached to entity displays which are config entities. This is where #2927349: Decouple the Layout Builder UI from entities came from, which is the issue that spawned the entity decorator and whatnot that concerned me.

But really it's out of scope here. This patch is a good API improvement; the existing storage is already in HEAD and this patch only adjusts the API without affecting the storage itself (plus the part here that decouples/abstracts the section storage is actually helpful for my concern).

My other major concern is also related to the storage piece, for layouts for the "page chrome" a.k.a. block placement within theme regions (currently via the Block UI). TLDR is I think that layouts need to be stored the same way for all three usecases, not in three separate ways that have to have the API calls forwarded on to them). But this isn't the issue for that either, so there's no reason to hold it up. I would like the opportunity to sign off on any of the followups related to the storage before they go in, though.

Overall the API improvements in this issue are shiny, so (belatedly) committed and pushed to 8.5.x. Thanks!

No, testbot. Stay.