Add getter for layout object in Layout Builder's ConfigureSectionForm

+++ b/core/modules/layout_builder/src/Form/ConfigureSectionForm.php
@@ -203,4 +210,24 @@ public function getSectionStorage() {
+   * Retrieve the layout being modified by the form.
...
+   * Retrieve the section being modified by the form.

Nit: Retrieves

Needs tests, see #3003666: Provide access to a Section or SectionComponent from within a $form_state for inspiration

I just added the tests in this patch.

While checking the latest patch to add the tests it didn't apply so I had to repatch it to fit the core version 8.9

I added 2 lines to the test so hopefully it does not fail.

Sorry, wrong naming in #10, re-uploading the interdiff

And the missing patch that works (assumptions on how interdiff worked, sorry for spamming)
The interdiff is "interdiff_7-12.txt"

Ran into an issue in contrib space where lb_ux and layout_builder_styles no longer played nicely together. This patch allows layout_builder_styles to get the current layout for the section forms.

+1 to RTBC.

Re: #15, LB Styles is overriding the form class of the layout_builder.configure_section route, and adding a new method. LB UX doesn't touch that route, that part is fine. But on another route, LB UX uses the original form class provided by Layout Builder. And since Layout Builder Styles' form_alter implementation assumes that that its version of that form class will always be used, it breaks.

This was changed to needs work because of a failed test in the media library.

Since the test just passed against 9.1 and there are already 2 RTBC's, I am setting this back to RTBC.

This needs a change record before it can be committed.

@iStryker we already have a patch. Making an empty branch is not very helpful.

Upgrade the patch, so that it applible on 9.2.x

Looks like #25 lost the tests, so this is a re-roll for 9.3.x from #13.

Not sure if I'm able to RTBC when I've done the latest reroll, but this is working for me.

Per #20, I've added a change record that can be reviewed here: https://www.drupal.org/node/3243396

1. +++ b/core/modules/layout_builder/src/Form/ConfigureSectionForm.php
   @@ -114,18 +121,18 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
        if ($this->isUpdate) {
   ...
   +      $this->section = $this->sectionStorage->getSection($this->delta);
   ...
        else {
   ...
   +      $this->section = new Section($plugin_id);
   
   @@ -231,4 +240,24 @@ public function getSectionStorage() {
   +  public function getCurrentSection() {
   +    return $this->section;
   

   One thought. What about having getCurrentSection() contain this logic, wrapped in a !isset($this->section)? Not that there's any chance of this being called before it's set currently, but just for consistency.

2. +++ b/core/modules/layout_builder/src/Form/ConfigureSectionForm.php
   @@ -231,4 +240,24 @@ public function getSectionStorage() {
   +  public function getCurrentLayout() {
   ...
   +  public function getCurrentSection() {
   

   These should have return types in PHP now, in addition to the @return docs

I've added in the recommendations from #29 (assuming I correctly understood the request).

That's exactly what I meant, thanks

1. +++ b/core/modules/layout_builder/src/Form/ConfigureSectionForm.php
   @@ -77,4 +77,11 @@
   +   * The plugin id.
   

   s/id/ID

2. +++ b/core/modules/layout_builder/src/Form/ConfigureSectionForm.php
   @@ -119,20 +126,19 @@
   -    }
   
   @@ -246,7 +252,7 @@
   +  public function getCurrentLayout() : LayoutInterface {
   
   @@ -256,7 +262,16 @@
   +  public function getCurrentSection() : Section {
   

   One nit: no space before the colon, only after.

Nits from #31 applied. For review.

+++ b/core/modules/layout_builder/src/Form/ConfigureSectionForm.php
@@ -179,14 +192,14 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
     if ($this->isUpdate) {
       $this->sectionStorage->getSection($this->delta)->setLayoutSettings($configuration);
     }
     else {
-      $this->sectionStorage->insertSection($this->delta, new Section($plugin_id, $configuration));
+      $this->section->setLayoutSettings($configuration);
+      $this->sectionStorage->insertSection($this->delta, $this->section);

The else case has a new call to setLayoutSettings() (needed since the new Section() call is done earlier now).
This is also in the if, and that one repeats the contents of $this->getCurrentSection().

In fact, this now introduces the only references to $this->section (outside the getter).

I think this could be refactored as follows:

$section = $this->getCurrentSection();
$section->setLayoutSettings($configuration);
if (!$this->isUpdate) {
  $this->sectionStorage->insertSection($this->delta, $section);
}

Sorry for the piecemeal reviews! I really think this is the last one 😅

Refactored with suggestion from #33.

I think this is good to go! Thanks

Back to RTBC per #35 after #3255836: Test fails due to Composer 2.2 solved the unrelated test failure.

I think we should be setting $this->section in the constructor and then

public function getCurrentSection(): Section {
  return $this->section;
}

The reason I'd do this is that $this->delta, $this->plugin and $this->isUpdate are all set in the constructor as well so setting this in constructor is consistent. I realise this is a revert from what was asked in #29. I'm concerned that the method is responsible for instantiation then these things could get out of sync.

Additionally it's inconsistent with $this->layout and the new getCurrentLayout(). this->layout is set in the constructor and retrieved from the section object.

Those are all passed into public function buildForm(), not the constructor.

Given #40, is this back to Needs Review, or is the feedback/NW in #39 still active/applicable?

I think this needs to be reviewed once after comments #39 and #40. So changing status to needs review.

This appears to look good to me. Excited for such a feature!

If there is more to it please move back.

The last patch says 'Build Successful', so manually returning it to RTBC?

So I learned if it says build successfully it means there were so many failures it couldn’t output. I just triggered a 9.5 test to see what happens

9.5 passed so don't see any issue

This is a feature request and should be testing 9.5.x instead of 9.3.x. And let's add a test on D10 as well.

@quietone I am not sure what you mean. I have spun up a Drupal 10 test, which passes. But that is so easy that I suppose you could have done that yourself.
So, then what did you mean?

Committed and pushed 29849ead9e to 10.1.x and 05233f3d00 to 10.0.x and 0660f7a20c to 9.5.x. Thanks!