From bd1641684895ff3ad5b26b1a557cd11737ed3529 Mon Sep 17 00:00:00 2001
From: jrfnl <jrfnl@users.noreply.github.com>
Date: Sat, 27 Jul 2019 15:50:41 +0200
Subject: [PATCH] PrefixAllGlobals: detect non-prefixed variables in list
 assignments

Global variables to which an assignment is made via the long/short `list()` construct should also be prefixed.

Includes unit tests.

Related to 1774
---
 .../PrefixAllGlobalsSniff.php                 | 61 ++++++++++++++++---
 .../PrefixAllGlobalsUnitTest.1.inc            | 42 +++++++++++++
 .../PrefixAllGlobalsUnitTest.php              |  9 +++
 3 files changed, 102 insertions(+), 10 deletions(-)

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/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':