Security/EscapeOutput: fix namespaced ::class detection #2605
Conversation
jrfnl
left a comment
jrfnl
left a comment
There was a problem hiding this comment.
@rodrigoprimo Thanks for this PR.
While I understand what you are trying to do, I don't think this is something the sniff should need to handle.
The whole point of using ::class is that PHP does the namespace resolution.
So using ::class on a fully qualified name or on a namespace relative name is not useful as that's what PHP does already.
💡 Maybe I should write a sniff to forbid that.... (using
::classon a fully qualified or namespace relative name)
// This...
_deprecated_function( __METHOD__, 'x.x.x', namespace\ClassName::class ); // OK.
// will literally result in the exact same thing as...
_deprecated_function( __METHOD__, 'x.x.x', ClassName::class ); // OK.See: https://3v4l.org/jg5nB
The only one which may make sense is using it on partially qualified names. I have no objection against handling that (and that alone).
Done. Will publish it to PHPCSExtra once Extra drops support for PHPCS 3.x (as writing that sniff for only 4.x is so much simpler 😉 ) |
I agree that it is not useful. I opted to add support for it on the basis that it is syntactically valid, and thus, I thought the sniff should support it. |
That's a fair point, but I don't think we need to go out of our way to support code which doesn't make any sense. As you've already done the work, let's go for it, but please add a comment with the tests annotating that that type of code doesn't have any value/doesn't make sense. |
Ok, I just added the comment. |
| $skip_tokens[ \T_STRING ] = \T_STRING; | ||
| $skip_tokens[ \T_NS_SEPARATOR ] = \T_NS_SEPARATOR; |
There was a problem hiding this comment.
For the record: these extra tokens are only relevant when the $i token is T_STRING or T_NAMESPACE.
I can't think of a valid code sample with T_VARIABLE or the hierarchy tokens where it would be problematic though, but give PHP a few years and no doubt it will be ;-)
(but no need to change it until PHP changes)
|
@rodrigoprimo Can you just rebase the last commit maybe so that we can have a nicer commit messages? |
The sniff now correctly identifies namespaced class references like `\MyNamespace\ClassName::class` and `namespace\ClassName::class` as safe output that doesn't require escaping. Support for `ClassName::class` was added in 2326, but I believe namespaced classes were not considered.
Remove extra closing parenthesis that was causing a syntax error in a ::class test case. This test was added in 2326 and there is nothing in the corresponding commit message that this indicates this was an intentional syntax error.
rodrigoprimo
force-pushed
the
escape-output-fix-namespaced-class-detection
branch
from
fd1ee5e to
5b28561
Compare
|
@dingo-d, I assumed you meant squashing the first and third commits, and I did that and force-pushed. Please let me know if you had something else in mind. |
rodrigoprimo
deleted the
escape-output-fix-namespaced-class-detection
branch
3 participants
Description
The sniff now correctly identifies namespaced class references like
\MyNamespace\ClassName::classandnamespace\ClassName::classas safe output that doesn't require escaping. Support forClassName::classwas added in 4ee9469 (PR #2326), but I believe namespaced classes were not considered at the time.I included a separate commit to fix what I believe to be an unintentional syntax error in one of the tests added in 4ee9469 when support for
ClassName::classwas introduced.Suggested changelog entry
Fixed:
WordPress.Security.EscapeOutput: namespaced class references using::classare now correctly recognized as safe output that doesn't require escaping.