8000 Remove Spaceless Blocks From Twig Templates by chrisguitarguy · Pull Request #11278 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

Remove Spaceless Blocks From Twig Templates #11278

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 9 commits into from

Conversation

chrisguitarguy
Copy link
Contributor

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
Copy link
Member
stof commented Jul 3, 2014

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member
stof 8000 commented Jul 3, 2014

@chrisguitarguy which kind of failures ?

@chrisguitarguy
Copy link
Contributor Author

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
Copy link
Member
stof commented Jul 3, 2014

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
Copy link
Contributor Author

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.

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
Copy link
Contributor
rybakit commented Jul 3, 2014

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

@stof
Copy link
Member
stof commented Jul 3, 2014

👍

@chrisguitarguy
Copy link
Contributor Author

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
Copy link
Member
stof commented Jul 3, 2014

@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
Copy link
Contributor Author

@stof makes sense, thanks for the explanation!

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove the group entirely here

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Tobion
Copy link
Contributor
Tobion commented Jul 3, 2014

👍 for removing spaceless, there are many still in place

@chrisguitarguy
Copy link
Contributor Author

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
Copy link
Member
stof commented Jul 3, 2014

@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
Copy link
Contributor Author

{% 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
Copy link
Contributor
Tobion commented Jul 3, 2014

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

@stof
Copy link
Member
stof commented Jul 9, 2014

there are still e few spaceless tag which are not removed

A3E2
@chrisguitarguy
Copy link
Contributor Author

there are still e few spaceless tag which are not removed

Sorry about that, thought I caught them all.

@stof
Copy link
Member
stof commented Jul 9, 2014

well, just do a search on the file :)

@chrisguitarguy
Copy link
Contributor Author

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
Copy link
Member
stof commented Jul 9, 2014

👍

@stof
Copy link
Member
stof commented Jul 9, 2014

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
Copy link
Member
fabpot commented Jul 11, 2014

👍

@fabpot
Copy link
Member
fabpot commented Jul 11, 2014

Indeed, merging this in 2.3 is not that easy.

@chrisguitarguy Can you submit a new PR for 2.3?

@Tobion
Copy link
Contributor
Tobion commented Jul 11, 2014

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
Copy link
Member
stof commented Jul 11, 2014

@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
Copy link
Contributor Author

Can you submit a new PR for 2.3?

#11386

@chrisguitarguy chrisguitarguy changed the title Remove Spaceless Block From Textarea Widgets Remove Spaceless Blocks From Twig Templates Jul 13, 2014
@fabpot
Copy link
Member
fabpot commented Jul 14, 2014

Closing in favor of #11386

@fabpot fabpot closed this Jul 14, 2014
fabpot added a commit that referenced this pull request Jul 14, 2014
…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
fabpot added a commit that referenced this pull request Jul 15, 2014
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0