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
NoTrailingWhitespaceFixer - trim space after opening tag #3957
NoTrailingWhitespaceFixer - trim space after opening tag #3957
Conversation
], | ||
[ | ||
'<?php ', // do not trim this as "<?php" is not valid PHP | ||
], |
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.
can you add don't fix
test for;
"<?php\t\t"
two tabs
"<?php "
two spaces
or should those be trimmed down to <?php
single space .. .hmmm ;) (I don't think these need fixing, but a test would be nice)
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'd say trim them without the character that is part of T_OPEN_TAG
.
&& 1 === Preg::match('/^(\R)(.*)$/', $tokens[$index + 1]->getContent(), $whitespaceMatches) | ||
) { | ||
$tokens[$index] = new Token([T_OPEN_TAG, $openTagMatches[1].$whitespaceMatches[1]]); | ||
if (empty($restOfWhitespace)) { |
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.
can we avoid empty
here? , $restOfWhitespace
is not defined here I think?
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.
Fixed
], | ||
[ | ||
'<?php ', // do not trim this as "<?php" is not valid PHP | ||
], |
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.
please add this case;
[
"<?php\n \n \n ",
]
// and
[
"<?php \n \n ",
]
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.
@SpacePossum added, I hope you mean 2nd one with fix (as the first set of spaces is in non-blank line).
"<?php\n \n \n ", | ||
], | ||
[ | ||
"<?php\n \n ", |
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.
Might be a dumb question, but how is this not fixed into "<?php\n\n "
instead? Different bug in the same fixer?
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.
The name of the fixer is a little misleading, here is description:
Remove trailing whitespace at the end of non-blank lines.
So, the only non-blank line is the one with open tag.
Thank you @kubawerlos. |
…ubawerlos) This PR was squashed before being merged into the 2.12 branch (closes #3957). Discussion ---------- NoTrailingWhitespaceFixer - trim space after opening tag Fixes #3956 That's what I call edge case :) The iteration over `$tokens` has to be reversed, so the whitespace after the `T_OPEN_TAG` is already trimmed (otherwise we could replace space with another space) when handing the open tag. Commits ------- e7daf23 NoTrailingWhitespaceFixer - trim space after opening tag
Fixes #3956
That's what I call edge case :)
The iteration over
$tokens
has to be reversed, so the whitespace after theT_OPEN_TAG
is already trimmed (otherwise we could replace space with another space) when handing the open tag.