Squiz/ClosingDeclarationComment: fix a non-problematic bug plus some other small improvements #381
Conversation
github-actions
bot
added
Type: bug
Focus: Comments/Docblocks
Standard: Squiz
Status: triage
labels
@rodrigoprimo There are currently a small number of sniffs which do this and this behaviour will be removed in PHPCS 4.0 as per squizlabs/PHP_CodeSniffer#2455 |
There was a problem hiding this comment.
Hi @rodrigoprimo Sorry for taking so long before reviewing this PR properly.
We've now looked at it together in a call, so you are already aware of the remarks I'm making.
Overall: looking good! Just some small changes needed to finish this off properly.
| function misplacedClosingCommentIndentation() { | ||
| } //end misplacedClosingCommentIndentation() |
There was a problem hiding this comment.
I think you meant to address this comment with this test ?
No tests with a misplaced comment with indentation.
While I do like this new test and I think we should keep it (though maybe rename the function ?), it doesn't actually address the above comment as the function keyword and the close brace are still at column 1 (start of line).
With indentation in the above comment, I meant a function/method in a class which has whitespace before the function keyword and before the closing brace, then a new line and then a closing comment on the next line, again with indentation before it.
And while writing this, I can think of another few variations, like:
function CommentHasMoreIndentationThanFunction() {
}
//end CommentHasMoreIndentationThanFunction()
function CommentHasLessIndentationThanFunction() {
}
//end CommentHasLessIndentationThanFunction()
src/Standards/Squiz/Tests/Commenting/ClosingDeclarationCommentUnitTest.1.inc
Outdated
Show resolved
Hide resolved
src/Standards/Squiz/Tests/Commenting/ClosingDeclarationCommentUnitTest.1.inc
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,8 @@ | |||
| <?php | |||
There was a problem hiding this comment.
While this is a good test to have anyway, this test doesn't actually highlight the problem with the missing $next !== false condition.
We looked at this during our call and the following additional test should be added to safeguard the fix:
//end closingBraceAtEndOfFileMissingComment()
<?php
function closingBraceAtEndOfFileMissingComment() {
}This test will show that without that extra condition the wrong error is being thrown (though an error is still being thrown).
Note: as the test framework as-is doesn't safeguard that the correct error code is being thrown for each line, it's not a hard safeguard, but having the test in place will allow us to safeguard this properly if/when the test framework gets updates to check per error code/per line.
rodrigoprimo
force-pushed
the
closing-declaration-comment-improvements
branch
from
ea98c6d to
1533fe5
Compare
|
Thanks for the review, @jrfnl! I just pushed a few new commits implementing all the changes that you suggested and this PR is ready for a final check. |
There was a problem hiding this comment.
Thanks for those updates @rodrigoprimo ! All looks good to me now!
This is necessary to add more tests that need to go into separate files.
This commit improve the test coverage for the Squiz.Commenting.ClosingDeclarationComment sniff by adding the following tests: - Function without a closing comment - Misplaced closing comment with indentation - Misplaced closing comment with multiple newlines - Class missing a opening or closing bracket It also documents via tests that anonymous classes and arrow functions do not require a closing comment.
This commit removes an if condition that cannot be true. The `$closingBracket` variable can never be null as the sniff already bows out before setting the variable when checking if `isset($tokens[$stackPtr]['scope_closer']) === false`.
This commit fixes a non-problematic bug on line 96. The code `if (rtrim($tokens[$next]['content']) === $comment)` was not taking into account that `$next` can be false. To fix it, a `$next !== false` check was added to the if condition. This problem did not cause any issues because when `$next` is `false`, the code would check the content of the first token (`$tokens[0]['content']`) and this token can only be the PHP open tag or inline HTML. The code would only produce false positives if the content of the first token would be `// end ...` which is unlikely and would be invalid HTML. A test was added exercising the code path where `$next` is `false`. And another tests which actually hits the bug and safeguards against potential regressions for the non-problematic bug fix.
jrfnl
force-pushed
the
closing-declaration-comment-improvements
branch
from
1533fe5 to
5b597d8
Compare
|
Rebased without changes, just some reorganization of the commits. Merging once the build passes. |
Description
This PR makes the following changes to the Squiz.Commenting.ClosingDeclarationComment sniff:
if (rtrim($tokens[$next]['content']) === $comment)was not taking into account that$nextcan be false. To fix it, a$next !== falsecheck was added to the if condition.@jrfnl suggested those changes while reviewing another PR: #340 (comment). In the same comment, it was also suggested that the sniff should handle traits. This will be addressed in a separate PR.
I noticed that this sniff triggers a warning when there is a missing opening or closing bracket. I imagine this could be misleading when using PHPCS with live code, and it is the first sniff that I see doing this. I'm highlighting this behavior in case we want to address it in another PR. For now, I added two tests documenting this behavior.
Suggested changelog entry
Squiz.Commenting.ClosingDeclarationComment: fixed a non-problematic bug when $next is false
Related issues/external references
Related to #340 (comment) and #146
Types of changes
PR checklist