Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PrefixAllGlobals/GlobalVariablesOverride: detect variables being set via list() #1783

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
120 changes: 118 additions & 2 deletions WordPress/Sniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
}

}
61 changes: 51 additions & 10 deletions WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php
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 ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be added as a util method in the Sniff.php like get_list_variables is? Because it's used in two places.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair question. IMO no.

process_...() methods are not utility methods, but sniff processing specific. IMO they can either be in the sniff itself or in an abstract sniff - or possibly a sniff helper trait.

As these two sniffs are basically the polar opposite of each other, a case can be made to refactor them and either work with an abstract class, a trait or let one sniff extend the other.
(and yes, I've been thinking about that myself as well)

That was not what this PR is about though. This PR adds certain missing functionality. To keep things clean and reviewable, the refactor should - again IMO - be done in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation, this makes more sense now. And I agree, implementing either a trait or an abstract class seems like overkill for now.

👍

$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