Generic/UpperCaseConstantName: improve code coverage #665
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
…ents after `define(` This commit fixes false negatives in the sniff when handling unconventional spacing and comments after `define(`. When checking for the constant name after the opening parenthesis, the sniff would incorrectly exclude only whitespaces while comments can also be placed between the opening parenthesis and the constant name. Looking for the constant name by getting the next non-empty token after the opening parenthesis fixes this problem. The added tests also include comments between `define` and the opening parenthesis but that was already handled correctly by the sniff.
…ents before `define` This commit fixes false positives in the sniff when handling unconventional spacing and comments before `define`. When checking to see if the `define` found is not a method call, the sniff would incorrectly exclude only whitespaces. But comments can also be placed between `define` and one of the tokens the sniff looks for (T_OBJECT_OPERATOR, T_DOUBLE_COLON or T_NULLSAFE_OBJECT_OPERATOR). Looking for the first non-empty token before `define` fixes this problem.
…"DEFINE" This commit fixes false positives when the sniff encounters a constant named "DEFINE". When looking for calls to `define()`, the code was checking only if there was a non-empty token after `define`. Instead, it should check if the next non-empty token after `define` is `T_OPEN_PARENTHESIS`.
This commit fixes false positives in the sniff when handling attributes called `define`. Before this change the sniff was not taking into account that `define` is a valid name for an attribute, and it would incorrectly handle these cases as calls to the `define()` function.
… and metrics This commit implements a minor improvement to the error message and metric recording when handling calls to `define()`. Now the line of the error or the line recorded in the metric will be the line of the constant name. Before it was the line of the T_DEFINE token.
In early versions of this sniff, constant names were checked not only when defined, but also when used. At that time, code was added to handle the dynamic retrieval of class constants in calls to `constant()` by checking if the constant name contained a double-colon (see squizlabs/PHP_CodeSniffer@610b0cc and https://pear.php.net/bugs/bug.php?id=18670). If so, the sniff would enforce upper case only for the substring after the double-colon. Later, another commit removed support for constant usage (squizlabs/PHP_CodeSniffer@4af13118b1fedd89faadd78b9bc and https://pear.php.net/bugs/bug.php?id=20090). It seems to me that the code that is removed in this commit, should have been removed when support for constant usage was removed. The removed code is only triggered when a constant is defined using `define()`. As far as I can tell, over time, PHP changed how it behaves when a double-colon is passed as part of the first parameter of `define()`. However, never in a way that justifies special treatment in the context of this sniff (https://3v4l.org/erZcK). At least not since PHP 5.0 (I'm assuming it is not needed to check PHP 4).
Doing this to be able to create tests with syntax errors on separate files.
This commit takes into account that `findNext()` may return `false` and properly handles this case in the modified `if` condition. There were no issues without this extra check, as when `$constPtr` is `false`, `$tokens[$constPrt]` evaluates to the first token in the stack and the first token can never be `T_CONSTANT_ENCAPSED_STRING`, so the sniff would bail early anyway.
Includes updating a sniff code comment to better describe what it does. Some of the cases described were not covered by tests before the changes in this commit.
jrfnl
reviewed
Nov 26, 2024
There was a problem hiding this comment.
@rodrigoprimo Thank you for this PR.
These changes look great! Well done on finding all these issues!
I've left a couple of remarks inline. Additionally, I have the following question:
- You recognize in one of the commits that (attribute) classes can be called
defineand handle that situation. Now what about non-attribute classes ?
I.e. does the sniff excludenew Define('value')correctly ?
Also, to finish off this PR, would it be an idea to move the parse error on line 37 in the first test case file to a separate test case file by itself ?
src/Standards/Generic/Tests/NamingConventions/UpperCaseConstantNameUnitTest.inc
Outdated
Show resolved
Hide resolved
src/Standards/Generic/Tests/NamingConventions/UpperCaseConstantNameUnitTest.1.inc
Outdated
Show resolved
Hide resolved
src/Standards/Generic/Tests/NamingConventions/UpperCaseConstantNameUnitTest.1.inc
Show resolved
Hide resolved
4 tasks
|
FYI: I've opened issue #733 with another improvement which should be made to the sniff, but which is out of scope for this PR (and an improvement which will not be easy to make comprehensively anyhow). |
…tion This commit fixes a false positive that would trigger an error when this sniff encountered the instantiation of a class named `Define`.
|
Thanks for the review!
Good catch! I added a new commit fixing this false positive and updated the changelog suggestion section in the PR description.
Good point. I moved the parse error test to a separate file. This PR is now ready for another review. |
jrfnl
approved these changes
Nov 27, 2024
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
This PR improves code coverage for the
Generic.NamingConventions.UpperCaseConstantNamesniff. It also fixes several bugs found while working on adding tests.This is a big PR and I wasn't sure if I should split it into multiple PRs. I decided to first open a single PR to share in a single place all the issues that I found. But I can split this into multiple PRs if you prefer.
While working on this PR, I noticed that the sniff does not support named parameters and created an issue to document it: #661.
Suggested changelog entry
Generic.NamingConventions.UpperCaseConstantName: fix false negatives when handling unconventional spacing and comments afterdefine(Generic.NamingConventions.UpperCaseConstantName: fix false positives when handling unconventional spacing and comments before a call todefine()Generic.NamingConventions.UpperCaseConstantName: fix false positives when a constant namedDEFINEis encounteredGeneric.NamingConventions.UpperCaseConstantName: fix false positives when handling attributes calleddefineGeneric.NamingConventions.UpperCaseConstantName: use the line of the constant name instead of the line of the call todefine()when displaying errors and recording metricsGeneric.NamingConventions.UpperCaseConstantName: remove special treatment for constant names that contain a double-colonGeneric.NamingConventions.UpperCaseConstantName: fix false positive when handling the instantiation of a class namedDefineRelated issues/external references
Part of #146
Types of changes
PR checklist