Remove Spaceless Blocks From Twig Templates #11278

chrisguitarguy · 2014-07-03T01:44:49Z

Leaving it in can only mangle values from data bound to the form.

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#11277
License	MIT
Doc PR

The tests pass here, but it doesn't seem like any tests really cover the actual rendering.

Leaving it in can only mangle values from data bound to the form.

stof · 2014-07-03T07:34:48Z

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).

chrisguitarguy · 2014-07-03T12:07:10Z

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 using form_widget directly rather than the larger form_for or form, 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)?

chrisguitarguy · 2014-07-03T12:43:04Z

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

Maybe a better fix is just removing spaces from the higher up blocks like form_row, form_widget, form_widget_compound, and form_widget_simple?

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

stof · 2014-07-03T12:50:51Z

@chrisguitarguy which kind of failures ?

chrisguitarguy · 2014-07-03T12:54:22Z

which kind of failures ?

Lots of things like this:

3) Symfony\Bridge\Twig\Tests\Extension\FormExtensionDivLayoutTest::testWidgetAttributeHiddenIfFalse
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-<input type="text" id="text" name="text" required="required" value="value" />
+    <input type="text" id="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

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

stof · 2014-07-03T12:56:40Z

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

chrisguitarguy · 2014-07-03T12:58:12Z

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 to Symfony\Component\Form\Tests\AbstractLayoutTest.

See symfony#11277

Removes the spaceless widget from: - form - form_row - form_rows - form_widget Instead, we use whitespace control to trim around the edges of those blocks.

rybakit · 2014-07-03T13:40:51Z

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

stof · 2014-07-03T13:53:31Z

👍

chrisguitarguy · 2014-07-03T13:56:52Z

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.

stof · 2014-07-03T14:00:49Z

@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

chrisguitarguy · 2014-07-03T14:02:13Z

@stof makes sense, thanks for the explanation!

Tobion · 2014-07-03T15:13:59Z

src/Symfony/Component/Form/Tests/AbstractLayoutTest.php

@@ -1969,4 +1969,30 @@ public function testButtonAttributeHiddenIfFalse()
        // no foo
        $this->assertSame('<button type="button" id="button" name="button">[trans]Button[/trans]</button>', $html);
    }
+
+    /**
+     * @group ticket-11277


if we agree to use group it should probably be the full url.
but https://github.com/chrisguitarguy/symfony/blob/bug_11277/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php#L1219 just uses a comment

Just let me know what the correct way to do it is. I acked for group annotations and saw a few ticket-XXX and just went with it.

@Tobion using a full url as group names makes it quite unusable (will you really pass the full url as CLI argument to filter things ?)

I would remove the group entirely here (filtering by ticket can be useful while you work on the PR, but it is not useful anymore once the PR is merged)

I would remove the group entirely here

Do you want a comment with the ticket link like @Tobion linked?

I would remove the group as well. There is no need to link to the ticket.

Tobion · 2014-07-03T15:32:44Z

👍 for removing spaceless, there are many still in place

chrisguitarguy · 2014-07-03T16:29:57Z

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

Removing {% spaceless %} around the attributes causes some issues. Spaceless trims off extra 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.

stof · 2014-07-03T16:59:04Z

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

chrisguitarguy · 2014-07-03T17:33:57Z

{% 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 attributes don't get rendered, but the 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:

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

No need to match against the whole string: only confirm that `false` attributes are not present.

Tobion · 2014-07-03T17:57:25Z

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

The assertions changed to be general enough to be pulled up.

stof · 2014-07-09T12:02:34Z

there are still e few spaceless tag which are not removed

chrisguitarguy · 2014-07-09T12:35:36Z

there are still e few spaceless tag which are not removed

Sorry about that, thought I caught them all.

stof · 2014-07-09T12:36:26Z

well, just do a search on the file :)

chrisguitarguy · 2014-07-09T13:58:55Z

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!

stof · 2014-07-09T14:02:20Z

👍

stof · 2014-07-09T14:07:02Z

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

fabpot · 2014-07-11T10:14:23Z

👍

fabpot · 2014-07-11T10:16:29Z

Indeed, merging this in 2.3 is not that easy.

@chrisguitarguy Can you submit a new PR for 2.3?

Tobion · 2014-07-11T11:55:40Z

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.

stof · 2014-07-11T12:03:20Z

@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)

chrisguitarguy · 2014-07-13T17:14:17Z

Can you submit a new PR for 2.3?

#11386

fabpot · 2014-07-14T05:06:28Z

Closing in favor of #11386

@fabpot

…targuy) This PR was merged into the 2.3 branch. Discussion ---------- Remove Spaceless Blocks from Twig Form Templates In favor of using Twig's whitespace control operators. See #11277 | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #11277 | License | MIT | Doc PR | Per @fabpot and @stof's requests in #11278, this is a PR for the 2.3 branch. Commits ------- 8f9ed3e Remove Spaceless Blocks from Twig Form Templates

This PR was submitted for the master branch but it was merged into the 2.4 branch instead (closes #11278). Discussion ---------- Remove Spaceless Blocks From Twig Templates Leaving it in can only mangle values from data bound to the form. | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #11277 | License | MIT | Doc PR | The tests pass here, but it doesn't seem like any tests really cover the actual rendering. Commits ------- 793a083 Remove Spaceless Blocks From Twig Templates

Remove Spaceless Block From Textarea Widgets

ed0a0e9

Leaving it in can only mangle values from data bound to the form.

chrisguitarguy added 2 commits July 3, 2014 09:08

Add Test Cases to Expose Whitespace Only Textarea Value Bug

43cc810

See symfony#11277

Remove Spaceless Blocks from Higher-Up Form Blocks

88db345

Removes the spaceless widget from: - form - form_row - form_rows - form_widget Instead, we use whitespace control to trim around the edges of those blocks.

Tobion reviewed Jul 3, 2014
View reviewed changes

chrisguitarguy added 2 commits July 3, 2014 11:46

Remove @group Annotation from Tests

1ac162a

Remove Spaceless in Favor of Whitespace Control

fca6cb8

Only Assert That Attribute is Not Present

de4a268

No need to match against the whole string: only confirm that `false` attributes are not present.

chrisguitarguy added 2 commits July 3, 2014 14:00

Pull False Widget Attribute Test Up in AbstractLayoutTest

0a81522

The assertions changed to be general enough to be pulled up.

Remove spaceless blocks from attributes

33812de

Remove the Final Spaceless Blocks

547489c

romainneutron added TwigBundle and removed TwigBundle labels Jul 12, 2014

chrisguitarguy mentioned this pull request Jul 13, 2014

Remove Spaceless Blocks from Twig Form Templates #11386

Merged

chrisguitarguy changed the title ~~Remove Spaceless Block From Textarea Widgets~~ Remove Spaceless Blocks From Twig Templates Jul 13, 2014

fabpot closed this Jul 14, 2014

Uh oh!

Remove Spaceless Blocks From Twig Templates #11278

Remove Spaceless Blocks From Twig Templates #11278

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!