Sniffs: add tests for namespaced names #2620

rodrigoprimo · 2025-09-19T18:10:20Z

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.

jrfnl

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.

WordPress/Tests/PHP/NoSilencedErrorsUnitTest.inc

rodrigoprimo · 2025-09-30T19:13:47Z

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 · 2025-11-10T20:34:50Z

@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

@rodrigoprimo Just posting the CronInterval follow up comments so you can use them.

Will continue the review after these have been addressed.

WordPress/Tests/WP/CronIntervalUnitTest.inc

jrfnl

Review status so far:
✅ NoSilencedErrors
✅ CronInterval

Rest still to be reviewed.

jrfnl

✅ DiscouragedConstants

jrfnl

✅ ValidHookName

jrfnl

✏️ PrefixAllGlobals

WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.php

WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.3.inc

jrfnl

✅ PrefixAllGlobals

jrfnl

📝 NonceVerification - didn't finish the review as I can't see the code anymore through the comments.

WordPress/Tests/Security/NonceVerificationUnitTest.1.inc

jrfnl

Looked the NonceVerification test over one more time

Definitely better, but this is a complex sniff, so we may still be missing some test variations, though I'm not sure that will be problematic for the migration to PHPCS 4.0.

The one thing which may still be good to cover via tests is the handling of a (global) user-defined customNonceVerificationFunctions when namespaced variations of that same function name are encountered. What do you think ?

jrfnl

✅ NonceVerification

jrfnl

🎉 Thanks for all your work on this @rodrigoprimo !

Would you like to rebase & squash the commits to atomic ones now ?

There were already a few tests with namespaced names, so this commit adds only what was missing in the existing tests.

Co-authored-by: Juliette <[email protected]>

Add tests safeguarding that namespaced calls to functions named `define` are handled correctly.

rodrigoprimo · 2025-11-12T03:28:31Z

Thanks for reviewing this PR, @jrfnl!

Would you like to rebase & squash the commits to atomic ones now ?

Sure, I just did that and force pushed.

rodrigoprimo added Type: Chores/Cleanup Status: Review ready labels Sep 19, 2025

jrfnl requested changes Sep 20, 2025

View reviewed changes

WordPress/Tests/PHP/NoSilencedErrorsUnitTest.inc Show resolved Hide resolved

jrfnl added Status: Awaiting feedback and removed Status: Review ready labels Sep 20, 2025

rodrigoprimo force-pushed the more-namespace-tests branch 2 times, most recently from d72fe47 to 521f28c Compare September 24, 2025 10:50

rodrigoprimo removed the Status: Awaiting feedback label Sep 30, 2025

rodrigoprimo mentioned this pull request Nov 6, 2025

AbstractClassRestrictionsSniff sniffs: add tests for namespaced names #2643

Open

This was referenced Nov 11, 2025

AbstractFunctionParameter sniffs: add tests for namespaced names #2633

Open

Security/EscapeOutput: fix false positive and add tests for namespaced names #2618

Open

jrfnl reviewed Nov 11, 2025

View reviewed changes

WordPress/Tests/WP/CronIntervalUnitTest.inc Outdated Show resolved Hide resolved

WordPress/Tests/WP/CronIntervalUnitTest.inc Show resolved Hide resolved

rodrigoprimo force-pushed the more-namespace-tests branch 2 times, most recently from c41adfb to 0f2d04b Compare November 11, 2025 18:58

jrfnl reviewed Nov 11, 2025

View reviewed changes

WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.php Outdated Show resolved Hide resolved

WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.3.inc Outdated Show resolved Hide resolved

jrfnl reviewed Nov 11, 2025

View reviewed changes

jrfnl reviewed Nov 12, 2025

View reviewed changes

rodrigoprimo force-pushed the more-namespace-tests branch from 026eb8a to d0c2fb0 Compare November 12, 2025 01:42

jrfnl reviewed Nov 12, 2025

View reviewed changes

jrfnl approved these changes Nov 12, 2025

View reviewed changes

jrfnl added the Status: Review ready label Nov 12, 2025

jrfnl added this to the 3.3.0 milestone Nov 12, 2025

rodrigoprimo force-pushed the more-namespace-tests branch from 066934b to 7f6ff91 Compare November 12, 2025 03:11

Utils/I18nTextDomainFixer: add tests for namespaced names

e60764e

There were already a few tests with namespaced names, so this commit adds only what was missing in the existing tests.

rodrigoprimo and others added 6 commits November 12, 2025 00:25

Security/NonceVerification: add tests for namespaced names

b39cffc

Co-authored-by: Juliette <[email protected]>

NamingConventions/ValidHookName: add tests for namespaced names

a8d703c

NamingConventions/PrefixAllGlobals: add tests for namespaced names

269da1b

PHP/NoSilencedErrors: add tests for namespaced names

4a34f38

Co-authored-by: Juliette <[email protected]>

WP/CronInterval: add tests for namespaced names

861c0e8

WP/DiscouragedConstants: add tests for namespaced function calls

b86f078

Add tests safeguarding that namespaced calls to functions named `define` are handled correctly.

rodrigoprimo force-pushed the more-namespace-tests branch from 7f6ff91 to b86f078 Compare November 12, 2025 03:26

rodrigoprimo requested a review from dingo-d November 14, 2025 12:49

dingo-d approved these changes Nov 17, 2025

View reviewed changes

dingo-d merged commit c41a317 into WordPress:develop Nov 17, 2025
29 checks passed

github-actions bot removed the Status: Review ready label Nov 17, 2025

rodrigoprimo deleted the more-namespace-tests branch November 18, 2025 13:31

rodrigoprimo mentioned this pull request Nov 29, 2025

AbstractFunctionRestrictionsSniff: improve namespaced names tests #2663

Open

Uh oh!

Sniffs: add tests for namespaced names #2620

Sniffs: add tests for namespaced names #2620

Uh oh!

Conversation

rodrigoprimo commented Sep 19, 2025

Description

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rodrigoprimo commented Sep 30, 2025

Uh oh!

rodrigoprimo commented Nov 10, 2025

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

rodrigoprimo commented Nov 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants