ConstantStringSniff: Take into account namespace prefixing #444
Conversation
rebeccahum
force-pushed
the
rebecca/namespacing_to_-ConstantStringSniff
branch
3 times, most recently
from
b066e0a
to
57a05d2
Compare
| $nextNextToken = $this->phpcsFile->findNext( Tokens::$emptyTokens, $nextToken + 1, null, true, null, true ); | ||
|
|
||
| if ( T_NS_C === $this->tokens[ $nextToken ]['code'] && T_STRING_CONCAT === $this->tokens[ $nextNextToken ]['code'] ) { | ||
| // Namespacing being used, skip to next. |
There was a problem hiding this comment.
Could is_token_namespaced() be used here?
There was a problem hiding this comment.
TIL.
I don't think it would work when using __NAMESPACE__ with concatenation, however: https://github.com/WordPress/WordPress-Coding-Standards/blob/06966b26d8deb06a91a5c0f99a04802a6ff5e5bf/WordPress/Sniff.php#L1655.
There was a problem hiding this comment.
I think you're right. I had a quick look to see if there were any other utility functions in WPCS that might be able to check for this, but couldn't spot any.
@jrfnl - one for your new Utils package?
There was a problem hiding this comment.
Possibly, but to be fair, when __NAMESPACE__ is used, it doesn't necessarily mean that the code is namespaced. __NAMESPACE__ can evaluate to an empty namespace.
If you like, you can open an issue already in the Utils repo. The more detailed the issue, the better.
My initial priority for the Utils repo is to get it set up and release v 1.0 with everything from the refactor PR and a little more.
After that I will look at any open issues, so it may be a little while before I even look at it.
GaryJones
force-pushed
the
rebecca/namespacing_to_-ConstantStringSniff
branch
from
c238bd5
to
40f63bc
Compare
The `define()` and `defined()` functions expect the name of a constant as a text string to be passed as the first parameter. This sniff is intended to guard against a coding mistake where the name of the constant would be passed as a_constant_. While passing the name of the constant as a constant _could_ be perfectly valid when that constant contains the name of another constant as a text string, more often than not, it will be a coding error. As things were, the sniff did not handle parameters build up using more than one token into account. It also did not handle valid situations, like passing the constant name as a text string via a variable to the functions, correctly. As outlined in #422 (comment) (Proposal 1), this limits the sniff to _only_ trigger an error when a plain constant has been passed as the "constant name" parameter and ignore all other cases. Note: The current implementation of this fix, uses the WPCS `get_function_call_parameter()` method to retrieve the stack pointer to the start and end of the parameter. Once PHPCSUtils is in place, this function call should be replaced by using the PHPCSUtils version of that method. Fixes 422 Fixes 480
jrfnl
force-pushed
the
rebecca/namespacing_to_-ConstantStringSniff
branch
from
40f63bc
to
b01bca1
Compare
|
I've added a new commit to this PR to implement option 1 from my comment in the original ticket. Commit details:Constants/ConstantString: prevent more false positivesThe While passing the name of the constant as a constant could be perfectly valid when that constant contains the name of another constant as a text string, more often than not, it will be a coding error. As things were, the sniff did not handle parameters build up using more than one token into account. It also did not handle valid situations, like passing the constant name as a text string via a variable to the functions, correctly. As outlined in #422 (comment) (Proposal 1), this limits the sniff to only trigger an error when a plain constant has been passed as the "constant name" parameter and ignore all other cases. Note: The current implementation of this fix, uses the WPCS |
Fixes #422.