Merge pull request #882 from Automattic/GaryJones/nopaging-review

Author: GaryJones

WordPress-VIP-Go/ruleset-test.inc

@@ -466,6 +466,9 @@ $args = array(
 	'nopaging'       => true, // Error.
 );
 _query_posts( 'nopaging=true' ); // Error.
+$args = array(
+	'posts_per_page' => -1, // Error.
+);
 
 // WordPressVIPMinimum.Performance.OrderByRand
 $args = array(

WordPress-VIP-Go/ruleset-test.php

@@ -87,15 +87,13 @@
 		462 => 1,
 		466 => 1,
 		468 => 1,
-		472 => 1,
-		474 => 1,
-		480 => 1,
-		486 => 1,
-		494 => 1,
-		507 => 1,
-		511 => 1,
-		512 => 1,
-		513 => 1,
+		470 => 1,
+		475 => 1,
+		477 => 1,
+		483 => 1,
+		489 => 1,
+		497 => 1,
+		510 => 1,
 		514 => 1,
 		515 => 1,
 		516 => 1,
@@ -104,16 +102,19 @@
 		519 => 1,
 		520 => 1,
 		521 => 1,
-		525 => 1,
-		527 => 1,
-		545 => 1,
-		560 => 1,
-		564 => 1,
-		565 => 1,
-		566 => 1,
+		522 => 1,
+		523 => 1,
+		524 => 1,
+		528 => 1,
+		530 => 1,
+		548 => 1,
+		563 => 1,
 		567 => 1,
-		572 => 1,
-		574 => 1,
+		568 => 1,
+		569 => 1,
+		570 => 1,
+		575 => 1,
+		577 => 1,
 	],
 	'warnings' => [
 		7   => 1,
@@ -223,14 +224,14 @@
 		454 => 1,
 		455 => 1,
 		456 => 1,
-		502 => 1,
-		503 => 1,
-		530 => 1,
+		505 => 1,
+		506 => 1,
 		533 => 1,
-		540 => 1,
-		550 => 1,
-		556 => 1,
-		579 => 1,
+		536 => 1,
+		543 => 1,
+		553 => 1,
+		559 => 1,
+		582 => 1,
 	],
 	'messages' => [
 		7   => [

WordPressVIPMinimum/Sniffs/Performance/NoPagingSniff.php

@@ -10,10 +10,11 @@
 
 namespace WordPressVIPMinimum\Sniffs\Performance;
 
+use PHPCSUtils\Utils\TextStrings;
 use WordPressCS\WordPress\AbstractArrayAssignmentRestrictionsSniff;
 
 /**
- * Flag returning high or infinite posts_per_page.
+ * Flag disabling pagination via `nopaging` or `posts_per_page`/`numberposts` set to `-1`.
  *
  * @link https://docs.wpvip.com/technical-references/code-review/#no-limit-queries
  */
@@ -28,11 +29,19 @@ public function getGroups() {
 		return [
 			'nopaging' => [
 				'type'    => 'error',
-				'message' => 'Disabling pagination is prohibited in VIP context, do not set `%s` to `%s` ever.',
+				'message' => 'Disabling pagination is prohibited in VIP context, do not set `%s` to `%s`.',
 				'keys'    => [
 					'nopaging',
 				],
 			],
+			'posts_per_page' => [
+				'type'    => 'error',
+				'message' => 'Setting `%s` to `%s` disables pagination and is prohibited in VIP context.',
+				'keys'    => [
+					'posts_per_page',
+					'numberposts',
+				],
+			],
 		];
 	}
 
@@ -49,6 +58,12 @@ public function getGroups() {
 	public function callback( $key, $val, $line, $group ) {
 		$key = strtolower( $key );
 
-		return ( $key === 'nopaging' && ( $val === 'true' || $val === '1' ) );
+		if ( $key === 'nopaging' ) {
+			return ( $val === 'true' || $val === '1' );
+		}
+
+		// posts_per_page / numberposts: flag -1 (no limit).
+		$val = TextStrings::stripQuotes( $val );
+		return ( $val === '-1' );
 	}
 }

WordPressVIPMinimum/Tests/Performance/NoPagingUnitTest.inc

@@ -1,15 +1,85 @@
 <?php
 
+// === nopaging tests ===
+
 $args = array(
 	'nopaging'       => true, // Bad.
 );
 
 _query_posts( 'nopaging=true' ); // Bad.
 
-$query_args['my_posts_per_page'] = -1; // OK.
+$query_args['my_posts_per_page'] = -1; // OK - not a recognized key.
 
 // Verify handling with no trailing comma at end of array.
 $args = array(
 	'nopaging' => true // Bad.
 );
 $args = [ 'nopaging' => true ]; // Bad.
+
+// nopaging with integer 1.
+$args = [ 'nopaging' => 1 ]; // Bad.
+
+// nopaging with false - should not trigger.
+$args = [ 'nopaging' => false ]; // OK.
+
+// nopaging with integer 0 - should not trigger.
+$args = [ 'nopaging' => 0 ]; // OK.
+
+// nopaging via variable assignment.
+$args['nopaging'] = true; // Bad.
+
+// nopaging via variable assignment with false.
+$args['nopaging'] = false; // OK.
+
+// === posts_per_page tests ===
+
+// posts_per_page set to -1 in long array syntax.
+$args = array(
+	'posts_per_page' => -1, // Bad.
+);
+
+// posts_per_page set to -1 in short array syntax.
+$args = [ 'posts_per_page' => -1 ]; // Bad.
+
+// posts_per_page set to -1 as string.
+$args = [ 'posts_per_page' => '-1' ]; // Bad.
+
+// posts_per_page set to a normal value - should not trigger.
+$args = [ 'posts_per_page' => 50 ]; // OK.
+
+// posts_per_page set to 1 - should not trigger.
+$args = [ 'posts_per_page' => 1 ]; // OK.
+
+// numberposts set to -1.
+$args = [ 'numberposts' => -1 ]; // Bad.
+
+// numberposts set to a normal value - should not trigger.
+$args = [ 'numberposts' => 10 ]; // OK.
+
+// posts_per_page via variable assignment.
+$args['posts_per_page'] = -1; // Bad.
+
+// posts_per_page via variable assignment with normal value.
+$args['posts_per_page'] = 10; // OK.
+
+// Query string with posts_per_page=-1.
+_query_posts( 'posts_per_page=-1&orderby=date' ); // Bad.
+
+// Query string with posts_per_page set to a normal value.
+_query_posts( 'posts_per_page=50' ); // OK.
+
+// Query string with numberposts=-1.
+_query_posts( 'numberposts=-1' ); // Bad.
+
+// === Ignoring with phpcs:ignore comments ===
+
+// phpcs:ignore WordPressVIPMinimum.Performance.NoPaging.posts_per_page_posts_per_page -- Intentional: fetching all items for export.
+$args = [ 'posts_per_page' => -1 ]; // OK - ignored.
+
+$args = [
+	// phpcs:ignore WordPressVIPMinimum.Performance.NoPaging.posts_per_page_numberposts -- Intentional: legacy API requires all results.
+	'numberposts' => -1, // OK - ignored.
+];
+
+// phpcs:ignore WordPressVIPMinimum.Performance.NoPaging.nopaging_nopaging -- Intentional: sitemap generation.
+$args = [ 'nopaging' => true ]; // OK - ignored.

WordPressVIPMinimum/Tests/Performance/NoPagingUnitTest.php

@@ -23,10 +23,19 @@ class NoPagingUnitTest extends AbstractSniffUnitTest {
 	 */
 	public function getErrorList() {
 		return [
-			4  => 1,
-			7  => 1,
-			13 => 1,
+			6  => 1,
+			9  => 1,
 			15 => 1,
+			17 => 1,
+			20 => 1,
+			29 => 1,
+			38 => 1,
+			42 => 1,
+			45 => 1,
+			54 => 1,
+			60 => 1,
+			66 => 1,
+			72 => 1,
 		];
 	}
 

WordPressVIPMinimum/ruleset-test.inc

@@ -463,6 +463,9 @@ $args = array( // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.Un
 	'nopaging'       => true, // Error.
 );
 _query_posts( 'nopaging=true' ); // Error.
+$args = array( // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable
+	'posts_per_page' => -1, // Error.
+);
 
 // WordPressVIPMinimum.Performance.OrderByRand
 $args = array( // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable

WordPressVIPMinimum/ruleset-test.php

@@ -155,15 +155,13 @@
 		451 => 1,
 		463 => 1,
 		465 => 1,
-		469 => 1,
-		471 => 1,
-		477 => 1,
-		483 => 1,
-		491 => 1,
-		505 => 1,
-		509 => 1,
-		510 => 1,
-		511 => 1,
+		467 => 1,
+		472 => 1,
+		474 => 1,
+		480 => 1,
+		486 => 1,
+		494 => 1,
+		508 => 1,
 		512 => 1,
 		513 => 1,
 		514 => 1,
@@ -172,29 +170,32 @@
 		517 => 1,
 		518 => 1,
 		519 => 1,
-		523 => 1,
-		525 => 1,
-		550 => 1,
-		551 => 1,
+		520 => 1,
+		521 => 1,
+		522 => 1,
+		526 => 1,
+		528 => 1,
+		553 => 1,
 		554 => 1,
-		569 => 1,
-		570 => 1,
+		557 => 1,
+		572 => 1,
 		573 => 1,
-		574 => 1,
-		575 => 1,
+		576 => 1,
+		577 => 1,
 		578 => 1,
 		581 => 1,
-		582 => 1,
-		583 => 1,
-		588 => 1,
-		590 => 1,
-		594 => 1,
-		595 => 1,
-		596 => 1,
+		584 => 1,
+		585 => 1,
+		586 => 1,
+		591 => 1,
+		593 => 1,
 		597 => 1,
-		612 => 1,
-		614 => 1,
-		621 => 1,
+		598 => 1,
+		599 => 1,
+		600 => 1,
+		615 => 1,
+		617 => 1,
+		624 => 1,
 	],
 	'warnings' => [
 		32  => 1,
@@ -273,21 +274,21 @@
 		457 => 1,
 		458 => 1,
 		459 => 1,
-		499 => 1,
-		500 => 1,
-		504 => 1,
-		528 => 1,
-		529 => 1,
-		530 => 1,
+		502 => 1,
+		503 => 1,
+		507 => 1,
 		531 => 1,
 		532 => 1,
+		533 => 1,
+		534 => 1,
 		535 => 1,
 		538 => 1,
-		545 => 1,
-		559 => 1,
-		565 => 1,
-		589 => 1,
-		618 => 1,
+		541 => 1,
+		548 => 1,
+		562 => 1,
+		568 => 1,
+		592 => 1,
+		621 => 1,
 	],
 	'messages' => [
 		130 => [