Merge pull request #592 from Automattic/fix/590-prevent-undefined-offset-notice

Functions/DynamicCalls: various bug fixes and improvements

Author: rebeccahum

WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php

@@ -7,47 +7,47 @@
 
 namespace WordPressVIPMinimum\Sniffs\Functions;
 
+use PHP_CodeSniffer\Util\Tokens;
 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:
- *
- * <code>
+ * ```php
  *   $func = 'func_num_args';
  *   $func();
- * </code>
+ * ```
  *
- * 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.
 	 *
 	 * @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',
+	private $function_names = [
+		'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,
 	];
 
 	/**
-	 * 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 +61,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,163 +85,104 @@ 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
 	 */
 	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'];
 
 		/*
-		 * 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 ],
+			Tokens::$emptyTokens,
 			$this->stackPtr + 1,
 			null,
 			true,
 			null,
 			true
 		);
 
-		if ( $t_item_key === false ) {
+		if ( $t_item_key === false || $this->tokens[ $t_item_key ]['code'] !== T_EQUAL ) {
 			return;
 		}
 
-		if ( $this->tokens[ $t_item_key ]['type'] !== 'T_EQUAL' ) {
-			return;
+		/*
+		 * 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;
+
+		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 ( $this->tokens[ $t_item_key ]['length'] !== 1 ) {
+		if ( isset( $value_ptr ) === false ) {
+			// Parse error. Bow out.
 			return;
 		}
 
 		/*
-		 * Find encapsulated string ( "" )
+		 * 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.
 		 */
-		$t_item_key = $this->phpcsFile->findNext(
-			[ T_CONSTANT_ENCAPSED_STRING ],
-			$t_item_key + 1,
-			null,
-			false,
-			null,
-			true
-		);
+		$current_var_value = $this->strip_quotes( $this->tokens[ $value_ptr ]['content'] );
 
-		if ( $t_item_key === false ) {
+		if ( isset( $this->function_names[ $current_var_value ] ) === false ) {
+			// Text string is not one of the ones we're looking for.
 			return;
 		}
 
 		/*
-		 * We have found variable-assignment,
-		 * register its name and value in the
-		 * internal array for later usage.
+		 * Register the variable name and value in the internal array for later usage.
 		 */
-
-		$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 ] = $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;
 		}
 
 		/*
-		 * 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
-		 * 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.
 		 */
-
-		$i = 0;
-
-		do {
-			$i++;
-		} while (
-			$this->tokens[ $this->stackPtr + $i ]['type'] ===
-				'T_WHITESPACE'
-		);
-
-		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.
-		 */
-
-		if ( ! in_array(
-			$this->variables_arr[ $this->tokens[ $this->stackPtr ]['content'] ],
-			$this->blacklisted_functions,
-			true
-		) ) {
+		$next = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $this->stackPtr + 1 ), null, true );
+		if ( $next === false || $this->tokens[ $next ]['code'] !== T_OPEN_PARENTHESIS ) {
 			return;
 		}
 
-		// We do, so report.
-		$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, $t_item_key, 'DynamicCalls', $data );
+		$this->phpcsFile->addError( $message, $this->stackPtr, 'DynamicCalls', $data );
 	}
 }

WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.inc

@@ -6,10 +6,33 @@ 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.
 
+$test_with_comment /*comment*/ = 'func_get_args';
+$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.
 
+$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 ?>
+<div>html</div>
+<?php
+echo 'extract';
+$test_getting_the_actual_value_3(); // OK. Broken function call, but not calling extract().
+
+$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.
+
+// Intentional parse error. This has to be the last test in the file.
+$my_notokay_func

WordPressVIPMinimum/Tests/Functions/DynamicCallsUnitTest.php

@@ -25,7 +25,9 @@ class DynamicCallsUnitTest extends AbstractSniffUnitTest {
 	 */
 	public function getErrorList() {
 		return [
-			9 => 1,
+			9  => 1,
+			15 => 1,
+			35 => 1,
 		];
 	}