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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants