Skip to content

Conversation

@rodrigoprimo
Copy link
Collaborator

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, and WordPress.WP.DiscouragedConstants.

Copy link
Member

@jrfnl jrfnl left a 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, like array_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 the needs_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 the extends...
  • 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 that define() 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.
Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>
@rodrigoprimo
Copy link
Collaborator Author

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).

The NonceVerification sniff is also handling other function calls, like array_key_exists(), is_string() etc to prevent false positives/negatives.
These also need tests for the variations of namespaced names.

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 needs_nonce_check() in a PR that I will pull later). That being said, I agree that it makes sense to include those tests here as well, so I added a new commit with those tests.

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 the extends...
PrefixAllGlobals: is missing tests for the variations of namespaced names for the hook functions.

Done

CronInterval: is missing tests for the variations of namespaced names for first class callables in the $callback parameter.

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?

@rodrigoprimo
Copy link
Collaborator Author

@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.

Copy link
Member

@jrfnl jrfnl left a 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

/*
Copy link
Member

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".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

2 participants