Add UUID to sections

Drupal 9.2.0-alpha1 → will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

I wish we'd written it this way originally!
I support changing it.
And it would be very straightforward to write an update path for defaults (entity_view_display config).

However, this might be extremely difficult to update for existing overrides (stored in field data) because we'd have to check every entity with LB overrides enabled, every revision of that entity, for every language that entity has been translated to.

I can't find the issue right now, but there's another LB issue where we got stuck with this same problem.
Here's another one that should be even easier (because it's about DB schema not actual content) but is still stuck on this sort of problem: 🐛 layout_builder__layout_section column hitting database limit Needs work
See also #3010975: Devise a locking/inheritance mechanism for overrides to mitigate when they become out of sync with the default layout →

Also, this reminds me of (or at least would block) a few other issues, which I'm adding as related.

However, this might be extremely difficult to update for existing overrides (stored in field data) because we'd have to check every entity with LB overrides enabled, every revision of that entity, for every language that entity has been translated to.

Yeah, especially for systems that have an extremely large volume of nodes/revisions. That update hook could take forever to run.

This is why I was kinda suggesting the alternative of allowing Section::getUuid() to generate and set it, if one doesn't already exist, because at least in that case, it's generating it on demand, instead of all at once. But that said, that could probably come with its own problems. Just a couple concerns off the top of my head:

1. Are there performance implications if this is happening in a lot of different nodes all at once on busy systems?
2. Would we set it for all revisions/languages when called, or just for the current revision? On the one hand, setting for just the current revision would be more performant, but on the other hand, if you roll a node back to and older revision, you'd need to make sure there's logic to copy that value to the revision.

Alternatively, we could just opt to return NULL in these scenarios. And any consumer of this method would need to know that is a possible return value. Then the next time the node is saved, it can generate a UUID if it's missing, and attribute it to the new revision only. There's still a question of what to do if a revision gets reverted, but in theory, you should be able to look at the current revision, grab the uuid value, revert the to the older revision and then update the uuid for that revision when it gets reactivated.

I'm unsure if having a NULL value return would be problematic in other use cases.

It would be convenient to be able to access the UUID Generator from the section class to generate a UUID if one wasn't provided. However, its not clear to me how you would add this to the Section class, since It is not a dependency injected class.

I pushed some code just in the hopes of getting something rolling and to provide the opportunity for feedback. I understand that what I have so far isn't workable as a solution yet. I'm still trying to understand the scope of this.

Well that would definitely beg the question of whether or not that's a good design pattern here, which I'm still not 100% convinced of. But, if it was something we decided to do, one possible solution would be to make a separate issue to create a UuidGeneratorTrait that can handle adding the getter/setter methods for any class, allowing the uuid service to get mocked in unit tests. I could see that being a useful trait, not just for layout builder, but anywhere else UUIDs get generated in core/contrib/custom modules, but that could definitely lead to a lot of refactoring across core, so that probably would have to happen over multiple steps.

**edit**:
To clarify, you don't have to use a separate trait here. You could just add getUuidGenerator() and setUuidGenerator() methods directly to the class and then have getUuidGenerator() do a conditional check to see if the service has been previously set, and if not then set it to \Drupal::service('uuid'). I just figure if we're going to establish that as a pattern for UUID generation, having a generic Trait outside of Layout Builder might be an option here.

Oh I was just made aware of this issue. And I am 100% in favor of it. The sooner the better.
I am having a lot of issues with the layout builder config in config split because the order of the sequence is important but there is no weight and there is no way of identifying a section so I can't even detect if sections have been reordered. The weight issue is a separate problem for which I will open another issue.

As for the config I think we can update it in a straight forward way indeed.

Then for the content I agree, it is probably not wise to do this in an update hook for large sites.
But fortunately there is an alternative (which also comes at some performance cost of course)
So when accessing the section via its API the UUID associated with it when it is not saved is:

1. random UUID. That means that potential new tools that try to detect this kind of change would be forever confused, but the solution is easy, just save the node. So that would be acceptable in my book.
2. predictable UUID: As defined in RFC 4122 generated with the site UUID and entity_type:id:field_name:field_delta:section_delta (and anything else needed for a unique section). I used ramsey's uuidv5 for that in a project.

And can't we do a similar thing for the section as we already do for the component? I think we don't need to implement a new trait to be used everywhere, if anything this is just for BC for sections that have not been saved with a UUID already. So the generator would be used where the section is created and if it is loaded from the database without a section we provide one on the fly that you can not influence from outside.

I like the predictable UUID option. That seems like it could have the lowest potential for negative performance impact, as it theoretically doesn't even require a save during the get process (assuming other info has already been retrieved and cached properly).

Just thinking out loud... Would there ever be a need for a UUID to be different across translations or would it always be the same? I mean, I suppose a translation could delete an entire section and create a new section, so that would probably cover that case, and otherwise we'd probably want the UUIDs to match. I'm unsure what's done elsewhere in core.

Drupal 9.5.0-beta2 → and Drupal 10.0.0-beta2 → were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.4.0-alpha1 → was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.3.0-rc1 → was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Sorry about all the noise related to switching target branches. I'm not used to this forking model, so I'm still figuring out how to get the issue fork up-to-date.

I started a new MR because there were some changes between 9.3.x and 10.1.x that were going to make it difficult to rebase and because those changes also made me realize some limitations of my old approach.

@tim.plunkett I would love to get an opinion on the possible approaches below:

• Completely non-breaking to work around existing dependence on $delta. This would probably involve adding a bunch of new methods such as getSectionByUuid($uuid) and then replacing the usage of existing methods.
• Completely re-write the system to use $uuid instead of $delta.
• Repurpose existing methods by changing $delta to $uuid and implement logic to still handle if $delta is passed in by checking that its an int or doesn't contain dashes.

I'd like to get a sense of what is the most realistic approach so I can move forward with that.

I've been ruminating on whether this task actually might require adding a weight property as well to the Section class. I think that @bircher was indicating this in his comment: https://www.drupal.org/project/drupal/issues/3208766#comment-14284893 ✨ Add UUID to sections Needs work . Using $delta as the method for ordering sections effectively served two purposes:

• $delta was a unique pseudo-ID
• $delta also indicated the order.

Unlike delta, the UUID property doesn't tell us anything about the order of items. We can maybe assume that they are always going to be in the correct order because our logic will make sure of it. I don't understand how config works well enough to say for certain whether it will be correctly written out when config is exported.

After thinking through this a bit more, I think that taking a more minimalist approach gets what is needed done and is more likely to happen sooner. The OP seems to make an assumption that we need to start storing sections using their UUIDs, but I don't see any reason why that is the case. While not ideal, the UUID being stored on the section allows us to find the section and act on it. I believe that this solution will also be sufficient to start making these work with config split patching (correct me if I'm wrong @bircher!) It also seems like we can get away without introducing breaking changes. It is an incremental change that solves a basic need in a narrow way and in that sense I think it is an elegant solution.

There are still a few things to be done, but I thought I'd set the MR to ready to see what happens when tests run.

pyrello → closed merge request !1396

I'm looking at why the tests are failing.

@bircher mentioned in Slack to me that he thinks weights are going to be necessary to have this work with config split. I have been trying to validate that, but I discovered that things weren't working when I picked this up again. I have made a commit with some changes that allowed me to get to the manage display screen again, but it is not currently functional. I wanted to capture these changes in case they are eventually actually necessary, but they may end up getting backed out. At least they will need to be refactored.

Also noting that when I'm debugging this, I'm seeing sections loaded into the LayoutBuilderEntityViewDisplay's third_party_settings using the UUID's as keys in some cases (e.g. loading the LB manage display page for recipe using the Umami demo). In other cases, they are still being loaded with numeric delta keys (e.g. when I try to add a section). This means that the changes I made to get loading based on UUID working break adding a section. I have been trying to figure out how the sections are getting hydrated into LBEVD, but haven't figured it out yet.

So, it looks like the updates for weights are mostly working with config split as expected:

adding:
  third_party_settings:
    layout_builder:
      sections:
        config_split_sequence_v48fde2um5oiptc6:
          uuid: 30c153a8-969c-44c1-9544-655eec0034d5
          layout_id: layout_twocol_section
          layout_settings:
            label: ''
            context_mapping: {  }
            column_widths: 50-50
          components:
            0eff9e1d-4e73-4748-b910-e5568df1d5aa:
              uuid: 0eff9e1d-4e73-4748-b910-e5568df1d5aa
              region: first
              configuration:
                id: 'field_block:node:recipe:field_recipe_category'
                label_display: '0'
                context_mapping:
                  entity: layout_builder.entity
                formatter:
                  type: entity_reference_label
                  label: inline
                  settings:
                    link: true
                  third_party_settings: {  }
              weight: 0
              additional: {  }
            eadd557c-6414-40e5-9a95-355720385477:
              uuid: eadd557c-6414-40e5-9a95-355720385477
              region: second
              configuration:
                id: 'field_block:node:recipe:field_tags'
                label_display: '0'
                context_mapping:
                  entity: layout_builder.entity
                formatter:
                  type: entity_reference_label
                  label: inline
                  settings:
                    link: true
                  third_party_settings: {  }
              weight: 0
              additional: {  }
          weight: 0
          third_party_settings: {  }
        config_split_sequence_e0lq5tg181locq7n:
          weight: 1
        config_split_sequence_io30t3nekqemg6fh:
          weight: 2
        config_split_sequence_r8a99dddoq0hav79:
          weight: 3
removing:
  third_party_settings:
    layout_builder:
      sections:
        config_split_sequence_60q770hmp05rktfs:
          components:
            eadd557c-6414-40e5-9a95-355720385477:
              uuid: eadd557c-6414-40e5-9a95-355720385477
              region: content
              configuration:
                id: 'field_block:node:recipe:field_tags'
                label_display: '0'
                context_mapping:
                  entity: layout_builder.entity
                formatter:
                  type: entity_reference_label
                  label: inline
                  settings:
                    link: true
                  third_party_settings: {  }
              weight: 3
              additional: {  }
            0eff9e1d-4e73-4748-b910-e5568df1d5aa:
              uuid: 0eff9e1d-4e73-4748-b910-e5568df1d5aa
              region: content
              configuration:
                id: 'field_block:node:recipe:field_recipe_category'
                label_display: '0'
                context_mapping:
                  entity: layout_builder.entity
                formatter:
                  type: entity_reference_label
                  label: inline
                  settings:
                    link: true
                  third_party_settings: {  }
              weight: 2
              additional: {  }
        config_split_sequence_e0lq5tg181locq7n:
          weight: 0
        config_split_sequence_io30t3nekqemg6fh:
          weight: 0
        config_split_sequence_r8a99dddoq0hav79:
          weight: 0

Need to look into why the weights being removed all are set to 0.

To reproduce:
1. Site install with Demo Umami profile
2. Export config
3. Enable Config Split module
4. Make a change to the Recipe content type full view display. In the above case, I added a two column section at the beginning and moved the recipe categories and tags blocks into it.
5. Create a split and add core.entity_view_display.node.recipe.full to the partial split section.
6. Export config again and inspect the config split patch file that is generated.

The MR is still in draft so not sure if it was fully ready.

But looks like it will still need it's own test cases.
And if everything is being updated sounds like it will need an upgrade path for existing sites. Which would need test coverage as well

Thanks.

And if everything is being updated sounds like it will need an upgrade path for existing sites. Which would need test coverage as well

I have an update hook in place, although it may need some additional scrutiny. As for adding test coverage for checking that the upgrade path works, I could use some input about what something like that would look like. A link to an existing example would be great!

Also, I would really like to prioritize working on this to finish it up, but I would really like to get some feedback from a maintainer that this approach is likely to be accepted before investing much more time with writing tests.

After an interesting conversation with ChatGPT, I think I have an idea for how the update hook test should work:
The test will need to exclude layout_builder from the $modules property.
Set the schema version for the layout_builder module to the update hook that is being tested so that it is not run when the module is installed.
Manually install the layout_builder module.
Create a section and assert that it does not contain a UUID.
Set the schema version for the layout_builder module to the update hook prior to the one being tested and trigger database updates to run.
Assert that the section now has a UUID.

Not sure how deep this needs to go. The update hook is also supposed to handle revisions and the temp store, so probably I'll need to add checks for each of those before and after the update hook is run.

Okay, after doing a little bit more searching it seems like this might actually be the path forward on this: https://git.drupalcode.org/project/drupal/-/blob/9.5.0/core/modules/layo...

It seems like I'll need to generate a new database dump? Probably based on 10.0.0.

Queuing the test to run.

It could have conflict with 🐛 To reduce database size, convert layout_builder__layout_section column to a hash of the content Active

Seems MR has a valid failure.

Sadly, this work has stalled out because of build errors. I haven't been able to get any clear help or guidance on how to resolve the issues beyond vague implications that maybe they are related to the MR itself, which I strongly believe they are not.

Hoping that maybe things will be cleared up by rebasing now after a couple weeks have passed.

The issue still happens after the change. I could actually kind of replicate the issue in drupalpod.

If you run ddev phpunit core/modules/layout_builder/tests/src/Kernel it runs for minutes and minutes and not a single test runs.

If you run ddev phpunit core/modules/comment/tests/src/Kernel all tests run without issue.

So it seems that some code in this MR is eating up a lot of memory, which would match the issues shown in the CI. Perhaps the __clone method? not sure, but in any case, this can be replicated in another system so there is a way forwards by debugging on that other system (ie: drupalpod).

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened → , as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

jayhuskins → closed merge request !4273

I'm just realizing that SectionListTrait::addBlankSection() is going to need to generate a new UUID when it is triggered.

At least the way I have handled generating a UUID for blank sections (random generation at the time of their creation), they are tricky to test for. I made it so that you could pass a UUID to the blank section when it gets created, but to fully implement this would require changing more function signatures related to section removal. My first inclination is to think this is the mark of a bad pattern.

@tim.plunkett (or anyone else) if you have any insight you can provide into the purpose of the blank section, that would be helpful.

Specifically, I'm wondering if they ever actually get saved into config or the database? If not, maybe we could hard code a UUID?

I'm using a hardcoded string for the blank layout section "UUID." I think this might be okay because it will never need to pass validation. We'll see.

I was looking into errors that are occurring in LayoutBuilderTest. It errors on https://git.drupalcode.org/project/drupal/-/blob/afea1f8088530d53e0e8db7...

It errors because it expects to see the overridden title text in the "Powered by" block that has just been saved. However, the block isn't there because it has not been saved.

I tested this using Umami and confirmed that it is an issue with the current state of the code in MR 3563 using the following steps:

1. Log in
2. Visit the content admin page, find an article and click its link
3. Click the "Layout" tab
4. Click the "Add block" button
5. Click the "Powered by" block link in the "System" section
6. Update the title to "Overridden"
7. Click "Display title"
8. Click the "Add block" button
9. Confirm you can see the "Powered by" block with "Overridden" title
10. Click the "Save layout" button
11. Frown when you realize that the "Powered by" block is not displaying
12. Another visit to the layout tab will show that is not showing up there anymore

Note that the block gets added to the section correctly for temp storage, but something seems to fail when the node (entity) is being saved.

...

Did a bit more troubleshooting and I've tracked this back to a discrepancy in the LayoutSectionItem::isEmpty() method. It does a empty($this->section) check, which is using the magic ::__get() method to return the $value['section'] property. However, instead of section, it is using the UUID for the key. Not sure how to fix this yet, but at least I understand the root of the issue now.

Checking in on tests.

pyrello → closed merge request !4353

Trying again to get tests to run

Just fyi and MR will have to be opened for 11.x and that merged first before 10.1

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

pyrello → closed merge request !3563

kunal.sachdev → made their first commit to this issue’s fork.

pyrello → closed merge request !3056

pyrello → closed merge request !3857

Added a draft change record for whenever this get past the finish line.

Thank you,
Hoping for this new important feature to land in Drupal 10.2.x

Fascinating issue! That update hook is 🤯👏

(I can't review Layout Builder internals. The config schema changes look fine though 👍)

narendraR → changed the visibility of the branch 11.x to hidden.

Based on @lleber's real-world testing, this MR's current update hook will cause failed/partial updates when applying the update through the UI.

pyrello → changed the visibility of the branch 10.1.x to hidden.

pyrello → changed the visibility of the branch 9.4.x to hidden.

pyrello → changed the visibility of the branch 9.3.x to hidden.

pyrello → changed the visibility of the branch 3208766-d10_1-key-sections-by-uuid-rebased to hidden.

Discussed this issue with @lauriii

1. Every section which does'n have uuid is updated see \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplayStorage::mapFromStorageRecords() and \Drupal\layout_builder\Section::fromArray() but the uuid will not be consistent in every request until the entity is saved, is that a concern?
2. The BC break could potentially be solved by providing new optional parameter $uuid \Drupal\layout_builder\SectionListInterface::getSection, \Drupal\layout_builder\SectionListInterface::insertSection, \Drupal\layout_builder\SectionListInterface::appendSection and if $delta is provided then giving a deprecation error.
3. And for LayoutSectionItemList::getSections() we could add a new optional parameter to return the sections keyed by uuid. In the MR currently it returns the sections keyed by uuid.

All tests are passing now and regarding the update hook I think if the answer to the first question in https://www.drupal.org/project/drupal/issues/3208766#comment-15354646 ✨ Add UUID to sections Needs work is that it's not a concern then we will not need any post-update/update hooks.

Changes related to weight needs to be added in example in IS and CR.

Marking NW to update issue summary, CR and suggested some changes in MR.

Changes related to weight added in example in IS and CR

I have updated the remaining tasks and proposed resolution. I will start working on the remaining tasks.

Marked as NW for CR update and few nits to address

yash.rode → made their first commit to this issue’s fork.

Updated CR and addressed feedback.

Changes looks good to me. Waiting for Needs framework manager review.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

IMO the batch strategy needs work.

It seems to batch on tables which doesn't account for HUGE tables. Consider the following:

• Layout field for `block_content` entities has 30 records
• Layout field for `node` has 300,000 records

The batch really doesn't help with memory constraints given it's not breaking up the CRUD operations on the excessively large table.

I've also re-ran the update and here were the results:

real    25m6.900s
user    1m12.659s
sys     0m7.445s

The end result still seems to be over 25 minutes of site downtime for ~50,000 entity revisions.

The current issue summary indicates we do not need an update hook at all:

No need of update hook because existing sections are also updated see \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplayStorage::mapFromStorageRecords() and \Drupal\layout_builder\Section::fromArray(). Also there is a question related to this see #83 ✨ Add UUID to sections Needs work

Yes, if the answer to the first question in https://www.drupal.org/project/drupal/issues/3208766#comment-15354646 ✨ Add UUID to sections Needs work is that it's not a concern then we will not need any post-update/update hooks. Hence, marking it again to "Needs review".

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

I would love to RTBC this but … 🤔

1. How can this need review if the tag is still present and the issue summary still lists in the remaining tasks?
2. #103 says the current issue summary even says no update path is necessary?! Why was the issue summary updated to say that if it also says tests are still needed?
3. So it seems we're saying that no update path is necessary, but then @Luke.Leber in #101 says he tested the MR and the UPDATE took 25 minutes. Huh?! And sure enough: layout_builder_update_9001() is still in the MR.
4. The issue summary says API changes are TBD … but one of the first changes in the MR is changing the signature of public function getSections() {, so there's definitely API changes. And they do not have a change record yet.

The current state of the issue is very unclear and it definitely is not ready for review until all that is clarified 😅 Looking forward to reviewing it when that's taken care of! 👍

@Wim Leers I think that it was marked as "Needs review" so that a person with a higher level of decision-making authority could weigh in on whether an update hook is needed.

Yes, I was waiting for someone to answer the first question in https://www.drupal.org/project/drupal/issues/3208766#comment-15354646 ✨ Add UUID to sections Needs work before removing the post-update hook. But now I think I'll remove the post-update hook for now and we can add it back if we need it. I am adding the question to the remaining tasks in IS.
Also, updated the IS in general.

I see! Sorry about the confusion. Which comment is #109 referring to? Ever since the GitLab integration, such fragment links are brittle. Please use the notation [#<issue ID>-#<comment number>] in the future 🙏 For example, to refer to your comment, you'd write: #3208766-109: Add UUID to sections → , which renders like this: #3208766-109: Add UUID to sections → .

@Wim Leers I was referring to #3208766-83 ✨ Add UUID to sections Needs work in #109 ✨ Add UUID to sections Needs work .

Created CR [#3428313]

1. Can you please update the change record to show the before vs after explicitly, so that readers do not need to open the class in their local editor to understand the change record? Also: its title is too generic, it needs to be as specific as possible.
2. Can you please update the issue summary to explain the full rationale of why no update path is necessary? We can't expect core committers to re-read the entire >100-comment long issue (plus many dozens of MR comments).

The first is for Drupal developers.

The second is for core committers first, Drupal developers second.

Updated CR and IS.

The issue summary now says:
There is no need of update hook because existing sections are also updated see <code>\Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplayStorage::mapFromStorageRecords() and

\Drupal\layout_builder\Section::fromArray().

… but if I look at the merge request, that file is not updated at all:

This is very confusing.

From the CR:

So I looked at that and … it's just CHANGING the accepted value, detecting the old parameter (integer deltas) and triggering a deprecation error for it (good! 👍) but then … just not handling the old values at all and just pretending it actually was a UUID and not a delta 😱 How is that a BC layer?!

Inevitable conclusion: this MR is not ready, the IS is incorrect and the CR is incomplete.

Per #116, I think this should have test coverage to prove that the BC layer works. The existing testGetSectionWithDelta() and testRemoveSectionWithDelta() only test the deprecation messages, not that the behavior is unchanged.

Hi, putting my framework manager hat on, I think we need an update layer here because the new code assumes that the stored value is keyed by UUID (unless I'm missing something), for example we're assuming that the weights are unique because the new code enforces it, but it may not be for existing config and content.

And its going to be one of those tricky ones where we need to support both content in the database, default configuration from the configuration store, but also install config which may not be in the config store until an optional dependency is found.

If we can write the code in a way that the logic works for both the old and the new structure, then we don't need it. But if I understand the current code in the MR, I think the assumptions in the code that the data is in the new format is problematic as it stands.

Also FWIW I'm very onboard with this idea as it opens up address-ability in JSON:API for layouts

It's LayoutBuilderEntityViewDisplayStorage and not LayoutBuilderEntityViewDisplay, corrected the IS.

and about this in

but if I look at the merge request, that file is not updated at all:

#116 ✨ Add UUID to sections Needs work in the file LayoutBuilderEntityViewDisplayStorage is not updated but the method LayoutBuilderEntityViewDisplayStorage::mapFromStorageRecords() uses a method \Drupal\layout_builder\Section::fromArray(). which is updated here in the MR.

Wonder if possible to close out or mark the threads that are complete.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.