-
-
Notifications
You must be signed in to change notification settings - Fork 519
Sniffs: add tests for namespaced names #2620
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Sniffs: add tests for namespaced names #2620
Conversation
There were already a few tests with namespaced names, so this commit adds only what was missing in the existing tests.
Add tests safeguarding that namespaced calls to functions named `define` are handled correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for setting up this PR @rodrigoprimo !
Similar to previous remarks in other PRs:
- The
NonceVerification
sniff is also handling other function calls, likearray_key_exists()
,is_string()
etc to prevent false positives/negatives.
These also need tests for the variations of namespaced names.
I suggest you have a good look at theneeds_nonce_check()
method. ValidHookName
: missing tests for the variations of namespaced names for the target functions themselves (do_action()
etc).PrefixAllGlobals
: I have a feeling (but haven't verified) that test case file 3 needs additional tests for theextends
...PrefixAllGlobals
: is missing tests for the variations of namespaced names for the hook functions.CronInterval
: is missing tests for the variations of namespaced names for first class callables in the$callback
parameter.
Other than that:
PrefixAllGlobals
: I've confirmed thatdefine()
in combination with variations of the namespaced names in the string for the constant name is sufficiently covered by tests already (line 311-315 in test case file 1)DiscouragedConstants
: verified that the "use of global constant" in all variations of namespaced names is covered sufficiently.
b67ce6a
to
d72fe47
Compare
Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>
d72fe47
to
521f28c
Compare
… namespaced names for the target functions themselves
…namespaced first class callable
Thanks for the review, @jrfnl! I added new commits, one per sniff. My idea is that those commits are squashed into the corresponding original commit for each sniff. Let me know if you prefer that I do that (either before or after you review this PR again).
I don't recall exactly why I did not include them initially (if it was a decision not to include or because I'm adding tests to the utility methods that are used in
Done
I just added a test in a new commit for a fully qualified call to a global function, which is the only case that the sniff is handling correctly at the moment. For the other cases, I suggest creating an issue to address this separately, as currently the sniff does not handle them correctly. What do you think? |
Description
This PR is a continuation of #2581.
In preparation for PHPCS 4.0, which changes the tokenization of namespaced names, this PR adds tests with all variants of namespace names (unqualified, partially qualified, fully qualified, and namespace relative) to the following sniffs:
WordPress.Utils.I18nTextDomainFixer
,WordPress.Security.NonceVerification
,WordPress.NamingConventions.ValidHookName
,WordPress.NamingConventions.PrefixAllGlobals
,WordPress.PHP.NoSilencedErrors
,WordPress.WP.CronInterval
, andWordPress.WP.DiscouragedConstants
.