WP_Query: force deterministic ordering

src/wp-includes/class-wp-query.php

@@ -1835,6 +1835,35 @@ protected function parse_order( $order ) {
 		}
 	}
 
+	/**
+	 * Determines whether an 'orderby' value already puts posts in a fixed sequence.
+	 *
+	 * True for the ID, for an explicit list of IDs, and for random ordering, where
+	 * an ID clause would either change nothing or undo a seeded shuffle.
+	 *
+	 * Not true for 'post_parent__in' or 'post_name__in': several posts can share
+	 * one parent, or one slug across post types, so those orderings can tie.
+	 *
+	 * @since 7.2.0
+	 *
+	 * @param string $orderby Single 'orderby' value, before it is parsed into SQL.
+	 * @return bool Whether the ordering is already determinate, making an ID clause unnecessary.
+	 */
+	protected function is_orderby_id( $orderby ) {
+		$orderby_id = array(
+			'ID',
+			'rand',
+			'post__in',
+		);
+
+		if ( in_array( $orderby, $orderby_id, true ) ) {
+			return true;
+		}
+
+		// Random ordering with a seed, for example 'RAND(5)', as parse_orderby() accepts it.
+		return 1 === preg_match( '/RAND\(([0-9]+)\)/i', $orderby );
+	}
+
 	/**
 	 * Sets the 404 property and saves whether query is feed.
 	 *
@@ -1892,6 +1921,8 @@ public function set( $query_var, $value ) {
 	 * database query.
 	 *
 	 * @since 1.5.0
+	 * @since 7.2.0 Adds the post ID to ORDER BY so that paginated queries do not
+	 *              return the same post on more than one page.
 	 *
 	 * @global wpdb $wpdb WordPress database abstraction object.
 	 *
@@ -2513,12 +2544,40 @@ public function get_posts() {
 			if ( isset( $query_vars['orderby'] ) && ( is_array( $query_vars['orderby'] ) || false === $query_vars['orderby'] ) ) {
 				$orderby = '';
 			} else {
-				$orderby = "{$wpdb->posts}.post_date " . $query_vars['order'];
+				/*
+				 * Sorting by post_date alone is not determinate: posts sharing a date are
+				 * returned in a different sequence each time the query runs, so a post
+				 * can appear on two pages at once or be missed entirely. The ID clause
+				 * gives every page a fixed sequence.
+				 */
+				$orderby = "{$wpdb->posts}.post_date {$query_vars['order']}, {$wpdb->posts}.ID {$query_vars['order']}";
 			}
-		} elseif ( 'none' === $query_vars['orderby'] ) {
+		} elseif ( 'none' === $query_vars['orderby'] || array( 'none' ) === array_keys( (array) $query_vars['orderby'] ) ) {
+			/*
+			 * 'none' blanks out ORDER BY. It arrives as a bare string from WP_Query, and
+			 * as the array's only key from get_pages(), which turns its 'sort_column'
+			 * into array( 'none' => $sort_order ). When 'none' appears in an array next
+			 * to real columns it is not the whole ordering, so it falls through and is
+			 * skipped below like any other unparseable key.
+			 */
 			$orderby = '';
 		} else {
 			$orderby_array = array();
+
+			/*
+			 * Whether the ordering already puts the posts in a fixed sequence, in which
+			 * case an ID clause would make no difference.
+			 */
+			$found_orderby_id = false;
+
+			/*
+			 * An array 'orderby' gives each column its own direction, so the query's
+			 * 'order' is not necessarily the one the last column used. Track it, so the
+			 * ID sorts the same way as the column above it and reversing the query
+			 * reverses the whole page.
+			 */
+			$last_order = $query_vars['order'];
+
 			if ( is_array( $query_vars['orderby'] ) ) {
 				foreach ( $query_vars['orderby'] as $_orderby => $order ) {
 					$orderby = wp_slash( urldecode( $_orderby ) );
@@ -2528,10 +2587,13 @@ public function get_posts() {
 						continue;
 					}
 
-					$orderby_array[] = $parsed . ' ' . $this->parse_order( $order );
-				}
-				$orderby = implode( ', ', $orderby_array );
+					$last_order      = $this->parse_order( $order );
+					$orderby_array[] = $parsed . ' ' . $last_order;
 
+					if ( $this->is_orderby_id( $orderby ) ) {
+						$found_orderby_id = true;
+					}
+				}
 			} else {
 				$query_vars['orderby'] = urldecode( $query_vars['orderby'] );
 				$query_vars['orderby'] = wp_slash( $query_vars['orderby'] );
@@ -2543,16 +2605,38 @@ public function get_posts() {
 						continue;
 					}
 
-					$orderby_array[] = $parsed;
+					$orderby_array[] = $parsed . ' ' . $query_vars['order'];
+
+					if ( $this->is_orderby_id( $orderby ) ) {
+						$found_orderby_id = true;
+					}
 				}
-				$orderby = implode( ' ' . $query_vars['order'] . ', ', $orderby_array );
 
-				if ( empty( $orderby ) ) {
-					$orderby = "{$wpdb->posts}.post_date " . $query_vars['order'];
-				} elseif ( ! empty( $query_vars['order'] ) ) {
-					$orderby .= " {$query_vars['order']}";
+				// If no valid clauses were found, order by post_date.
+				if ( empty( $orderby_array ) ) {
+					$orderby_array[] = "{$wpdb->posts}.post_date " . $query_vars['order'];
 				}
 			}
+
+			/*
+			 * To ensure determinate sorting, always include an ID clause. Posts sharing
+			 * the same value for the requested column are otherwise returned in a
+			 * different sequence each time the query runs, so a post can appear on two
+			 * pages at once or be missed entirely.
+			 *
+			 * An array 'orderby' whose keys all failed to parse stays empty here and
+			 * produces no ORDER BY at all, as it always has.
+			 */
+			if ( ! $found_orderby_id && ! empty( $orderby_array ) ) {
+				/*
+				 * $last_order is already 'ASC', 'DESC', or '' here. Blank stays blank:
+				 * 'order' is forced empty for the FIELD()-based orderings, whose clauses
+				 * sort implicitly ascending, and the ID should sort the same way.
+				 */
+				$orderby_array[] = trim( "{$wpdb->posts}.ID " . $last_order );
+			}
+
+			$orderby = trim( implode( ', ', $orderby_array ) );
 		}
 
 		// Order search results by relevance only when another "orderby" is not specified in the query.
@@ -3165,10 +3249,15 @@ public function get_posts() {
 			 */
 			$clauses = (array) apply_filters_ref_array( 'posts_clauses_request', array( compact( $pieces ), &$this ) );
 
-			$where    = $clauses['where'] ?? '';
-			$groupby  = $clauses['groupby'] ?? '';
-			$join     = $clauses['join'] ?? '';
-			$orderby  = $clauses['orderby'] ?? '';
+			$where   = $clauses['where'] ?? '';
+			$groupby = $clauses['groupby'] ?? '';
+			$join    = $clauses['join'] ?? '';
+			/*
+			 * Keep the ORDER BY built above, and any change the 'posts_orderby_request'
+			 * filter made to it, when this filter returns no 'orderby' of its own.
+			 * Dropping it here would leave the query with no ORDER BY at all.
+			 */
+			$orderby  = $clauses['orderby'] ?? $orderby;
 			$distinct = $clauses['distinct'] ?? '';
 			$fields   = $clauses['fields'] ?? '';
 			$limits   = $clauses['limits'] ?? '';
@@ -3267,8 +3356,18 @@ public function get_posts() {
 		}
 
 		if ( $query_vars['cache_results'] && $id_query_is_cacheable ) {
-			$new_request = str_replace( $fields, "{$wpdb->posts}.*", $this->request );
-			$cache_key   = $this->generate_cache_key( $query_vars, $new_request );
+			/*
+			 * Normalize the selected columns so that queries differing only in 'fields'
+			 * share a cache key. Only the first occurrence is replaced: the same column
+			 * names also appear in ORDER BY, and rewriting them there would give the
+			 * same query two different keys.
+			 */
+			$pos         = strpos( $this->request, $fields );
+			$new_request = false === $pos
+				? $this->request
+				: substr_replace( $this->request, "{$wpdb->posts}.*", $pos, strlen( $fields ) );
+
+			$cache_key = $this->generate_cache_key( $query_vars, $new_request );
 
 			$cache_found = false;
 			if ( null === $this->posts ) {

tests/phpunit/tests/admin/wpPrivacyRequestsTable.php

@@ -99,7 +99,15 @@ public function test_columns_should_be_sortable( $order, $orderby, $search, $exp
 		unset( $_REQUEST['orderby'] );
 		unset( $_REQUEST['s'] );
 
-		$this->assertStringContainsString( "ORDER BY {$wpdb->posts}.{$expected}", $this->sql );
+		$expected_query = explode( ', ', $expected );
+		$expected_query = array_map(
+			function ( $item ) use ( $wpdb ) {
+				return "{$wpdb->posts}.{$item}";
+			},
+			$expected_query
+		);
+
+		$this->assertStringContainsString( 'ORDER BY ' . implode( ', ', $expected_query ), $this->sql );
 	}
 
 	/**
@@ -136,42 +144,42 @@ public function data_columns_should_be_sortable() {
 				'order'    => null,
 				'orderby'  => null,
 				's'        => null,
-				'expected' => 'post_date DESC',
+				'expected' => 'post_date DESC, ID DESC',
 			),
 			// Default order (ID) DESC.
 			array(
 				'order'    => '',
 				'orderby'  => '',
 				's'        => '',
-				'expected' => 'post_date DESC',
+				'expected' => 'post_date DESC, ID DESC',
 			),
 			// Order by requester (post_title) ASC.
 			array(
 				'order'    => 'ASC',
 				'orderby'  => 'requester',
 				's'        => '',
-				'expected' => 'post_title ASC',
+				'expected' => 'post_title ASC, ID ASC',
 			),
 			// Order by requester (post_title) DESC.
 			array(
 				'order'    => 'DESC',
 				'orderby'  => 'requester',
 				's'        => null,
-				'expected' => 'post_title DESC',
+				'expected' => 'post_title DESC, ID DESC',
 			),
 			// Order by requested (post_date) ASC.
 			array(
 				'order'    => 'ASC',
 				'orderby'  => 'requested',
 				's'        => null,
-				'expected' => 'post_date ASC',
+				'expected' => 'post_date ASC, ID ASC',
 			),
 			// Order by requested (post_date) DESC.
 			array(
 				'order'    => 'DESC',
 				'orderby'  => 'requested',
 				's'        => null,
-				'expected' => 'post_date DESC',
+				'expected' => 'post_date DESC, ID DESC',
 			),
 			// Search and order by relevance.
 			array(
@@ -185,14 +193,14 @@ public function data_columns_should_be_sortable() {
 				'order'    => 'ASC',
 				'orderby'  => 'requester',
 				's'        => 'foo',
-				'expected' => 'post_title ASC',
+				'expected' => 'post_title ASC, ID ASC',
 			),
 			// Search and order by requested (post_date) ASC.
 			array(
 				'order'    => 'ASC',
 				'orderby'  => 'requested',
 				's'        => 'foo',
-				'expected' => 'post_date ASC',
+				'expected' => 'post_date ASC, ID ASC',
 			),
 		);
 	}