Conversation
… PHP 8.1 octal literals Using the PHPCSUtils `Numbers` class and using the decimal value determined via that class prevents issues with PHP 7.4+ numeric literals with underscores and PHP 8.1+ octal literals when the sniffs are being run on PHP < 7.4. Includes tests.
…ives" data provider more robust (and easier to maintain).
… parse and compile errors These parse errors and compile errors did not affect the tests in any way. The changes in the function/class names are intended to make it more straight-forward to see what is being tested in each case (aside from fixing compile errors "redeclaration of function ....").
... for code in the sniff previously uncovered.
…variations of FQN function calls
…ation A few lines earlier there is already a `continue` for when `$afterVar` would be `false`.
…empty" for the loop ... to prevent some unnecessary token walking. Also: as we are searching a known set of tokens, we know that the "previous token" will always exist, so no need to check against `false`.
…f constant in debug*backtrace() The `DEBUG_BACKTRACE_IGNORE_ARGS` should only be recognized as a reason to ignore the call to the target functions if it refers to the PHP native global constant. This commit fixes the sniff to not have false negatives if a namespaced constant is used instead of the global one. Includes tests.
…ptions in debug*backtrace() As per the PHP manual, while the use of the PHP native constants is recommended, it is also perfectly fine to use a numeric value to set the option flags and this is even documented as such in the manual. This commit adds handling of the documented/known numeric option values which mean the "args" index will not be set, to the sniff. Note: this handling presumes that a plain decimal number would have been passed. It is possible to pass a non-decimal number or even a calculation, and while that is supported in PHP, I deem it extremely unlikely anyone would do so and if they do, it just means they get the warning/error, despite the numeric value being equal to one of the values we're now detecting as flags to ignore the function call, so no harm, no foul. Includes tests. Ref: https://www.php.net/manual/en/function.debug-backtrace.php#refsect1-function.debug-backtrace-parameters
…tion names case-insensitively This was already done correctly for the target functions, but not for the check whether a call to any of the target functions was nested in a call to `array_slice()` or `array_splice()`. Includes tests.
…tives for methods and namespaced functions The sniff contains special casing for when any of the target functions are nested within the PHP native global `array_splice()` or `array_splice()` functions. However, this special casing did not take namespaced functions or methods mirroring the name of the PHP native functions, into account. This commit adds more defensive coding to verify we're really dealing with the PHP native global functions. As the code to check this was already used elsewhere too, it has been moved to a function, so it can be used in both places. Includes tests.
…t value This variable is only accessed within conditions and those conditions already sufficiently ensure that the variable will be set for the conditions path. Even so, PHPStan cannot handle this, so we may as well declare a sensible default for this variable to make things more explicit.
…list assignments Array variables can be destructured via lists and the sniff did not take this into account until now. The net effect of this was: * False positive warning "needs inspection" when a function parameter is list-destructured to variables which are not parameters. * Warning instead of error when a function parameter is assigned a new value via list-destructuring. * False positive error when a function parameter is used as a key in list-destructuring. The key is just used to retrieve the target value from the array and is not changed, so we can safely ignore these. * Accidentally correct handling (warning "needs inspection") for when a reference assignment is used in the list-destructuring of a function parameter. This commit adds dedicated handling of lists to this sniff, which fixes each of these problems. Includes tests.
…array_slice/splice [1] The offset passed to `array_slice()`/`array_splice()` can be a negative number (offset from the end of the array). Until now, the sniff did not handle this and would threat all numbers as positives numbers, which could lead to both false positives as well as false negatives. Fixed now. Includes tests.
…array_slice/splice [2] Both `array_slice()`/`array_splice()` take a `$length` parameter which affects which elements are retrieved. Until now, the sniff did not handle this and would presume all parameter from `$offset` to the end of the list should be taken into account, while if we take length into account, we can prevent some more false positives. Fixed now. Includes tests.
…umbers While probably rare, it is perfectly valid to pass the `$arg_num` parameter for `func_get_arg()` or the `$offset` or `$length` parameters for `array_slice()` and `array_splice()` as non-decimal numbers. And while the second case (`array_slice()`/`array_splice()`) was fixed in a previous commit, the former (`$arg_num`) was not. Fixed now. Includes tests.
….4 underscore integers These are handled correctly since some of the fixes in the previous commit went in (same fixes were needed for other reasons). These tests just safeguard this.
….1 octal literals These are handled correctly since support for non-decimal numbers was added (same fix was needed).
…+ null coalesce assignment In contrast to all other assignment operators, the null coalesce assignment operator _may_ or _may not_ change the value of a variable. With that in mind, I believe it is more appropriate to throw a warning when a parameter is being assigned to with the null coalesce operator (in contrast to the error being thrown for other assignment operators). Includes tests, both for a variety of assignment operators as well as for the null coalesce assignment operator.
…simple control structure conditions If a function parameter is only used in a control structure condition without further conditions/comparisons, we can be sure it has not been changed. This commit adds support for detecting this to reduce false positives further. Notes: * `foreach()` is special cased as we can ignore use of the variable before the `as`, but should error on use of the variable after the `as`. * `catch()` is special cased as any variable in catch will always be an assignment, so we should error on that. * In all other cases, when we're in a control structure condition without other non-empty tokens, we can ignore the use of the variable. Includes handling of PHP 8.0+ `match()` control structures. Includes tests. Loosely related to 796
…ons in PHP 7.4+ arrow functions Arrow functions can engage with variables outside of their own scope, so should not blindly be skipped over, but if one of the target functions is seen within the arrow function body, it applies to the arrow function, not the outside scope, so this needs careful handling. Now, while arrow functions can only contain a single statement in their body, they can still run into the problem this sniff flags (see example code in the tests). Having said that, arrow functions were introduced in PHP 7.4, four years after the change this sniff checks for entered PHP, so anyone who uses any of the target functions in the body of an arrow function should expect the "new" behaviour. With this in mind, this commit explicit does NOT add checking of arrow functions to the sniff. However, it does change the sniff to prevent false positives on any of the target functions being seen within the body of an arrow function. Includes tests.
…ves on PHP 8.0+ attributes `T_STRING` tokens in PHP 8.0+ attributes are either class names or possibly constant names (as a parameter for the class instantiation). They are never function calls. This commit ensures that `T_STRING` tokens in attributes are not confused with function calls. Includes tests.
…ude PHP 8.0 trailing commas in param lists
…::class If a the class name of an object parameter is retrieved using the `$obj::class` syntax, we know the parameter is not being changed, so we can safely ignore it. Includes tests.
…8.0+ named arguments in function calls For the `array_*()` functions to was incidentally and partially added in some of the previous commits, though the code still needed to be adjusted to allow for an unexpected parameter order. For the "target functions", support was not handled so far, so this commit adds support for named parameters. Verified the parameter name used is in line with the name as per the PHP 8.0 release. PHP itself renamed a lot of parameters in PHP 8.0. As named parameters did not exist before PHP 8.0, the parameter name as per PHP 8.0 (or above) is the only relevant name. Name verification reference: * `func_get_arg`: https://3v4l.org/liZU1#veol * `debug_backtrace`: https://3v4l.org/sFqA7#veol * `debug_print_backtrace`: https://3v4l.org/LtnLq#veol * `array_slice`: https://3v4l.org/7hUY7#veol * `array_splice`: https://3v4l.org/XU9KW#veol Includes adding the unit tests to include tests using named parameters. Related to 1239 :point_right: This commit will be more straight-forward to review while ignoring whitespace changes.
…0+ constructor property promotion Shouldn't have any impact, just safeguarding this with a test.
…e as PHP 8.1+ first class callable Using the `func_get_arg*()` functions as first class callables is not allowed in PHP. See: https://3v4l.org/ISt2r and https://3v4l.org/aTjjQ However, using the `debug_*backtrace()` function as first class callable is allowed, but seems like a really bad idea.... See: https://3v4l.org/j8lWg and https://3v4l.org/8qbJF This commit adds handling for this by: * Ignoring the use of `func_get_arg*()` functions when used as first class callables. That's forbidden by PHP, so not our concern. * And flagging the use of `debug_*backtrace()` functions when used as first class callables with a warning with its own error code. Includes tests.
41 tasks
Base automatically changed from
feature/tokengroup-isnumber-allow-for-php-7.4-and-8.1-numeric-literals
to
develop
September 1, 2025 16:03
wimg
approved these changes
Sep 1, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FunctionUse/ArgumentFunctionsReportCurrentValue: test case file - tabs vs spaces
FunctionUse/ArgumentFunctionsReportCurrentValue: make "no false positives" data provider more robust
(and easier to maintain).
FunctionUse/ArgumentFunctionsReportCurrentValue: test case file - fix parse and compile errors
These parse errors and compile errors did not affect the tests in any way.
The changes in the function/class names are intended to make it more straight-forward to see what is being tested in each case (aside from fixing compile errors "redeclaration of function ....").
FunctionUse/ArgumentFunctionsReportCurrentValue: add some extra tests
... for code in the sniff previously uncovered.
FunctionUse/ArgumentFunctionsReportCurrentValue: add more tests with variations of FQN function calls
FunctionUse/ArgumentFunctionsReportCurrentValue: minor code simplification
A few lines earlier there is already a
continuefor when$afterVarwould befalse.FunctionUse/ArgumentFunctionsReportCurrentValue: simplify code with PHPCSUtils [1]
FunctionUse/ArgumentFunctionsReportCurrentValue: simplify code with PHPCSUtils [2]
FunctionUse/ArgumentFunctionsReportCurrentValue: simplify code with PHPCSUtils [3]
FunctionUse/ArgumentFunctionsReportCurrentValue: track "previous non empty" for the loop
... to prevent some unnecessary token walking.
Also: as we are searching a known set of tokens, we know that the "previous token" will always exist, so no need to check against
false.FunctionUse/ArgumentFunctionsReportCurrentValue: bug fix - handling of constant in debug*backtrace()
The
DEBUG_BACKTRACE_IGNORE_ARGSshould only be recognized as a reason to ignore the call to the target functions if it refers to the PHP native global constant.This commit fixes the sniff to not have false negatives if a namespaced constant is used instead of the global one.
Includes tests.
FunctionUse/ArgumentFunctionsReportCurrentValue: allow for numeric $options in debug*backtrace()
As per the PHP manual, while the use of the PHP native constants is recommended, it is also perfectly fine to use a numeric value to set the option flags and this is even documented as such in the manual.
This commit adds handling of the documented/known numeric option values which mean the "args" index will not be set, to the sniff.
Note: this handling presumes that a plain decimal number would have been passed.
It is possible to pass a non-decimal number or even a calculation, and while that is supported in PHP, I deem it extremely unlikely anyone would do so and if they do, it just means they get the warning/error, despite the numeric value being equal to one of the values we're now detecting as flags to ignore the function call, so no harm, no foul.
Includes tests.
Ref: https://www.php.net/manual/en/function.debug-backtrace.php#refsect1-function.debug-backtrace-parameters
FunctionUse/ArgumentFunctionsReportCurrentValue: bug fix - check function names case-insensitively
This was already done correctly for the target functions, but not for the check whether a call to any of the target functions was nested in a call to
array_slice()orarray_splice().Includes tests.
FunctionUse/ArgumentFunctionsReportCurrentValue: bug fix - false negatives for methods and namespaced functions
The sniff contains special casing for when any of the target functions are nested within the PHP native global
array_splice()orarray_splice()functions.However, this special casing did not take namespaced functions or methods mirroring the name of the PHP native functions, into account.
This commit adds more defensive coding to verify we're really dealing with the PHP native global functions.
As the code to check this was already used elsewhere too, it has been moved to a function, so it can be used in both places.
Includes tests.
FunctionUse/ArgumentFunctionsReportCurrentValue: set redundant default value
This variable is only accessed within conditions and those conditions already sufficiently ensure that the variable will be set for the conditions path.
Even so, PHPStan cannot handle this, so we may as well declare a sensible default for this variable to make things more explicit.
#FunctionUse/ArgumentFunctionsReportCurrentValue: improve handling of list assignments
Array variables can be destructured via lists and the sniff did not take this into account until now.
The net effect of this was:
This commit adds dedicated handling of lists to this sniff, which fixes each of these problems.
Includes tests.
FunctionUse/ArgumentFunctionsReportCurrentValue: improve handling of array_slice/splice [1]
The offset passed to
array_slice()/array_splice()can be a negative number (offset from the end of the array).Until now, the sniff did not handle this and would threat all numbers as positives numbers, which could lead to both false positives as well as false negatives.
Fixed now.
Includes tests.
FunctionUse/ArgumentFunctionsReportCurrentValue: improve handling of array_slice/splice [2]
Both
array_slice()/array_splice()take a$lengthparameter which affects which elements are retrieved.Until now, the sniff did not handle this and would presume all parameter from
$offsetto the end of the list should be taken into account, while if we take length into account, we can prevent some more false positives.Fixed now.
Includes tests.
FunctionUse/ArgumentFunctionsReportCurrentValue: handle non-decimal numbers
While probably rare, it is perfectly valid to pass the
$arg_numparameter forfunc_get_arg()or the$offsetor$lengthparameters forarray_slice()andarray_splice()as non-decimal numbers.And while the second case (
array_slice()/array_splice()) was fixed in a previous commit, the former ($arg_num) was not.Fixed now.
Includes tests.
FunctionUse/ArgumentFunctionsReportCurrentValue: add tests with PHP 7.4 underscore integers
These are handled correctly since some of the fixes in the previous commit went in (same fixes were needed for other reasons).
These tests just safeguard this.
FunctionUse/ArgumentFunctionsReportCurrentValue: add tests with PHP 8.1 octal literals
These are handled correctly since support for non-decimal numbers was added (same fix was needed).
FunctionUse/ArgumentFunctionsReportCurrentValue: special case PHP 7.4+ null coalesce assignment
In contrast to all other assignment operators, the null coalesce assignment operator may or may not change the value of a variable.
With that in mind, I believe it is more appropriate to throw a warning when a parameter is being assigned to with the null coalesce operator (in contrast to the error being thrown for other assignment operators).
Includes tests, both for a variety of assignment operators as well as for the null coalesce assignment operator.
FunctionUse/ArgumentFunctionsReportCurrentValue: improve handling of simple control structure conditions
If a function parameter is only used in a control structure condition without further conditions/comparisons, we can be sure it has not been changed.
This commit adds support for detecting this to reduce false positives further.
Notes:
foreach()is special cased as we can ignore use of the variable before theas, but should error on use of the variable after theas.catch()is special cased as any variable in catch will always be an assignment, so we should error on that.Includes handling of PHP 8.0+
match()control structures.Includes tests.
Loosely related to #796
FunctionUse/ArgumentFunctionsReportCurrentValue: ignore target functions in PHP 7.4+ arrow functions
Arrow functions can engage with variables outside of their own scope, so should not blindly be skipped over, but if one of the target functions is seen within the arrow function body, it applies to the arrow function, not the outside scope, so this needs careful handling.
Now, while arrow functions can only contain a single statement in their body, they can still run into the problem this sniff flags (see example code in the tests).
Having said that, arrow functions were introduced in PHP 7.4, four years after the change this sniff checks for entered PHP, so anyone who uses any of the target functions in the body of an arrow function should expect the "new" behaviour.
With this in mind, this commit explicit does NOT add checking of arrow functions to the sniff.
However, it does change the sniff to prevent false positives on any of the target functions being seen within the body of an arrow function.
Includes tests.
FunctionUse/ArgumentFunctionsReportCurrentValue: prevent false positives on PHP 8.0+ attributes
T_STRINGtokens in PHP 8.0+ attributes are either class names or possibly constant names (as a parameter for the class instantiation).They are never function calls.
This commit ensures that
T_STRINGtokens in attributes are not confused with function calls.Includes tests.
FunctionUse/ArgumentFunctionsReportCurrentValue: update tests to include PHP 8.0 trailing commas in param lists
FunctionUse/ArgumentFunctionsReportCurrentValue: ignore PHP 8.0+ $obj::class
If a the class name of an object parameter is retrieved using the
$obj::classsyntax, we know the parameter is not being changed, so we can safely ignore it.Includes tests.
FunctionUse/ArgumentFunctionsReportCurrentValue: add support for PHP 8.0+ named arguments in function calls
For the
array_*()functions to was incidentally and partially added in some of the previous commits, though the code still needed to be adjusted to allow for an unexpected parameter order.For the "target functions", support was not handled so far, so this commit adds support for named parameters.
Verified the parameter name used is in line with the name as per the PHP 8.0 release. PHP itself renamed a lot of parameters in PHP 8.0. As named parameters did not exist before PHP 8.0, the parameter name as per PHP 8.0 (or above) is the only relevant name.
Name verification reference:
func_get_arg: https://3v4l.org/liZU1#veoldebug_backtrace: https://3v4l.org/sFqA7#veoldebug_print_backtrace: https://3v4l.org/LtnLq#veolarray_slice: https://3v4l.org/7hUY7#veolarray_splice: https://3v4l.org/XU9KW#veolIncludes adding the unit tests to include tests using named parameters.
Related to #1239
👉 This commit will be more straight-forward to review while ignoring whitespace changes.
FunctionUse/ArgumentFunctionsReportCurrentValue: add test with PHP 8.0+ constructor property promotion
Shouldn't have any impact, just safeguarding this with a test.
FunctionUse/ArgumentFunctionsReportCurrentValue: add new error for use as PHP 8.1+ first class callable
Using the
func_get_arg*()functions as first class callables is not allowed in PHP. See: https://3v4l.org/ISt2r and https://3v4l.org/aTjjQHowever, using the
debug_*backtrace()function as first class callable is allowed, but seems like a really bad idea....See: https://3v4l.org/j8lWg and https://3v4l.org/8qbJF
This commit adds handling for this by:
func_get_arg*()functions when used as first class callables. That's forbidden by PHP, so not our concern.debug_*backtrace()functions when used as first class callables with a warning with its own error code.Includes tests.