From d0d422c333ce7aaa34d2294f380e57e9b059279c Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 19 Aug 2018 01:28:48 +0200 Subject: [PATCH 1/9] Move `is_foreach_as()` method from the PrefixAllGlobals to the Sniff base class --- WordPress/Sniff.php | 35 +++++++++++++++++++ .../PrefixAllGlobalsSniff.php | 32 ----------------- 2 files changed, 35 insertions(+), 32 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 51bd48776a..f630506a7d 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -2676,4 +2676,39 @@ public function is_use_of_global_constant( $stackPtr ) { return true; } + /** + * Determine if a variable is in the `as $key => $value` part of a foreach condition. + * + * @since 1.0.0 + * @since 1.1.0 Moved from the PrefixAllGlobals sniff to the Sniff base class. + * + * @param int $stackPtr Pointer to the variable. + * + * @return bool True if it is. False otherwise. + */ + protected function is_foreach_as( $stackPtr ) { + if ( ! isset( $this->tokens[ $stackPtr ]['nested_parenthesis'] ) ) { + return false; + } + + $nested_parenthesis = $this->tokens[ $stackPtr ]['nested_parenthesis']; + $close_parenthesis = end( $nested_parenthesis ); + $open_parenthesis = key( $nested_parenthesis ); + if ( ! isset( $this->tokens[ $close_parenthesis ]['parenthesis_owner'] ) ) { + return false; + } + + if ( \T_FOREACH !== $this->tokens[ $this->tokens[ $close_parenthesis ]['parenthesis_owner'] ]['code'] ) { + return false; + } + + $as_ptr = $this->phpcsFile->findNext( \T_AS, ( $open_parenthesis + 1 ), $close_parenthesis ); + if ( false === $as_ptr ) { + // Should never happen. + return false; + } + + return ( $stackPtr > $as_ptr ); + } + } diff --git a/WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php b/WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php index 41ed05b808..1b098d7549 100644 --- a/WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php +++ b/WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php @@ -795,38 +795,6 @@ private function variable_prefixed_or_whitelisted( $stackPtr, $name ) { return $this->is_prefixed( $stackPtr, $name ); } - /** - * Determine if a variable is in the `as $key => $value` part of a foreach condition. - * - * @param int $stackPtr Pointer to the variable. - * - * @return bool True if it is. False otherwise. - */ - private function is_foreach_as( $stackPtr ) { - if ( ! isset( $this->tokens[ $stackPtr ]['nested_parenthesis'] ) ) { - return false; - } - - $nested_parenthesis = $this->tokens[ $stackPtr ]['nested_parenthesis']; - $close_parenthesis = end( $nested_parenthesis ); - $open_parenthesis = key( $nested_parenthesis ); - if ( ! isset( $this->tokens[ $close_parenthesis ]['parenthesis_owner'] ) ) { - return false; - } - - if ( \T_FOREACH !== $this->tokens[ $this->tokens[ $close_parenthesis ]['parenthesis_owner'] ]['code'] ) { - return false; - } - - $as_ptr = $this->phpcsFile->findNext( \T_AS, ( $open_parenthesis + 1 ), $close_parenthesis ); - if ( false === $as_ptr ) { - // Should ever happen. - return false; - } - - return ( $stackPtr > $as_ptr ); - } - /** * Validate an array of prefixes as passed through a custom property or via the command line. * From 545486750cb8e3e7545aa88abb093147f0493268 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 19 Aug 2018 05:55:46 +0200 Subject: [PATCH 2/9] GlobalVariablesOverride: rename unit test case file ... and allow for multiple test case files in the test provider file. --- ... => GlobalVariablesOverrideUnitTest.1.inc} | 0 .../WP/GlobalVariablesOverrideUnitTest.php | 50 +++++++++++-------- 2 files changed, 29 insertions(+), 21 deletions(-) rename WordPress/Tests/WP/{GlobalVariablesOverrideUnitTest.inc => GlobalVariablesOverrideUnitTest.1.inc} (100%) diff --git a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.inc b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc similarity index 100% rename from WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.inc rename to WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc diff --git a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.php b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.php index c4021435a5..4ffcafa3d3 100644 --- a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.php +++ b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.php @@ -26,29 +26,37 @@ class GlobalVariablesOverrideUnitTest extends AbstractSniffUnitTest { /** * Returns the lines where errors should occur. * + * @param string $testFile The name of the file being tested. + * * @return array => */ - public function getErrorList() { - return array( - 3 => 1, - 6 => 1, - 16 => 1, - 17 => 1, - 18 => 1, - 25 => 1, - 35 => 1, - 36 => 1, - 54 => 1, - 95 => 1, - 128 => 1, - 133 => 1, - 139 => 1, - 140 => 1, - 141 => 2, - 142 => 1, - 143 => 1, - 146 => 1, - ); + public function getErrorList( $testFile = '' ) { + switch ( $testFile ) { + case 'GlobalVariablesOverrideUnitTest.1.inc': + return array( + 3 => 1, + 6 => 1, + 16 => 1, + 17 => 1, + 18 => 1, + 25 => 1, + 35 => 1, + 36 => 1, + 54 => 1, + 95 => 1, + 128 => 1, + 133 => 1, + 139 => 1, + 140 => 1, + 141 => 2, + 142 => 1, + 143 => 1, + 146 => 1, + ); + + default: + return array(); + } } /** From 63781a9a792b5847346d485d3092559c1de341c5 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 19 Aug 2018 06:11:29 +0200 Subject: [PATCH 3/9] GlobalVariablesOverride: improve ignoring overrides in test classes * Register class/trait tokens. * Bow out earlier if a class/trait token turns out to translate to a test class. * Skip over the rest of that class. --- .../WP/GlobalVariablesOverrideSniff.php | 40 ++++++++++++++++--- .../WP/GlobalVariablesOverrideUnitTest.1.inc | 9 +++++ 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php b/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php index 51abc9a208..b410a85c51 100644 --- a/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php +++ b/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php @@ -31,25 +31,52 @@ class GlobalVariablesOverrideSniff extends Sniff { /** * Returns an array of tokens this test wants to listen for. * + * @since 0.3.0 + * @since 1.1.0 Added class tokens for improved test classes skipping. + * * @return array */ public function register() { return array( \T_GLOBAL, \T_VARIABLE, + + // Only used to skip over test classes. + \T_CLASS, + \T_TRAIT, + \T_ANON_CLASS, ); } /** * Processes this test, when one of its tokens is encountered. * + * @since 0.3.0 + * * @param int $stackPtr The position of the current token in the stack. * - * @return void + * @return int|void Integer stack pointer to skip forward or void to continue + * normal file processing. */ public function process_token( $stackPtr ) { + $token = $this->tokens[ $stackPtr ]; + // Ignore variable overrides in test classes. + if ( \T_CLASS === $token['code'] || \T_TRAIT === $token['code'] || \T_ANON_CLASS === $token['code'] ) { + + if ( true === $this->is_test_class( $stackPtr ) + && $token['scope_condition'] === $stackPtr + && isset( $token['scope_closer'] ) + ) { + // Skip forward to end of test class. + return $token['scope_closer']; + } + + // Otherwise ignore the tokens as they were only registered to enable skipping over test classes. + return; + } + if ( \T_VARIABLE === $token['code'] && '$GLOBALS' === $token['content'] ) { $bracketPtr = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true ); @@ -177,15 +204,18 @@ public function process_token( $stackPtr ) { } /** - * Add the error if there is no whitelist comment present and the assignment - * is not done from within a test method. + * Add the error if there is no whitelist comment present. + * + * @since 0.11.0 + * @since 1.1.0 - Visibility changed from public to protected. + * - Check for being in a test class moved to the process_token() method. * * @param int $stackPtr The position of the token to throw the error for. * * @return void */ - public function maybe_add_error( $stackPtr ) { - if ( ! $this->is_token_in_test_method( $stackPtr ) && ! $this->has_whitelist_comment( 'override', $stackPtr ) ) { + protected function maybe_add_error( $stackPtr ) { + if ( $this->has_whitelist_comment( 'override', $stackPtr ) === false ) { $this->phpcsFile->addError( 'Overriding WordPress globals is prohibited. Found assignment to %s', $stackPtr, diff --git a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc index 47b275d24f..88a3c4b4b1 100644 --- a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc +++ b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc @@ -161,3 +161,12 @@ function acronymFunction() { break; } } + +// Anonymous test class. +$test = new class extends PHPUnit_Framework_TestCase { + + public function test_something() { + global $cat_id; + $cat_id = 50; // Ok. + } +}; From cf0070b4323c3c241e51d717c97ec5be03f7c08a Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 19 Aug 2018 06:33:07 +0200 Subject: [PATCH 4/9] GlobalVariablesOverride: split token specific code off from the process_token() method --- .../WP/GlobalVariablesOverrideSniff.php | 209 ++++++++++-------- 1 file changed, 119 insertions(+), 90 deletions(-) diff --git a/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php b/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php index b410a85c51..a38374420d 100644 --- a/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php +++ b/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php @@ -52,6 +52,7 @@ public function register() { * Processes this test, when one of its tokens is encountered. * * @since 0.3.0 + * @since 1.1.0 Split the token specific logic off into separate methods. * * @param int $stackPtr The position of the current token in the stack. * @@ -78,125 +79,153 @@ public function process_token( $stackPtr ) { } if ( \T_VARIABLE === $token['code'] && '$GLOBALS' === $token['content'] ) { - $bracketPtr = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true ); + return $this->process_variable_assignment( $stackPtr ); + } elseif ( \T_GLOBAL === $token['code'] ) { + return $this->process_global_statement( $stackPtr ); + } + } - if ( false === $bracketPtr || \T_OPEN_SQUARE_BRACKET !== $this->tokens[ $bracketPtr ]['code'] || ! isset( $this->tokens[ $bracketPtr ]['bracket_closer'] ) ) { - return; - } + /** + * 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. + * + * @return void + */ + protected function process_variable_assignment( $stackPtr ) { - // Bow out if the array key contains a variable. - $has_variable = $this->phpcsFile->findNext( \T_VARIABLE, ( $bracketPtr + 1 ), $this->tokens[ $bracketPtr ]['bracket_closer'] ); - if ( false !== $has_variable ) { - return; - } + $bracketPtr = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true ); - // Retrieve the array key and avoid getting tripped up by some simple obfuscation. - $var_name = ''; - $start = ( $bracketPtr + 1 ); - for ( $ptr = $start; $ptr < $this->tokens[ $bracketPtr ]['bracket_closer']; $ptr++ ) { - if ( \T_CONSTANT_ENCAPSED_STRING === $this->tokens[ $ptr ]['code'] ) { - $var_name .= $this->strip_quotes( $this->tokens[ $ptr ]['content'] ); - } + if ( false === $bracketPtr || \T_OPEN_SQUARE_BRACKET !== $this->tokens[ $bracketPtr ]['code'] || ! isset( $this->tokens[ $bracketPtr ]['bracket_closer'] ) ) { + return; + } + + // Bow out if the array key contains a variable. + $has_variable = $this->phpcsFile->findNext( \T_VARIABLE, ( $bracketPtr + 1 ), $this->tokens[ $bracketPtr ]['bracket_closer'] ); + if ( false !== $has_variable ) { + return; + } + + // Retrieve the array key and avoid getting tripped up by some simple obfuscation. + $var_name = ''; + $start = ( $bracketPtr + 1 ); + for ( $ptr = $start; $ptr < $this->tokens[ $bracketPtr ]['bracket_closer']; $ptr++ ) { + if ( \T_CONSTANT_ENCAPSED_STRING === $this->tokens[ $ptr ]['code'] ) { + $var_name .= $this->strip_quotes( $this->tokens[ $ptr ]['content'] ); } + } - if ( ! isset( $this->wp_globals[ $var_name ] ) ) { - return; + if ( ! isset( $this->wp_globals[ $var_name ] ) ) { + return; + } + + if ( true === $this->is_assignment( $this->tokens[ $bracketPtr ]['bracket_closer'] ) ) { + $this->maybe_add_error( $stackPtr ); + } + } + + /** + * Check that global variables imported into a function scope using a global statement + * are not being overruled. + * + * @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. + * + * @return void + */ + protected function process_global_statement( $stackPtr ) { + $search = array(); // Array of globals to watch for. + $ptr = ( $stackPtr + 1 ); + while ( $ptr ) { + if ( ! isset( $this->tokens[ $ptr ] ) ) { + break; } - if ( true === $this->is_assignment( $this->tokens[ $bracketPtr ]['bracket_closer'] ) ) { - $this->maybe_add_error( $stackPtr ); + $var = $this->tokens[ $ptr ]; + + // Halt the loop at end of statement. + if ( \T_SEMICOLON === $var['code'] ) { + break; } - } elseif ( \T_GLOBAL === $token['code'] ) { - $search = array(); // Array of globals to watch for. - $ptr = ( $stackPtr + 1 ); - while ( $ptr ) { - if ( ! isset( $this->tokens[ $ptr ] ) ) { - break; + + if ( \T_VARIABLE === $var['code'] ) { + if ( isset( $this->wp_globals[ substr( $var['content'], 1 ) ] ) ) { + $search[] = $var['content']; } + } - $var = $this->tokens[ $ptr ]; + $ptr++; + } + unset( $var ); - // Halt the loop at end of statement. - if ( \T_SEMICOLON === $var['code'] ) { - break; - } + if ( empty( $search ) ) { + return; + } - if ( \T_VARIABLE === $var['code'] ) { - if ( isset( $this->wp_globals[ substr( $var['content'], 1 ) ] ) ) { - $search[] = $var['content']; - } - } + // Only search from the end of the "global ...;" statement onwards. + $start = ( $this->phpcsFile->findEndOfStatement( $stackPtr ) + 1 ); + $end = $this->phpcsFile->numTokens; + $global_scope = true; - $ptr++; - } - unset( $var ); + // Is the global statement within a function call or closure ? + // If so, limit the token walking to the function scope. + $function_token = $this->phpcsFile->getCondition( $stackPtr, \T_FUNCTION ); + if ( false === $function_token ) { + $function_token = $this->phpcsFile->getCondition( $stackPtr, \T_CLOSURE ); + } - if ( empty( $search ) ) { + if ( false !== $function_token ) { + if ( ! isset( $this->tokens[ $function_token ]['scope_closer'] ) ) { + // Live coding, unfinished function. return; } - // Only search from the end of the "global ...;" statement onwards. - $start = ( $this->phpcsFile->findEndOfStatement( $stackPtr ) + 1 ); - $end = $this->phpcsFile->numTokens; - $global_scope = true; + $end = $this->tokens[ $function_token ]['scope_closer']; + $global_scope = false; + } - // Is the global statement within a function call or closure ? - // If so, limit the token walking to the function scope. - $function_token = $this->phpcsFile->getCondition( $stackPtr, \T_FUNCTION ); - if ( false === $function_token ) { - $function_token = $this->phpcsFile->getCondition( $stackPtr, \T_CLOSURE ); - } + // Check for assignments to collected global vars. + for ( $ptr = $start; $ptr < $end; $ptr++ ) { - if ( false !== $function_token ) { - if ( ! isset( $this->tokens[ $function_token ]['scope_closer'] ) ) { - // Live coding, unfinished function. + // If the global statement was in the global scope, skip over functions, classes and the likes. + if ( true === $global_scope && \in_array( $this->tokens[ $ptr ]['code'], array( \T_FUNCTION, \T_CLOSURE, \T_CLASS, \T_ANON_CLASS, \T_INTERFACE, \T_TRAIT ), true ) ) { + if ( ! isset( $this->tokens[ $ptr ]['scope_closer'] ) ) { + // Live coding, skip the rest of the file. return; } - $end = $this->tokens[ $function_token ]['scope_closer']; - $global_scope = false; + $ptr = $this->tokens[ $ptr ]['scope_closer']; + continue; } - // Check for assignments to collected global vars. - for ( $ptr = $start; $ptr < $end; $ptr++ ) { - - // If the global statement was in the global scope, skip over functions, classes and the likes. - if ( true === $global_scope && \in_array( $this->tokens[ $ptr ]['code'], array( \T_FUNCTION, \T_CLOSURE, \T_CLASS, \T_ANON_CLASS, \T_INTERFACE, \T_TRAIT ), true ) ) { - if ( ! isset( $this->tokens[ $ptr ]['scope_closer'] ) ) { - // Live coding, skip the rest of the file. - return; - } - - $ptr = $this->tokens[ $ptr ]['scope_closer']; + if ( \T_VARIABLE === $this->tokens[ $ptr ]['code'] + && \in_array( $this->tokens[ $ptr ]['content'], $search, true ) + ) { + // Don't throw false positives for static class properties. + $previous = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $ptr - 1 ), null, true, null, true ); + if ( false !== $previous && \T_DOUBLE_COLON === $this->tokens[ $previous ]['code'] ) { continue; } - if ( \T_VARIABLE === $this->tokens[ $ptr ]['code'] - && \in_array( $this->tokens[ $ptr ]['content'], $search, true ) - ) { - // Don't throw false positives for static class properties. - $previous = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $ptr - 1 ), null, true, null, true ); - if ( false !== $previous && \T_DOUBLE_COLON === $this->tokens[ $previous ]['code'] ) { - continue; - } + if ( true === $this->is_assignment( $ptr ) ) { + $this->maybe_add_error( $ptr ); + continue; + } - if ( true === $this->is_assignment( $ptr ) ) { + // Check if this is a variable assignment within a `foreach()` declaration. + if ( isset( $this->tokens[ $ptr ]['nested_parenthesis'] ) ) { + $nested_parenthesis = $this->tokens[ $ptr ]['nested_parenthesis']; + $close_parenthesis = end( $nested_parenthesis ); + if ( isset( $this->tokens[ $close_parenthesis ]['parenthesis_owner'] ) + && \T_FOREACH === $this->tokens[ $this->tokens[ $close_parenthesis ]['parenthesis_owner'] ]['code'] + && ( false !== $previous + && ( \T_DOUBLE_ARROW === $this->tokens[ $previous ]['code'] + || \T_AS === $this->tokens[ $previous ]['code'] ) ) + ) { $this->maybe_add_error( $ptr ); - continue; - } - - // Check if this is a variable assignment within a `foreach()` declaration. - if ( isset( $this->tokens[ $ptr ]['nested_parenthesis'] ) ) { - $nested_parenthesis = $this->tokens[ $ptr ]['nested_parenthesis']; - $close_parenthesis = end( $nested_parenthesis ); - if ( isset( $this->tokens[ $close_parenthesis ]['parenthesis_owner'] ) - && \T_FOREACH === $this->tokens[ $this->tokens[ $close_parenthesis ]['parenthesis_owner'] ]['code'] - && ( false !== $previous - && ( \T_DOUBLE_ARROW === $this->tokens[ $previous ]['code'] - || \T_AS === $this->tokens[ $previous ]['code'] ) ) - ) { - $this->maybe_add_error( $ptr ); - } } } } From 2a3113332490495c059000672c6a3bb5a947018d Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 19 Aug 2018 06:37:35 +0200 Subject: [PATCH 5/9] GlobalVariablesOverride: bow out earlier if a whitelist comment is found If the token being examined is a `T_VARIABLE` token and has a whitelist comment, the sniff can bow out before examining the token in detail. Includes splitting the `maybe_add_error()` method up to prevent checking twice for a whitelist comment. For the `T_GLOBAL` token this is not possible as the whitelist comment will not be on the line with the `$stackPtr` pointing to the `global` keyword. --- .../WP/GlobalVariablesOverrideSniff.php | 31 ++++++++++++++----- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php b/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php index a38374420d..085af1d7bc 100644 --- a/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php +++ b/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php @@ -96,6 +96,10 @@ public function process_token( $stackPtr ) { */ protected function process_variable_assignment( $stackPtr ) { + if ( $this->has_whitelist_comment( 'override', $stackPtr ) === true ) { + return; + } + $bracketPtr = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true ); if ( false === $bracketPtr || \T_OPEN_SQUARE_BRACKET !== $this->tokens[ $bracketPtr ]['code'] || ! isset( $this->tokens[ $bracketPtr ]['bracket_closer'] ) ) { @@ -122,7 +126,7 @@ protected function process_variable_assignment( $stackPtr ) { } if ( true === $this->is_assignment( $this->tokens[ $bracketPtr ]['bracket_closer'] ) ) { - $this->maybe_add_error( $stackPtr ); + $this->add_error( $stackPtr ); } } @@ -245,13 +249,26 @@ protected function process_global_statement( $stackPtr ) { */ protected function maybe_add_error( $stackPtr ) { if ( $this->has_whitelist_comment( 'override', $stackPtr ) === false ) { - $this->phpcsFile->addError( - 'Overriding WordPress globals is prohibited. Found assignment to %s', - $stackPtr, - 'OverrideProhibited', - array( $this->tokens[ $stackPtr ]['content'] ) - ); + $this->add_error( $stackPtr ); } } + /** + * Add the error. + * + * @since 1.1.0 + * + * @param int $stackPtr The position of the token to throw the error for. + * + * @return void + */ + protected function add_error( $stackPtr ) { + $this->phpcsFile->addError( + 'Overriding WordPress globals is prohibited. Found assignment to %s', + $stackPtr, + 'OverrideProhibited', + array( $this->tokens[ $stackPtr ]['content'] ) + ); + } + } From 6a8f064518d742b8dab7fab632d871b37cff3fcc Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 19 Aug 2018 07:02:45 +0200 Subject: [PATCH 6/9] GlobalVariablesOverride: only examine global statements in a function scope * Minor simplification of how the `$search` array is gathered. * Search only within the function scope. * Skip over nested functions/closures/classes. * Minor code changes to lower the nesting levels. --- .../WP/GlobalVariablesOverrideSniff.php | 135 ++++++++++-------- .../WP/GlobalVariablesOverrideUnitTest.1.inc | 27 ++++ .../WP/GlobalVariablesOverrideUnitTest.php | 2 + 3 files changed, 108 insertions(+), 56 deletions(-) diff --git a/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php b/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php index 085af1d7bc..2a07b6ca47 100644 --- a/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php +++ b/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php @@ -28,6 +28,27 @@ */ class GlobalVariablesOverrideSniff extends Sniff { + /** + * Scoped object and function structures to skip over as + * variables will have a different scope within those. + * + * {@internal Once the minimum PHPCS version goes up to PHPCS 3.1.0, + * this array can be partially created in the register() method + * using the upstream `Tokens::$ooScopeTokens` array.}} + * + * @since 1.1.0 + * + * @var array + */ + private $skip_over = array( + \T_FUNCTION => true, + \T_CLOSURE => true, + \T_CLASS => true, + \T_ANON_CLASS => true, + \T_INTERFACE => true, + \T_TRAIT => true, + ); + /** * Returns an array of tokens this test wants to listen for. * @@ -78,9 +99,17 @@ public function process_token( $stackPtr ) { return; } + /* + * Examine variables within a function scope based on a `global` statement in the + * function. + */ + $in_function_scope = $this->phpcsFile->hasCondition( $stackPtr, array( \T_FUNCTION, \T_CLOSURE ) ); + if ( \T_VARIABLE === $token['code'] && '$GLOBALS' === $token['content'] ) { return $this->process_variable_assignment( $stackPtr ); - } elseif ( \T_GLOBAL === $token['code'] ) { + } elseif ( \T_GLOBAL === $token['code'] + && true === $in_function_scope + ) { return $this->process_global_statement( $stackPtr ); } } @@ -141,13 +170,12 @@ protected function process_variable_assignment( $stackPtr ) { * @return void */ protected function process_global_statement( $stackPtr ) { - $search = array(); // Array of globals to watch for. + /* + * Collect the variables to watch for. + */ + $search = array(); $ptr = ( $stackPtr + 1 ); - while ( $ptr ) { - if ( ! isset( $this->tokens[ $ptr ] ) ) { - break; - } - + while ( isset( $this->tokens[ $ptr ] ) ) { $var = $this->tokens[ $ptr ]; // Halt the loop at end of statement. @@ -169,68 +197,63 @@ protected function process_global_statement( $stackPtr ) { return; } - // Only search from the end of the "global ...;" statement onwards. - $start = ( $this->phpcsFile->findEndOfStatement( $stackPtr ) + 1 ); - $end = $this->phpcsFile->numTokens; - $global_scope = true; - - // Is the global statement within a function call or closure ? - // If so, limit the token walking to the function scope. - $function_token = $this->phpcsFile->getCondition( $stackPtr, \T_FUNCTION ); - if ( false === $function_token ) { - $function_token = $this->phpcsFile->getCondition( $stackPtr, \T_CLOSURE ); - } - - if ( false !== $function_token ) { - if ( ! isset( $this->tokens[ $function_token ]['scope_closer'] ) ) { - // Live coding, unfinished function. - return; - } - - $end = $this->tokens[ $function_token ]['scope_closer']; - $global_scope = false; + /* + * Search for assignments to the imported global variables within the function scope. + */ + $function_cond = $this->phpcsFile->getCondition( $stackPtr, \T_FUNCTION ); + $closure_cond = $this->phpcsFile->getCondition( $stackPtr, \T_CLOSURE ); + $scope_cond = max( $function_cond, $closure_cond ); // If false, it will evaluate as zero, so this is fine. + if ( isset( $this->tokens[ $scope_cond ]['scope_closer'] ) === false ) { + // Live coding or parse error. + return; } - // Check for assignments to collected global vars. + $start = $ptr; + $end = $this->tokens[ $scope_cond ]['scope_closer']; for ( $ptr = $start; $ptr < $end; $ptr++ ) { - // If the global statement was in the global scope, skip over functions, classes and the likes. - if ( true === $global_scope && \in_array( $this->tokens[ $ptr ]['code'], array( \T_FUNCTION, \T_CLOSURE, \T_CLASS, \T_ANON_CLASS, \T_INTERFACE, \T_TRAIT ), true ) ) { + // Skip over nested functions, classes and the likes. + if ( isset( $this->skip_over[ $this->tokens[ $ptr ]['code'] ] ) ) { if ( ! isset( $this->tokens[ $ptr ]['scope_closer'] ) ) { - // Live coding, skip the rest of the file. - return; + // Live coding or parse error. + break; } $ptr = $this->tokens[ $ptr ]['scope_closer']; continue; } - if ( \T_VARIABLE === $this->tokens[ $ptr ]['code'] - && \in_array( $this->tokens[ $ptr ]['content'], $search, true ) - ) { - // Don't throw false positives for static class properties. - $previous = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $ptr - 1 ), null, true, null, true ); - if ( false !== $previous && \T_DOUBLE_COLON === $this->tokens[ $previous ]['code'] ) { - continue; - } + if ( \T_VARIABLE !== $this->tokens[ $ptr ]['code'] ) { + continue; + } - if ( true === $this->is_assignment( $ptr ) ) { - $this->maybe_add_error( $ptr ); - continue; - } + if ( \in_array( $this->tokens[ $ptr ]['content'], $search, true ) === false ) { + // Not one of the variables we're interested in. + continue; + } + + // Don't throw false positives for static class properties. + $previous = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $ptr - 1 ), null, true, null, true ); + if ( false !== $previous && \T_DOUBLE_COLON === $this->tokens[ $previous ]['code'] ) { + continue; + } - // Check if this is a variable assignment within a `foreach()` declaration. - if ( isset( $this->tokens[ $ptr ]['nested_parenthesis'] ) ) { - $nested_parenthesis = $this->tokens[ $ptr ]['nested_parenthesis']; - $close_parenthesis = end( $nested_parenthesis ); - if ( isset( $this->tokens[ $close_parenthesis ]['parenthesis_owner'] ) - && \T_FOREACH === $this->tokens[ $this->tokens[ $close_parenthesis ]['parenthesis_owner'] ]['code'] - && ( false !== $previous - && ( \T_DOUBLE_ARROW === $this->tokens[ $previous ]['code'] - || \T_AS === $this->tokens[ $previous ]['code'] ) ) - ) { - $this->maybe_add_error( $ptr ); - } + if ( true === $this->is_assignment( $ptr ) ) { + $this->maybe_add_error( $ptr ); + continue; + } + + // Check if this is a variable assignment within a `foreach()` declaration. + if ( isset( $this->tokens[ $ptr ]['nested_parenthesis'] ) ) { + $nested_parenthesis = $this->tokens[ $ptr ]['nested_parenthesis']; + $close_parenthesis = end( $nested_parenthesis ); + if ( isset( $this->tokens[ $close_parenthesis ]['parenthesis_owner'] ) + && \T_FOREACH === $this->tokens[ $this->tokens[ $close_parenthesis ]['parenthesis_owner'] ]['code'] + && ( false !== $previous + && ( \T_DOUBLE_ARROW === $this->tokens[ $previous ]['code'] + || \T_AS === $this->tokens[ $previous ]['code'] ) ) + ) { + $this->maybe_add_error( $ptr ); } } } diff --git a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc index 88a3c4b4b1..cacc4dd730 100644 --- a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc +++ b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc @@ -170,3 +170,30 @@ $test = new class extends PHPUnit_Framework_TestCase { $cat_id = 50; // Ok. } }; + +// Verify skipping over nested scoped structures. +function global_vars() { + global $pagenow; + $closure = function ( $pagenow ) { // OK, local to the closure. + $pagenow = 'something'; // OK, local to the closure. + }; + + $pagenow = 'something else'; // Bad. + + return $closure( $pagenow ); // OK, not an assignment. +} + +// Verify skipping over rest of the function when live coding/parse error in nested scope structure. +function global_vars() { + global $pagenow; + + $closure = function ( $pagenow ) { + global $feeds; + + $nested_closure_with_parse_error = function ( $feeds ) + + $feeds = 'something'; // Bad, but ignored because of the parse error in the closure. + }; + + $pagenow = 'something'; // Bad, should be picked up. Tests that skipping on parse error doesn't skip too far. +} diff --git a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.php b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.php index 4ffcafa3d3..72abdd285a 100644 --- a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.php +++ b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.php @@ -52,6 +52,8 @@ public function getErrorList( $testFile = '' ) { 142 => 1, 143 => 1, 146 => 1, + 181 => 1, + 198 => 1, ); default: From 44382741d01a62f41e2b4ac6945d91f4ee8e9b69 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 19 Aug 2018 07:10:46 +0200 Subject: [PATCH 7/9] GlobalVariablesOverride: refactor to examine variables in the global scope Examine variables found in the global scope and always examine the `$GLOBALS` variable independently of scope. * Minor efficiency fix when checking the array key of a `$GLOBALS` variable - no need to walk the token array twice. * Error prevention for when the `$GLOBALS` array key would not contain a `T_CONSTANT_ENCAPSED_STRING` token. * Don't throw errors for default values for function parameters in function declarations. * Don't throw errors for class properties. --- .../WP/GlobalVariablesOverrideSniff.php | 88 +++++++++++++++---- .../WP/GlobalVariablesOverrideUnitTest.1.inc | 19 +++- .../WP/GlobalVariablesOverrideUnitTest.2.inc | 29 ++++++ .../WP/GlobalVariablesOverrideUnitTest.php | 17 ++++ 4 files changed, 134 insertions(+), 19 deletions(-) create mode 100644 WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.2.inc diff --git a/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php b/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php index 2a07b6ca47..4b5fa44452 100644 --- a/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php +++ b/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php @@ -23,6 +23,7 @@ * @since 0.13.0 Class name changed: this class is now namespaced. * @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. * * @uses \WordPress\Sniff::$custom_test_class_whitelist */ @@ -102,10 +103,15 @@ 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` + * variable based on the variable token. */ $in_function_scope = $this->phpcsFile->hasCondition( $stackPtr, array( \T_FUNCTION, \T_CLOSURE ) ); - if ( \T_VARIABLE === $token['code'] && '$GLOBALS' === $token['content'] ) { + if ( \T_VARIABLE === $token['code'] + && ( '$GLOBALS' === $token['content'] + || false === $in_function_scope ) + ) { return $this->process_variable_assignment( $stackPtr ); } elseif ( \T_GLOBAL === $token['code'] && true === $in_function_scope @@ -129,34 +135,84 @@ protected function process_variable_assignment( $stackPtr ) { return; } - $bracketPtr = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true ); + $token = $this->tokens[ $stackPtr ]; + $var_name = substr( $token['content'], 1 ); // Strip the dollar sign. + + // Determine the variable name for `$GLOBALS['array_key']`. + if ( 'GLOBALS' === $var_name ) { + $bracketPtr = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true ); + + if ( false === $bracketPtr || \T_OPEN_SQUARE_BRACKET !== $this->tokens[ $bracketPtr ]['code'] || ! isset( $this->tokens[ $bracketPtr ]['bracket_closer'] ) ) { + return; + } + + // Retrieve the array key and avoid getting tripped up by some simple obfuscation. + $var_name = ''; + $start = ( $bracketPtr + 1 ); + for ( $ptr = $start; $ptr < $this->tokens[ $bracketPtr ]['bracket_closer']; $ptr++ ) { + /* + * If the globals array key contains a variable, constant, function call + * or interpolated variable, bow out. + */ + if ( \T_VARIABLE === $this->tokens[ $ptr ]['code'] + || \T_STRING === $this->tokens[ $ptr ]['code'] + || \T_DOUBLE_QUOTED_STRING === $this->tokens[ $ptr ]['code'] + ) { + return; + } + + if ( \T_CONSTANT_ENCAPSED_STRING === $this->tokens[ $ptr ]['code'] ) { + $var_name .= $this->strip_quotes( $this->tokens[ $ptr ]['content'] ); + } + } + + if ( '' === $var_name ) { + // Shouldn't happen, but just in case. + return; + } + } - if ( false === $bracketPtr || \T_OPEN_SQUARE_BRACKET !== $this->tokens[ $bracketPtr ]['code'] || ! isset( $this->tokens[ $bracketPtr ]['bracket_closer'] ) ) { + /* + * Is this one of the WP global variables ? + */ + if ( isset( $this->wp_globals[ $var_name ] ) === false ) { return; } - // Bow out if the array key contains a variable. - $has_variable = $this->phpcsFile->findNext( \T_VARIABLE, ( $bracketPtr + 1 ), $this->tokens[ $bracketPtr ]['bracket_closer'] ); - if ( false !== $has_variable ) { + /* + * Check if the variable value is being changed. + */ + if ( false === $this->is_assignment( $stackPtr ) + && false === $this->is_foreach_as( $stackPtr ) + ) { return; } - // Retrieve the array key and avoid getting tripped up by some simple obfuscation. - $var_name = ''; - $start = ( $bracketPtr + 1 ); - for ( $ptr = $start; $ptr < $this->tokens[ $bracketPtr ]['bracket_closer']; $ptr++ ) { - if ( \T_CONSTANT_ENCAPSED_STRING === $this->tokens[ $ptr ]['code'] ) { - $var_name .= $this->strip_quotes( $this->tokens[ $ptr ]['content'] ); + /* + * 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'] ) ) { + 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'] + || \T_CLOSURE === $this->tokens[ $this->tokens[ $opener ]['parenthesis_owner'] ]['code'] ) + ) { + return; + } } + unset( $opener, $closer ); } - if ( ! isset( $this->wp_globals[ $var_name ] ) ) { + /* + * Class property declarations with the same name as WP global variables are fine. + */ + if ( true === $this->is_class_property( $stackPtr ) ) { return; } - if ( true === $this->is_assignment( $this->tokens[ $bracketPtr ]['bracket_closer'] ) ) { - $this->add_error( $stackPtr ); - } + // Still here ? In that case, the WP global variable is being tampered with. + $this->add_error( $stackPtr ); } /** diff --git a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc index cacc4dd730..1bb2ff7903 100644 --- a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc +++ b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc @@ -5,7 +5,7 @@ $GLOBALS['wpdb'] = 'test'; // Bad. global $wpdb; $wpdb = 'test'; // Bad. -$post = get_post( 1 ); // Ok, $post has not been made global yet. +$post = get_post( 1 ); // Bad. global $post; $post = get_post( 1 ); // Override ok. @@ -55,7 +55,7 @@ add_filter( 'comments_open', function( $open, $post_id ) { return 'page' === $page->post_type; }, 10, 2 ); -$page = 'test'; // Ok, check against cross-contaminiation from within a closure. +$closure = function() { $page = 'test' }; // Ok, check against cross-contaminiation from within a closure. // Allow overriding globals in functions within unit test classes. // https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/300#issuecomment-158778606 @@ -98,7 +98,7 @@ class Test_Class_C extends NonTestClass { // Ok: overriding class property with same name as global variable. trait My_Class { - private static $page; + public static $page = 'something'; public function do_something() { global $page; @@ -197,3 +197,16 @@ function global_vars() { $pagenow = 'something'; // Bad, should be picked up. Tests that skipping on parse error doesn't skip too far. } + +$GLOBALS[] = 'something'; +$GLOBALS[103] = 'something'; + +class MyClass { + // All ok, class properties, not the global variables. + public $is_apache = true; + protected $l10n = array(); + private $phpmailer = null; +} + +// Test assigning to multiple variables at once. +$is_NS4 = $is_opera = $is_safari = $GLOBALS['is_winIE'] = true; // Bad x 4. diff --git a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.2.inc b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.2.inc new file mode 100644 index 0000000000..3584e07769 --- /dev/null +++ b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.2.inc @@ -0,0 +1,29 @@ + $order ) {} // Bad x 2. +while ( ( $posts = function_call() ) === true ) {} // Bad. +for ( $totals = 0; $totals < 10; $totals++ ) {} // Bad. + +switch( true ) { + case ($year = 'abc'): // Bad. + break; +} + +$domain['subkey'] = 'something else'; // Bad. + +$GLOBALS['domain']['subkey'] = 'something else'; // Bad. diff --git a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.php b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.php index 72abdd285a..809f43d034 100644 --- a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.php +++ b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.php @@ -36,6 +36,7 @@ public function getErrorList( $testFile = '' ) { return array( 3 => 1, 6 => 1, + 8 => 1, 16 => 1, 17 => 1, 18 => 1, @@ -44,6 +45,7 @@ public function getErrorList( $testFile = '' ) { 36 => 1, 54 => 1, 95 => 1, + 124 => 1, 128 => 1, 133 => 1, 139 => 1, @@ -54,6 +56,21 @@ public function getErrorList( $testFile = '' ) { 146 => 1, 181 => 1, 198 => 1, + 212 => 4, + ); + + case 'GlobalVariablesOverrideUnitTest.2.inc': + return array( + 12 => 1, + 13 => 1, + 16 => 1, + 17 => 1, + 18 => 2, + 19 => 1, + 20 => 1, + 23 => 1, + 27 => 1, + 29 => 1, ); default: From 7c5443b9a15cb6bdc59acac17b82dc0045d775e1 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 19 Aug 2018 07:24:40 +0200 Subject: [PATCH 8/9] GlobalVariablesOverride: add `treat_files_as_scoped` property By setting the `treat_files_as_scoped` property to `true`, the sniff will regard each file as being included from with a function, making it scoped to the function holding the `include` statement. This basically allows for the sniff to revert back to the original behaviour of only examining variables in the global scope if a `global` statement had been found or if the variable used was `$GLOBALS`. --- .../WP/GlobalVariablesOverrideSniff.php | 50 +++++++++++++------ .../WP/GlobalVariablesOverrideUnitTest.3.inc | 31 ++++++++++++ .../WP/GlobalVariablesOverrideUnitTest.php | 5 ++ 3 files changed, 72 insertions(+), 14 deletions(-) create mode 100644 WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.3.inc diff --git a/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php b/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php index 4b5fa44452..3280d6f3c9 100644 --- a/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php +++ b/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php @@ -29,6 +29,22 @@ */ class GlobalVariablesOverrideSniff extends Sniff { + /** + * Whether to treat all files as if they were included from + * a within function. + * + * This is mostly useful for projects containing views which are being + * included from within a function in another file, like themes. + * + * Note: enabling this is discouraged as there is no guarantee that + * the file will *never* be included from the global scope. + * + * @since 1.1.0 + * + * @var bool + */ + public $treat_files_as_scoped = false; + /** * Scoped object and function structures to skip over as * variables will have a different scope within those. @@ -110,13 +126,13 @@ public function process_token( $stackPtr ) { if ( \T_VARIABLE === $token['code'] && ( '$GLOBALS' === $token['content'] - || false === $in_function_scope ) + || ( false === $in_function_scope && false === $this->treat_files_as_scoped ) ) ) { return $this->process_variable_assignment( $stackPtr ); } elseif ( \T_GLOBAL === $token['code'] - && true === $in_function_scope + && ( true === $in_function_scope || true === $this->treat_files_as_scoped ) ) { - return $this->process_global_statement( $stackPtr ); + return $this->process_global_statement( $stackPtr, $in_function_scope ); } } @@ -221,11 +237,12 @@ protected function process_variable_assignment( $stackPtr ) { * * @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_function_scope Whether the global statement is within a scoped function/closure. * * @return void */ - protected function process_global_statement( $stackPtr ) { + protected function process_global_statement( $stackPtr, $in_function_scope ) { /* * Collect the variables to watch for. */ @@ -254,18 +271,23 @@ protected function process_global_statement( $stackPtr ) { } /* - * Search for assignments to the imported global variables within the function scope. + * Search for assignments to the imported global variables within the relevant scope. */ - $function_cond = $this->phpcsFile->getCondition( $stackPtr, \T_FUNCTION ); - $closure_cond = $this->phpcsFile->getCondition( $stackPtr, \T_CLOSURE ); - $scope_cond = max( $function_cond, $closure_cond ); // If false, it will evaluate as zero, so this is fine. - if ( isset( $this->tokens[ $scope_cond ]['scope_closer'] ) === false ) { - // Live coding or parse error. - return; + $start = $ptr; + if ( true === $in_function_scope ) { + $function_cond = $this->phpcsFile->getCondition( $stackPtr, \T_FUNCTION ); + $closure_cond = $this->phpcsFile->getCondition( $stackPtr, \T_CLOSURE ); + $scope_cond = max( $function_cond, $closure_cond ); // If false, it will evaluate as zero, so this is fine. + if ( isset( $this->tokens[ $scope_cond ]['scope_closer'] ) === false ) { + // Live coding or parse error. + return; + } + $end = $this->tokens[ $scope_cond ]['scope_closer']; + } else { + // Global statement in the global namespace with file is being treated as scoped. + $end = ( $this->phpcsFile->numTokens + 1 ); } - $start = $ptr; - $end = $this->tokens[ $scope_cond ]['scope_closer']; for ( $ptr = $start; $ptr < $end; $ptr++ ) { // Skip over nested functions, classes and the likes. diff --git a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.3.inc b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.3.inc new file mode 100644 index 0000000000..9dd7f6c876 --- /dev/null +++ b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.3.inc @@ -0,0 +1,31 @@ + $order ) {} // Bad x 2. +while ( ( $posts = function_call() ) === true ) {} // OK. +for ( $totals = 0; $totals < 10; $totals++ ) {} // OK. + +switch( true ) { + case ($year = 'abc'): // OK. + break; +} + +$domain['subkey'] = 'something else'; // OK. + +$GLOBALS['domain']['subkey'] = 'something else'; // Still bad. + +// @codingStandardsChangeSetting WordPress.WP.GlobalVariablesOverride treat_files_as_scoped false diff --git a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.php b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.php index 809f43d034..81b855496f 100644 --- a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.php +++ b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.php @@ -73,6 +73,11 @@ public function getErrorList( $testFile = '' ) { 29 => 1, ); + case 'GlobalVariablesOverrideUnitTest.3.inc': + return array( + 29 => 1, + ); + default: return array(); } From d6e478239afacca2ddc06c5827d4633051c48f17 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 19 Aug 2018 07:25:50 +0200 Subject: [PATCH 9/9] GlobalVariablesOverride: improve the error message when an assignment to $GLOBALS is found Previously, the error message would just show the rather non-descript `$GLOBALS`, now it will show `$GLOBALS['array_key']`. --- .../WP/GlobalVariablesOverrideSniff.php | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php b/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php index 3280d6f3c9..1d628bbab3 100644 --- a/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php +++ b/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php @@ -153,6 +153,7 @@ protected function process_variable_assignment( $stackPtr ) { $token = $this->tokens[ $stackPtr ]; $var_name = substr( $token['content'], 1 ); // Strip the dollar sign. + $data = array(); // Determine the variable name for `$GLOBALS['array_key']`. if ( 'GLOBALS' === $var_name ) { @@ -186,6 +187,9 @@ protected function process_variable_assignment( $stackPtr ) { // Shouldn't happen, but just in case. return; } + + // Set up the data for the error message. + $data[] = '$GLOBALS[\'' . $var_name . '\']'; } /* @@ -228,7 +232,7 @@ protected function process_variable_assignment( $stackPtr ) { } // Still here ? In that case, the WP global variable is being tampered with. - $this->add_error( $stackPtr ); + $this->add_error( $stackPtr, $data ); } /** @@ -359,16 +363,23 @@ protected function maybe_add_error( $stackPtr ) { * * @since 1.1.0 * - * @param int $stackPtr The position of the token to throw the error for. + * @param int $stackPtr The position of the token to throw the error for. + * @param array $data Optional. Array containing one entry holding the + * name of the variable being overruled. + * Defaults to the 'content' of the $stackPtr token. * * @return void */ - protected function add_error( $stackPtr ) { + protected function add_error( $stackPtr, $data = array() ) { + if ( empty( $data ) ) { + $data[] = $this->tokens[ $stackPtr ]['content']; + } + $this->phpcsFile->addError( 'Overriding WordPress globals is prohibited. Found assignment to %s', $stackPtr, 'OverrideProhibited', - array( $this->tokens[ $stackPtr ]['content'] ) + $data ); }