Form blocks rendered inside layout builder break save

Oh no, that's not good.
Must have broken in #3004536: Move the Layout Builder UI into an entity form for better integration with other content authoring modules and core features.

First step to fixing this is an automated test, tagging for that.
I'd suggest using the User Login form, so we don't have to mess with search.module in our test.

Not sure if it's truly fixable, but in the meantime, here's a workaround to always show a placeholder when a block contains a form.

Marking #3046667: Can not save override layout if layout contains Form API form that fails validation as a duplicate.

I had to deal with some blocks containing content with form tags added directly to templates, so my workaround was so swap all rendered <form> tags with <div> tags. I don't have tests written for this, as I'm not sure it's a great way to deal with the issue.

Super tricky!

This expands (and simplifies the approach from #8) but I'm not sure if it's the right one. Not submitting as patch yet, until we validate if we should actually do it this way.

So basically something like:

      if ($event->inPreview()) {
        /** @var \Drupal\Core\Render\RendererInterface $renderer */
        $renderer = \Drupal::service('renderer');

        $build_copy = $build;

        $markup = $renderer->render($build_copy);

        $html = Html::load((string) $markup);

        if ($html->getElementsByTagName('form')->count() > 0) {
          $build['content'] = [];
          $is_content_empty = TRUE;
        }
      }

would produce:

This is a failing test. (It passes with code in patch #9 and probably #3) But this wouldn't fail for #7, so we should probably expand this and add a block plugin in a test module that renders a form and probably move this to its own test class with that scenario covered as well.

Not setting to NR.

Related issue might help

From a technical perspective, #9 seems to be the safer (albeit nuclear) solution, but from an end-user perspective #8 would be more desirable. It would be useful to see a non-functioning preview of the actual form you want to add to the page, so you can decide if it is the right form or not (etc) without having to go through multiple Save changes /=> View page /=> Edit layout /=> Save Changes /=> View page steps.

So then the question becomes how to effectively show a form without visually disabling the components in the form while also ensuring it doesn't interfere with the outer Layout Builder form. The first part is swapping <form> with <div> like #8 does, but I presume something else would need to be done so that the form elements don't actually submit with the form. Would removing the name="" attributes from all the form elements work?

I've applied patch-8 and when I go to edit the layout I get the following errors:
I'm using Drupal core: 8.7.1.

Fatal error: Maximum function nesting level of '256' reached, aborting! in docroot/core/lib/Drupal/Core/TypedData/DataReferenceDefinition.php on line 48.

Fatal error: Maximum function nesting level of '256' reached, aborting! in Unknown on line 0.

Fatal error: Maximum function nesting level of '256' reached, aborting! in vendor/symfony/http-foundation/Session/Storage/Proxy/SessionHandlerProxy.php on line 65.

Fatal error: Maximum function nesting level of '256' reached, aborting! in /var/www/mndor/docroot/core/includes/bootstrap.inc on line 585.

I just rerolled #3.

Setting back to Needs Review & Triggering bots.

This patch takes a slightly different approach that resolves the issue without having to alter block output.

The patch uses a #pre_render added to the form and a theme hook suggestion to move the layout builder out of the <form></form> tags.

All changes to layouts seem to be made through interactions with the temp-store factory anyway (AFAIK), so nothing from the layout builder element actually needs to be submitted with the form (happy to be corrected on this).

This works a treat for DefaultsEntityForm and OverridesEntityForm out of the box (and doesn't change what users would see at all). If you wanted to display other fields on the OverridesEntityForm then these would always appear above the layout, regardless of weight.

Fixed code style and test errors.

Manual testing of #21 results in a layout tab with no content in it. (workflow actions are present but none of the content or sections or LB interface renders.)

@ccasals is that with overrides or defaults?

@rlmumford - with overrides. We have a site with existing content, including form blocks, which recreates this issue. The forms are a user login form, a webform views exposed filter form, and a custom form plugin. This is NOT a clean install of drupal.

On further investigation it was the exposed view filter that was causing the issue. Here's an update to #14 that makes it an even bigger hammer. Nested to prevent "undefined index['#type']" errors. That test will still fail, but this at least provides a bandaid for any other sites out there that have content editing blocked by this issue.

Edit for clarity: manual testing of #14 did not resolve the issue. Views form was the culprit.

In case context is helpful: The test failing in #25 was added in #2973921: Interactive controls inside preview block in the Layout Builder form should be disabled.. This issue disabled interactive elements (including form fields) in Layout Builder preview. The tests included adding a search block to confirm its form elements were disabled.
If the solution to this issue is winds up being the current approach of removing forms entirely, you can address the test failures by removing the parts of LayoutBuilderDisableInteractionsTest that refer to the search block (assuming there is test coverage elsewhere that confirming that forms wont even get rendered in preview).

I ran into this issue when embedding several Views blocks into a Layout. The patch in #25 worked and allowed me to save the layout changes. However, the content editors managing the layout were surprised to see the Placeholders for the Views blocks rather than the content.

Wondering if there is a way to still show the block content but not have it affect the Save action. Or at least show Views that do not contain any exposed filters or forms. In my case, the block causing the problem was a Solr search form. But that was only one of several views on the page. I'd be fine if the search form didn't render but the rest did. As it stands this sort of ruins the WYSIWYG-like Layout builder experience people were excited about. And yet this is definitely a major issue that has broken sites that were working with Layout builder prior to 8.7.

We were still finding forms that could sneak through. Refactoring the approach in #25 to look more like #8/9 by actually checking for
in rendered content.
The failing test still needs to be updated IF this is an acceptable approach (per points made in #27).

Yes, #29 worked better for me. Normal Views blocks are shown, but not the search form. Layout saves. Nice job!

We were still getting the issue You have unsaved changes On click of Save Layout. I am adding just 2 blocks in the layout a view block and exposed filter block. The version we were using is 8.7.1

Steps to reproduce:
1. Go to any content which uses layout builder.
2. Add any 'view block' to a section.
3. Save the layout. It will be success.
4. Re-open the layout builder.
5. Add any 'Exposed filter block' to the section.
6. Now try to save the layout. You can see You have unsaved changes.

Simple update to #29 to work with php 7.1.x sites. "count" is not an available method on https://www.php.net/manual/en/class.domnodelist.php prior to 7.2.

I encounter the same bug.

Patch #32 Works for me :)

Ok I just updated the patch to use the interface in the dependency injection instead of the class directly.

I discovered a less disruptive solution for the search form while working on #3025231: Concurrent editing of layouts is very confusing.

In \Drupal\layout_builder\EventSubscriber\BlockComponentRenderArray::onBuildRender() just after $content = $block->build(); I added

if (isset($content['#type']) && $content['#type'] === 'form' && $event->inPreview()) {
        unset($content['#theme_wrappers']);
      }

When #theme_wrappers was present, the Layout Builder form's CSRF token was being rendered outside of the form.

This addresses the inability to save without any UI changes.

It's possible that this only works in combination with other changes made in the issue - I have not tested that. If just the change in onBuildRender() does not fix the issue for search form, check #3025231, which should soon have a patch with that + the other changes that fix the save issue.

Updates for unit tests, building on update in #34. The functional js test (LayoutBuilderDisableInteractionsTest) will still fail

Also opened a new issue: https://www.drupal.org/project/drupal/issues/3060656
This is for a longer term better approach that might still allow rendering of forms, if that's possible.

Triggering the bots to ensure unit tests pass

This patch is an entirely different approach so there's no interdiff as it's starting from scratch.

With this approach, there is no visible difference with non-views forms, in a render array, it removes the #theme_wrapper key from any item where #type === 'form'

I couldn't do it as cleanly for views blocks with exposed filters since the logic is inside the views executable object. In this case, the views exposed filter form does not render. On the plus side, the rest of the view renders just fine, so it's still a more robust preview than just a label.

I'm not quite sure that'll work, since I believe required form elements still add validation messages when saving the layout. It might be worth coming up with a few test forms to use as the baseline and for testing whichever chosen solution we go with.

Would #41 still result in actual nested <form>..<form>..</form>..</form>? It seems like it would, but maybe I am missing the significance of those theme wrappers.

Even if we transform <form> tags to <div>s, this could still break styles that targeted form elements directly which would be hard to solve for and communicate around I would think.

Added tests that replicate the steps that cause the problem to happen manually, but the tests won't fail. This is not entirely surprising as there are existing tests that also should have failed as they include adding a block with a form and saving a layout.

Fortunately (albeit confusingly), the changes made in #3025231-67: Concurrent editing of layouts is very confusing -- a test-passing patch -- make it so manual and automated testing are consistent. Most likely this patch made it possible to surface these errors in tests because AJAX rebuilds were changed to target the entire entity form instead of just the Layout Builder render element. Based on this, I've attached the following:

1. 3045171-44-TEST-ONLY.patch Recreates the steps required to cause the problem reported in this issue. There are iterations with the Search Block, Login Block, and a View with exposed filters. Despite recreating the steps that surface the problem manually, these tests pass but shouldn't.
2. 3045171-44-TESTONLY-PLUS_3025231-67.patch All the changes in #1 + adds the working patch from #3025231-67: Concurrent editing of layouts is very confusing . (minus one change to BlockComponentRenderArray which was added to address the symptoms reported here). The tests now fail as expected.
3. 3045171-44-with-3025231-67.patchEverything in #2 + the fixes I added to BlockComponentRenderArray in #41 The tests should pass as expected (running the test again, there was an unrelated fail due to a known media library issue, but the Layout Builder tests pass as expected)
4. 3045171-44.patch The tests from #1 + the fixes I added to BlockComponentRenderArray in #41. Tests pass because these tests pass without the patch from 3025231...

Talked with @tim.plunkett about #44 and we concluded this should be postponed until #3025231: Concurrent editing of layouts is very confusing is completed, as it isn't possible to write test coverage for this issue without it.

In the meantime 3045171--44.patch from #44 should take care of the issue with minimal changes to the Layout Builder UI.

For what it's worth 3045171--44.patch works great for Drupal 8.7.3.

I had originally used a workaround similar to #44 in our project, but realized it didn't solve the issue for webforms as mentioned in #7, or other render elements that have <form> tags in the template.

I think behavior I have observed is that such blocks might save in LB the first time they are added to the layout, (though possibly not if the form contains any required fields). However, saving after going back and re-editing the layout subsequently will fail.

Good lord, I just ran into a combination that #44 doesn't address. I have a view with an exposed for in QuickTabs. Those form tags are still present in LayoutBuilder. Not sure how to proceed. I guess I could temporarily disable the exposed filters, update the layout, then re-enable. Let's how we can get #3025231 in quickly :)

Dang. Almost feel like this bug is borderline critical. In my case, if I want to edit a layout with a form(s), I need to remove the forms, make the edits, save -- then re-edit and place the forms again.

@mpaler is the most recent patch solving your issue

@tim.plunkett -- sadly it's not. I'm on D8.7.3 -- and I have https://www.drupal.org/files/issues/2019-06-19/3045171--44.patch applied. Just tested again. I my case I have a straight up content block with a
inside the block. Trying to save and it never saves, even if i toggle "Show content preview". Removing the block saving a few times (with and without "Show content preview") and the node saves.

FWIW, I also have the following patch applied:

https://www.drupal.org/files/issues/2019-06-18/3053906-11-default-region...

Thanks for all you guys do!

hi all, do you any thing new today?

@mpaler two things would help narrow down why the current fixes are not working for you

• It's possible that 3045171-44-with-3025231-67.patch may work for you where the main patch from #44 did not. If you're able to check with that patch, please share the outcome here. To be on the safe side, manually clear caches after applying the patch.
• If you're not able to test with that other patch, or it does not address you issue, we could use more details about the save-breaking form content in your block so we can reproduce the problem locally and expand the fix to address your use case.

@bnjmnm,

3045171-44-with-3025231-67.patch is not applying on 8.7.3 -- even with no other core patches.

Going back to 3045171--44.patch and going to work around for now.

Here's my custom block code in entirety, minus the complete post destination:

<div class="row">
<div class="col-xs-12 col-sm-6 sign-up">Sign up for our monthly newsletter</div>

<div class="col-xs-12 col-sm-6 col-lg-5 form">
<form action="https://go.pardot.com" method="post"><input class="email-address" id="vresp-email" name="email_address" placeholder="Enter your email address here" size="15" type="text" value="" /><input class="btn btn-primary" id="vresp-submit" name="op" type="submit" value="Sign up" />&nbsp;</form>
</div>

<div class="hidden-xs hidden-xs hidden-sm hidden-md col-lg-1 spacer">&nbsp;</div>
</div>

Thank you.

I just want to mention that I'm still using #3 with the modifications mentioned by #7 (I was testing it with a web form) with great results.

It removes the form and displays a text which is not ideal but it lets the UI usable for the user while this is fixed properly :-)

@gnuget -- thanks for the tip. Went that route as we are on a tight timeline. Attached is a re-roll with the webform conditional in case it helps anyone else in a similar crunch.

Any thoughts on how to deal with more deeply nested forms, such as with QuickTabs with Views blocks containing exposed filters? I haven't tested against Block Tabs or related modules, but I am wondering is this patch would address such cases.

@ccasals had an approach earlier that rendered the HTML and parsed the DOM. I think that's the only bulletproof solution that would also address hand-written forms like the one in #56
That's been split out to #3060656: Explore a better solution for nested forms in Layout Builder

Hi, @scottsawyer.

I think #58 should work for your case as well.

the containsForm is recursive and it will iterate in all the render array.

If somehow the form is already rendered you could try #36 instead which render first the markup and after that check for the form tag.

@gnuget Could not apply #58 to 8.7.3. I have a few other core patches installed, I am going to see if they touch any of the same files.

• https://www.drupal.org/files/issues/2019-05-02/2663316-70.patch
• https://www.drupal.org/files/issues/2019-04-16/2174633-views-273.patch
• https://www.drupal.org/files/issues/2018-12-06/2820347-105.patch

@scottsawyer -- that was my bad. Please try this one.

Ok, that patch applied, thanks for jumping on it.

However, I still get a form tag in my QuickTabs block in layout builder when the QuickTab contains a views block with an exposed form, and still unable to save the layout with the QuickTabs block.

#36 is the way to go then :-)

I even think that #36 should be committed and any different/better solution should be explored on #3060656: Explore a better solution for nested forms in Layout Builder

With #36 I am able to save a layout with QuickTabs. Thanks everyone for your efforts.

This builds off of the approach in #36, but instead of hiding the blocks entirely, it replaces all occurrences of <form> with <div>. This preserves the UI.

Tests were also expanded to cover the hard-coded form use case reported in #53

If you're going to use a regex to perform the replacements, then I suggest not loading into a dom object at all to verify if a form tag exists. I would just use a regex to test if the form element exists as well. Regex is not super reliable for HTML detection and manipulation BTW, since it can take so many different formats. Like < form>... </ form >.

We can be reasonably sure that any <form> tags generated by Drupal will be found here.
Any custom code with malformed HTML isn't really something we can be responsible for...

I agree that parsing form tags is hazardous, but it should be rare to have irregular, though not illegal form tags, so unless a better approach is presented, I think this is a 99% solution.

Here's a patch using DOM shenanigans to convert the form. Also removed detokenizeNestedForms() since it did not appear to be in use any longer.

#66 & #71 is not working when we have a block type that inserts a media field (uses media browser in modal).

Noting: The patch in #3042190: Creating a block that uses Ajax adds multiple blocks solves the problem for me when we have block types that have media fields.

@tim.plunkett (#60) in #21 theres a patch that takes the rendered layout builder out of the form. There was a bug with the overrides, but that approach seems to solve all of the above issues for any form, and renders a realistic preview of the form. Is that not ideal?

philosurfer, can you provide steps to reproduce?

I applied patch from #66. In layout builder, I can add a views block that has exposed form and save just fine. I can also add a custom text block that has a media reference field on it and have no problem selecting a media item from the media browser (both core's media library widget and the entity browser contrib module widget).

The current approaches perform the modifications in \Drupal\layout_builder\EventSubscriber\BlockComponentRenderArray but this is not correct and is particularly problematic for the approach in started in #67. This approach performs early rendering of the section render array, and as such prevents other subscribers to the event LayoutBuilderEvents::SECTION_COMPONENT_BUILD_RENDER_ARRAY from acting on the render data. So this breaks contrib (like the Layout Builder Styles module).

Instead, the early rendering and form replacement could be done in SectionComponent::toRenderArray(), after all event subscribers have finished.

But even then, it seems dangerous to do this early rendering, since layout templates may want to modify the render data and will no longer be able to.

I wonder - is a pure JavaScript solution a possibility? Why can't we swap the <form> tag with a <div> using JS?

I've rerolled/downrolled the patch from #71 (without tests) so that it can apply to 8.7.1, if it helps anyone else.

Confirmed in 8.7.5 version build with webform version 5.1.0 build, not available any save both - multilingual and single language,
any possibility to roll patch with backport to previous version?

regards,
Mike

Re-roll of patch in #21.

#77 works good on 8.7.5 using Webform w/ Layout Builder

Hi all,

This came up recently in conversation so I figured I would clarify here: there are multiple approaches to patching this problem in this issue thread! If you are just looking to patch your site, please patch carefully.

Moving latest to oldest:

Patch thread ending in #79 (started from #21): this approach take the entire layout builder form out of rendering, then re-adds it using a preprocess hook. Has not been tested by the community yet.

Patch thread ending in #77 (builds on #71, #66, #44, #41): this approach renders each block before placing it in the form and if it finds <form> elements it converts them to divs. It will work on any block with a form in it (no matter how deeply nested). This means that forms will probably look right, but are mutated html while in the Layout Builder preview. This has been reviewed and tested by the community and bypasses the problem.

Patch thread ending in #63 (builds on 58): this approach looks for content with #type set to 'form'. If it is set to form, it uses the "placeholder for [block name]" functionality to prevent break. This will work for many types of forms, but there are cases where it won't catch nested forms (aka, in a view block with filter). YMMV

Patch thread ending in #36 (builds on 34, 32, 28, 25, 14, 3): this approach renders the block content, looks for a <form> element. If it finds a form in the block, it uses the "placeholder for [block name]" functionality to prevent break. It will work on any block with a form in it (no matter how deeply nested). This has been reviewed and tested by the community and bypasses the problem.

I think at this point, we need to make a decision on which of these approaches is best on balance, and doesn't impact things like accessibility (I have concerns about malforming semantic forms to look like divs, for instance. I think assistive technologies will probably choke on this.) Every approach has a tradeoff though, so we're going to need to pick which one is least problematic.

#77 Works for my. TY

@ccasals Anyway need remove or replace child tags

+1 for patch #77

patch #77 work for me too.

I'm checking and #77 really works well for a lot of members.

I applied the patch, tested and it works here as well.

Updating status for RTBC.

Thank you, guys.

Sorry, but I do not think this is a viable solution. I understand it's working for many people, but the forced early rendering - particularly where it's being done - would prevent some contrib modules from working. See my comment in #76

Re-adding the "Needs tests" tag as the patch that has tests does not appear to be the one people are in favor of.

This issue can't be considered RTBC without a test that replicates the error when unpatched, and has no errors when it is patched

Hi,

Applying patch #77 gives me below error,

`ArgumentCountError: Too few arguments to function Drupal\layout_builder\EventSubscriber\BlockComponentRenderArray::__construct(), 1 passed in /var/www/globalcms/docroot/core/lib/Drupal/Component/DependencyInjection/Container.php on line 269 and exactly 2 expected in Drupal\layout_builder\EventSubscriber\BlockComponentRenderArray->__construct() (line 52 of core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php).`

In our case we are following below steps,

1) We are adding embed form in block.
2) Add the block through layout builder.
3) Able to save the layout.
4) Edit and make any other change and hit save.
5) Unable to save with message `You have unsaved changes`

@Rohit Tiwari, clearing Drupal cache after applying patch should fix this error.

Patch submitted in #79 works well.

I'm using Drupal 8.7.8.

@bkosborne none of the approaches from #81 is working for your usecase? #63 seems to be doing the same thing as you describe.

Hi mirom, my concerns with the approach taken in #77 is outlined in my comment #76. To recap, my issue is that it forces early rendering of the block component before other event subscribers have a chance to alter the render data. This early rendering is performed even if there is no form present in the data.

Contrib that has event subscribers to this block component render event will not have a chance to act on the render data in a meaningful way. For example, the Layout Builder Styles module subscribes to this same event to add CSS classes and template hints to the block. That cannot happen now because the render data has been converted to just #markup with HTML already rendered.

The solution is to do this form replacement in a different area, after the event subscribers have all run.

What about an additional event subscriber that is set with -1000 priority that performs this rendering?

Can't it just go in SectionComponent::toRenderArray() after all the subscribers are done?

A couple reasons.

1. I like how relatively clean the method is right now
2. Adding this by an alterable mechanism allows a custom module to prevent this from running or replace the functionality if needed
3. Similarly, if needed, a custom module could run *after* this code

Ok, that makes sense.

I suggest also that the code be modified so that it doesn't perform the early rendering if there is no form found. Right now it's updating the render data by reference. I think it should clone the render data first, and then if a form is found replace the original. That reduces the side effects this may have.

Building upon #77 so items that don't have forms, don't render the content as part of this process and therefore skipping theme preprocessing and templates. In my case, after using patch #77 it fixed the save issue as advertised but disrupted the display of existing themed blocks.

#99 Works for my.
Thank guys

#77 works fine and #99 works fine too . Thanks

But still a problem ... The form has to be the only block in the section.

If i add the form and a view in the same section i get this 'you have unsaved changes' again

Here's the same code as #99, but it's moved to a separate event subscriber with a very low priority. This resolves my original concern.

I cannot reproduce the problem from #101. I added several blocks to the section, including several form blocks, and I was able to save the layout. Can you provide more detailed steps to reproduce?

Empty file uploaded by accident... Those Gitlab integrations can't come soon enough...

I was seeing the same as #101, but switching to the new patch in #103 allowed me to save the node. In my case, I was embedding two product entities via the WYSIWYG in two custom blocks added in the same section. The products render inside an add to cart form.

I have tested patch #103. It works for me. I have added the layout builder in one of the content type and tried managing the content layout. I added search form block or any form entity or node block , it is saving the layout but if I make changes in the block it is showing me unsaved changes tag even after clicking on save layout.
But later i applied the patch #103 it is save the layout without showing the unsaved changes tag.

Hi @shimpy, this issue still has the "needs tests" tag, so it should be marked as "Needs review" or "Needs work" until those are done.

Hi,

I have checked the patch and it worked as design.

Below are my updates.

1). I have enabled the layout builder for the "basic page" content type.
2). I have added a search form using a layout builder. After that, I was not able to save the layout for a basic page.

3). I was able to save the layout after applying the patch.

4). Also, I have added a view in the same section below the search form and it saved successfully.

There was one error as per the Drupal coding standard. I have fixed it and uploaded the patch with interdiff.

Thanks,
Ren

I have tested the patch provided in #107. It applied cleanly and worked as expected.
Here is my test usecase:
- Patch tested on a fresh install of Drupal 8.9 with webform module enabled.
- I conducted testing on basic page content type, adding a few different form elements (either one form at a time or in a few different combination)

• Webform block
• Search block
• Comment block
• A views block with exposed filter

The patch worked as designed in all different combination of content.
Cheers

@kualee, unfortunately the patch still needs tests. NW for those.

@Sam152 ah sorry I have just re-read your comment in #106. Would you be able to provide some guideline/point me to the direction to work on the automated test? Really keen to get this issue RTBC but I have never contributed anything so not sure where to start. Thanks!

Hi @kualee,

The documentation to get started on writing tests for core issues is here

https://www.drupal.org/contributor-tasks/write-tests

Without looking too deeply into it, I'd suggest scouring the Drupal 8.9 source code for examples of tests that relate to Layout Builder and possibly tests that relate to event subscribers to get an idea of how tests are being written elsewhere in core.

#71 has tests if you want a starting point for adding one to #103.

And, thanks for the work everyone.

#105 worked for me, thank you everyone.

Spotted a spelling error. $orginal_content should be $original_content.

#107 worked for me. I have corrected the spelling as per #115. Please Review.

#116 worked for me.

Thanks.

1. index 76faef1..80ad9fa 100644
   --- a/core/modules/automated_cron/automated_cron.module
   
   --- a/core/modules/automated_cron/automated_cron.module
   +++ b/core/modules/automated_cron/automated_cron.module
   

   Please remove these unrelated changes.

2. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentPreviewFormReplace.php
   @@ -0,0 +1,108 @@
   +class BlockComponentPreviewFormReplace implements EventSubscriberInterface {
   ...
   +   * Creates a BlockComponentRenderArray object.
   

   Wrong docblock

3. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentPreviewFormReplace.php
   @@ -0,0 +1,108 @@
   +   * Change forms to divs if in layout builder's preview mode.
   

   Changes

4. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentPreviewFormReplace.php
   @@ -0,0 +1,108 @@
   +   * Convert form tags to div when displayed in the Layout Builder UI form.
   

   Converts

I think this also needs an empty post_update hook to allow the new service to be discovered after updating. See layout_builder.post_update.php for examples.

Finally, this still needs tests

Have implemented changes suggested in #118.

Changes in #120 are fine, but this still needs tests.

Patch in #120 works fine for me.

Here is a failing test. 3 different types of blocks are added to both default and overrides layouts:

• Block containing form api form
• Block containing inline template which renders a form tag
• Views block with exposed form

I think these three should provide comprehensive (if not redundant) coverage of blocks containing forms.

It seems that the initial adding of the blocks and saving may succeed, it's just after subsequent editing of the layout with the blocks already in place, it's not possible to save (or discard or revert).

deleted*

I still think the approach outlined in #120 is misguided. It changes the POST data submitted when someone clicks the save button. If there are buttons in the form blocks, clicking them will have the effect of submitting the form.

For example, if any of the forms in the blocks contain a button with the name "Discard Changes", clicking it accidentally or otherwise will result in the layout changes being discarded.

Any form input in the block forms will still be submitted when the layout form is submitted - this just seems wrong. I'm guessing that means that a form_build_id will be submitted for every form displayed in the layout.

Taking the layout out of the form entirely is a better way to go, as it ensures that only the expected data is being submitted when you click "Save Layout" and "Discard Changes". It also stops any submit buttons in the layout blocks having any effect on the layout form. This is the process outlined in #79 and #21.

Reroll of #79 with a test. The test uses the user login form, but i don't know if that will render because the user will be logged in. The search form might be a better fit.

Moved the pre render callback into a class for security.

I like the idea in #131 of rendering the layout builder element outside the form. I have an alternate approach to do that by decorating the entity form controller service so that a new theme entry isn't required.

But first, a fix/update to the tests in #124

Patch with fix and tests.

Realized logic in the controller could be simplified.

Probably better to delay moving the layout builder element outside the form render array at least until after the form is built.

The patch in #136 works amazingly for me.

+1 RTBC
I have tested the patch in #136 and it worked for all of the scenarios that I tested
- Webform
- Comment form
- Views with exposed filter
Tested with either one form at a time or combination of a few
Awesome work @godotislate
And thanks @jwilson3 @damontgomery for the suggestion, I ended up never finding the time to get around to do it :(
Maybe someone could review the source code? I only performed all the tests, not reviewing the actual code. I will leave this as needs review for now

This is soooo close now! Just a couple small things. But I think the approach is inspired, and I'm excited to see it work out so cleanly

1. +++ b/core/modules/layout_builder/layout_builder.services.yml
   @@ -51,3 +51,10 @@ services:
   +    # Override the entity form controller to handle the entity layout_builder
   +    # operation.
   +    decorates: controller.entity_form
   +    class: Drupal\layout_builder\Controller\LayoutBuilderHtmlEntityFormController
   +    public: false
   

   👏🏻👏🏻👏🏻

2. +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderHtmlEntityFormController.php
   @@ -0,0 +1,58 @@
   +class LayoutBuilderHtmlEntityFormController {
   +  use DependencySerializationTrait;
   

   Nit: missing a blank line here

3. +++ b/core/modules/layout_builder/tests/modules/layout_builder_form_block_test/src/Plugin/Block/TestFormApiFormBlock.php
   @@ -0,0 +1,115 @@
   +class TestFormApiFormBlock extends BlockBase implements ContainerFactoryPluginInterface, FormInterface {
   ...
   +   * SearchBannerFormBlock constructor.
   

   Bad copy/paste here

4. +++ b/core/modules/layout_builder/tests/modules/layout_builder_form_block_test/src/Plugin/Block/TestFormApiFormBlock.php
   @@ -0,0 +1,115 @@
   +   * @var \Drupal\Core\Form\FormBuilder
   ...
   +   * @param \Drupal\Core\Form\FormBuilder $form_builder
   ...
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, FormBuilder $form_builder) {
   

   FormBuilderInterface

5. +++ b/core/modules/layout_builder/tests/modules/layout_builder_form_block_test/src/Plugin/Block/TestFormApiFormBlock.php
   @@ -0,0 +1,115 @@
   +   *   The plugin_id for the plugin instance.
   

   The plugin ID for the plugin instance.

   (there's another issue about fixing it where you copied it from)

6. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderNestedFormUiTest.php
   @@ -0,0 +1,165 @@
   +  protected $defaultTheme = 'classy';
   

   Can this be done with stark instead?

Thanks for the review! Updated patch per comments.

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderNestedFormUiTest.php
@@ -12,7 +12,7 @@
-   * The form block labels used a add block link text.
+   * The form block labels used as text for links to add blocks.

Nice, I didn't even notice this, thanks for fixing.

#140 addresses all my feedback, RTBC

+1 RTBC
Might be a bit overkill, but I have just retested all the scenarios I did in my previous test with patch #140 applied.

@kualee in this case, it was overkill because the last set of changes were only to documentation and some tests. But it's good practice and a good habit anyway, and I appreciate your thoroughness! Thanks!

@godotislate thanks for taking that approach and running with it! Some really good improvements and I'm looking forward to this getting committed!

I was using #77, today I tested with #140 and really works well here.

+1 to #RTBC

This looks amazing! +1 RTBC.

+1 RTBC Works very well on a layout with several embedded views blocks with 50+ forms inside them

Nice! Patch #140 fixed an issue we were still seeing while using #103 with a custom block that was posting to mailchimp. Thanks!

Confirming this patch also worked great on a site I maintain, was able to delete a lot of custom code that emptied forms out on layout builder pages.

One note on the implementation, you could avoid overriding controller.entity_form by specifying a custom _controller for just the layout builder form route that required this fix.

As I'm writing this, I've realised if the layout builder widget found itself on other entity forms somehow, the fix would then fail to be applied, so not sure it's actually an improvement. +1 RTBC.

Does anyone think it's worth moving layoutBuilderElementGetKeys() to a trait?

Testbot fluke (The "https://packages.drupal.org/8/drupal/provider-2019-3%249934973132b9718f8..." file could not be downloaded)

I don't think the two identical one-liner static methods warrant a trait.

Thanks you so much guys!
#140 is working. Drupal 8.8.1

Before committing this it'd be great if someone could update the issue summary to reflect the current solution because it doesn't.

This looks like really nice work and a great test.

IS updated.

@godotislate thank you for the issue summary update.

Committed and pushed 0a16b0f112 to 9.0.x and 87bdfdfc49 to 8.9.x. Thanks!

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderNestedFormUiTest.php
@@ -0,0 +1,165 @@
+    $assert_session->assertWaitOnAjaxRequest();
+    $assert_session->linkExists($label);
...
+    $assert_session->assertWaitOnAjaxRequest();
+    $page->pressButton('Add block');
+    $assert_session->assertWaitOnAjaxRequest();
+    $assert_session->pageTextContains($label);

Can we get a follow-up to replace these with:

• \Drupal\FunctionalJavascriptTests\JSWebAssert::waitForLink
• \Drupal\FunctionalJavascriptTests\JSWebAssert::waitForButton
• \Drupal\FunctionalJavascriptTests\JSWebAssert::waitForText

diff --git a/core/modules/layout_builder/tests/modules/layout_builder_form_block_test/src/Plugin/Block/TestFormApiFormBlock.php b/core/modules/layout_builder/tests/modules/layout_builder_form_block_test/src/Plugin/Block/TestFormApiFormBlock.php
old mode 100755
new mode 100644

I fixed the file mode on commit.

+++ b/core/modules/layout_builder/tests/modules/layout_builder_form_block_test/layout_builder_form_block_test.info.yml
@@ -0,0 +1,6 @@
+core: 8.x

This broke the D9 build (obvs and oops) - test modules no longer need to declare a core version. I hotfixed this.

+++ b/core/modules/layout_builder/layout_builder.post_update.php
@@ -190,6 +190,13 @@ function layout_builder_post_update_update_permissions() {
 
+/**
+ * Clear caches due to addition of service decorator for entity form controller.
+ */
+function layout_builder_post_update_override_entity_form_controller() {
+  // Empty post-update hook.
+}
+

If this is only to rebuild the container, it doesn't need an empty post-update - since the container cache key includes Drupal::VERSION

If it's to clear caches too then we do need it.

I am using Drupal 8.8.2, can i install this patch from #140 to my Drupal install? I noticed on the patch page it says it is for 8.9.x branch.

Just to mention it, #140 applies cleanly against 8.8.x, is there any chance this could be committed there too? Thank you.

@rfletcher73: Yes, you could use patch #140 against your 8.8.x codebase.

Thank you. I am having problem installing it on my local drupal build for testing.

I originally installed Drupal with Composer so i think i should use Composer to install the patch file too.

I've updated my composer.json file by adding this block under extra: {..}

"enable-patching": true,
        "patches": {
            "drupal/layout_builder": {
                "Fix layout builder save issue": "https://www.drupal.org/files/issues/2020-02-04/layout-builder-save-issue-3045171-140.patch"
            }
}

Then from the command prompt, i am running composer install. I am getting response back in the tmerinal, but it is not installing the patch. What am i doing wrong here?

@rfletcher73: The composer.json file's "patches" item should refer to the package that the patch is applied to, which is "drupal/core" and not "drupal/layout_builder", e.g.:

            "drupal/core": {
                "1356276 - Allow profiles to provide a base/parent profile and load them in the correct order": "https://www.drupal.org/files/issues/2019-12-02/core-1356276-598-89x.patch",
                "#3045171-140: Forms break Layout Builder saving.": "https://www.drupal.org/files/issues/2020-02-04/layout-builder-save-issue-3045171-140.patch"
            },

That is what I had at first, but when i ran composer install, it didnt pull the patch in. So, i guessed Layout Builder being a module needed the module way of updating packages. I guessed wrong. :)

I changed it back to drupal/core. I also updated the package information to match what you have, but I think that doesn't have any effect on it, its just there to describe what the patch does. Is that right?

With those changes, when i ran composer install, it worked!!! My first patch applied to Drupal. Thank you!!!

Could a release manager please review whether this could be backported to 8.8.x? It affects production sites today (which is how I came across it). Thank you.

I think it would need a new patch that combines in the hotfix from #163

The patch still applies cleanly to 8.8.x and (in my personal testing) resolves the problem, moving back to RTBC.

Using the 3045171-140.patch with 8.8.x-dev

The patch from #140 also solves our case and our form consists of all sorts of types of fields when the issue was occurring, dependent fields, custom fields, computed fields, inline entity forms, comment fields etc. +1 to RTBC.

Patch #140 works great with core 8.8.4. Thanks!!!

I got this problem when use layout builder and block webforms.
It is good with patch #140 (core 8.8.4).
Thanks a lot.

Same for me - #140 works a charm - thank you for everyone's work on this :-)

Thank you @godotislate and all for the patch.
On Drupal 8.8.4, the #140 worked for saving the basic page.

Still I keep having (since Drupal 8.8.3) a symptomn : the webform block doesn't show up on a basic page. I am not sure if it is related.
@nguyen.phan, do you see this issue ?

EDIT : In fact, the webform block is fine. There is no trouble now ;-)
The issue was that the setting Allow each content item to have its layout customized was not set anymore. This went unseen because the layout of the basic page still could be customised.

Confirming RTBC for #140 on 8.8.x, thank you all very much!

#140 worked me. Thanks!

Discussed this with xjm, and neither of us are very keen on backporting this to 8.8 (new post update, new service etc.), running the patch against 8.8 is of course viable, and it's not too long before 8.9.0 gets a release candidate.

Moving this back to fixed against 8.9.x

Great stuff!

The new LayoutBuilderHtmlEntityFormController does not comply with decoration contract though, causing a regression.

Hi all,

The #140 patch got applied to our version 8.8.4 but the layout builder is not saving still. We are using an exposed form filter view inside the layout builder. If i remove the view from the page then I can save it.

Update: Cache clear was needed after the patch. working now!!

As per #181.

One thing to note about not backporting to 8.8, is that install profiles using composer-patches have no way to conditionally apply patches to specific older versions of projects, which then requires individual sites to manually add the patches-ignore section. Just something to consider for bug fixes, but I get the hesitation about why not to backport this issue.

Was this applied to the 8.9 production release a few days ago?

Exactly. Production 8.9

Thank you for the work on this patch. I ran into this issue today with Drupal 9.0.7 trying to display a search filter in layout builder. I see that it was committed to 9.0.x. Does anyone know when this would be pushed to a D9 stable release?

This issue has not been fixed in 9.x. I just encountered it with 9.1.4.

Yes, I have the same on 9.2.0, I think issue should be reopened.

need restore section storage from tempstore to resolve it.

I can confirm this is still an issue in 9.3.3.

@rskleven this was fixed in Drupal 8.
If you've got a similar issue in D9, its a different problem - can you please open a new issue with the details of your scenario?

encountered this issue on drupal 9.4.5

To reinforce what @larowlan said in #194 - commenting that you're having a problem a closed issue is not going to result in anything getting fixed. Two year old issues aren't reopened, especially one that automated test coverage objectively proved to have fixed some issues.

It looks like some people are experiencing additional bugs and if those were reported in a new issue something could actually be done about it. Details on how to reproduce the issue or providing a failing test would be necessary, too. The The steps to reproduce in this specific issue no longer cause an issue. It's certainly possible there's a use case that was not accounted for that could be fixed, but "I'm still getting the problem in (version)" is not enough to go on.

If you're just commenting to vent, vent away. If you're hoping a comment here will result in a fix, it won't. There needs to be a new issue reporting the bug, and there needs to be specific steps on how to reproduce the problem.

Re-rolling the patch for 10.3.X

#197 worked for me

Just noting, I had the same issue with a views exposed form inside the layout builder layout.

On save, nothing except "op" was submitted and nothing was saved.

The attached patch in #197 fixed the issue for me too, in Drupal 11.2.x