Squiz/EmbeddedPhp: add support for short open tag and various bug fixes #196
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
11 tasks
rodrigoprimo
reviewed
Jan 8, 2024
jrfnl
force-pushed
the
feature/27-squiz-embeddedphp-support-short-open-tag
branch
2 times, most recently
from
37011f6
to
f9a0bde
Compare
The removed condition in the `validateInlineEmbeddedPhp()` method could never be `true` as this is already checked in the `process()` method before this method is ever called. As that means we will always already have determined the valid close tag, we may as well pass it to both underlying methods instead of determining it again. This should also yield a small performance improvement, especially when dealing with long files with only an open tag and no a closing tag, as it saves doing the token walking over the complete file for a second time. Includes moving a comment to a more appropriate place.
... to allow for easier debugging.
... just as an extra safeguard.
The sniff contains a condition which prevents checks from being run on the last tag set when it is a non-single line tag set. Adding a final open tag will prevent the _last_ test in the file accidentally being skipped, independently of what type of tag set this is, making the test case file more stable.
As things were, the "empty embed" fixers would in certain circumstances leave trailing spaces behind. For most codebases, this means the sniff would always need to be run in conjunction with a sniff which removes trailing spaces and that there is always a second fixer round needed: the first to fix the embedded PHP formatting, the second to allow the other sniff to remove the trailing spaces introduced by the fixes made by this sniff. Aside from that, it also makes updating the test case file fiddly. This commit fixes the "empty embed" fixers to remove trailing spaces, when applicable, and consolidates the code for this in a separate method. Note: _surrounding_ HTML whitespace, which is not trailing, will not be affected by this fix as the sniff should have no opinion on that. Covered by pre-existing tests + some additional newly added tests.
…aces As things were, the "opener on own line / content after" fixer would always leave at least one trailing space behind. For most codebases, this means the sniff would always need to be run in conjunction with a sniff which removes trailing spaces and that there is always a second fixer round needed: the first to fix the embedded PHP formatting, the second to allow the other sniff to remove the trailing spaces introduced by the fixes made by this sniff. It also makes updating the test case file fiddly. Secondly, if the space between the open tag and the first content was larger than exactly one space, the sniff would also need an extra fixer round to fix the indent of the "first content", which was moved to the next line and given indent, but with the extra space having being moved as well, the indent would now be too large. This commit fixes the "opener on own line / content after" fixer to remove trailing spaces and prevent the potential extra fixer loop to fix the indent. Covered by pre-existing tests + an additional newly added test.
…paces As things were, the "closer on own line" fixers would, in most cases, leave trailing whitespace behind. For most codebases, this means the sniff would always need to be run in conjunction with a sniff which removes trailing spaces and that there is always a second fixer round needed: the first to fix the embedded PHP formatting, the second to allow the other sniff to remove the trailing spaces introduced by the fixes made by this sniff. It also makes updating the test case file fiddly. Additionally, when the next token is inline HTML, it could lead to weird/incorrect indents, which would not be fixable via a sniff (as determining intended indent for inline HTML is non-trivial). This commit fixes the "closer on own line" fixers to remove trailing spaces and prevents them from affecting the indent of inline HTML unnecessarily. Covered by pre-existing tests + additional newly added tests.
When the close tag of a previous multi-line embedded PHP snippet and the open tag of the next multi-line embedded PHP snippet are on the same line, the sniff would add a stray blank line with trailing whitespace between the two tokens due to the fixers both running in the same loop. This commit adds a small tweak preventing this stray blank line from being added. Includes unit tests proving the issue and safeguarding the fix.
... to allow for additional test case files to be added.
…HP blocks are never ignored The exception conditions for the first/last tag set only apply to multi-line PHP blocks and PHP blocks without a closing tag. Single-line embedded PHP with a complete set of tags should not be affected. This is already handled correctly. This commit just adds tests to safeguard this.
As things were, the `Squiz.PHP.EmbeddedPhp` sniff would only check the formatting for code embedded using long PHP open tags, while short PHP open tags are pretty common for embedded PHP.
This commit adds support for examining short PHP open tags to the sniff.
Notes:
* The error codes used are the same for both type of open tags, with one exception: for single line embedded statements, it is checked that there is a semi-colon at the end of the statement.
The "statement" in an embedded snippet using short open tags may just be a single variable and in _some_ lines of thinking it is acceptable to leave out the semi-colon in that case.
To accommodate this, the error code for the missing semi-colon differentiates between whether the statement uses long open tags `NoSemicolon` (same as before) or short open tags `ShortOpenEchoNoSemicolon`. This will allows for selectively excluding the `ShortOpenEchoNoSemicolon` error code.
* I've considered enforcing that short open tags always are always used as a single-line statement, or adding an option to enforce this, but have decided against this at this time.
After all, this sniff is about _consistent_ formatting, single line stays single line and multi-line is made consistently/properly multi-line.
It could be considered future scope to add a separate sniff to enforce short open echo tag sets to always be single line when they only contain a single statement.
Fixes 27
… to do No need to go through the rest of the logic for the multi-line PHP block handling if there is nothing to do for the sniff.
…tent at end of file
… of file [1] As things are, the last tag block in a file, at the end of a file, will always be ignored if it contains a PHP close tag, even when it would otherwise trigger errors. These tests document this behaviour.
… of file [2] The last tag block in a file will **not** be ignored if it is followed by non-empty inline HTML. These tests document this behaviour.
…ted correctly When the PHP close tag is on the same line as the first PHP content in a multi-line embedded PHP block and the indent of the first PHP content in the block is incorrect, the indent for the close tag when it is moved to its own line will be based on the (incorrect) indent of the first PHP content line and not on the corrected indent for the first PHP content, which the indent fixer uses. This commit fixes this. Covered by pre-existing tests and adjusting the "fixed" expectations for those. Additionally, an extra test has been added for the case when `$indent` is not pre-calculated (= when the first content in the block is a scope closer).
The intended content indent calculation contained a pretty nasty bug, though it is unlikely to have been hit much in real code bases.
What was happening:
* The sniff would try to find the first whitespace token on the line containing the stack pointer.
If found, the size of the whitespace was taken as the required indent. This part was working correctly.
* If no whitespace token was found, the sniff would try and find the first inline HTML token.
If an inline HTML token was found, the leading whitespace in the inline HTML token was taken as the indent. This, again, is correct.
* However, now it gets iffy... what if no whitespace or inline HTML token was found on the line containing the open tag ? I.e. the situation where the open tag _is_ the first token on the line.
This situation was not accounted for by the sniff.
The `File::findFirstOnLine()` method would return `false` in that case, which means that the indent calculation would end up like this:
```php
$indent = (strlen($tokens[false]['content']) - strlen(ltrim($tokens[false]['content'])));
```
Now, `false` is not a valid array key, so this would end up being juggled to `0`, meaning the length of the leading whitespace for the first token in the **file** was being used.
Fixed now.
Includes various additional unit tests safeguarding the fix and one (in a separate file) demonstrating the situation where the bug would have had a real life impact.
…culation Basically, this is the same bug as fixed in a previous commit for the intended content indent calculation. The indentation of the first token in the file would be used to calculate the intended indentation, instead of the indentation for the first token on the line containing the PHP open tag. Fixed now, includes test. Note: The only reason the test doesn't actually show the bug is because of the earlier "fixer adds stray new line" bugfix in this PR, which means that the fixer for the previous close tag will prevent the buggy fixer from running. Without _that_ fix, this bug would be visible when running with `-vvv` (indent would get set to 32 and then corrected to 4 on the next fixer round via the `OpenTagIndent` fixer).
Yet another indentation calculation bug. If the close tag is not on its own line, but is on a line containing a subsequent line of a star-slash style comment, the indentation would not be calculated correctly as the comment token will include the indentation. Fixed now, includes test.
Basically, this is the same bug as fixed in a previous commit for the intended close tag indent calculation. If the first token on the line before the open tag would be a subsequent line for a star-slash comment, the indentation would not be calculated correctly, but would always be set to `0`, even when there was indentation. Fixed now, includes test.
`$firstContentAfterBlock` should not be able to be `false` at this point as it is already checked and handled at the top of the function, but better to be safe than sorry.
… of file [3] While the last tag block in a file will **not** be ignored if it is followed by non-empty inline HTML, the "blank lines before the close tag" check _will_ be ignored if this is the last embedded PHP block. This test documents this behaviour.
jrfnl
force-pushed
the
feature/27-squiz-embeddedphp-support-short-open-tag
branch
from
f9a0bde
to
50ae189
Compare
|
Rebased without changes before merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Sometimes I hate my brain.... I came to this sniff to fix #27 and figured I'd get the code coverage up to 100% while I was looking at the sniff anyway (related to #146)... I ended up doing a complete sniff review as wherever I looked, I saw issues in the sniff....
Squiz/EmbeddedPhp: remove some redundant code
The removed condition in the
validateInlineEmbeddedPhp()method could never betrueas this is already checked in theprocess()method before this method is ever called.As that means we will always already have determined the valid close tag, we may as well pass it to both underlying methods instead of determining it again.
This should also yield a small performance improvement, especially when dealing with long files with only an open tag and no a closing tag, as it saves doing the token walking over the complete file for a second time.
Includes moving a comment to a more appropriate place.
Squiz/EmbeddedPhp: rename two confusingly named variables
Squiz/EmbeddedPhp: make test cases more descriptive
... to allow for easier debugging.
Squiz/EmbeddedPhp: add some extra tests with ignore annotations
... just as an extra safeguard.
Squiz/EmbeddedPhp: prevent tests being skipped
The sniff contains a condition which prevents checks from being run on the last tag set when it is a non-single line tag set.
Adding a final open tag will prevent the last test in the file accidentally being skipped, independently of what type of tag set this is, making the test case file more stable.
Squiz/EmbeddedPhp: empty tag set fixer should remove trailing spaces
As things were, the "empty embed" fixers would in certain circumstances leave trailing spaces behind.
For most codebases, this means the sniff would always need to be run in conjunction with a sniff which removes trailing spaces and that there is always a second fixer round needed: the first to fix the embedded PHP formatting, the second to allow the other sniff to remove the trailing spaces introduced by the fixes made by this sniff.
Aside from that, it also makes updating the test case file fiddly.
This commit fixes the "empty embed" fixers to remove trailing spaces, when applicable, and consolidates the code for this in a separate method.
Note: surrounding HTML whitespace, which is not trailing, will not be affected by this fix as the sniff should have no opinion on that.
Covered by pre-existing tests + some additional newly added tests.
Squiz/EmbeddedPhp: opener on own line fixer should remove trailing spaces
As things were, the "opener on own line / content after" fixer would always leave at least one trailing space behind.
For most codebases, this means the sniff would always need to be run in conjunction with a sniff which removes trailing spaces and that there is always a second fixer round needed: the first to fix the embedded PHP formatting, the second to allow the other sniff to remove the trailing spaces introduced by the fixes made by this sniff.
It also makes updating the test case file fiddly.
Secondly, if the space between the open tag and the first content was larger than exactly one space, the sniff would also need an extra fixer round to fix the indent of the "first content", which was moved to the next line and given indent, but with the extra space having being moved as well, the indent would now be too large.
This commit fixes the "opener on own line / content after" fixer to remove trailing spaces and prevent the potential extra fixer loop to fix the indent.
Covered by pre-existing tests + an additional newly added test.
Squiz/EmbeddedPhp: closer on own line fixers should remove trailing spaces
As things were, the "closer on own line" fixers would, in most cases, leave trailing whitespace behind.
For most codebases, this means the sniff would always need to be run in conjunction with a sniff which removes trailing spaces and that there is always a second fixer round needed: the first to fix the embedded PHP formatting, the second to allow the other sniff to remove the trailing spaces introduced by the fixes made by this sniff.
It also makes updating the test case file fiddly.
Additionally, when the next token is inline HTML, it could lead to weird/incorrect indents, which would not be fixable via a sniff (as determining intended indent for inline HTML is non-trivial).
This commit fixes the "closer on own line" fixers to remove trailing spaces and prevents them from affecting the indent of inline HTML unnecessarily.
Covered by pre-existing tests + additional newly added tests.
Squiz/EmbeddedPhp: bug fix - fixer adds stray new line
When the close tag of a previous multi-line embedded PHP snippet and the open tag of the next multi-line embedded PHP snippet are on the same line, the sniff would add a stray blank line with trailing whitespace between the two tokens due to the fixers both running in the same loop.
This commit adds a small tweak preventing this stray blank line from being added.
Includes unit tests proving the issue and safeguarding the fix.
Squiz/EmbeddedPhp: rename test case file
... to allow for additional test case files to be added.
Squiz/EmbeddedPhp: add tests safeguarding that single line embedded PHP blocks are never ignored
The exception conditions for the first/last tag set only apply to multi-line PHP blocks and PHP blocks without a closing tag.
Single-line embedded PHP with a complete set of tags should not be affected.
This is already handled correctly. This commit just adds tests to safeguard this.
Squiz/EmbeddedPhp: sniff for short open tags
As things were, the
Squiz.PHP.EmbeddedPhpsniff would only check the formatting for code embedded using long PHP open tags, while short PHP open tags are pretty common for embedded PHP.This commit adds support for examining short PHP open tags to the sniff.
Notes:
The "statement" in an embedded snippet using short open tags may just be a single variable and in some lines of thinking it is acceptable to leave out the semi-colon in that case.
To accommodate this, the error code for the missing semi-colon differentiates between whether the statement uses long open tags
NoSemicolon(same as before) or short open tagsShortOpenNoSemicolonShortOpenEchoNoSemicolon. This will allows for selectively excluding theShortOpenNoSemicolonShortOpenEchoNoSemicolonerror code.After all, this sniff is about consistent formatting, single line stays single line and multi-line is made consistently/properly multi-line.
It could be considered future scope to add a separate sniff to enforce short open echo tag sets to always be single line when they only contain a single statement.
Fixes #27
Squiz/EmbeddedPhp: add dedicated tests related to ignoring of first open tag
Squiz/EmbeddedPhp: efficiency fix - bow out early if there is nothing to do
No need to go through the rest of the logic for the multi-line PHP block handling if there is nothing to do for the sniff.
Squiz/EmbeddedPhp: document behaviour for unclosed tag block with content at end of file
Squiz/EmbeddedPhp: document the behaviour for closed tag block at end of file [1]
As things are, the last tag block in a file, at the end of a file, will always be ignored if it contains a PHP close tag, even when it would otherwise trigger errors.
These tests document this behaviour.
Squiz/EmbeddedPhp: document the behaviour for closed tag block at end of file [2]
The last tag block in a file will not be ignored if it is followed by non-empty inline HTML.
These tests document this behaviour.
Squiz/EmbeddedPhp: bug fix - closer on own line indent is not calculated correctly
When the PHP close tag is on the same line as the first PHP content in a multi-line embedded PHP block and the indent of the first PHP content in the block is incorrect, the indent for the close tag when it is moved to its own line will be based on the (incorrect) indent of the first PHP content line and not on the corrected indent for the first PHP content, which the indent fixer uses.
This commit fixes this.
Covered by pre-existing tests and adjusting the "fixed" expectations for those.
Additionally, an extra test has been added for the case when
$indentis not pre-calculated (= when the first content in the block is a scope closer).Squiz/EmbeddedPhp: add test safeguarding handling of blank lines and scope closers
Squiz/EmbeddedPhp: bug fix - content intended indent calculation
The intended content indent calculation contained a pretty nasty bug, though it is unlikely to have been hit much in real code bases.
What was happening:
If found, the size of the whitespace was taken as the required indent. This part was working correctly.
If an inline HTML token was found, the leading whitespace in the inline HTML token was taken as the indent. This, again, is correct.
This situation was not accounted for by the sniff.
The
File::findFirstOnLine()method would returnfalsein that case, which means that the indent calculation would end up like this:falseis not a valid array key, so this would end up being juggled to0, meaning the length of the leading whitespace for the first token in the file was being used.Fixed now.
Includes various additional unit tests safeguarding the fix and one (in a separate file) demonstrating the situation where the bug would have had a real life impact.
Squiz/EmbeddedPhp: bug fix - open tag on own line intended indent calculation
Basically, this is the same bug as fixed in a previous commit for the intended content indent calculation.
The indentation of the first token in the file would be used to calculate the intended indentation, instead of the indentation for the first token on the line containing the PHP open tag.
Fixed now, includes test.
Note: The only reason the test doesn't actually show the bug is because of the earlier "fixer adds stray new line" bugfix in this PR, which means that the fixer for the previous close tag will prevent the buggy fixer from running.
Without that fix, this bug would be visible when running with
-vvv(indent would get set to 32 and then corrected to 4 on the next fixer round via theOpenTagIndentfixer).Squiz/EmbeddedPhp: bug fix - close tag intended indent calculation
Yet another indentation calculation bug.
If the close tag is not on its own line, but is on a line containing a subsequent line of a star-slash style comment, the indentation would not be calculated correctly as the comment token will include the indentation.
Fixed now, includes test.
Squiz/EmbeddedPhp: bug fix - open tag intended indent calculation
Basically, this is the same bug as fixed in a previous commit for the intended close tag indent calculation.
If the first token on the line before the open tag would be a subsequent line for a star-slash comment, the indentation would not be calculated correctly, but would always be set to
0, even when there was indentation.Fixed now, includes test.
Squiz/EmbeddedPhp: add some extra defensive coding
$firstContentAfterBlockshould not be able to befalseat this point as it is already checked and handled at the top of the function, but better to be safe than sorry.Squiz/EmbeddedPhp: document the behaviour for closed tag block at end of file [3]
While the last tag block in a file will not be ignored if it is followed by non-empty inline HTML, the "blank lines before the close tag" check will be ignored if this is the last embedded PHP block.
This test documents this behaviour.
Additional observations
The tag sets for the first PHP open tag and the last PHP close tag, providing the set is multi-line, are explicitly exempt from this sniff to prevent conflicts with sniffs specifically about the start/end of file.
There are two problems with this:
For now, I've left that part of the sniff as-is, but this is something which may need to be re-evaluated in the future.
Another issue I see in this sniff is that in multiple places, the sniff looks for the first non-whitespace token after a closing tag. However, unless there is a PHP open tag straight after the close tag, the next token will always be
T_INLINE_HTML, which may or may not contain any content.Suggested changelog entry
Added
ShortOpenEchoNoSemicolonerrorcode to allow for selectively ignoring missing semicolons in single line embedded PHP snippets within short open echo tags.Changed
Fixed
Related issues/external references
Fixes #27
Related to #146
Types of changes