Prettier than#18504, looks great! Maybe a test would be good.

This looks cleaner than having an empty div.

Maybe a test would be good.

@HeahDude Do we have test suites on Symfony for templates? I would be glad to add some. Could you please indicate where to go?

Take a look athttps://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Tests/AbstractLayoutTest.php,https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Tests/AbstractDivLayoutTest.php,https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Tests/AbstractBootstrap3LayoutTest.php,https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Tests/AbstractBootstrap3HorizontalLayoutTest.php.

They are the main files for testing form templating in TwigBridge and FrameworkBundle.

There is alsohttps://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Tests/AbstractTableLayoutTest.php but it should not be concerned here. Maybe it is :)

@HeahDude I added a test. Please tell me if it looks OK.

It looks good to me. But if you can attach screenshot before / after in the PR description, it would be very nice.

@lyrixx I updated the PR body. 👍

Note: We can have one edge case AFAIK:

->add('test', TextType::class, ['mapped' =>false,'label' =>false,'attr' => ['placeholder' =>'My label goes here',    ]])

Renders:

I think this case is very rare comparing to the other, but can still happen.

Maybe could we have anattr "option" like you did with*-inline classes forcheckbox_widget andradio_widget?

Also maybe we should create a new block for the form-group class? Because ATM, user will have to copy/paste the whole logic if they want to change it:

{%blockform_group_class %}    {%iflabelis notsame as(false) -%}        col-md-9    {%-else -%}        col-md-12    {%-endif %}{%endblockform_group_class %}

Or maybe aform_group_class andform_group_without_label_class?

My two cents..

1. Your code will still render theform-group-div. Maybe it's not wrong but having a form-group inside another form-group doesn't look good.
2. I think it's a BC-break removing the "level one"-label column if the label is false. Even if it's rare doing so we should not change the behavior.

Can confirm the form I used in my pr looks the same with this code. 👍

Your code will still render the form-group-div. Maybe it's not wrong but having a form-group inside another form-group doesn't look good.

Theform-group is needed to keep consistent margins. Otherwise, you will get something like this:

Consider this as a row for a form. AFAIK, we can make them nested.

I think it's a BC-break removing the "level one"-label column if the label is false. Even if it's rare doing so we should not change the behavior.

Please see#19247 (comment).

We're solving two different thing here. Collections and nested forms. Consider my nested form type theform-group-div inside anotherform-group is not a clean solution.

Form from my pr

<divclass="form-group"><divclass="col-sm-12"><divid="foo_foo2"><divclass="form-group"><labelclass="col-sm-2 control-label required"for="foo_foo2_child">Child</label><divclass="col-sm-10"><inputtype="text"id="foo_foo2_child"name="foo[foo2][child]"required="required"class="form-control"/></div></div></div></div></div>

When rendering the collection type I see the need for it. Is there a way to know the difference between them?

Regarding BC-breaks. Yes, that comment is what I was thinking of. I don't think we should change the behavior and render the input the whole width.

You mean about:

<divclass="form-group"><divclass="col-sm-12">

?

I don't see the issue. This is like having a.row and acol-sm-12 to me. This does not violate Bootstrap convention AFAIK.

I mean

Maybe it's not wrong but having a lot of nested form types the markup wont look pretty.

The code would be a lot complexified, not sure it worth it just for an esthetic case...

@lyrixx Could we please have your opinion for this and#19247 (comment)?

Agreed! I'm more concerned about the BC-break.

@soullivaneuh Looks like there aren't that many things to do to finish this one, can you have a look at the latest comments? Thanks.

@fabpot I corrected the review and answered for this one:#19247 (comment)

Also I think I could add a commit to improve a little the process.

IMHO, we should introduce some new blocks:

• form_group_without_label_class: Class to put when no label
• form_group_with_label_class: Class to put with an existing label

With the current state of this PR, if we want to change the col classes like this:

{%use'bootstrap_3_horizontal_layout.html.twig' %}{%blockform_label_class -%}    col-sm-3{%-endblockform_label_class %}

We will have a broken form like this:

So we have to overrideform_group_class too. But in this case, we have to copy/paste the vendor logic:

{%blockform_group_class -%}    {%-iflabelissame as(false) -%}        col-sm-12    {%-else -%}        col-sm-9{# This is the ONLY real override here#}    {%-endif -%}{%-endblockform_group_class %}

If we have the two blocks I proposed, this will just looks like this (final state):

{%use'bootstrap_3_horizontal_layout.html.twig' %}{%blockform_label_class -%}    col-sm-3{%-endblockform_label_class %}{%blockform_group_with_label_class -%}    col-sm-9{%-endblockform_group_with_label_class %}

What do you think? Maybe we can find a simplier solution for block name, I waiting your feedbacks for that. But I think we really should avoid the ugly copy/paste constraint.

@soullivaneuh would it make sense to only introduce a new block calledform_group_without_label_class and keep the existing block for the common case of having a label?

Sorry guys, but I don't have any more time to work on this PR right now.

I allowed edit from maintainer, if anyone want to continue it.

@javiereguiluz@HeahDude Can you take over this one? Or anyone else wanting to take over?

Closing as there is apparently nobody wanting to take over this PR. Feel free to reopen.