-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Conversation
Leaving it in can only mangle values from data bound to the form.
This chnage is not enough: given that parent forms will have the This could even make the form rendering a bit faster, by avoiding to apply a regex processing in many places (especially as |
Good call. The template where I discovered this was using
I can do this, seems like a much bigger change? Would it break BC in any way (I'm thinking not)? |
Maybe a better fix is just removing spaces from the higher up blocks like Removing all the spaceless tags causes a lot of tests to fail. |
@chrisguitarguy which kind of failures ? |
Lots of things like this:
Spaceless |
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 |
Cool, I'll push some commits in a bit. I'm going to try and add a few test cases for #11277 to |
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.
👍 for removing all |
👍 |
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! |
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
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 ack
ed for group annotations and saw a few ticket-XXX
and just went with it.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
👍 for removing spaceless, there are many still in place |
I've got the tests passing with most of the Removing |
@chrisguitarguy |
I know. Removing it causes a few tests to fail. 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 |
No need to match against the whole string: only confirm that `false` attributes are not present.
@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.
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 :) |
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) |
|
Closing in favor of #11386 |
…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
Leaving it in can only mangle values from data bound to the form.
The tests pass here, but it doesn't seem like any tests really cover the actual rendering.