AbstractArrayAssignmentRestrictions: implement PHPCSUtils, bug fixes, modern PHP and more

WordPress/AbstractArrayAssignmentRestrictionsSniff.php

@@ -9,6 +9,8 @@
 
 namespace WordPressCS\WordPress;
 
+use PHP_CodeSniffer\Util\Tokens;
+use PHPCSUtils\Utils\GetTokensAsString;
 use PHPCSUtils\Utils\MessageHelper;
 use PHPCSUtils\Utils\TextStrings;
 use WordPressCS\WordPress\Helpers\RulesetPropertyHelper;
@@ -36,7 +38,7 @@ abstract class AbstractArrayAssignmentRestrictionsSniff extends Sniff {
 	 * @since 1.0.0 This property now expects to be passed an array.
 	 *              Previously a comma-delimited string was expected.
 	 *
-	 * @var array
+	 * @var string[]
 	 */
 	public $exclude = array();
 
@@ -45,7 +47,7 @@ abstract class AbstractArrayAssignmentRestrictionsSniff extends Sniff {
 	 * Don't use this in extended classes, override getGroups() instead.
 	 * This is only used for Unit tests.
 	 *
-	 * @var array
+	 * @var array<string, array>
 	 */
 	public static $groups = array();
 
@@ -54,7 +56,7 @@ abstract class AbstractArrayAssignmentRestrictionsSniff extends Sniff {
 	 *
 	 * @since 0.11.0
 	 *
-	 * @var array
+	 * @var array<string, bool>
 	 */
 	protected $excluded_groups = array();
 
@@ -63,7 +65,7 @@ abstract class AbstractArrayAssignmentRestrictionsSniff extends Sniff {
 	 *
 	 * @since 0.13.0
 	 *
-	 * @var array
+	 * @var array<string, array>
 	 */
 	protected $groups_cache = array();
 
@@ -94,13 +96,13 @@ public function register() {
 	 * Example: groups => array(
 	 *  'groupname' => array(
 	 *      'type'     => 'error' | 'warning',
-	 *      'message'  => 'Dont use this one please!',
+	 *      'message'  => 'Descriptive error message. The error message will be passed the $key and $val of the current array assignment.',
 	 *      'keys'     => array( 'key1', 'another_key' ),
 	 *      'callback' => array( 'class', 'method' ), // Optional.
 	 *  )
 	 * )
 	 *
-	 * @return array
+	 * @return array<string, array>
 	 */
 	abstract public function getGroups();
 
@@ -145,42 +147,65 @@ public function process_token( $stackPtr ) {
 		$token = $this->tokens[ $stackPtr ];
 
 		if ( \T_CLOSE_SQUARE_BRACKET === $token['code'] ) {
-			$equal = $this->phpcsFile->findNext( \T_WHITESPACE, ( $stackPtr + 1 ), null, true );
-			if ( \T_EQUAL !== $this->tokens[ $equal ]['code'] ) {
-				return; // This is not an assignment!
+			$equalPtr = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true );
+			if ( \T_EQUAL !== $this->tokens[ $equalPtr ]['code']
+				&& \T_COALESCE_EQUAL !== $this->tokens[ $equalPtr ]['code']
+			) {
+				// This is not an assignment. Bow out.
+				return;
 			}
 		}
 
-		// Instances: Multi-dimensional array, keyed by line.
+		// Instances: Multi-dimensional array.
 		$inst = array();
 
 		/*
 		 * Covers array assignments:
 		 * `$foo = array( 'bar' => 'taz' );`
 		 * `$foo['bar'] = $taz;`
 		 */
-		if ( \in_array( $token['code'], array( \T_CLOSE_SQUARE_BRACKET, \T_DOUBLE_ARROW ), true ) ) {
+		if ( \T_CLOSE_SQUARE_BRACKET === $token['code'] || \T_DOUBLE_ARROW === $token['code'] ) {
 			$operator = $stackPtr; // T_DOUBLE_ARROW.
 			if ( \T_CLOSE_SQUARE_BRACKET === $token['code'] ) {
-				$operator = $this->phpcsFile->findNext( \T_EQUAL, ( $stackPtr + 1 ) );
+				$operator = $equalPtr;
 			}
 
-			$keyIdx = $this->phpcsFile->findPrevious( array( \T_WHITESPACE, \T_CLOSE_SQUARE_BRACKET ), ( $operator - 1 ), null, true );
-			if ( ! is_numeric( $this->tokens[ $keyIdx ]['content'] ) ) {
-				$key            = TextStrings::stripQuotes( $this->tokens[ $keyIdx ]['content'] );
-				$valStart       = $this->phpcsFile->findNext( array( \T_WHITESPACE ), ( $operator + 1 ), null, true );
-				$valEnd         = $this->phpcsFile->findNext( array( \T_COMMA, \T_SEMICOLON ), ( $valStart + 1 ), null, false, null, true );
-				$val            = $this->phpcsFile->getTokensAsString( $valStart, ( $valEnd - $valStart ) );
-				$val            = TextStrings::stripQuotes( $val );
-				$inst[ $key ][] = array( $val, $token['line'] );
+			$keyIdx = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true );
+			if ( isset( Tokens::$stringTokens[ $this->tokens[ $keyIdx ]['code'] ] )
+				&& ! is_numeric( $this->tokens[ $keyIdx ]['content'] )
+			) {
+				$key      = TextStrings::stripQuotes( $this->tokens[ $keyIdx ]['content'] );
+				$valStart = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $operator + 1 ), null, true );
+				$valEnd   = $this->phpcsFile->findEndOfStatement( $valStart, \T_COLON );
+				if ( \T_COMMA === $this->tokens[ $valEnd ]['code']
+					|| \T_SEMICOLON === $this->tokens[ $valEnd ]['code']
+				) {
+					// FindEndOfStatement includes the comma/semi-colon if that's the end of the statement.
+					// That's not what we want (and inconsistent), so remove it.
+					--$valEnd;
+				}
+
+				$val          = trim( GetTokensAsString::compact( $this->phpcsFile, $valStart, $valEnd, true ) );
+				$inst[ $key ] = array(
+					'value'  => $val,
+					'line'   => $token['line'],
+					'keyptr' => $keyIdx,
+				);
 			}
-		} elseif ( \in_array( $token['code'], array( \T_CONSTANT_ENCAPSED_STRING, \T_DOUBLE_QUOTED_STRING ), true ) ) {
-			// Covers assignments via query parameters: `$foo = 'bar=taz&other=thing';`.
+		} elseif ( isset( Tokens::$stringTokens[ $token['code'] ] ) ) {
+			/*
+			 * Covers assignments via query parameters: `$foo = 'bar=taz&other=thing';`.
+			 */
 			if ( preg_match_all( '#(?:^|&)([a-z_]+)=([^&]*)#i', TextStrings::stripQuotes( $token['content'] ), $matches ) <= 0 ) {
 				return; // No assignments here, nothing to check.
 			}
-			foreach ( $matches[1] as $i => $_k ) {
-				$inst[ $_k ][] = array( $matches[2][ $i ], $token['line'] );
+
+			foreach ( $matches[1] as $match_nr => $key ) {
+				$inst[ $key ] = array(
+					'value'  => $matches[2][ $match_nr ],
+					'line'   => $token['line'],
+					'keyptr' => $stackPtr,
+				);
 			}
 		}
 
@@ -196,33 +221,29 @@ public function process_token( $stackPtr ) {
 
 			$callback = ( isset( $group['callback'] ) && is_callable( $group['callback'] ) ) ? $group['callback'] : array( $this, 'callback' );
 
-			foreach ( $inst as $key => $assignments ) {
-				foreach ( $assignments as $occurance ) {
-					list( $val, $line ) = $occurance;
-
-					if ( ! \in_array( $key, $group['keys'], true ) ) {
-						continue;
-					}
-
-					$output = \call_user_func( $callback, $key, $val, $line, $group );
-
-					if ( ! isset( $output ) || false === $output ) {
-						continue;
-					} elseif ( true === $output ) {
-						$message = $group['message'];
-					} else {
-						$message = $output;
-					}
-
-					MessageHelper::addMessage(
-						$this->phpcsFile,
-						$message,
-						$stackPtr,
-						( 'error' === $group['type'] ),
-						MessageHelper::stringToErrorcode( $groupName . '_' . $key ),
-						array( $key, $val )
-					);
+			foreach ( $inst as $key => $assignment ) {
+				if ( ! \in_array( $key, $group['keys'], true ) ) {
+					continue;
 				}
+
+				$output = \call_user_func( $callback, $key, $assignment['value'], $assignment['line'], $group );
+
+				if ( ! isset( $output ) || false === $output ) {
+					continue;
+				} elseif ( true === $output ) {
+					$message = $group['message'];
+				} else {
+					$message = $output;
+				}
+
+				MessageHelper::addMessage(
+					$this->phpcsFile,
+					$message,
+					$assignment['keyptr'],
+					( 'error' === $group['type'] ),
+					MessageHelper::stringToErrorcode( $groupName . '_' . $key ),
+					array( $key, $assignment['value'] )
+				);
 			}
 		}
 	}
@@ -232,12 +253,13 @@ public function process_token( $stackPtr ) {
 	 *
 	 * This method must be extended to add the logic to check assignment value.
 	 *
-	 * @param  string $key   Array index / key.
-	 * @param  mixed  $val   Assigned value.
-	 * @param  int    $line  Token line.
-	 * @param  array  $group Group definition.
-	 * @return mixed         FALSE if no match, TRUE if matches, STRING if matches
-	 *                       with custom error message passed to ->process().
+	 * @param string $key   Array index / key.
+	 * @param mixed  $val   Assigned value.
+	 * @param int    $line  Token line.
+	 * @param array  $group Group definition.
+	 *
+	 * @return mixed FALSE if no match, TRUE if matches, STRING if matches
+	 *               with custom error message passed to ->process().
 	 */
 	abstract public function callback( $key, $val, $line, $group );
 }

WordPress/Tests/WP/PostsPerPageUnitTest.inc

@@ -28,3 +28,56 @@ $query_args['posts_per_page'] = 100; // OK.
 $query_args['posts_per_page'] = 200; // OK.
 $query_args['posts_per_page'] = 300; // Bad.
 // phpcs:set WordPress.WP.PostsPerPage posts_per_page 100
+
+// phpcs:set WordPress.WP.PostsPerPage exclude[] posts_per_page
+$query_args['posts_per_page'] = 300; // OK, group excluded.
+// phpcs:set WordPress.WP.PostsPerPage exclude[]
+
+// Ensure there will be no false positive for array access brackets when not used for an assignment.
+$var = $query_args['posts_per_page']; // OK.
+$firstChars = $text[0] . $text[1]; // OK.
+
+// Text strings which are not query strings should be ignored.
+_query_posts( 'numberposts' ); // OK.
+
+// Assignments to non-string keys should be ignored. Note: PHP will auto-cast numeric strings to ints, so those should also be disregarded.
+$var[10] = 300; // OK.
+$var[] = 400; // OK.
+$var['239'] = 500; // OK.
+
+// Ensure the sniff disregards comments.
+$query_args['posts_per_page' /* high */ ] = 999; // Bad.
+
+$query_args['posts_per_page'] /* high */  = 999; // Bad.
+
+$args = array(
+	'posts_per_page' /* high */ => 999, // Bad.
+);
+
+$query_args['posts_per_page'] = /* high */ 999; // Bad.
+$args = array(
+	'posts_per_page' => /* high */ 999, // Bad.
+);
+
+// Safeguard that when a query string contains duplicate key, the value of the last one is used.
+_query_posts( 'posts_per_page=999&nopaging=true&posts_per_page=50' ); // OK.
+
+// Ensure the error gets reported on the key pointer.
+$query_args[
+	'posts_per_page'
+] = 300; // Bad, error should be reported on the above line.
+
+// Ensure that PHP 7.4 null coalesce equals get picked up on.
+$query_args['posts_per_page'] ??= 50; // OK.
+$query_args['posts_per_page'] ??= 200; // Bad.
+
+// Ensure that the sniff does not report on PHP 8.0 match arms.
+$val = match($val) {
+	'posts_per_page' => 999, // OK, not an array assignment.
+};
+
+// Verify handling of arrays without trailing comma after the last array item.
+$args = array( 'posts_per_page' => 999 ); // Bad.
+$args = [
+	'posts_per_page' => 999
+]; // Bad.

WordPress/Tests/WP/PostsPerPageUnitTest.php

@@ -51,6 +51,15 @@ public function getWarningList() {
 			23 => 1,
 			24 => 1,
 			29 => 1,
+			49 => 1,
+			51 => 1,
+			54 => 1,
+			57 => 1,
+			59 => 1,
+			67 => 1,
+			72 => 1,
+			80 => 1,
+			82 => 1,
 		);
 	}
 }