-
- Notifications
You must be signed in to change notification settings - Fork 522
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.
jrfnl left a comment
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
NonceVerificationsniff 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$callbackparameter.
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? |
…the namespaced tests based on PR review
… the namespaced tests based on PR review
…e for the namespaced tests based on PR review
…case for the namespaced tests based on PR review
…amespaced tests based on PR review
…add_filter() function
…ase for the calls to `define()` as discussed during PR review
| @jrfnl, as we discussed during our call earlier today, I added new commits to this PR to ensure that the namespaced tests use different function names when possible and also to ensure that names with different cases are tested for both non-qualified and fully-qualified global function calls. |
jrfnl left a comment
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.
@rodrigoprimo Just posting the CronInterval follow up comments so you can use them.
Will continue the review after these have been addressed.
| * the two first-class callable examples below as if referencing the global function first_class_six_min_schedule() | ||
| * and not a namespaced function with the same name. Fixing this incorrect behavior is not trivial. | ||
| */ | ||
| add_filter( 'cron_schedules', MyNamespace\first_class_six_min_schedule(...)); // Warning: 6 min. |
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.
| add_filter( 'cron_schedules', MyNamespace\first_class_six_min_schedule(...)); // Warning: 6 min. | |
| add_filter( 'cron_schedules', namespace\Sub\first_class_six_min_schedule(...)); // False positive - Warning: 6 min. Should be marked "Undetermined" instead. | |
| add_filter( 'cron_schedules', MyNamespace\first_class_six_min_schedule(...)); // False positive - Warning: 6 min. Should be marked "Undetermined" instead. |
| add_filter( 'cron_schedules', \first_class_six_min_schedule(...)); // Warning: 6 min. | ||
| add_filter( 'cron_schedules', namespace\first_class_six_min_schedule(...)); // Warning: 6 min. | ||
| | ||
| /* |
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.
As you are now documenting the sniff behaviour for a situation the sniff doesn't handle (yet), let's make sure the set of tests is complete for that issue.
What I'm still missing, is the same kind of "namespace name" variation for the "OK" test (line 318).
Those are currently false negatives (marked, ok, while in reality we don't know as we can't find the function). At some point in the future, those should be flagged as "Undetermined" instead.
Example:
add_filter( 'cron_schedules', MyNamespace\first_class_weekly_schedule(...)); // False negative - Ok: > 15 min, but should be marked "undetermined".
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.