From 47c1c2c77347be64972c8747d62324b0a2ea6022 Mon Sep 17 00:00:00 2001
From: jrfnl <jrfnl@users.noreply.github.com>
Date: Tue, 22 Aug 2023 23:03:59 +0200
Subject: [PATCH] Performance/WPQueryParams: prevent false positives for
 'exclude' with get_users()

As reported in 672 and 729, the `get_users()` function also takes an array parameter which takes an `'exclude'` key. That key is not our target, so should not be flagged.

This commit adds a hard-coded exception specifically for that situation.

If at a later point in time, more situations which need exceptions would be discovered, this solution can be made more flexible, but for now, there is no need (or insight into where the flexibility should be).

As the `AbstractArrayAssignmentRestrictionsSniff::callback()` method does not have access to the `$stackPtr`, the logic which can be used in the `callback()` is limited. Also see the review notes from upstream 2266, which basically already pointed out this exact problem.

To get round this, I'm overloading the `process_token()` method to set a temporary `$in_get_users` property, which can then be read out in the `callback()` method to be used in the actual determination of whether the exception should be made or not.

Includes tests.

Fixes 672
---
 .../Sniffs/Performance/WPQueryParamsSniff.php | 27 +++++++++++++++++++
 .../Performance/WPQueryParamsUnitTest.inc     |  8 ++++++
 .../Performance/WPQueryParamsUnitTest.php     |  3 +++
 3 files changed, 38 insertions(+)

diff --git a/WordPressVIPMinimum/Sniffs/Performance/WPQueryParamsSniff.php b/WordPressVIPMinimum/Sniffs/Performance/WPQueryParamsSniff.php
index 301a9019..38911bfb 100644
--- a/WordPressVIPMinimum/Sniffs/Performance/WPQueryParamsSniff.php
+++ b/WordPressVIPMinimum/Sniffs/Performance/WPQueryParamsSniff.php
@@ -9,6 +9,7 @@
 namespace WordPressVIPMinimum\Sniffs\Performance;
 
 use WordPressCS\WordPress\AbstractArrayAssignmentRestrictionsSniff;
+use WordPressCS\WordPress\Helpers\ContextHelper;
 
 /**
  * Flag suspicious WP_Query and get_posts params.
@@ -19,6 +20,13 @@
  */
 class WPQueryParamsSniff extends AbstractArrayAssignmentRestrictionsSniff {
 
+	/**
+	 * Whether the current $stackPtr being scanned is nested in a function call to get_users().
+	 *
+	 * @var bool
+	 */
+	private $in_get_users = false;
+
 	/**
 	 * Groups of variables to restrict.
 	 *
@@ -48,6 +56,20 @@ public function getGroups() {
 		];
 	}
 
+	/**
+	 * Processes this test, when one of its tokens is encountered.
+	 *
+	 * @param int $stackPtr The position of the current token in the stack.
+	 *
+	 * @return void
+	 */
+	public function process_token( $stackPtr ) {
+		$this->in_get_users = ContextHelper::is_in_function_call( $this->phpcsFile, $stackPtr, [ 'get_users' => true ], true, true );
+
+		parent::process_token( $stackPtr );
+	}
+
+
 	/**
 	 * Callback to process a confirmed key which doesn't need custom logic, but should always error.
 	 *
@@ -64,6 +86,11 @@ public function callback( $key, $val, $line, $group ) {
 				return ( $val === 'true' );
 
 			case 'PostNotIn':
+				if ( $key === 'exclude' && $this->in_get_users !== false ) {
+					// This is not an array used by get_posts(). See #672.
+					return false;
+				}
+
 				return true;
 
 			default:
diff --git a/WordPressVIPMinimum/Tests/Performance/WPQueryParamsUnitTest.inc b/WordPressVIPMinimum/Tests/Performance/WPQueryParamsUnitTest.inc
index 2aecfee5..d7c624d5 100644
--- a/WordPressVIPMinimum/Tests/Performance/WPQueryParamsUnitTest.inc
+++ b/WordPressVIPMinimum/Tests/Performance/WPQueryParamsUnitTest.inc
@@ -21,3 +21,11 @@ $q = new WP_query( $query_args );
 get_posts( [ 'exclude' => $post_ids ] ); // Warning.
 
 $exclude = [ 1, 2, 3 ];
+
+// Issue #672 / #729.
+get_users( [ 'exclude' => $post_ids ] ); // OK.
+get_users( My\get_args( [ 'exclude' => $post_ids ] ) ); // OK - arbitrary as the call to `My\get_args()` on its own would be flagged, but let's allow it.
+
+$context_unknown = [ 'exclude' => $post_ids ]; // Warning.
+other_fn_calls_still_throw_warning( [ 'exclude' => $post_ids ] ); // Warning.
+get_users( [ 'suppress_filters' => true ] ); // Error - not necessarily valid, but the exception being made is specifically about `exclude`.
diff --git a/WordPressVIPMinimum/Tests/Performance/WPQueryParamsUnitTest.php b/WordPressVIPMinimum/Tests/Performance/WPQueryParamsUnitTest.php
index d1a2b4e7..63d6e7d9 100644
--- a/WordPressVIPMinimum/Tests/Performance/WPQueryParamsUnitTest.php
+++ b/WordPressVIPMinimum/Tests/Performance/WPQueryParamsUnitTest.php
@@ -27,6 +27,7 @@ public function getErrorList() {
 		return [
 			5  => 1,
 			17 => 1,
+			31 => 1,
 		];
 	}
 
@@ -40,6 +41,8 @@ public function getWarningList() {
 			4  => 1,
 			11 => 1,
 			21 => 1,
+			29 => 1,
+			30 => 1,
 		];
 	}
 }