From af82f36bff8986caec2f2defd9acb21c94ff0ba6 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 23 Oct 2020 19:23:55 +0200 Subject: [PATCH 01/12] Functions/DynamicCalls: annotate the unit tests ... to show which ones should fail and which should pass. --- WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc index 7cde79e4..adf80eaf 100644 --- a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc @@ -6,10 +6,10 @@ function my_test() { $my_notokay_func = 'extract'; -$my_notokay_func(); +$my_notokay_func(); // Bad. $my_okay_func = 'my_test'; -$my_okay_func(); +$my_okay_func(); // OK. From d8f958d4f6718eeddc6d5ddbabbf9cef6f088364 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 23 Oct 2020 18:47:46 +0200 Subject: [PATCH 02/12] Functions/DynamicCalls: remove redundant conditions [1] This sniff only listens to `T_VARIABLE` tokens, so checking that what was received is a `T_VARIABLE` is redundant. --- .../Sniffs/Functions/DynamicCallsSniff.php | 22 ------------------- 1 file changed, 22 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php index 660bd0e1..90721673 100644 --- a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php +++ b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php @@ -94,17 +94,6 @@ public function process_token( $stackPtr ) { * @return void */ private function collect_variables() { - /* - * Make sure we are working with a variable, - * get its value if so. - */ - - if ( - $this->tokens[ $this->stackPtr ]['type'] !== - 'T_VARIABLE' - ) { - return; - } $current_var_name = $this->tokens[ $this->stackPtr ]['content']; @@ -181,17 +170,6 @@ private function find_dynamic_calls() { return; } - /* - * Make sure we do have a variable to work with. - */ - - if ( - $this->tokens[ $this->stackPtr ]['type'] !== - 'T_VARIABLE' - ) { - return; - } - /* * If variable is not found in our registry of * variables, do nothing, as we cannot be From 0f55b11a01e5aff98496848e4d8eaf412eee34af Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 23 Oct 2020 18:49:11 +0200 Subject: [PATCH 03/12] Functions/DynamicCalls: remove redundant conditions [2] The condition above checks if a token is `T_EQUAL` and bows out if it is not. The only code matching on `T_EQUAL` is the `=` operator, which will always have a `length` of `1`. --- WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php | 4 ---- 1 file changed, 4 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php index 90721673..b8530878 100644 --- a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php +++ b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php @@ -120,10 +120,6 @@ private function collect_variables() { return; } - if ( $this->tokens[ $t_item_key ]['length'] !== 1 ) { - return; - } - /* * Find encapsulated string ( "" ) */ From db135726e0c85dd204a97f9990d7e54041ffca37 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 23 Oct 2020 19:19:21 +0200 Subject: [PATCH 04/12] Functions/DynamicCalls: improve code readability * Cutting code lines off at 50 chars is maybe taking it a little too far, especially as it makes assignments hard to read. * Use proper tags in docblocks. * Improve (fix) documentation (and move it to the right place). --- .../Sniffs/Functions/DynamicCallsSniff.php | 83 +++++++------------ 1 file changed, 28 insertions(+), 55 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php index b8530878..c7d38801 100644 --- a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php +++ b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php @@ -10,22 +10,20 @@ use WordPressVIPMinimum\Sniffs\Sniff; /** - * This sniff enforces that certain functions are not - * dynamically called. + * This sniff enforces that certain functions are not dynamically called. * * An example: - * - * + * ```php * $func = 'func_num_args'; * $func(); - * + * ``` * - * See here: http://php.net/manual/en/migration71.incompatible.php + * Note that this sniff does not catch all possible forms of dynamic calling, only some. * - * Note that this sniff does not catch all possible forms of dynamic - * calling, only some. + * @link http://php.net/manual/en/migration71.incompatible.php */ class DynamicCallsSniff extends Sniff { + /** * Functions that should not be called dynamically. * @@ -44,10 +42,11 @@ class DynamicCallsSniff extends Sniff { ]; /** - * Array of functions encountered, along with their values. - * Populated on run-time. + * Array of variable assignments encountered, along with their values. * - * @var array + * Populated at run-time. + * + * @var array The key is the name of the variable, the value, its assigned value. */ private $variables_arr = []; @@ -61,8 +60,6 @@ class DynamicCallsSniff extends Sniff { /** * Returns the token types that this sniff is interested in. * - * We want everything variable- and function-related. - * * @return array(int) */ public function register() { @@ -87,9 +84,8 @@ public function process_token( $stackPtr ) { } /** - * Finds any variable-definitions in the file being processed, - * and stores them internally in a private array. The data stored - * is the name of the variable and its assigned value. + * Finds any variable-definitions in the file being processed and stores them + * internally in a private array. * * @return void */ @@ -98,11 +94,9 @@ private function collect_variables() { $current_var_name = $this->tokens[ $this->stackPtr ]['content']; /* - * Find assignments ( $foo = "bar"; ) - * -- do this by finding all non-whitespaces, and - * check if the first one is T_EQUAL. + * Find assignments ( $foo = "bar"; ) by finding all non-whitespaces, + * and checking if the first one is T_EQUAL. */ - $t_item_key = $this->phpcsFile->findNext( [ T_WHITESPACE ], $this->stackPtr + 1, @@ -121,7 +115,7 @@ private function collect_variables() { } /* - * Find encapsulated string ( "" ) + * Find encapsulated string ( "" ). */ $t_item_key = $this->phpcsFile->findNext( [ T_CONSTANT_ENCAPSED_STRING ], @@ -137,74 +131,53 @@ private function collect_variables() { } /* - * We have found variable-assignment, - * register its name and value in the + * We have found variable-assignment, register its name and value in the * internal array for later usage. */ + $current_var_value = $this->tokens[ $t_item_key ]['content']; - $current_var_value = - $this->tokens[ $t_item_key ]['content']; - - $this->variables_arr[ $current_var_name ] = - str_replace( "'", '', $current_var_value ); + $this->variables_arr[ $current_var_name ] = str_replace( "'", '', $current_var_value ); } /** * Find any dynamic calls being made using variables. - * Report on this when found, using name of the function - * in the message. + * + * Report on this when found, using the name of the function in the message. * * @return void */ private function find_dynamic_calls() { - /* - * No variables detected; no basis for doing - * anything - */ - + // No variables detected; no basis for doing anything. if ( empty( $this->variables_arr ) ) { return; } /* - * 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 - * blacklisted ones. + * 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 blacklisted ones. */ - - if ( ! isset( - $this->variables_arr[ $this->tokens[ $this->stackPtr ]['content'] ] - ) ) { + if ( ! isset( $this->variables_arr[ $this->tokens[ $this->stackPtr ]['content'] ] ) ) { return; } /* - * Check if we have an '(' next, or separated by whitespaces - * from our current position. + * Check if we have an '(' next, or separated by whitespaces from our current position. */ $i = 0; do { $i++; - } while ( - $this->tokens[ $this->stackPtr + $i ]['type'] === - 'T_WHITESPACE' - ); + } while ( $this->tokens[ $this->stackPtr + $i ]['type'] === 'T_WHITESPACE' ); - if ( - $this->tokens[ $this->stackPtr + $i ]['type'] !== - 'T_OPEN_PARENTHESIS' - ) { + if ( $this->tokens[ $this->stackPtr + $i ]['type'] !== 'T_OPEN_PARENTHESIS' ) { return; } $t_item_key = $this->stackPtr + $i; /* - * We have a variable match, but make sure it contains name - * of a function which is on our blacklist. + * We have a variable match, but make sure it contains name of a function which is on our blacklist. */ if ( ! in_array( From b6ebf8669c500241893cd384178e80696418bcad Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 23 Oct 2020 19:19:53 +0200 Subject: [PATCH 05/12] Functions/DynamicCalls: minor simplification Join two conditions which both return anyway. --- WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php index c7d38801..9738b162 100644 --- a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php +++ b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php @@ -106,11 +106,7 @@ private function collect_variables() { true ); - if ( $t_item_key === false ) { - return; - } - - if ( $this->tokens[ $t_item_key ]['type'] !== 'T_EQUAL' ) { + if ( $t_item_key === false || $this->tokens[ $t_item_key ]['code'] !== T_EQUAL ) { return; } From df8c596f1e1b6013f15bc54d65fe6eaa93ba334c Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 23 Oct 2020 18:52:33 +0200 Subject: [PATCH 06/12] Functions/DynamicCalls: bug fix - ignore comments [1] Skip over both whitespace, as well as comments. This reduces false negatives. Includes unit test. --- WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php | 3 ++- WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc | 4 ++-- WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.php | 3 ++- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php index 9738b162..0c6210f5 100644 --- a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php +++ b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php @@ -7,6 +7,7 @@ namespace WordPressVIPMinimum\Sniffs\Functions; +use PHP_CodeSniffer\Util\Tokens; use WordPressVIPMinimum\Sniffs\Sniff; /** @@ -98,7 +99,7 @@ private function collect_variables() { * and checking if the first one is T_EQUAL. */ $t_item_key = $this->phpcsFile->findNext( - [ T_WHITESPACE ], + Tokens::$emptyTokens, $this->stackPtr + 1, null, true, diff --git a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc index adf80eaf..49f207a4 100644 --- a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc @@ -11,5 +11,5 @@ $my_notokay_func(); // Bad. $my_okay_func = 'my_test'; $my_okay_func(); // OK. - - +$test_with_comment /*comment*/ = 'func_get_args'; +$test_with_comment(); // Bad. diff --git a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.php b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.php index 98057179..51f06e72 100644 --- a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.php +++ b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.php @@ -25,7 +25,8 @@ class DynamicCallsUnitTest extends AbstractSniffUnitTest { */ public function getErrorList() { return [ - 9 => 1, + 9 => 1, + 15 => 1, ]; } From 8e6362745f750d1743c4a2fc37eedecdb1a01aa2 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 23 Oct 2020 19:35:38 +0200 Subject: [PATCH 07/12] Functions/DynamicCalls: bug fix - don't blindly use the next text string A variable value may be build up of multiple tokens. As it was, the sniff would look for the first text string token after the equal sign within the variable assignment statement, but this disregards that: 1. The text string token found may not be the only token in the statement. 2. A statement can end on a PHP close tag (possibly a bug in PHPCS itself, but that's another matter), which would lead the sniff to look at the next statement for text strings. Fixed now. Includes unit tests, the first four of which resulted in false positives previously. --- .../Sniffs/Functions/DynamicCallsSniff.php | 36 ++++++++++++------- .../Tests/Functions/DynamicCallsUnitTest.inc | 17 +++++++++ 2 files changed, 40 insertions(+), 13 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php index 0c6210f5..acbc6661 100644 --- a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php +++ b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php @@ -112,26 +112,36 @@ private function collect_variables() { } /* - * Find encapsulated string ( "" ). + * Find assignments which only assign a plain text string. */ - $t_item_key = $this->phpcsFile->findNext( - [ T_CONSTANT_ENCAPSED_STRING ], - $t_item_key + 1, - null, - false, - null, - true - ); + $end_of_statement = $this->phpcsFile->findNext( [ T_SEMICOLON, T_CLOSE_TAG ], ( $t_item_key + 1 ) ); + $value_ptr = null; + + for ( $i = $t_item_key + 1; $i < $end_of_statement; $i++ ) { + if ( isset( Tokens::$emptyTokens[ $this->tokens[ $i ]['code'] ] ) === true ) { + continue; + } + + if ( $this->tokens[ $i ]['code'] !== T_CONSTANT_ENCAPSED_STRING ) { + // Not a plain text string value. Value cannot be determined reliably. + return; + } + + $value_ptr = $i; + } - if ( $t_item_key === false ) { + if ( isset( $value_ptr ) === false ) { + // Parse error. Bow out. return; } /* - * We have found variable-assignment, register its name and value in the - * internal array for later usage. + * If we reached the end of the loop and the $value_ptr was set, we know for sure + * this was a plain text string variable assignment. + * + * Register its name and value in the internal array for later usage. */ - $current_var_value = $this->tokens[ $t_item_key ]['content']; + $current_var_value = $this->tokens[ $value_ptr ]['content']; $this->variables_arr[ $current_var_name ] = str_replace( "'", '', $current_var_value ); } diff --git a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc index 49f207a4..cbacf107 100644 --- a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc @@ -13,3 +13,20 @@ $my_okay_func(); // OK. $test_with_comment /*comment*/ = 'func_get_args'; $test_with_comment(); // Bad. + +$test_getting_the_actual_value_1 = function_call( 'extract' ); +$test_getting_the_actual_value_1(); // OK. Unclear what the actual variable value will be. + +$test_getting_the_actual_value_2 = $array['compact']; +$test_getting_the_actual_value_2(); // OK. Unclear what the actual variable value will be. + +$test_getting_the_actual_value_3 = 10 ?> +
html
+ Date: Fri, 23 Oct 2020 19:52:27 +0200 Subject: [PATCH 08/12] Functions/DynamicCalls: bug fix - allow for double quotes Text strings can use both single quotes as well as double quotes. When the text string contains an interpolated variable, it will be tokenized as `T_DOUBLE_QUOTED_STRING`, but when it is a plain text string, a double quoted text string will be tokenized as `T_CONSTANT_ENCAPSED_STRING`, same as single quoted text string. The sniff did not take this into account, leading to false negatives. The sniff also would strip quotes from within a text - `'my\'text'` - . This did not cause a problem for this sniff as function names cannot have back slashes in them, but it was still wrong. Fixed now by using the WPCS `strip_quotes()` method. Includes unit test which would fail previously. --- WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php | 4 ++-- WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc | 3 +++ WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.php | 1 + 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php index acbc6661..715cbf5f 100644 --- a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php +++ b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php @@ -141,9 +141,9 @@ private function collect_variables() { * * Register its name and value in the internal array for later usage. */ - $current_var_value = $this->tokens[ $value_ptr ]['content']; + $current_var_value = $this->strip_quotes( $this->tokens[ $value_ptr ]['content'] ); - $this->variables_arr[ $current_var_name ] = str_replace( "'", '', $current_var_value ); + $this->variables_arr[ $current_var_name ] = $current_var_value; } /** diff --git a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc index cbacf107..b8510e09 100644 --- a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc @@ -30,3 +30,6 @@ $test_getting_the_actual_value_4 = 'get_defined_vars' . $source; $test_getting_the_actual_value_4(); // OK. Unclear what the actual variable value will be. $ensure_no_notices_are_thrown_on_parse_error = /*comment*/ ; + +$test_double_quoted_string = "assert"; +$test_double_quoted_string(); // Bad. diff --git a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.php b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.php index 51f06e72..4b8e764d 100644 --- a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.php +++ b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.php @@ -27,6 +27,7 @@ public function getErrorList() { return [ 9 => 1, 15 => 1, + 35 => 1, ]; } From b093d623e4d67e22939cc14f38f8b88d06922bab Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 23 Oct 2020 20:21:18 +0200 Subject: [PATCH 09/12] Functions/DynamicCalls: bug fix - fix memory + performance issue The sniff maintains a cache of all the variables it has seen and their assigned value. When a variable is encountered, it would: * Check if it was an (plain text) assignment and if so, register the variable name + value to the cache. * Next, call the `find_dynamic_calls()` method, which first checks if any variables have been registered to the cache before doing anything. * And then checks for dynamic function calls and if one is found, checks if the variable used is one registered in the cache with a value we are looking for. This is highly inefficient as text string variable assignments are common and, as it was, _every single one_ would be added to the cache. With a large code base, that means that the cache could grow pretty large. It also means that the logic to determine if something is a dynamic function call would be executed even when there would be no text strings registered in the cache which could match any of the ones we're looking for. By changing the order of the logic, the memory leak and performance inefficiency is removed. With the updated logic, the sniff will: * Check if it was an (plain text) assignment **and if the text string matches one we're looking for** and if so, register the variable name + value to the cache. * Next, call the `find_dynamic_calls()` method, which first checks if any variables have been registered to the cache before doing anything. * And then checks for dynamic function calls. This means that if none of the previous assignments encountered matches any of the target text strings (~ 99% of the time), this sniff will bow out at step 2 before executing the logic to check if a variable assignment is a dynamic function call. --- .../Sniffs/Functions/DynamicCallsSniff.php | 41 ++++++++----------- 1 file changed, 17 insertions(+), 24 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php index 715cbf5f..b861e595 100644 --- a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php +++ b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php @@ -31,15 +31,15 @@ class DynamicCallsSniff extends Sniff { * @var array */ private $blacklisted_functions = [ - 'assert', - 'compact', - 'extract', - 'func_get_args', - 'func_get_arg', - 'func_num_args', - 'get_defined_vars', - 'mb_parse_str', - 'parse_str', + 'assert' => true, + 'compact' => true, + 'extract' => true, + 'func_get_args' => true, + 'func_get_arg' => true, + 'func_num_args' => true, + 'get_defined_vars' => true, + 'mb_parse_str' => true, + 'parse_str' => true, ]; /** @@ -138,11 +138,17 @@ private function collect_variables() { /* * If we reached the end of the loop and the $value_ptr was set, we know for sure * this was a plain text string variable assignment. - * - * Register its name and value in the internal array for later usage. */ $current_var_value = $this->strip_quotes( $this->tokens[ $value_ptr ]['content'] ); + if ( isset( $this->blacklisted_functions[ $current_var_value ] ) === false ) { + // Text string is not one of the ones we're looking for. + return; + } + + /* + * Register the variable name and value in the internal array for later usage. + */ $this->variables_arr[ $current_var_name ] = $current_var_value; } @@ -183,19 +189,6 @@ private function find_dynamic_calls() { $t_item_key = $this->stackPtr + $i; - /* - * We have a variable match, but make sure it contains name of a function which is on our blacklist. - */ - - if ( ! in_array( - $this->variables_arr[ $this->tokens[ $this->stackPtr ]['content'] ], - $this->blacklisted_functions, - true - ) ) { - return; - } - - // We do, so report. $message = 'Dynamic calling is not recommended in the case of %s.'; $data = [ $this->variables_arr[ $this->tokens[ $this->stackPtr ]['content'] ] ]; $this->phpcsFile->addError( $message, $t_item_key, 'DynamicCalls', $data ); From 6b4a43dce66c6d4faacbfe81fad632240bcfd1d4 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 23 Oct 2020 20:26:03 +0200 Subject: [PATCH 10/12] Functions/DynamicCalls: rename private property Rename the `private` `$blacklisted_functions` property to `$function_names` to get rid of the use of a non-inclusive term. --- WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php index b861e595..485674bb 100644 --- a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php +++ b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php @@ -30,7 +30,7 @@ class DynamicCallsSniff extends Sniff { * * @var array */ - private $blacklisted_functions = [ + private $function_names = [ 'assert' => true, 'compact' => true, 'extract' => true, @@ -141,7 +141,7 @@ private function collect_variables() { */ $current_var_value = $this->strip_quotes( $this->tokens[ $value_ptr ]['content'] ); - if ( isset( $this->blacklisted_functions[ $current_var_value ] ) === false ) { + if ( isset( $this->function_names[ $current_var_value ] ) === false ) { // Text string is not one of the ones we're looking for. return; } From 3118358cf36a3abbff065dc527f3eea59babb773 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 23 Oct 2020 20:40:20 +0200 Subject: [PATCH 11/12] Functions/DynamicCalls: bug fix - ignore comments [2] Skip over both whitespace, as well as comments and take live coding into account. This reduces false negatives, as well as fixing issue 590. Includes unit tests. Fixes 590 --- .../Sniffs/Functions/DynamicCallsSniff.php | 16 ++++------------ .../Tests/Functions/DynamicCallsUnitTest.inc | 5 ++++- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php index 485674bb..afb64832 100644 --- a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php +++ b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php @@ -174,23 +174,15 @@ private function find_dynamic_calls() { } /* - * Check if we have an '(' next, or separated by whitespaces from our current position. + * Check if we have an '(' next. */ - - $i = 0; - - do { - $i++; - } while ( $this->tokens[ $this->stackPtr + $i ]['type'] === 'T_WHITESPACE' ); - - if ( $this->tokens[ $this->stackPtr + $i ]['type'] !== 'T_OPEN_PARENTHESIS' ) { + $next = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $this->stackPtr + 1 ), null, true ); + if ( $next === false || $this->tokens[ $next ]['code'] !== T_OPEN_PARENTHESIS ) { return; } - $t_item_key = $this->stackPtr + $i; - $message = 'Dynamic calling is not recommended in the case of %s.'; $data = [ $this->variables_arr[ $this->tokens[ $this->stackPtr ]['content'] ] ]; - $this->phpcsFile->addError( $message, $t_item_key, 'DynamicCalls', $data ); + $this->phpcsFile->addError( $message, $this->stackPtr, 'DynamicCalls', $data ); } } diff --git a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc index b8510e09..fc307b2f 100644 --- a/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc @@ -12,7 +12,7 @@ $my_okay_func = 'my_test'; $my_okay_func(); // OK. $test_with_comment /*comment*/ = 'func_get_args'; -$test_with_comment(); // Bad. +$test_with_comment /*comment*/ (); // Bad. $test_getting_the_actual_value_1 = function_call( 'extract' ); $test_getting_the_actual_value_1(); // OK. Unclear what the actual variable value will be. @@ -33,3 +33,6 @@ $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 From 1d1a9777122ee1c57eb292050dbfefa48e4e7191 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 23 Oct 2020 20:43:53 +0200 Subject: [PATCH 12/12] Functions/DynamicCalls: error message tweak --- WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php index afb64832..36c92021 100644 --- a/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php +++ b/WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php @@ -181,7 +181,7 @@ private function find_dynamic_calls() { return; } - $message = 'Dynamic calling is not recommended in the case of %s.'; + $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 ); }