This chnage is not enough: given that parent forms will have the{% spaceless %} as well, it will also remove these spaces because of the parent rendering. The real fix would be to remove all{% spaceless %} tags

This could even make the form rendering a bit faster, by avoiding to apply a regex processing in many places (especially as{% spaceless %} is applied on each field, not only once).

This chnage is not enough: given that parent forms will have the {% spaceless %} as well, it will also remove these spaces because of the parent rendering.

Good call. The template where I discovered this was usingform_widget directly rather than the largerform_for orform, my focus was a bit skewed.

The real fix would be to remove all {% spaceless %} tags

I can do this, seems like a much bigger change? Would it break BC in any way (I'm thinking not)?

The real fix would be to remove all {% spaceless %} tags

Maybe a better fix is just removing spaces from the higher up blocks likeform_row,form_widget,form_widget_compound, andform_widget_simple?

Removing all the spaceless tags causes a lot of tests to fail.

@chrisguitarguy which kind of failures ?

which kind of failures ?

Lots of things like this:

3) Symfony\Bridge\Twig\Tests\Extension\FormExtensionDivLayoutTest::testWidgetAttributeHiddenIfFalseFailed asserting that two strings are identical.--- Expected+++ Actual@@ @@-<input type="text" name="text" required="required" value="value" />+    <input type="text" name="text" required="required" value="value" />/Users/chris/Code/symfony/src/Symfony/Bridge/Twig/vendor/symfony/form/Symfony/Component/Form/Tests/AbstractLayoutTest.php:1933

Spacelesstrims around the input. I got everything passing again just removing the spaceless blocks from higher up tags (listed above) and using some of twigswhitespace control.

using whitespace control is better in this case. It is also much faster, because it is a compile-time whitespace trimming rather than beign a regex replacement on the rendering output.

I would be in favor of going this way

I would be in favor of going this way

Cool, I'll push some commits in a bit. I'm going to try and add a few test cases for#11277 toSymfony\Component\Form\Tests\AbstractLayoutTest.

👍 for removing all{% spaceless %} tags, I never understood their value.

👍

Not sure what to do about the travis failure here. Looks like two test failures: one is from PHP 5.3 in the Process component, the other is 5.6 in the Debug component. I don't think the changes here caused the failures.

@chrisguitarguy these are tests which are performing some timing assertions. When Travis is under load, their VMs can become slower and thus fail the timing tests even if we already allow a range of result (and increasing the range to fail less often would start to make the test useless).

Note that it is in the Stopwatch component in 5.6, not in Debug

@stof makes sense, thanks for the explanation!

👍 for removing spaceless, there are many still in place

I've got the tests passing with most of the{% spaceless %} blocks removed.

Removing{% spaceless %} around theattributes causes some issues. Spaceless trims offextra spaces when an attribute is set to false. That attribute rendering code can be DRY'd up a bit, so I'll try to take a pass at that a bit later.

@chrisguitarguy{% spaceless %} is useless in this attributes block, because there is no tags in it (which is why it already uses whitespace trimming btw)

{% spaceless %} is useless in this attributes block, because there is no tags in it (which is why it already uses whitespace trimming btw)

I know.

Removing it causes a few tests to fail.{% spaceless %} also trims the entire block.false value attributesdon't get rendered, butthe extra space from them gets trimmed via{% spaceless %}.

So we'd either need to change the tests or change the attributes block rendering a bit.

These are the tests that fail:

• https://github.com/symfony/symfony/blob/v2.5.0/src/Symfony/Component/Form/Tests/AbstractLayoutTest.php#L1924-L1934
• https://github.com/symfony/symfony/blob/v2.5.0/src/Symfony/Component/Form/Tests/AbstractLayoutTest.php#L1961-L1971

Both would probably be better served by$this->assertNotContains('foo="', $html) rather than matching against the whole rendered string.

@chrisguitarguy yes you can change the tests, having an extra space there doesn't hurt

there are still e few spaceless tag which are not removed

there are still e few spaceless tag which are not removed

Sorry about that, thought I caught them all.

well, just do a search on the file :)

well, just do a search on the file :)

I did, ack's whitelisting of files lulled me into thinking I was done. All spaceless blocks should now be gone!

👍

given this is a bugfix, we should apply it to 2.3 (it will also make merging branches easier by avoiding to have huge differences between branches in the template). But I think that the patch will not backport to 2.3 easily because of the differences between branches

👍

Indeed, merging this in 2.3 is not that easy.

@chrisguitarguy Can you submit a new PR for 2.3?

Also there are probably blocks in later branches that are not in 2.3. So just merging 2.3 in later branches won't be enough.

@Tobion that's right (even though I don't think we have added lots of blocks since 2.3), but the work should begin in 2.3 and then be updated for newer branches after the merge (or when merging branches together)

Can you submit a new PR for 2.3?

#11386

Closing in favor of#11386