Repository: WordPress/WordPress-Coding-Standards (php)

Issue:
False positive "High pagination limit" when `posts_per_page` is set to a function.
<!--
PLEASE FILL OUT THE TEMPLATE COMPLETELY.
BUG REPORTS WHICH CANNOT BE REPRODUCED BASED ON THE INFORMATION PROVIDED WILL BE CLOSED.
-->

## Bug Description
<!--
Please provide a clear and concise description of what the bug is.

What did you expect to happen? What actually happened?
-->

When setting `posts_per_page` to a function, wpcs logs "Detected high pagination limit, `posts_per_page` is set to..." warning.
It should [be treated like `-1`](https://github.com/WordPress/WordPress-Coding-Standards/issues/1638#issuecomment-459141404), and have no error shown.

## Minimal Code Snippet
<!-- Please provide example code that allows us to reproduce the issue. Do NOT paste screenshots of code! -->

The issue happens when running this command:
```bash
phpcs ...
```

... over a file containing this code:
```php
function get_query_args( int $first, int $last, int $default_min = 10 ) : array {
  return array(
    // other query args...
    'posts_per_page' => min( max( $first, $last ), $default_min ), 
  );
}
```

## Error Code
<!--
The error code for the sniff that is (or should be) being triggered (you
can see the sniff error codes by running `phpcs` with the `-s` flag).
e.g. `WordPress.PHP.NoSilencedErrors.Discouraged`

You can leave this section empty if you are reporting a false negative.
-->
`WordPress.WP.PostsPerPage.posts_per_page_posts_per_page`

## Custom ruleset
<!--
If the issue cannot be reproduced when using `--standard=WordPress` on the command line,
please post the relevant part of your custom ruleset here.
-->

## Environment
<!--
To find out the versions used:
* PHP: run `php -v`.
* PHPCS: run `[vendor/bin/]phpcs --version`
* WPCS: run `composer [global] info` for a Composer install.
-->

| Question               | Answer
| ------------------------| -------
| PHP version             | 8.0.15
| PHP_CodeSniffer version | 3.6.2
| WPCS version            | 2.3.0, dev-develop
| WPCS install type       | Composer project local
| IDE (if relevant)       | VSCode on wsl2


## Tested Against `develop` branch?
- [x] I have verified the issue still exists in the `develop` branch of WPCS.


Pull request description:
WP/PostsPerPage: various bug fixes
### WP/PostsPerPage: bug fix - non-numeric values for "posts_per_page" should be disregarded

This expands the tests for the "value capturing" fix in commit 1c969ab6146625fe24cfb7107e900e45142a5755 which was included in #2266.

Additionally, it adds a safeguard to the `WordPress.WP.PostsPerPage` sniff `callback()` method to prevent reporting an error when the value captured is not purely numeric.

This safeguard is needed as the way [text to number comparisons](https://wiki.php.net/rfc/string_to_number_comparison) are executed has changed in PHP 8.0 and that change affects this sniff.
See: https://3v4l.org/nOlYL#veol

Fixes #2027

### WP/PostsPerPage: bug fix - allow for explicitly positive numbers

This prevents false negatives when the number given is explicitly positive.

Includes tests.

### WP/PostsPerPage: allow for non-decimal nrs, PHP 7.4 numeric literals, PHP 8.1 octal literals

While it will probably be exceedingly rare for the `posts_per_page` value to be passed as anything but a decimal number (except to bypass the sniff), the sniff should still treat numbers passed using a different base correctly.

Along the same lines, decimal numbers passed using PHP 7.4 numeric literal notation should also be handled correctly.

Fixed now. Includes safeguard to only handle text string values when they are purely numeric.

Includes tests.

Patch:
diff --git a/WordPress/Sniffs/WP/PostsPerPageSniff.php b/WordPress/Sniffs/WP/PostsPerPageSniff.php
index def9def7..b98e30db 100644
--- a/WordPress/Sniffs/WP/PostsPerPageSniff.php
+++ b/WordPress/Sniffs/WP/PostsPerPageSniff.php
@@ -9,6 +9,8 @@
 
 namespace WordPressCS\WordPress\Sniffs\WP;
 
+use PHPCSUtils\Utils\Numbers;
+use PHPCSUtils\Utils\TextStrings;
 use WordPressCS\WordPress\AbstractArrayAssignmentRestrictionsSniff;
 
 /**
@@ -67,6 +69,34 @@ final class PostsPerPageSniff extends AbstractArrayAssignmentRestrictionsSniff {
 	 * @return bool FALSE if no match, TRUE if matches.
 	 */
 	public function callback( $key, $val, $line, $group ) {
-		return ( $val > (int) $this->posts_per_page );
+		$stripped_val = TextStrings::stripQuotes( $val );
+
+		if ( $val !== $stripped_val ) {
+			// The value was a text string. For text strings, we only accept purely numeric values.
+			if ( preg_match( '`^[0-9]+$`', $stripped_val ) !== 1 ) {
+				// Not a purely numeric value, so any comparison would be a false comparison.
+				return false;
+			}
+
+			// Purely numeric string, treat it as an integer from here on out.
+			$val = $stripped_val;
+		}
+
+		$first_char = $val[0];
+		if ( '-' === $first_char || '+' === $first_char ) {
+			$val = ltrim( $val, '-+' );
+		} else {
+			$first_char = '';
+		}
+
+		$real_value = Numbers::getDecimalValue( $val );
+		if ( false === $real_value ) {
+			// This wasn't a purely numeric value, so any comparison would be a false comparison.
+			return false;
+		}
+
+		$val = $first_char . $real_value;
+
+		return ( (int) $val > (int) $this->posts_per_page );
 	}
 }


Test patch:
diff --git a/WordPress/Tests/WP/PostsPerPageUnitTest.inc b/WordPress/Tests/WP/PostsPerPageUnitTest.inc
index cddd9797..7474ba6c 100644
--- a/WordPress/Tests/WP/PostsPerPageUnitTest.inc
+++ b/WordPress/Tests/WP/PostsPerPageUnitTest.inc
@@ -81,3 +81,46 @@ $args = array( 'posts_per_page' => 999 ); // Bad.
 $args = [
 	'posts_per_page' => 999
 ]; // Bad.
+
+// Verify that the complete value is being captured correctly and that non-numeric values are disregarded.
+$args = array(
+	'posts_per_page' => min( max( $first, $last ), $default_min ), // Should be ignored as undetermined.
+	'posts_per_page' => 10 + $extra, // Should be ignored as undetermined.
+	'posts_per_page' => $value[0][1], // Should be ignored as undetermined.
+	'posts_per_page' => $value ? 10 * $value : 300, // Should be ignored as undetermined.
+	'posts_per_page' => get_value( name: 'post_per_page', type: 'query' ), // Should be ignored as undetermined.
+	'posts_per_page' => function($a): int {
+		return do_something( $a );
+	}, // Should be ignored as undetermined.
+	'posts_per_page' => [ 100, 200, 300 ], // Should be ignored as undetermined.
+	'posts_per_page' => array(100, 200, 300), // Should be ignored as undetermined.
+);
+$query_args['posts_per_page'] = my\get_posts_per_page(); // Should be ignored as undetermined.
+$query_args['posts_per_page'] = '1e3'; // Should be ignored as undetermined. Would evaluate to 1000 with an int cast, but WP doesn't cast the value.
+
+// Purely numeric strings should probably be accepted still as this won't make a difference for the database query.
+$query_args['posts_per_page'] = '50'; // OK.
+$query_args['posts_per_page'] = '200'; // Bad.
+$query_args['posts_per_page'] = "200"; // Bad.
+
+// Verify handling of explicitly positive numbers.
+$args = array(
+	'posts_per_page' => +50, // OK.
+	'posts_per_page' => +200, // Bad.
+);
+
+// Verify handling of PHP 7.4+ numeric literals, PHP 8.1 octal literals, non-decimal numbers and floats.
+$args = array(
+	'posts_per_page' => 0b1001011, // OK (75).
+	'posts_per_page' => 0b10010110, // Bad (150).
+	'posts_per_page' => 0x4B, // OK (75).
+	'posts_per_page' => 0x96, // Bad (150).
+	'posts_per_page' => 0113, // OK (75).
+	'posts_per_page' => 0226, // Bad (150).
+	'posts_per_page' => 0O113, // OK (75).
+	'posts_per_page' => 0o226, // Bad (150).
+	'posts_per_page' => 7_5, // OK (75).
+	'posts_per_page' => 1_50, // Bad (150).
+	'posts_per_page' => 75.0, // OK (75).
+	'posts_per_page' => 150.000, // Bad (150).
+);
diff --git a/WordPress/Tests/WP/PostsPerPageUnitTest.php b/WordPress/Tests/WP/PostsPerPageUnitTest.php
index 7dbd98b3..3cc98c41 100644
--- a/WordPress/Tests/WP/PostsPerPageUnitTest.php
+++ b/WordPress/Tests/WP/PostsPerPageUnitTest.php
@@ -43,23 +43,32 @@ final class PostsPerPageUnitTest extends AbstractSniffUnitTest {
 	 */
 	public function getWarningList() {
 		return array(
-			4  => 1,
-			10 => 1,
-			11 => 1,
-			13 => 1,
-			22 => 1,
-			23 => 1,
-			24 => 1,
-			29 => 1,
-			49 => 1,
-			51 => 1,
-			54 => 1,
-			57 => 1,
-			59 => 1,
-			67 => 1,
-			72 => 1,
-			80 => 1,
-			82 => 1,
+			4   => 1,
+			10  => 1,
+			11  => 1,
+			13  => 1,
+			22  => 1,
+			23  => 1,
+			24  => 1,
+			29  => 1,
+			49  => 1,
+			51  => 1,
+			54  => 1,
+			57  => 1,
+			59  => 1,
+			67  => 1,
+			72  => 1,
+			80  => 1,
+			82  => 1,
+			103 => 1,
+			104 => 1,
+			109 => 1,
+			115 => 1,
+			117 => 1,
+			119 => 1,
+			121 => 1,
+			123 => 1,
+			125 => 1,
 		);
 	}
 }