Skip to content

Commit

Permalink
GlobalVariablesOverride: detect global variable overrides in list ass…
Browse files Browse the repository at this point in the history 
…ignments

Global variables to which an assignment is made via the long/short `list()` construct should also be checked to make sure they don't override a WP global variable.

Includes unit tests.

Related to 1774
  • Loading branch information
@jrfnl
jrfnl committed Jul 28, 2019
1 parent bd16416 commit f47cf7b
Show file tree
Hide file tree
Showing 3 changed files with 99 additions and 7 deletions.
84 changes: 77 additions & 7 deletions WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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 ) )
) {
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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']
Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
}
Expand Down
17 changes: 17 additions & 0 deletions WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
5 changes: 5 additions & 0 deletions WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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':
Expand Down

0 comments on commit f47cf7b

Please sign in to comment.