Skip to content

Conversation

@rodrigoprimo
Copy link
Collaborator

@rodrigoprimo rodrigoprimo commented Aug 21, 2025

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 sniffs that extend the AbstractFunctionRestrictions class when necessary (some commits already contained part of the required tests).

The only sniff that is missing in this PR is WordPress.Security.EscapeOutput, which will be addressed separately.

Before adding the tests for the WordPress.WP.DeprecatedFunctions sniff, I opted to rename the test case file, as it works differently from the other test case files, and adding the namespaced names to it seemed incorrect.

Note: there will be more sniffs which will need additional tests, so this is only one of the first of a range of PRs adding more tests to help get ready for PHPCS 4.0.

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.

Some feedback - not necessarily blocking:

  • The "header" comment used Safeguard support for PHP 8.0+ tokenization of namespaced "names". is confusing.
    These tests are not directly related to PHP 8.0, as on PHPCS 3.x, the "old" tokenization is also used for PHP 8.0 - current, while on PHPCS 4.x, the "PHP 8.0 tokenization" is also used for PHP 7.2 - 7.4.
    In my opinion, it would be clearer to call this "Safeguard correct handling of [all types of] namespaced function calls" or "Safeguard against false positives on namespaced function calls".
  • All the new tests per sniff use the "same" function name/code. When a sniff is looking for multiple functions, it's never a bad thing to have some variation in the tests.
    Example: DateTime/RestrictedFunctions - the namespaced name tests all use the date_default_timezone_set function name, while the sniff also looks for date().
  • For the \target_function() test (FQN call to global function), if the test file in general has enough tests, like the DB/RestrictedFunctions sniff, it would have been perfectly fine to update a few of the pre-existing tests instead of adding a new test for the FQN function call.
  • A lot of the test files do not contain any tests with non-lowercase function calls to the target functions. Similar to the previous point - if the test file contains sufficient tests, updating a few of these to be non-lowercase (mixed case, uppercase) would not go amiss.
  • A lot of the test files also miss tests against false positives for "same name" method calls/constants/properties etc. While not strictly necessary for PHPCS 4.0, still may not be a bad idea to have these tests.
  • The // The sniff should start flagging this once it can resolve relative namespaces. comment seen in quite some of the test files is only partially correct.
    namespace\target_function() will start to be flagged when in the global namespace, but not when within a namespace.
    When namespace and import use tracking is added, we need to make sure both situations are tested (but that's for later, not now).

@rodrigoprimo
Copy link
Collaborator Author

rodrigoprimo commented Sep 9, 2025

Thanks for your review, @jrfnl. This PR is ready for another check. Below, I'm replying to the points that you raised.

The "header" comment used Safeguard support for PHP 8.0+ tokenization of namespaced "names". is confusing.
These tests are not directly related to PHP 8.0, as on PHPCS 3.x, the "old" tokenization is also used for PHP 8.0 - current, while on PHPCS 4.x, the "PHP 8.0 tokenization" is also used for PHP 7.2 - 7.4.
In my opinion, it would be clearer to call this "Safeguard correct handling of [all types of] namespaced function calls" or "Safeguard against false positives on namespaced function calls".

I pushed commits updating this header. I opted to push one commit per test file to make it possible to combine them with the original commit if the idea is to merge this PR with one commit per changed test file. Happy to help with that after you approve this PR if you want.

All the new tests per sniff use the "same" function name/code. When a sniff is looking for multiple functions, it's never a bad thing to have some variation in the tests.
Example: DateTime/RestrictedFunctions - the namespaced name tests all use the date_default_timezone_set function name, while the sniff also looks for date().

Since you mentioned that those comments are not blocking, I will keep this in mind for future PRs.

For the \target_function() test (FQN call to global function), if the test file in general has enough tests, like the DB/RestrictedFunctions sniff, it would have been perfectly fine to update a few of the pre-existing tests instead of adding a new test for the FQN function call.

I opted to include a new FQN call to a global function test together with the others to make it easier to spot that all possible cases are covered.

A lot of the test files do not contain any tests with non-lowercase function calls to the target functions. Similar to the previous point - if the test file contains sufficient tests, updating a few of these to be non-lowercase (mixed case, uppercase) would not go amiss.

I created a new commit addressing this, and I will pull it in a separate PR once this one is merged.

A lot of the test files also miss tests against false positives for "same name" method calls/constants/properties etc. While not strictly necessary for PHPCS 4.0, still may not be a bad idea to have these tests.

I haven't created yet, but I will try to find time to prepare a separate PR to include those tests that I agree are important.

The // The sniff should start flagging this once it can resolve relative namespaces. comment seen in quite some of the test files is only partially correct.
namespace\target_function() will start to be flagged when in the global namespace, but not when within a namespace.
When namespace and import use tracking is added, we need to make sure both situations are tested (but that's for later, not now).

I opted to add this comment the way it is because those tests are in the global namespace. I agree that when namespace and import use tracking is added, we will need to include more tests.

@rodrigoprimo rodrigoprimo force-pushed the add-abstract-function-restrictions-tests branch 4 times, most recently from 76600e3 to 821535d Compare September 15, 2025 14:38
@jrfnl jrfnl added this to the 3.2.x milestone Sep 15, 2025
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 @rodrigoprimo !

@jrfnl jrfnl modified the milestones: 3.2.x, 3.3.0 Sep 16, 2025
Copy link
Member

@GaryJones GaryJones left a comment

Choose a reason for hiding this comment

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

Good job!

@jrfnl
Copy link
Member

jrfnl commented Sep 17, 2025

@rodrigoprimo Could you please rebase the PR to make it mergable ?

The DiscouragedFunctionsUnitTest.inc tests are currently used to test the `AbstractFunctionRestrictions` class and mostly covered everything that was necessary. Except fully qualified call to a non-global function. I opted to add the test in the middle of the file and remove a blank line to keep the namespaced calls tests together.
Warnings and errors in this test case file are declared in a way that is different from the other test case files.

It seems to me that it is better not to add a few tests safeguarding the way that namespaced names are handled to this file, instead they should be added to a new file. Thus, it is necessary to rename this one.
@rodrigoprimo rodrigoprimo force-pushed the add-abstract-function-restrictions-tests branch from 821535d to 97b2e96 Compare September 18, 2025 13:11
@rodrigoprimo
Copy link
Collaborator Author

rodrigoprimo commented Sep 18, 2025

Force-pushed to rebase this PR without changes. See #2572 (comment) for more details on the failing PHPStan job.

@jrfnl jrfnl merged commit ae34825 into WordPress:develop Sep 18, 2025
35 of 36 checks passed
@rodrigoprimo rodrigoprimo deleted the add-abstract-function-restrictions-tests branch September 19, 2025 13:20
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.

3 participants