From b9f3242cf80b1c0079da5430a3c00e38c7e5c492 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 27 Jul 2025 05:07:13 +0200 Subject: [PATCH 1/5] Functions/DynamicCalls: move parse error test to own file --- ...nitTest.inc => DynamicCallsUnitTest.1.inc} | 3 --- .../Functions/DynamicCallsUnitTest.2.inc | 4 ++++ .../Tests/Functions/DynamicCallsUnitTest.php | 21 +++++++++++++------ 3 files changed, 19 insertions(+), 9 deletions(-) rename WordPressVIPMinimum/Tests/Functions/{DynamicCallsUnitTest.inc => DynamicCallsUnitTest.1.inc} (92%) create mode 100644 WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.2.inc diff --git a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.1.inc similarity index 92% rename from WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc rename to WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.1.inc index fc307b2f..020383bb 100644 --- a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.1.inc @@ -33,6 +33,3 @@ $ensure_no_notices_are_thrown_on_parse_error = /*comment*/ ; $test_double_quoted_string = "assert"; $test_double_quoted_string(); // Bad. - -// Intentional parse error. This has to be the last test in the file. -$my_notokay_func diff --git a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.2.inc b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.2.inc new file mode 100644 index 00000000..fca40064 --- /dev/null +++ b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.2.inc @@ -0,0 +1,4 @@ + Key is the line number, value is the number of expected errors. */ - public function getErrorList() { - return [ - 9 => 1, - 15 => 1, - 35 => 1, - ]; + public function getErrorList( $testFile = '' ) { + + switch ( $testFile ) { + case 'DynamicCallsUnitTest.1.inc': + return [ + 9 => 1, + 15 => 1, + 35 => 1, + ]; + + default: + return []; + } } /** From 0f6f71b03c84cd2c5b640e6f8ee43bedf4936a56 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 27 Jul 2025 05:41:22 +0200 Subject: [PATCH 2/5] Functions/DynamicCalls: add extra test ... to raise code coverage. --- WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.1.inc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.1.inc b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.1.inc index 020383bb..129d0717 100644 --- a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.1.inc +++ b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.1.inc @@ -3,7 +3,7 @@ function my_test() { echo esc_html( "foo" ); } - +$irrelevant = 10; $my_notokay_func = 'extract'; $my_notokay_func(); // Bad. From 3a3da1a5fd7f86b8444ec44baeff17209f5f28aa Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 27 Jul 2025 05:33:39 +0200 Subject: [PATCH 3/5] Functions/DynamicCalls: remove unnecessary property The `$stackPtr` is not being changed in the functions using it, so there is no need to store it, it can just be passed as a parameter. --- .../Sniffs/Functions/DynamicCallsSniff.php | 33 ++++++++----------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php index a6bf96a3..68ab3091 100644 --- a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php +++ b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php @@ -54,13 +54,6 @@ class DynamicCallsSniff extends Sniff { */ private $variables_arr = []; - /** - * The position in the stack where the token was found. - * - * @var int - */ - private $stackPtr; - /** * Returns the token types that this sniff is interested in. * @@ -78,24 +71,24 @@ public function register() { * @return void */ public function process_token( $stackPtr ) { - $this->stackPtr = $stackPtr; - // First collect all variables encountered and their values. - $this->collect_variables(); + $this->collect_variables( $stackPtr ); // Then find all dynamic calls, and report them. - $this->find_dynamic_calls(); + $this->find_dynamic_calls( $stackPtr ); } /** * Finds any variable-definitions in the file being processed and stores them * internally in a private array. * + * @param int $stackPtr The position in the stack where the token was found. + * * @return void */ - private function collect_variables() { + private function collect_variables( $stackPtr ) { - $current_var_name = $this->tokens[ $this->stackPtr ]['content']; + $current_var_name = $this->tokens[ $stackPtr ]['content']; /* * Find assignments ( $foo = "bar"; ) by finding all non-whitespaces, @@ -103,7 +96,7 @@ private function collect_variables() { */ $t_item_key = $this->phpcsFile->findNext( Tokens::$emptyTokens, - $this->stackPtr + 1, + $stackPtr + 1, null, true, null, @@ -160,9 +153,11 @@ private function collect_variables() { * * Report on this when found, using the name of the function in the message. * + * @param int $stackPtr The position in the stack where the token was found. + * * @return void */ - private function find_dynamic_calls() { + private function find_dynamic_calls( $stackPtr ) { // No variables detected; no basis for doing anything. if ( empty( $this->variables_arr ) ) { return; @@ -172,20 +167,20 @@ private function find_dynamic_calls() { * If variable is not found in our registry of variables, do nothing, as we cannot be * sure that the function being called is one of the disallowed ones. */ - if ( ! isset( $this->variables_arr[ $this->tokens[ $this->stackPtr ]['content'] ] ) ) { + if ( ! isset( $this->variables_arr[ $this->tokens[ $stackPtr ]['content'] ] ) ) { return; } /* * Check if we have an '(' next. */ - $next = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $this->stackPtr + 1 ), null, true ); + $next = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true ); if ( $next === false || $this->tokens[ $next ]['code'] !== T_OPEN_PARENTHESIS ) { return; } $message = 'Dynamic calling is not recommended in the case of %s().'; - $data = [ $this->variables_arr[ $this->tokens[ $this->stackPtr ]['content'] ] ]; - $this->phpcsFile->addError( $message, $this->stackPtr, 'DynamicCalls', $data ); + $data = [ $this->variables_arr[ $this->tokens[ $stackPtr ]['content'] ] ]; + $this->phpcsFile->addError( $message, $stackPtr, 'DynamicCalls', $data ); } } From 3e73a966fedb5d01c1de3407647098e5c85ec266 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 27 Jul 2025 05:40:36 +0200 Subject: [PATCH 4/5] Functions/DynamicCalls: minor tweak For a `findNext()` to find the first non-empty token, limiting to the "current statement" is not really necessary as it will stop quickly anyway. --- .../Sniffs/Functions/DynamicCallsSniff.php | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php index 68ab3091..312a2b36 100644 --- a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php +++ b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php @@ -94,15 +94,7 @@ private function collect_variables( $stackPtr ) { * Find assignments ( $foo = "bar"; ) by finding all non-whitespaces, * and checking if the first one is T_EQUAL. */ - $t_item_key = $this->phpcsFile->findNext( - Tokens::$emptyTokens, - $stackPtr + 1, - null, - true, - null, - true - ); - + $t_item_key = $this->phpcsFile->findNext( Tokens::$emptyTokens, $stackPtr + 1, null, true ); if ( $t_item_key === false || $this->tokens[ $t_item_key ]['code'] !== T_EQUAL ) { return; } From 14e1be4d3896d7e59704ddf972adcf6e9d02b97d Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 27 Jul 2025 05:46:55 +0200 Subject: [PATCH 5/5] Functions/DynamicCalls: bug fix - better end of statement determination Stabilize the "find end of statement" search. After all, assignments may end on a comma or various other tokens too. Includes two tests to demonstrate the bug. --- .../Sniffs/Functions/DynamicCallsSniff.php | 25 ++++++++++++++++--- .../Functions/DynamicCallsUnitTest.1.inc | 13 ++++++++++ .../Tests/Functions/DynamicCallsUnitTest.php | 3 +++ 3 files changed, 38 insertions(+), 3 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php index 312a2b36..2236d44c 100644 --- a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php +++ b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php @@ -45,6 +45,22 @@ class DynamicCallsSniff extends Sniff { 'parse_str' => true, ]; + /** + * Potential end tokens for which the end pointer has to be set back by one. + * + * {@internal The PHPCS `findEndOfStatement()` method is not completely consistent + * in how it returns the statement end. This is just a simple way to bypass + * the inconsistency for our purposes.} + * + * @var array + */ + private $inclusiveStopPoints = [ + T_COLON => true, + T_COMMA => true, + T_DOUBLE_ARROW => true, + T_SEMICOLON => true, + ]; + /** * Array of variable assignments encountered, along with their values. * @@ -102,10 +118,13 @@ private function collect_variables( $stackPtr ) { /* * Find assignments which only assign a plain text string. */ - $end_of_statement = $this->phpcsFile->findNext( [ T_SEMICOLON, T_CLOSE_TAG ], ( $t_item_key + 1 ) ); - $value_ptr = null; + $end_of_statement = $this->phpcsFile->findEndOfStatement( ( $t_item_key + 1 ) ); + if ( isset( $this->inclusiveStopPoints[ $this->tokens[ $end_of_statement ]['code'] ] ) === true ) { + --$end_of_statement; + } - for ( $i = $t_item_key + 1; $i < $end_of_statement; $i++ ) { + $value_ptr = null; + for ( $i = $t_item_key + 1; $i <= $end_of_statement; $i++ ) { if ( isset( Tokens::$emptyTokens[ $this->tokens[ $i ]['code'] ] ) === true ) { continue; } diff --git a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.1.inc b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.1.inc index 129d0717..976c4c00 100644 --- a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.1.inc +++ b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.1.inc @@ -33,3 +33,16 @@ $ensure_no_notices_are_thrown_on_parse_error = /*comment*/ ; $test_double_quoted_string = "assert"; $test_double_quoted_string(); // Bad. + +function hasStaticVars() { + static $staticvar_foo = 'irrelevant', $staticvar_bar = 'func_num_args', $staticvar_baz = 'nothing'; + $staticvar_foo(); // OK. + $staticvar_bar(); // Bad. + $staticvar_baz(); // OK. +} + +function functionParams( $param_foo = 'func_get_arg', $param_bar = 'nothing', $param_baz = 'func_num_args') { + $param_foo(); // Bad. + $param_bar(); // OK. + $param_baz(); // Bad. +} diff --git a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.php b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.php index ab7a08e3..1b1c238e 100644 --- a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.php +++ b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.php @@ -31,6 +31,9 @@ public function getErrorList( $testFile = '' ) { 9 => 1, 15 => 1, 35 => 1, + 40 => 1, + 45 => 1, + 47 => 1, ]; default: