AbstractFunctionRestrictionsSniff::is_targetted_token() incorrectly treats first class callable as a function call

[rodrigoprimo]

Bug Description

The method AbstractFunctionRestrictionsSniff::is_targetted_token() is used to determine if a given token is a function call or not. It does not handle first class callables properly and identifies them as function calls, which is incorrect.

I found this while using AbstractFunctionParameterSniff::process_no_parameters() to create the sniff for #2473. AbstractFunctionParameterSniff is affected by this bug and sees first class callables as a function call without parameters.

Loosely related to this PHPCSUtils PR: PHPCSStandards/PHPCSUtils#362

Minimal Code Snippet

The issue happens when running this command:

phpcs --standard=WordPress --sniffs=WordPress.Utils.I18nTextDomainFixer test.php

Over a file containing this code:

<?php
// phpcs:set WordPress.Utils.I18nTextDomainFixer old_text_domain[] text-domain
// phpcs:set WordPress.Utils.I18nTextDomainFixer new_text_domain something-else

$callable = __(...);

Error Code

FILE: test.php
---------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------------------------------------------
 5 | WARNING | Missing $domain arg and preceding argument(s) (WordPress.Utils.I18nTextDomainFixer.MissingArgs)
---------------------------------------------------------------------------------------------------------------

Environment

Question	Answer
PHP version	8.3
PHP_CodeSniffer version	3.10.1
WordPressCS version	develop branch
PHPCSUtils version	1.0.12
PHPCSExtra version	1.2.1
WordPressCS install type	git clone

Additional Context (optional)

I can try to create a PR to fix this problem, but it seems there are no tests for the AbstractFunctionRestrictionsSniff::is_targetted_token() and for the rest of the AbstractFunctionRestrictionsSniff. I believe it would be important to start by creating tests first.

Tested Against develop Branch?

• I have verified the issue still exists in the develop branch of WordPressCS.

[jrfnl]

The method AbstractFunctionRestrictionsSniff::is_targetted_token() is used to determine if a given token is a function call or not. It does not handle first class callables properly and identifies them as function calls, which is incorrect.

@rodrigoprimo Well, no. The abstract tries to determine whether a function is being used, not whether it is called. That includes recognizing use function... statements and first class callables.

I found this while using AbstractFunctionParameterSniff::process_no_parameters() [snip]. AbstractFunctionParameterSniff is affected by this bug and sees first class callables as a function call without parameters.

That's something to think about. Should the sniffs using that method handle this potential situation ? Should the AbstractFunctionParameterSniff ignore first class callables ? or should the abstract get a new processFirstClassCallable() method ?

Also note that the WPCS native abstracts will be phased out once the planned (and fully tested) abstracts in PHPCSUtils will be available.

I can try to create a PR to fix this problem, but it seems there are no tests for the AbstractFunctionRestrictionsSniff::is_targetted_token() and for the rest of the AbstractFunctionRestrictionsSniff. I believe it would be important to start by creating tests first.

At this time, the abstracts are tested via the sniffs, in particular via the DB\RestrictedFunctions and PHP\StrictInArray sniffs. Considering the abstracts will be deprecated and removed in the foreseeable future, I don't think adding dedicated tests is the way to go. Enhancing the tests in the aforementioned sniffs, is, of course, welcome.

[rodrigoprimo]

@rodrigoprimo Well, no. The abstract tries to determine whether a function is being used, not whether it is called. That includes recognizing use function... statements and first class callable.

@jrfnl, maybe then the short description of the DocBlock for AbstractFunctionRestrictionsSniff::is_targetted_token() should be updated? It directly says that the method verifies if the current token is a function call and not whether a function is being used:

WordPress-Coding-Standards/WordPress/AbstractFunctionRestrictionsSniff.php

Line 217 in dfd6c3a

* Verify is the current token is a function call.

That's something to think about. Should the sniffs using that method handle this potential situation ? Should the AbstractFunctionParameterSniff ignore first class callables ? or should the abstract get a new processFirstClassCallable() method ?

My understanding is that the AbstractFunctionParameterSniff is an abstract to be used by sniffs that deal with function parameters when a given list of functions is called. First class callables and use function are not function calls, and parameters are not passed to the functions when they are used in these two contexts, so I'm inclined to think that we should modify AbstractFunctionParameterSniff to bail in those cases.

That being said, I also see some reasons that make me wonder if it is worth doing this change at this point:

• The abstract is going to be replaced.
• This would be a breaking change so we would need to check all the sniffs that are currently using the abstract to see if they are affected or not.
• To the best of my knowledge, this issue was not reported before, which indicates that it is not a high priority fix.

What do you think?

At this time, the abstracts are tested via the sniffs, in particular via the DB\RestrictedFunctions and PHP\StrictInArray sniffs. Considering the abstracts will be deprecated and removed in the foreseeable future, I don't think adding dedicated tests is the way to go. Enhancing the tests in the aforementioned sniffs, is, of course, welcome.

Thanks for the additional context. I agree that it doesn't make sense to add dedicated tests as those abstracts will be replaced.

[rodrigoprimo]

@jrfnl, I completely forgot about this issue when working on #2518 and saw it now by chance. I believe the main point was addressed in #2518, and the issue can be closed. There is just one outstanding remark:

@rodrigoprimo Well, no. The abstract tries to determine whether a function is being used, not whether it is called. That includes recognizing use function... statements and first class callable.

@jrfnl, maybe then the short description of the DocBlock for AbstractFunctionRestrictionsSniff::is_targetted_token() should be updated? It directly says that the method verifies if the current token is a function call and not whether a function is being used:

WordPress-Coding-Standards/WordPress/AbstractFunctionRestrictionsSniff.php

Line 217 in dfd6c3a

* Verify is the current token is a function call.

[jrfnl]

There is just one outstanding remark:

I'd merge a PR for that without question. Having said that, I'm not going to bother creating one myself as these abstracts are going to be phased out.

[rodrigoprimo]

Fair enough, I will leave it as is. Closing this issue.