The bug as reported in #537 highlights an area where a full sniff review would not be amiss.
Basically any time a findPrevious()/findNext() call searches for a T_SEMICOLON token to find the end of a statement, or when using "manual" token walking, but again looking for a T_SEMICOLON, there is a chance that the sniff should also check for the T_CLOSE_TAG token (and/or possibly the T_OPEN_TAG/T_OPEN_TAG_WITH_ECHO tokens).
Keep in mind: the ?> in PHP will automatically inject a semi-colon if one is not already there.
Task list
A quick search of the code base yields the following files/sniffs which will need to be reviewed. I only expect a small number of these to need changes.
[x] Generic.CodeAnalysis.ForLoopShouldBeWhileLoop - @jrfnl: sniff only looks within for condition. This is fine.
[x] Generic.CodeAnalysis.ForLoopWithTestFunctionCall - @jrfnl: sniff only looks within for condition. This is fine.
[x] Generic.WhiteSpace.LanguageConstructSpacing - @jrfnl: the semicolon check is used to bow out early when no space is needed (between the keyword and the semi-colon). I think it's reasonable for the sniff to expect a space between the keyword and a PHP close tag though, so I don't think any changes are needed.
[x] MySource.PHP.EvalObjectFactory - @jrfnl: well, this sniff should probably also look for T_CLOSE_TAG, but this is a deprecated sniff and just looking at the code, there's a lot more which needs fixing if the sniff wasn't deprecated already. IMO it is not worth spending time on this sniff.
[x] PSR2.Files.ClosingTag - @jrfnl: this sniff is about the close tag itself, so the close tag is already taken into account.
[x] Zend.Files.ClosingTag - @jrfnl: this sniff is about the close tag itself, so the close tag is already taken into account.
Guidelines
Some sniffs/code may already handle this correctly and the review would just confirm this.
There are some statements in PHP which cannot really contain embedded PHP, so in those cases, no check for T_CLOSE_TAG is needed.
In all other cases, a critical look at the code referencing the T_SEMICOLON token is warranted.
Any changes made should be accompanied by one or more tests demonstrating the false positives/false negatives prevented by the change.
PRs for this ticket should preferably contain the changes for one sniff per PR.
If you want to contribute to this task, please leave a comment "claiming" one or more files you will be reviewing. This way we can prevent multiple people working on the same thing.
If a review concludes that no changes are needed, just note that in a comment as well and I'll update the above task list.
The bug as reported in #537 highlights an area where a full sniff review would not be amiss.
Basically any time a
findPrevious()/findNext()call searches for aT_SEMICOLONtoken to find the end of a statement, or when using "manual" token walking, but again looking for aT_SEMICOLON, there is a chance that the sniff should also check for theT_CLOSE_TAGtoken (and/or possibly theT_OPEN_TAG/T_OPEN_TAG_WITH_ECHOtokens).Keep in mind: the
?>in PHP will automatically inject a semi-colon if one is not already there.Task list
A quick search of the code base yields the following files/sniffs which will need to be reviewed. I only expect a small number of these to need changes.
To Do
FileclassGeneric.CodeAnalysis.AssignmentInConditionGeneric.CodeAnalysis.EmptyPHPStatementGeneric.CodeAnalysis.JumbledIncrementerGeneric.CodeAnalysis.UnusedFunctionParameterGeneric.ControlStructures.InlineControlStructureGeneric.Formatting.DisallowMultipleStatementsGeneric.Formatting.MultipleStatementAlignmentGeneric.NamingConventions.UpperCaseConstantNameGeneric.PHP.LowerCaseConstantGeneric.PHP.LowerCaseTypePEAR.Commenting.FileCommentPEAR.Functions.FunctionCallSignaturePEAR.Functions.FunctionDeclarationPSR1.Files.SideEffectsPSR2.Classes.PropertyDeclarationPSR2.ControlStructures.SwitchDeclarationPSR2.Namespaces.UseDeclarationPSR12.Classes.AnonClassDeclarationPSR12.Files.DeclareStatementPSR12.Traits.UseDeclarationSquiz.Commenting.FunctionCommentSquiz.Commenting.InlineCommentSquiz.Commenting.LongConditionClosingCommentSquiz.Commenting.PostStatementCommentSquiz.ControlStructures.ControlSignatureSquiz.ControlStructures.ForLoopDeclarationSquiz.ControlStructures.InlineIfDeclarationSquiz.Operators.ComparisonOperatorUsageSquiz.Operators.IncrementDecrementUsageSquiz.PHP.DisallowSizeFunctionsInLoopsSquiz.PHP.EmbeddedPhpSquiz.PHP.NonExecutableCodeSquiz.WhiteSpace.ControlStructureSpacingSquiz.WhiteSpace.FunctionSpacingSquiz.WhiteSpace.LanguageConstructSpacingSquiz.WhiteSpace.MemberVarSpacingSquiz.WhiteSpace.ScopeKeywordSpacingSquiz.WhiteSpace.SemicolonSpacingTokenizers\PHPclassTokenizers\Tokenizerclass* Note: a few CSS/JS specific files have been excluded from the above task list as this is a PHP specific issue.
Under review
nothing yet
Reviewed and PR submitted with update
Generic.CodeAnalysis.UselessOverridingMethod- @jrfnl PR #554PSR2.Classes.ClassDeclaration- @jrfnl: no code change needed, but adding a test is warranted. PR #556Squiz.Classes.SelfMemberReference- @jrfnl PR #555Squiz.PHP.DisallowMultipleAssignments- @jrfnl PR #538Reviewed and concluded no changes needed
Generic.CodeAnalysis.ForLoopShouldBeWhileLoop- @jrfnl: sniff only looks withinforcondition. This is fine.Generic.CodeAnalysis.ForLoopWithTestFunctionCall- @jrfnl: sniff only looks withinforcondition. This is fine.Generic.WhiteSpace.LanguageConstructSpacing- @jrfnl: the semicolon check is used to bow out early when no space is needed (between the keyword and the semi-colon). I think it's reasonable for the sniff to expect a space between the keyword and a PHP close tag though, so I don't think any changes are needed.MySource.Objects.AssignThis- @jrfnl: JS-only sniffMySource.Objects.CreateWidgetTypeCallback- @jrfnl: JS-only sniffMySource.PHP.EvalObjectFactory- @jrfnl: well, this sniff should probably also look forT_CLOSE_TAG, but this is a deprecated sniff and just looking at the code, there's a lot more which needs fixing if the sniff wasn't deprecated already. IMO it is not worth spending time on this sniff.PSR2.Files.ClosingTag- @jrfnl: this sniff is about the close tag itself, so the close tag is already taken into account.Squiz.Strings.EchoedStrings- @jrfnl: already handled correctlyZend.Files.ClosingTag- @jrfnl: this sniff is about the close tag itself, so the close tag is already taken into account.Guidelines
T_CLOSE_TAGis needed.T_SEMICOLONtoken is warranted.Any changes made should be accompanied by one or more tests demonstrating the false positives/false negatives prevented by the change.
PRs for this ticket should preferably contain the changes for one sniff per PR.
If you want to contribute to this task, please leave a comment "claiming" one or more files you will be reviewing. This way we can prevent multiple people working on the same thing.
If a review concludes that no changes are needed, just note that in a comment as well and I'll update the above task list.