diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 560fd62c13..1ff5c36221 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -2740,12 +2740,61 @@ protected function find_array_open_close( $stackPtr ) { } else { // Short array syntax. $opener = $stackPtr; + $closer = $this->tokens[ $stackPtr ]['bracket_closer']; + } - if ( isset( $this->tokens[ $stackPtr ]['bracket_closer'] ) ) { - $closer = $this->tokens[ $stackPtr ]['bracket_closer']; + if ( isset( $opener, $closer ) ) { + return array( + 'opener' => $opener, + 'closer' => $closer, + ); + } + + return false; + } + + /** + * Find the list opener & closer based on a T_LIST or T_OPEN_SHORT_ARRAY token. + * + * @since 2.2.0 + * + * @param int $stackPtr The stack pointer to the array token. + * + * @return array|bool Array with two keys `opener`, `closer` or false if + * not a (short) list token or if either or these + * could not be determined. + */ + protected function find_list_open_close( $stackPtr ) { + /* + * Determine the list opener & closer. + */ + if ( \T_LIST === $this->tokens[ $stackPtr ]['code'] ) { + // PHPCS 3.5.0. + if ( isset( $this->tokens[ $stackPtr ]['parenthesis_opener'] ) ) { + $opener = $this->tokens[ $stackPtr ]['parenthesis_opener']; + + } else { + // PHPCS < 3.5.0. + $next_non_empty = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true ); + if ( false !== $next_non_empty + && \T_OPEN_PARENTHESIS === $this->tokens[ $next_non_empty ]['code'] + ) { + $opener = $next_non_empty; + } + } + + if ( isset( $opener, $this->tokens[ $opener ]['parenthesis_closer'] ) ) { + $closer = $this->tokens[ $opener ]['parenthesis_closer']; } } + if ( \T_OPEN_SHORT_ARRAY === $this->tokens[ $stackPtr ]['code'] + && $this->is_short_list( $stackPtr ) === true + ) { + $opener = $stackPtr; + $closer = $this->tokens[ $stackPtr ]['bracket_closer']; + } + if ( isset( $opener, $closer ) ) { return array( 'opener' => $opener, @@ -3266,4 +3315,71 @@ protected function is_short_list( $stackPtr ) { return $this->is_short_list( $parentOpen ); } + /** + * Get a list of the token pointers to the variables being assigned to in a list statement. + * + * @internal No need to take special measures for nested lists. Nested or not, + * each list part can only contain one variable being written to. + * + * @since 2.2.0 + * + * @param int $stackPtr The position of the T_LIST or T_OPEN_SHORT_ARRAY + * token in the stack. + * @param array $list_open_close Optional. Array containing the token pointers to + * the list opener and closer. + * + * @return array Array with the stack pointers to the variables or an empty + * array when not a (short) list. + */ + protected function get_list_variables( $stackPtr, $list_open_close = array() ) { + if ( \T_LIST !== $this->tokens[ $stackPtr ]['code'] + && \T_OPEN_SHORT_ARRAY !== $this->tokens[ $stackPtr ]['code'] + ) { + return array(); + } + + if ( empty( $list_open_close ) ) { + $list_open_close = $this->find_list_open_close( $stackPtr ); + if ( false === $list_open_close ) { + // Not a (short) list. + return array(); + } + } + + $var_pointers = array(); + $current = $list_open_close['opener']; + $closer = $list_open_close['closer']; + $last = false; + do { + ++$current; + $next_comma = $this->phpcsFile->findNext( \T_COMMA, $current, $closer ); + if ( false === $next_comma ) { + $next_comma = $closer; + $last = true; + } + + // Skip over the "key" part in keyed lists. + $arrow = $this->phpcsFile->findNext( \T_DOUBLE_ARROW, $current, $next_comma ); + if ( false !== $arrow ) { + $current = ( $arrow + 1 ); + } + + /* + * Each list item can only have one variable to which an assignment is being made. + * This can be an array with a (variable) index, but that doesn't matter, we're only + * concerned with the actual variable. + */ + $var = $this->phpcsFile->findNext( \T_VARIABLE, $current, $next_comma ); + if ( false !== $var ) { + // Not an empty list item. + $var_pointers[] = $var; + } + + $current = $next_comma; + + } while ( false === $last ); + + return $var_pointers; + } + } diff --git a/WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php b/WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php index a00f0cf2ad..50ba9c7acb 100644 --- a/WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php +++ b/WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php @@ -21,6 +21,7 @@ * @since 0.12.0 * @since 0.13.0 Class name changed: this class is now namespaced. * @since 1.2.0 Now also checks whether namespaces are prefixed. + * @since 2.2.0 Now also checks variables assigned via the list() construct. * * @uses \WordPressCS\WordPress\Sniff::$custom_test_class_whitelist */ @@ -200,11 +201,13 @@ public function register() { // Set the sniff targets. $targets = array( - \T_NAMESPACE => \T_NAMESPACE, - \T_FUNCTION => \T_FUNCTION, - \T_CONST => \T_CONST, - \T_VARIABLE => \T_VARIABLE, - \T_DOLLAR => \T_DOLLAR, // Variable variables. + \T_NAMESPACE => \T_NAMESPACE, + \T_FUNCTION => \T_FUNCTION, + \T_CONST => \T_CONST, + \T_VARIABLE => \T_VARIABLE, + \T_DOLLAR => \T_DOLLAR, // Variable variables. + \T_LIST => \T_LIST, + \T_OPEN_SHORT_ARRAY => \T_OPEN_SHORT_ARRAY, ); $targets += Tokens::$ooScopeTokens; // T_ANON_CLASS is only used for skipping over test classes. @@ -303,6 +306,11 @@ public function process_token( $stackPtr ) { return $this->process_variable_assignment( $stackPtr ); + } elseif ( \T_LIST === $this->tokens[ $stackPtr ]['code'] + || \T_OPEN_SHORT_ARRAY === $this->tokens[ $stackPtr ]['code'] + ) { + return $this->process_list_assignment( $stackPtr ); + } elseif ( \T_NAMESPACE === $this->tokens[ $stackPtr ]['code'] ) { $namespace_name = $this->get_declared_namespace_name( $stackPtr ); @@ -573,20 +581,24 @@ protected function process_variable_variable( $stackPtr ) { * Check that defined global variables are prefixed. * * @since 0.12.0 + * @since 2.2.0 Added $in_list parameter. * - * @param int $stackPtr The position of the current token in the stack. + * @param int $stackPtr The position of the current token in the stack. + * @param bool $in_list Whether or not this is a variable in a list assignment. + * Defaults to false. * * @return int|void Integer stack pointer to skip forward or void to continue * normal file processing. */ - protected function process_variable_assignment( $stackPtr ) { + protected function process_variable_assignment( $stackPtr, $in_list = false ) { /* * We're only concerned with variables which are being defined. * `is_assigment()` will not recognize property assignments, which is good in this case. * However it will also not recognize $b in `foreach( $a as $b )` as an assignment, so * we need a separate check for that. */ - if ( false === $this->is_assignment( $stackPtr ) + if ( false === $in_list + && false === $this->is_assignment( $stackPtr ) && false === $this->is_foreach_as( $stackPtr ) ) { return; @@ -642,7 +654,7 @@ protected function process_variable_assignment( $stackPtr ) { } } else { // Function parameters do not need to be prefixed. - if ( isset( $this->tokens[ $stackPtr ]['nested_parenthesis'] ) ) { + if ( false === $in_list && isset( $this->tokens[ $stackPtr ]['nested_parenthesis'] ) ) { foreach ( $this->tokens[ $stackPtr ]['nested_parenthesis'] as $opener => $closer ) { if ( isset( $this->tokens[ $opener ]['parenthesis_owner'] ) && ( \T_FUNCTION === $this->tokens[ $this->tokens[ $opener ]['parenthesis_owner'] ]['code'] @@ -655,7 +667,7 @@ protected function process_variable_assignment( $stackPtr ) { } // Properties in a class do not need to be prefixed. - if ( true === $this->is_class_property( $stackPtr ) ) { + if ( false === $in_list && true === $this->is_class_property( $stackPtr ) ) { return; } @@ -716,6 +728,35 @@ protected function process_variable_assignment( $stackPtr ) { } } + /** + * Check that global variables declared via a list construct are prefixed. + * + * @internal No need to take special measures for nested lists. Nested or not, + * each list part can only contain one variable being written to. + * + * @since 2.2.0 + * + * @param int $stackPtr The position of the current token in the stack. + * + * @return int|void Integer stack pointer to skip forward or void to continue + * normal file processing. + */ + protected function process_list_assignment( $stackPtr ) { + $list_open_close = $this->find_list_open_close( $stackPtr ); + if ( false === $list_open_close ) { + // Short array, not short list. + return; + } + + $var_pointers = $this->get_list_variables( $stackPtr, $list_open_close ); + foreach ( $var_pointers as $ptr ) { + $this->process_variable_assignment( $ptr, true ); + } + + // No need to re-examine these variables. + return $list_open_close['closer']; + } + /** * Process the parameters of a matched function. * diff --git a/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php b/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php index bff0788597..7f2be17c5c 100644 --- a/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php +++ b/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php @@ -24,6 +24,7 @@ * @since 1.0.0 This sniff has been moved from the `Variables` category to the `WP` * category and renamed from `GlobalVariables` to `GlobalVariablesOverride`. * @since 1.1.0 The sniff now also detects variables being overriden in the global namespace. + * @since 2.2.0 The sniff now also detects variable assignments via the list() construct. * * @uses \WordPressCS\WordPress\Sniff::$custom_test_class_whitelist */ @@ -88,6 +89,8 @@ public function register() { $targets = array( \T_GLOBAL, \T_VARIABLE, + \T_LIST, + \T_OPEN_SHORT_ARRAY, ); // Only used to skip over test classes. @@ -129,12 +132,19 @@ public function process_token( $stackPtr ) { /* * Examine variables within a function scope based on a `global` statement in the * function. - * Examine variable not within a function scope and access to the `$GLOBALS` + * Examine variables not within a function scope, but within a list construct, based + * on that. + * Examine variables not within a function scope and access to the `$GLOBALS` * variable based on the variable token. */ $in_function_scope = $this->phpcsFile->hasCondition( $stackPtr, array( \T_FUNCTION, \T_CLOSURE ) ); - if ( \T_VARIABLE === $token['code'] + if ( ( \T_LIST === $token['code'] || \T_OPEN_SHORT_ARRAY === $token['code'] ) + && false === $in_function_scope + && false === $this->treat_files_as_scoped + ) { + return $this->process_list_assignment( $stackPtr ); + } elseif ( \T_VARIABLE === $token['code'] && ( '$GLOBALS' === $token['content'] || ( false === $in_function_scope && false === $this->treat_files_as_scoped ) ) ) { @@ -146,16 +156,47 @@ public function process_token( $stackPtr ) { } } + /** + * Check that global variables declared via a list construct are prefixed. + * + * @internal No need to take special measures for nested lists. Nested or not, + * each list part can only contain one variable being written to. + * + * @since 2.2.0 + * + * @param int $stackPtr The position of the current token in the stack. + * + * @return int|void Integer stack pointer to skip forward or void to continue + * normal file processing. + */ + protected function process_list_assignment( $stackPtr ) { + $list_open_close = $this->find_list_open_close( $stackPtr ); + if ( false === $list_open_close ) { + // Short array, not short list. + return; + } + + $var_pointers = $this->get_list_variables( $stackPtr, $list_open_close ); + foreach ( $var_pointers as $ptr ) { + $this->process_variable_assignment( $ptr, true ); + } + + // No need to re-examine these variables. + return $list_open_close['closer']; + } + /** * Check that defined global variables are prefixed. * * @since 1.1.0 Logic was previously contained in the process_token() method. * - * @param int $stackPtr The position of the current token in the stack. + * @param int $stackPtr The position of the current token in the stack. + * @param bool $in_list Whether or not this is a variable in a list assignment. + * Defaults to false. * * @return void */ - protected function process_variable_assignment( $stackPtr ) { + protected function process_variable_assignment( $stackPtr, $in_list = false ) { if ( $this->has_whitelist_comment( 'override', $stackPtr ) === true ) { return; @@ -219,7 +260,8 @@ protected function process_variable_assignment( $stackPtr ) { /* * Check if the variable value is being changed. */ - if ( false === $this->is_assignment( $stackPtr ) + if ( false === $in_list + && false === $this->is_assignment( $stackPtr ) && false === $this->is_foreach_as( $stackPtr ) ) { return; @@ -229,7 +271,7 @@ protected function process_variable_assignment( $stackPtr ) { * Function parameters with the same name as a WP global variable are fine, * including when they are being assigned a default value. */ - if ( isset( $this->tokens[ $stackPtr ]['nested_parenthesis'] ) ) { + if ( false === $in_list && isset( $this->tokens[ $stackPtr ]['nested_parenthesis'] ) ) { foreach ( $this->tokens[ $stackPtr ]['nested_parenthesis'] as $opener => $closer ) { if ( isset( $this->tokens[ $opener ]['parenthesis_owner'] ) && ( \T_FUNCTION === $this->tokens[ $this->tokens[ $opener ]['parenthesis_owner'] ]['code'] @@ -244,7 +286,7 @@ protected function process_variable_assignment( $stackPtr ) { /* * Class property declarations with the same name as WP global variables are fine. */ - if ( true === $this->is_class_property( $stackPtr ) ) { + if ( false === $in_list && true === $this->is_class_property( $stackPtr ) ) { return; } @@ -325,6 +367,34 @@ protected function process_global_statement( $stackPtr, $in_function_scope ) { continue; } + // Make sure to recognize assignments to variables in a list construct. + if ( \T_LIST === $this->tokens[ $ptr ]['code'] + || \T_OPEN_SHORT_ARRAY === $this->tokens[ $ptr ]['code'] + ) { + $list_open_close = $this->find_list_open_close( $ptr ); + + if ( false === $list_open_close ) { + // Short array, not short list. + continue; + } + + $var_pointers = $this->get_list_variables( $ptr, $list_open_close ); + foreach ( $var_pointers as $ptr ) { + $var_name = $this->tokens[ $ptr ]['content']; + if ( '$GLOBALS' === $var_name ) { + $var_name = '$' . $this->strip_quotes( $this->get_array_access_key( $ptr ) ); + } + + if ( \in_array( $var_name, $search, true ) ) { + $this->process_variable_assignment( $ptr, true ); + } + } + + // No need to re-examine these variables. + $ptr = $list_open_close['closer']; + continue; + } + if ( \T_VARIABLE !== $this->tokens[ $ptr ]['code'] ) { continue; } diff --git a/WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.1.inc b/WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.1.inc index 3bf03276fe..9ff1b6100a 100644 --- a/WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.1.inc +++ b/WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.1.inc @@ -429,4 +429,46 @@ function acronym_content_width() { $GLOBALS['content_width'] = apply_filters( 'acronym_content_width', 640 ); } +/* + * Issue #1774: detect variables being set via the list() construct. + */ +// Empty list, not allowed since PHP 7.0, but not our concern. +list() = $array; // OK. +list(, ,) = $array; // OK. + +// Ordinary list. +list( $var1, , $var2 ) = $array; // Bad x 2. +list( $acronym_var1, $acronym_var2 ) = $array; // OK. + +// Short list. +[ $var1, $var2 ] = $array; // Bad x 2. +[ $acronym_var1, $acronym_var2 ] = $array; // OK. + +// Keyed list. Keys are not assignments. +list((string)$a => $store["B"], (string)$c => $store["D"]) = $e->getIndexable(); // Bad x 2. +[$foo => $GLOBALS['bar']] = $bar; // Bad x 1. + +// Nested list. +list( $var1, , list( $var2, $var3, ), $var4 ) = $array; // Bad x 4. + +// List with array assignments. +list( $foo['key'], $foo[ $bar ] ) = $array; // Bad x 2. Variable array key should be ignored. + +function acronym_lists_in_function_scope() { + global $store, $c; + + list( $var1, , $var2 ) = $array; // OK. + [ $var1, $var2 ] = $array; // OK. + + // Keyed list. Keys are not assignments. + list((string)$a => $store["B"], (string)$c => $store["D"]) = $e->getIndexable(); // Bad x 2. + [$foo => $GLOBALS['bar']] = $bar; // Bad x 1. + + // Nested list. + list( $var1, , list( $c, $var3, ), $var4 ) = $array; // Bad x 1 - $c. + + // List with array assignments. + list( $foo['key'], $foo[ $c ] ) = $array; // OK. Variable array key should be ignored. +} + // phpcs:set WordPress.NamingConventions.PrefixAllGlobals prefixes[] diff --git a/WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.php b/WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.php index aa169b940d..1e1cbdb72b 100644 --- a/WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.php +++ b/WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.php @@ -70,6 +70,15 @@ public function getErrorList( $testFile = 'PrefixAllGlobalsUnitTest.1.inc' ) { 403 => 1, 415 => 1, 423 => 1, + 440 => 2, + 444 => 2, + 448 => 2, + 449 => 1, + 452 => 4, + 455 => 2, + 464 => 2, + 465 => 1, + 468 => 1, ); case 'PrefixAllGlobalsUnitTest.4.inc': diff --git a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc index 115e3fde61..f544783056 100644 --- a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc +++ b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc @@ -223,3 +223,20 @@ function global_content_width() { } $content_width = 1000; + +// Issue #1743: detect var override via list construct. +function acronym_prepare_items() { + global $wp_query, $post_mime_types, $avail_post_mime_types, $mode; + list( $post_mime_types, $avail_post_mime_types ) = get_an_array(); // Bad x 2. + [ $post_mime_types, $avail_post_mime_types ] = get_an_array(); // PHP 7.1 short list syntax, bad x 2. + + // Keyed list. Keys are not assignments. + list( (string) $wp_query => $GLOBALS['mode']["B"], (string) $c => $mode["D"] ) = $e->getIndexable(); // Bad x 2. + [ $mode => $not_global ] = $bar; // OK. +} + +// Nested list. +list( $active_signup, list( $s => $typenow, $GLOBALS['status'], ), $ignore ) = $array; // Bad x 3. + +// List with array assignments. +[ $path[ $type ], , ] = $array; // Bad x 1. diff --git a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.php b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.php index b29c957436..bd611b2830 100644 --- a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.php +++ b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.php @@ -56,6 +56,11 @@ public function getErrorList( $testFile = '' ) { 181 => 1, 198 => 1, 212 => 4, + 230 => 2, + 231 => 2, + 234 => 2, + 239 => 3, + 242 => 1, ); case 'GlobalVariablesOverrideUnitTest.2.inc':