Components inside a section should be rendered in order of region and component weight

Interesting. So the resulting markup is correct, but the order of execution is what is causing problems for you?

Just fixed the custom command failure errors in #2. Retaining status as "Needs work" for tests, thanks!

Switching to an MR and fixing the broken test. Still needs dedicated test coverage. NR to kick off test bot

Tagging for manual testing on 9.4.

Patch applies cleanly for Drupal 9.3, 9.4, and 10.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

For the tests requested in #4

Did not test.

Perusing the issue queue, I think this is the right issue for this. I also noticed that the 'storage' of the data is stored 'out' of order, as OP reports. It is stored by time of adding. If you edit, items never seem to move, only their region and weight notation.

Here is an example.

I edited a node layout and changed the order of these blocks (field blocks). I would expect that they are stored in the database in the order that they are set. Only the field data has them out of order; I moved a block from the 'second' region to the 'fourth' region, and changed the order of blocks in the third region.

The data (pardon the verbosity, it is serialized):

   ["00488840-db50-4afe-9c30-a123e6707fa9"]=>
    object(__PHP_Incomplete_Class)#7 (6) {
      ["__PHP_Incomplete_Class_Name"]=>
      string(38) "Drupal\layout_builder\SectionComponent"
      ["uuid":protected]=>
      string(36) "00488840-db50-4afe-9c30-a123e6707fa9"
      ["region":protected]=>
      string(6) "fourth"
      ["configuration":protected]=>
      array(4) {
        ["id"]=>
        string(46) "field_block:node:recipe:field_preparation_time"
        ["label_display"]=>
        string(1) "0"
        ["context_mapping"]=>
        array(1) {
          ["entity"]=>
          string(21) "layout_builder.entity"
        }
        ["formatter"]=>
        array(4) {
          ["type"]=>
          string(14) "number_integer"
          ["label"]=>
          string(6) "inline"
          ["settings"]=>
          array(2) {
            ["thousand_separator"]=>
            string(0) ""
            ["prefix_suffix"]=>
            bool(true)
          }
          ["third_party_settings"]=>
          array(0) {
          }
        }
      }
      ["weight":protected]=>
      int(0)
      ["additional":protected]=>
      array(0) {
      }
    }
    ["df8bfafc-210c-4d86-9745-e47081ab0fd4"]=>
    object(__PHP_Incomplete_Class)#8 (6) {
      ["__PHP_Incomplete_Class_Name"]=>
      string(38) "Drupal\layout_builder\SectionComponent"
      ["uuid":protected]=>
      string(36) "df8bfafc-210c-4d86-9745-e47081ab0fd4"
      ["region":protected]=>
      string(5) "third"
      ["configuration":protected]=>
      array(4) {
        ["id"]=>
        string(40) "field_block:node:recipe:field_difficulty"
        ["label_display"]=>
        string(1) "0"
        ["context_mapping"]=>
        array(1) {
          ["entity"]=>
          string(21) "layout_builder.entity"
        }
        ["formatter"]=>
        array(4) {
          ["type"]=>
          string(12) "list_default"
          ["label"]=>
          string(6) "inline"
          ["settings"]=>
          array(0) {
          }
          ["third_party_settings"]=>
          array(0) {
          }
        }
      }
      ["weight":protected]=>
      int(1)
      ["additional":protected]=>
      array(0) {
      }
    }
    ["f91febc6-d924-47a2-8ffd-b71d3b2597c7"]=>
    object(__PHP_Incomplete_Class)#9 (6) {
      ["__PHP_Incomplete_Class_Name"]=>
      string(38) "Drupal\layout_builder\SectionComponent"
      ["uuid":protected]=>
      string(36) "f91febc6-d924-47a2-8ffd-b71d3b2597c7"
      ["region":protected]=>
      string(5) "third"
      ["configuration":protected]=>
      array(4) {
        ["id"]=>
        string(42) "field_block:node:recipe:field_cooking_time"
        ["label_display"]=>
        string(1) "0"
        ["context_mapping"]=>
        array(1) {
          ["entity"]=>
          string(21) "layout_builder.entity"
        }
        ["formatter"]=>
        array(4) {
          ["type"]=>
          string(14) "number_integer"
          ["label"]=>
          string(6) "inline"
          ["settings"]=>
          array(2) {
            ["thousand_separator"]=>
            string(0) ""
            ["prefix_suffix"]=>
            bool(true)
          }
          ["third_party_settings"]=>
          array(0) {
          }
        }
      }
      ["weight":protected]=>
      int(0)
      ["additional":protected]=>
      array(0) {
      }
    }

I would expect that everything is stored in the order it is in the form.

To that end, correcting just the render array is not enough. The output from serialization should also be correct so that anyone using JSON:API for example gets the data in the correct order and does not have to waste frontend resources trying to figure it out.

Related: #2942975: [PP-1] Expose Layout Builder data to REST and JSON:API

While working with that issue and patch, I had to add this sorting code to a JSON Include parser:

  protected function sortSectionComponents(array &$section_data = []): array {
    foreach ($section_data as &$section_components) {
      /** @var \Drupal\Core\Layout\LayoutDefinition $layout */
      $layout = $this->layoutPluginManager->getDefinition($section_components['layout_id']);

      if (count($layout->getRegions()) > 1) {
        // We can only assume the defined region list is the 'order'.
        $regions = array_flip(array_keys($layout->getRegions()));

        // Sort the regions
        usort($section_components['components'], function($a, $b) use ($regions) {
          return $regions[$a['region']] <=> $regions[$b['region']];
        });

        // Now sort within the regions
        usort($section_components['components'], function($a, $b) use ($regions) {
          if ($regions[$a['region']] === $regions[$b['region']]) {
            return $a['weight'] <=> $b['weight'];
          }

          // What can we do with blocks that have the same weight value in one region? Can that happen?
          return 0;
        });
      } else {
        usort($section_components['components'], fn($a, $b) => $a['weight'] <=> $b['weight']);
      }
    }

    return $section_data;
  }

Where $section_data is just one sections worth of components. Ugly, but as far as I could tell it put the data in the order that someone had saved Layout Builder components for JSON:API.

I would posit that core itself should ensure the storage of the data is accurate and not leave it up to themers or API consumers to figure it out themselves. To that end, it seems like it would improve overhead performance by not needing to re-sort items for forms, render, or APIs because the data is stored as it was entered.

Also, I had to assume that the regions are ordered (weighted) by how they are defined in the layout YAML. I am not sure if that would always be true, I do not know how people are constructing their own YAML layout definitions.

So, when you get the data from Drupal regardless of the method, it should come like:

sections
--layout
----regions (ordered)
------components (ordered)
--layout
----regions (ordered)
------components (ordered)

etc

Patch #5 works great on 10.3.6!

The order that the blocks are put in, is now also the order of that they are rendered!