Skip to content

Commit

Permalink
PrefixAllGlobals: detect non-prefixed variables in list assignments
Browse files Browse the repository at this point in the history 
Global variables to which an assignment is made via the long/short `list()` construct should also be prefixed.

Includes unit tests.

Related to 1774
  • Loading branch information
@jrfnl
jrfnl committed Jul 28, 2019
1 parent c2bae67 commit bd16416
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 10 deletions.
61 changes: 51 additions & 10 deletions WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -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.

Expand Down Expand Up @@ -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 );

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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']
Expand All @@ -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;
}

Expand Down Expand Up @@ -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.
*
Expand Down
42 changes: 42 additions & 0 deletions WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.1.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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[]
9 changes: 9 additions & 0 deletions WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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':
Expand Down

0 comments on commit bd16416

Please sign in to comment.