diff --git a/src/wp-includes/class-wp-query.php b/src/wp-includes/class-wp-query.php index 9385ae832ff66..d60e3e9b3049e 100644 --- a/src/wp-includes/class-wp-query.php +++ b/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 ) { diff --git a/tests/phpunit/tests/admin/wpPrivacyRequestsTable.php b/tests/phpunit/tests/admin/wpPrivacyRequestsTable.php index 66e3e02501cfb..1fc58f88e2059 100644 --- a/tests/phpunit/tests/admin/wpPrivacyRequestsTable.php +++ b/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', ), ); } diff --git a/tests/phpunit/tests/query/deterministicOrdering.php b/tests/phpunit/tests/query/deterministicOrdering.php new file mode 100644 index 0000000000000..42aa664e870c0 --- /dev/null +++ b/tests/phpunit/tests/query/deterministicOrdering.php @@ -0,0 +1,799 @@ +post->create( + array( + 'post_title' => "Mixed status $i", + 'post_date' => '2023-01-01 10:00:00', + 'post_status' => ( 0 === $i % 2 ) ? 'private' : 'publish', + ) + ); + } + + // menu_order has no index at all, so ties in it are never ordered. + for ( $i = 1; $i <= 20; $i++ ) { + self::$menu_order_ids[] = $factory->post->create( + array( + 'post_type' => 'page', + 'post_title' => "Page $i", + 'menu_order' => 0, + ) + ); + } + + for ( $i = 1; $i <= 20; $i++ ) { + self::$same_title_ids[] = $factory->post->create( + array( + 'post_title' => 'Same title', + 'post_date' => '2023-02-' . str_pad( (string) $i, 2, '0', STR_PAD_LEFT ) . ' 10:00:00', + ) + ); + } + } + + /** + * Returns the post IDs on one page of a query. + * + * @param array $args Query arguments. 'posts_per_page' and 'paged' are set by the caller. + * @return int[] Post IDs, in the order the query returned them. + */ + private function get_page_of_ids( $args ) { + $query = new WP_Query( $args ); + return wp_list_pluck( $query->posts, 'ID' ); + } + + /** + * Asserts that paging through a query returns every post exactly once. + * + * @param array $args Query arguments, without 'posts_per_page' or 'paged'. + * @param int $per_page Posts per page. + * @param int $pages Number of pages to walk. + * @param int $expected Total number of posts expected across those pages. + * @param string $message Message describing the ordering under test. + */ + private function assertPagesDoNotRepeatPosts( $args, $per_page, $pages, $expected, $message ) { + $seen = array(); + + for ( $page = 1; $page <= $pages; $page++ ) { + $seen = array_merge( + $seen, + $this->get_page_of_ids( + array_merge( + $args, + array( + 'posts_per_page' => $per_page, + 'paged' => $page, + ) + ) + ) + ); + } + + $this->assertSameSets( array_unique( $seen ), $seen, $message . ': a post appeared on more than one page' ); + $this->assertCount( $expected, array_unique( $seen ), $message . ': the pages did not add up to every post' ); + } + + /** + * Ordering by date is stable when posts share a date. + * + * Two statuses are queried together so the database sorts the rows itself + * rather than reading them from an index that already ends in ID. + * + * @ticket 44349 + */ + public function test_paging_by_date_returns_each_post_once() { + $this->assertPagesDoNotRepeatPosts( + array( + 'post_type' => 'post', + 'post_status' => array( 'publish', 'private' ), + 'post__in' => self::$mixed_status_ids, + 'orderby' => 'date', + 'order' => 'DESC', + ), + 10, + 2, + 20, + 'Ordering by date' + ); + } + + /** + * Ordering by menu_order is stable when posts share a menu_order. + * + * @ticket 44349 + * @ticket 46294 + */ + public function test_paging_by_menu_order_returns_each_post_once() { + $this->assertPagesDoNotRepeatPosts( + array( + 'post_type' => 'page', + 'post__in' => self::$menu_order_ids, + 'orderby' => 'menu_order', + 'order' => 'ASC', + ), + 10, + 2, + 20, + 'Ordering by menu_order' + ); + } + + /** + * Ordering by title is stable when posts share a title. + * + * @ticket 44349 + */ + public function test_paging_by_title_returns_each_post_once() { + $this->assertPagesDoNotRepeatPosts( + array( + 'post_type' => 'post', + 'post__in' => self::$same_title_ids, + 'orderby' => 'title', + 'order' => 'ASC', + ), + 10, + 2, + 20, + 'Ordering by title' + ); + } + + /** + * Ordering by a meta value is stable when posts share the value. + * + * @ticket 44349 + */ + public function test_paging_by_meta_value_returns_each_post_once() { + global $wpdb; + + $post_ids = array(); + for ( $i = 1; $i <= 12; $i++ ) { + $post_id = self::factory()->post->create( array( 'post_title' => "Meta post $i" ) ); + add_post_meta( $post_id, 'shared_value', 'identical' ); + $post_ids[] = $post_id; + } + + $args = array( + 'post_type' => 'post', + 'post__in' => $post_ids, + 'meta_key' => 'shared_value', + 'orderby' => 'meta_value', + 'order' => 'ASC', + ); + + $this->assertPagesDoNotRepeatPosts( $args, 5, 3, 12, 'Ordering by meta_value' ); + + // The ID clause follows the meta clause in the SQL. + $query = new WP_Query( array_merge( $args, array( 'posts_per_page' => 5 ) ) ); + $this->assertStringContainsString( ".meta_value ASC, {$wpdb->posts}.ID ASC", $query->request ); + } + + /** + * The same query run twice returns the same page in the same order. + * + * @ticket 44349 + */ + public function test_repeating_a_query_returns_the_same_page() { + $args = array( + 'post_type' => 'page', + 'post__in' => self::$menu_order_ids, + 'orderby' => 'menu_order', + 'order' => 'ASC', + 'posts_per_page' => 10, + 'paged' => 1, + 'cache_results' => false, // Make the second run hit the database, not the query cache. + ); + + $this->assertSame( + $this->get_page_of_ids( $args ), + $this->get_page_of_ids( $args ), + 'Running the same query twice returned a different page' + ); + } + + /** + * The ID clause sorts the same way as the column above it. + * + * @ticket 44349 + * + * @dataProvider data_orderby_directions + * + * @param array $args Query arguments. + * @param string $expected Expected ORDER BY clause, with {posts} standing in for the table name. + */ + public function test_id_clause_follows_the_sort_direction( $args, $expected ) { + global $wpdb; + + $query = new WP_Query( array_merge( $args, array( 'posts_per_page' => 5 ) ) ); + + $this->assertStringContainsString( + 'ORDER BY ' . str_replace( '{posts}', $wpdb->posts, $expected ), + $query->request + ); + } + + /** + * Data provider. + * + * @return array[] + */ + public function data_orderby_directions() { + return array( + 'descending date' => array( + array( + 'orderby' => 'date', + 'order' => 'DESC', + ), + '{posts}.post_date DESC, {posts}.ID DESC', + ), + 'ascending date' => array( + array( + 'orderby' => 'date', + 'order' => 'ASC', + ), + '{posts}.post_date ASC, {posts}.ID ASC', + ), + 'no orderby given' => array( + array(), + '{posts}.post_date DESC, {posts}.ID DESC', + ), + 'ascending menu_order' => array( + array( + 'orderby' => 'menu_order', + 'order' => 'ASC', + ), + '{posts}.menu_order ASC, {posts}.ID ASC', + ), + 'two columns' => array( + array( + 'orderby' => array( + 'title' => 'DESC', + 'date' => 'ASC', + ), + ), + '{posts}.post_title DESC, {posts}.post_date ASC, {posts}.ID ASC', + ), + 'unparseable column' => array( + array( 'orderby' => 'a_column_that_does_not_exist' ), + '{posts}.post_date DESC, {posts}.ID DESC', + ), + ); + } + + /** + * Ordering that already fixes the sequence gets no extra ID clause. + * + * @ticket 44349 + * + * @dataProvider data_orderby_that_fixes_the_sequence + * + * @param array $args Query arguments. + */ + public function test_orderby_id_gets_no_extra_id_clause( $args ) { + global $wpdb; + + $query = new WP_Query( array_merge( $args, array( 'posts_per_page' => 5 ) ) ); + + preg_match( '/ORDER BY(.*?)LIMIT/s', $query->request, $matches ); + $orderby = isset( $matches[1] ) ? $matches[1] : ''; + + $this->assertSame( + 1, + substr_count( $orderby, "{$wpdb->posts}.ID" ), + 'The ID appeared more than once in the ORDER BY' + ); + } + + /** + * Data provider. + * + * @return array[] + */ + public function data_orderby_that_fixes_the_sequence() { + return array( + 'ID' => array( + array( + 'orderby' => 'ID', + 'order' => 'ASC', + ), + ), + 'ID, descending' => array( + array( + 'orderby' => 'ID', + 'order' => 'DESC', + ), + ), + 'ID given as an array' => array( array( 'orderby' => array( 'ID' => 'DESC' ) ) ), + 'ID named after another' => array( + array( + 'orderby' => 'title ID', + 'order' => 'ASC', + ), + ), + 'ID named before another' => array( + array( + 'orderby' => array( + 'ID' => 'DESC', + 'title' => 'ASC', + ), + ), + ), + ); + } + + /** + * Random ordering is left alone. + * + * A seed is passed to get the same shuffle back on every page, which sorting + * by ID afterwards would undo. + * + * @ticket 44349 + * + * @dataProvider data_random_orderby + * + * @param string $orderby The 'orderby' value. + */ + public function test_random_ordering_gets_no_id_clause( $orderby ) { + global $wpdb; + + $query = new WP_Query( + array( + 'orderby' => $orderby, + 'posts_per_page' => 5, + ) + ); + + preg_match( '/ORDER BY(.*?)LIMIT/s', $query->request, $matches ); + + $this->assertStringNotContainsString( "{$wpdb->posts}.ID", $matches[1] ); + } + + /** + * Data provider. + * + * @return array[] + */ + public function data_random_orderby() { + return array( + 'unseeded' => array( 'rand' ), + 'seeded' => array( 'RAND(5)' ), + 'seeded with zero' => array( 'RAND(0)' ), + 'seeded, lower case' => array( 'rand(5)' ), + 'seeded, large number' => array( 'RAND(99999999999)' ), + ); + } + + /** + * A seed returns the same shuffle on every page. + * + * @ticket 44349 + */ + public function test_a_random_seed_gives_the_same_order_each_time() { + $args = array( + 'post_type' => 'page', + 'post__in' => self::$menu_order_ids, + 'orderby' => 'RAND(5)', + 'posts_per_page' => 5, + 'paged' => 1, + ); + + $this->assertSame( + $this->get_page_of_ids( $args ), + $this->get_page_of_ids( $args ), + 'A seeded random order changed between two runs of the same query' + ); + } + + /** + * An explicit list of IDs keeps the order it was given in. + * + * @ticket 44349 + */ + public function test_post__in_keeps_its_own_order() { + $ids = array_slice( self::$menu_order_ids, 0, 5 ); + shuffle( $ids ); + + $query = new WP_Query( + array( + 'post_type' => 'page', + 'post__in' => $ids, + 'orderby' => 'post__in', + 'posts_per_page' => 5, + ) + ); + + $this->assertSame( $ids, wp_list_pluck( $query->posts, 'ID' ) ); + } + + /** + * Ordering by a list of parents or slugs still gets the ID clause. + * + * Unlike post__in, these lists do not order posts uniquely: several posts can + * share one parent, or one slug across post types. + * + * @ticket 44349 + */ + public function test_field_orderings_get_the_id_clause() { + $parent = self::factory()->post->create( + array( + 'post_type' => 'page', + 'post_title' => 'FIELD parent', + ) + ); + self::factory()->post->create( + array( + 'post_type' => 'page', + 'post_parent' => $parent, + ) + ); + self::factory()->post->create( + array( + 'post_type' => 'page', + 'post_parent' => $parent, + ) + ); + + $query = new WP_Query( + array( + 'post_type' => 'page', + 'post_parent__in' => array( $parent ), + 'orderby' => 'post_parent__in', + 'posts_per_page' => 5, + ) + ); + + global $wpdb; + preg_match( '/ORDER BY(.*?)LIMIT/s', $query->request, $matches ); + + $this->assertStringContainsString( "FIELD( {$wpdb->posts}.post_parent,", $matches[1] ); + $this->assertStringContainsString( "{$wpdb->posts}.ID", $matches[1] ); + } + + /** + * Paging posts that all share one parent returns each post exactly once. + * + * FIELD( wp_posts.post_parent, ... ) gives every child of the same parent the + * same sort value, so without the ID clause the whole result set is one tie. + * + * @ticket 44349 + */ + public function test_paging_by_post_parent__in_returns_each_post_once() { + $parent = self::factory()->post->create( + array( + 'post_type' => 'page', + 'post_title' => 'Shared parent', + ) + ); + + $children = array(); + for ( $i = 1; $i <= 12; $i++ ) { + $children[] = self::factory()->post->create( + array( + 'post_type' => 'page', + 'post_title' => "Child $i", + 'post_parent' => $parent, + ) + ); + } + + $this->assertPagesDoNotRepeatPosts( + array( + 'post_type' => 'page', + 'post_parent__in' => array( $parent ), + 'orderby' => 'post_parent__in', + ), + 5, + 3, + 12, + 'Ordering by post_parent__in' + ); + } + + /** + * An 'orderby' of 'none' still produces no ORDER BY. + * + * @ticket 44349 + * + * @dataProvider data_orderby_that_blanks_the_clause + * + * @param mixed $orderby The 'orderby' value. + */ + public function test_orderby_can_still_be_blanked( $orderby ) { + $query = new WP_Query( + array( + 'orderby' => $orderby, + 'posts_per_page' => 5, + ) + ); + + $this->assertStringNotContainsString( 'ORDER BY', $query->request ); + } + + /** + * Data provider. + * + * @return array[] + */ + public function data_orderby_that_blanks_the_clause() { + return array( + 'the string none' => array( 'none' ), + 'none as an array key' => array( array( 'none' => 'DESC' ) ), + 'an empty array' => array( array() ), + 'false' => array( false ), + 'an array of only invalid fields' => array( array( 'a_column_that_does_not_exist' => 'ASC' ) ), + ); + } + + /** + * 'none' next to real columns does not blank the ordering. + * + * get_pages( array( 'sort_column' => 'post_title,none' ) ) produces + * array( 'post_title' => ..., 'none' => ... ); only the 'none' part is + * dropped, the rest orders as requested. + * + * @ticket 44349 + */ + public function test_none_beside_real_columns_keeps_the_ordering() { + global $wpdb; + + $query = new WP_Query( + array( + 'orderby' => array( + 'title' => 'ASC', + 'none' => 'DESC', + ), + 'posts_per_page' => 5, + ) + ); + + $this->assertStringContainsString( + "ORDER BY {$wpdb->posts}.post_title ASC, {$wpdb->posts}.ID ASC", + $query->request + ); + } + + /** + * get_pages() can still ask for no ordering. + * + * It passes 'sort_column' through as an array key rather than a bare string. + * + * @ticket 44349 + */ + public function test_get_pages_can_still_ask_for_no_ordering() { + global $wpdb; + + get_pages( array( 'sort_column' => 'none' ) ); + + $this->assertStringNotContainsString( 'ORDER BY', $wpdb->last_query ); + } + + /** + * Clause filters are handed the ORDER BY that will actually run. + * + * @ticket 44349 + * + * @dataProvider data_orderby_filters + * + * @param string $filter Name of the filter under test. + */ + public function test_filters_receive_the_final_orderby( $filter ) { + global $wpdb; + + $received = null; + $is_array = str_contains( $filter, 'clauses' ); + + $callback = static function ( $value ) use ( &$received, $is_array ) { + if ( null === $received ) { + $received = $is_array ? $value['orderby'] : $value; + } + return $value; + }; + + add_filter( $filter, $callback ); + new WP_Query( + array( + 'orderby' => 'date', + 'order' => 'ASC', + 'posts_per_page' => 5, + ) + ); + remove_filter( $filter, $callback ); + + $this->assertSame( "{$wpdb->posts}.post_date ASC, {$wpdb->posts}.ID ASC", $received ); + } + + /** + * Data provider. + * + * @return array[] + */ + public function data_orderby_filters() { + return array( + 'posts_orderby' => array( 'posts_orderby' ), + 'posts_clauses' => array( 'posts_clauses' ), + 'posts_orderby_request' => array( 'posts_orderby_request' ), + 'posts_clauses_request' => array( 'posts_clauses_request' ), + ); + } + + /** + * A filtered ORDER BY is used exactly as the filter returned it. + * + * Nothing is appended afterwards, so a filter cannot be handed back SQL it + * did not write. + * + * @ticket 44349 + */ + public function test_a_filtered_orderby_is_used_verbatim() { + global $wpdb; + + $callback = static function () use ( $wpdb ) { + return "{$wpdb->posts}.post_title ASC"; + }; + + add_filter( 'posts_orderby', $callback ); + $query = new WP_Query( array( 'posts_per_page' => 5 ) ); + remove_filter( 'posts_orderby', $callback ); + + $this->assertStringContainsString( "ORDER BY {$wpdb->posts}.post_title ASC", $query->request ); + $this->assertStringNotContainsString( "post_title ASC, {$wpdb->posts}.ID", $query->request ); + } + + /** + * A filter returning an unchanged clause keeps the ID clause. + * + * Incidental changes such as added trailing whitespace must not matter: the + * ID clause is part of the value the filter receives, not compared against it. + * + * @ticket 44349 + * + * @dataProvider data_filters_that_change_nothing + * + * @param callable $callback Filter callback. + */ + public function test_a_filter_that_changes_nothing_keeps_the_id_clause( $callback ) { + global $wpdb; + + add_filter( 'posts_orderby', $callback ); + $query = new WP_Query( + array( + 'orderby' => 'date', + 'order' => 'ASC', + 'posts_per_page' => 5, + ) + ); + remove_filter( 'posts_orderby', $callback ); + + $this->assertStringContainsString( "{$wpdb->posts}.ID ASC", $query->request ); + } + + /** + * Data provider. + * + * @return array[] + */ + public function data_filters_that_change_nothing() { + return array( + 'returns the value it was given' => array( + static function ( $orderby ) { + return $orderby; + }, + ), + 'adds a trailing space' => array( + static function ( $orderby ) { + return $orderby . ' '; + }, + ), + ); + } + + /** + * The ORDER BY survives a posts_clauses_request filter that leaves it out. + * + * @ticket 44349 + */ + public function test_orderby_survives_a_clauses_filter_that_omits_it() { + global $wpdb; + + $callback = static function ( $clauses ) { + unset( $clauses['orderby'] ); + return $clauses; + }; + + add_filter( 'posts_clauses_request', $callback ); + $query = new WP_Query( array( 'posts_per_page' => 5 ) ); + remove_filter( 'posts_clauses_request', $callback ); + + $this->assertStringContainsString( + "ORDER BY {$wpdb->posts}.post_date DESC, {$wpdb->posts}.ID DESC", + $query->request + ); + } + + /** + * posts_orderby_request survives a later filter that returns no ordering. + * + * @ticket 44349 + */ + public function test_orderby_request_survives_a_later_clauses_filter() { + global $wpdb; + + $set_orderby = static function () use ( $wpdb ) { + return "{$wpdb->posts}.post_title ASC"; + }; + $drop_orderby = static function ( $clauses ) { + unset( $clauses['orderby'] ); + return $clauses; + }; + + add_filter( 'posts_orderby_request', $set_orderby ); + add_filter( 'posts_clauses_request', $drop_orderby ); + $query = new WP_Query( array( 'posts_per_page' => 5 ) ); + remove_filter( 'posts_orderby_request', $set_orderby ); + remove_filter( 'posts_clauses_request', $drop_orderby ); + + $this->assertStringContainsString( "ORDER BY {$wpdb->posts}.post_title ASC", $query->request ); + } + + /** + * Searching still orders by relevance first. + * + * @ticket 44349 + */ + public function test_search_relevance_still_comes_first() { + global $wpdb; + + $query = new WP_Query( + array( + 's' => 'Same title', + 'posts_per_page' => 5, + ) + ); + + $this->assertStringContainsString( 'ORDER BY (CASE WHEN', $query->request ); + $this->assertStringContainsString( "{$wpdb->posts}.ID DESC", $query->request ); + } +} diff --git a/tests/phpunit/tests/rest-api/rest-attachments-controller.php b/tests/phpunit/tests/rest-api/rest-attachments-controller.php index 4dd0b60172cb4..9a72c8f096ebb 100644 --- a/tests/phpunit/tests/rest-api/rest-attachments-controller.php +++ b/tests/phpunit/tests/rest-api/rest-attachments-controller.php @@ -423,6 +423,49 @@ public function test_get_items() { $this->check_get_posts_response( $response ); } + /** + * Paging through media ordered by a shared column returns each item once. + * + * Unattached uploads all have a post_parent of 0, so ordering by it leaves + * every attachment tied. The media library and its DataViews table sort on + * that column, which is where this surfaced. + * + * @ticket 44349 + * @ticket 46294 + */ + public function test_get_items_paged_by_parent_returns_each_attachment_once() { + wp_set_current_user( self::$editor_id ); + + $expected = 10; + for ( $i = 0; $i < $expected; $i++ ) { + self::factory()->attachment->create_object( + array( + 'file' => "image-$i.jpg", + 'post_parent' => 0, + 'post_mime_type' => 'image/jpeg', + 'post_status' => 'inherit', + ) + ); + } + + $seen = array(); + for ( $page = 1; $page <= 2; $page++ ) { + $request = new WP_REST_Request( 'GET', '/wp/v2/media' ); + $request->set_param( 'orderby', 'parent' ); + $request->set_param( 'order', 'asc' ); + $request->set_param( 'per_page', 5 ); + $request->set_param( 'page', $page ); + + $response = rest_get_server()->dispatch( $request ); + $this->assertSame( 200, $response->get_status() ); + + $seen = array_merge( $seen, wp_list_pluck( $response->get_data(), 'id' ) ); + } + + $this->assertSameSets( array_unique( $seen ), $seen, 'An attachment was returned on more than one page' ); + $this->assertCount( $expected, array_unique( $seen ), 'The pages did not add up to every attachment' ); + } + public function test_get_items_logged_in_editor() { wp_set_current_user( self::$editor_id ); $id1 = self::factory()->attachment->create_object( diff --git a/tests/phpunit/tests/rest-api/rest-pages-controller.php b/tests/phpunit/tests/rest-api/rest-pages-controller.php index 9717a7fcda1c6..61ededb874862 100644 --- a/tests/phpunit/tests/rest-api/rest-pages-controller.php +++ b/tests/phpunit/tests/rest-api/rest-pages-controller.php @@ -282,6 +282,44 @@ public function test_get_items_menu_order_query() { $this->assertErrorResponse( 'rest_invalid_param', $response, 400 ); } + /** + * Paging pages that share a menu_order returns each page exactly once. + * + * Pages default to menu_order 0, so a site's pages usually all tie on it. + * + * @ticket 44349 + * @ticket 46294 + */ + public function test_get_items_paged_by_menu_order_returns_each_page_once() { + $expected = 10; + for ( $i = 0; $i < $expected; $i++ ) { + self::factory()->post->create( + array( + 'post_status' => 'publish', + 'post_type' => 'page', + 'menu_order' => 0, + ) + ); + } + + $seen = array(); + for ( $page = 1; $page <= 2; $page++ ) { + $request = new WP_REST_Request( 'GET', '/wp/v2/pages' ); + $request->set_param( 'orderby', 'menu_order' ); + $request->set_param( 'order', 'asc' ); + $request->set_param( 'per_page', 5 ); + $request->set_param( 'page', $page ); + + $response = rest_get_server()->dispatch( $request ); + $this->assertSame( 200, $response->get_status() ); + + $seen = array_merge( $seen, wp_list_pluck( $response->get_data(), 'id' ) ); + } + + $this->assertSameSets( array_unique( $seen ), $seen, 'A page was returned on more than one result page' ); + $this->assertCount( $expected, array_unique( $seen ), 'The result pages did not add up to every page' ); + } + public function test_get_items_min_max_pages_query() { $request = new WP_REST_Request( 'GET', '/wp/v2/pages' ); $request->set_param( 'per_page', 0 ); diff --git a/tests/phpunit/tests/rest-api/rest-posts-controller.php b/tests/phpunit/tests/rest-api/rest-posts-controller.php index 212ddde70dd83..38a43fef488c7 100644 --- a/tests/phpunit/tests/rest-api/rest-posts-controller.php +++ b/tests/phpunit/tests/rest-api/rest-posts-controller.php @@ -488,7 +488,7 @@ public function test_get_items_include_query( $method ) { $this->assertSame( 2, $headers['X-WP-Total'], 'Failed asserting that the number of posts is correct.' ); } - $this->assertPostsOrderedBy( '{posts}.post_date DESC' ); + $this->assertPostsOrderedBy( '{posts}.post_date DESC, {posts}.ID DESC' ); // 'orderby' => 'include'. $request->set_param( 'orderby', 'include' ); @@ -544,7 +544,7 @@ public function test_get_items_orderby_author_query() { $this->assertSame( self::$editor_id, $data[1]['author'] ); $this->assertSame( self::$editor_id, $data[2]['author'] ); - $this->assertPostsOrderedBy( '{posts}.post_author DESC' ); + $this->assertPostsOrderedBy( '{posts}.post_author DESC, {posts}.ID DESC' ); } public function test_get_items_orderby_modified_query() { @@ -568,7 +568,7 @@ public function test_get_items_orderby_modified_query() { $this->assertSame( $id3, $data[1]['id'] ); $this->assertSame( $id2, $data[2]['id'] ); - $this->assertPostsOrderedBy( '{posts}.post_modified DESC' ); + $this->assertPostsOrderedBy( '{posts}.post_modified DESC, {posts}.ID DESC' ); } public function test_get_items_orderby_parent_query() { @@ -606,7 +606,7 @@ public function test_get_items_orderby_parent_query() { $this->assertSame( 0, $data[1]['parent'] ); $this->assertSame( 0, $data[2]['parent'] ); - $this->assertPostsOrderedBy( '{posts}.post_parent DESC' ); + $this->assertPostsOrderedBy( '{posts}.post_parent DESC, {posts}.ID DESC' ); } public function test_get_items_exclude_query() { @@ -976,14 +976,14 @@ public function test_get_items_order_and_orderby() { $response = rest_get_server()->dispatch( $request ); $data = $response->get_data(); $this->assertSame( 'Apple Sauce', $data[0]['title']['rendered'] ); - $this->assertPostsOrderedBy( '{posts}.post_title DESC' ); + $this->assertPostsOrderedBy( '{posts}.post_title DESC, {posts}.ID DESC' ); // 'order' => 'asc'. $request->set_param( 'order', 'asc' ); $response = rest_get_server()->dispatch( $request ); $data = $response->get_data(); $this->assertSame( 'Apple Cobbler', $data[0]['title']['rendered'] ); - $this->assertPostsOrderedBy( '{posts}.post_title ASC' ); + $this->assertPostsOrderedBy( '{posts}.post_title ASC, {posts}.ID ASC' ); // 'order' => 'asc,id' should error. $request->set_param( 'order', 'asc,id' ); @@ -1068,7 +1068,7 @@ public function test_get_items_with_orderby_slug() { // Default ORDER is DESC. $this->assertSame( 'xyz', $data[0]['slug'] ); $this->assertSame( 'abc', $data[1]['slug'] ); - $this->assertPostsOrderedBy( '{posts}.post_name DESC' ); + $this->assertPostsOrderedBy( '{posts}.post_name DESC, {posts}.ID DESC' ); } public function test_get_items_with_orderby_slugs() { @@ -1120,7 +1120,7 @@ public function test_get_items_with_orderby_relevance() { $this->assertCount( 2, $data ); $this->assertSame( $id1, $data[0]['id'] ); $this->assertSame( $id2, $data[1]['id'] ); - $this->assertPostsOrderedBy( '{posts}.post_title LIKE \'%relevant%\' DESC, {posts}.post_date DESC' ); + $this->assertPostsOrderedBy( '{posts}.post_title LIKE \'%relevant%\' DESC, {posts}.post_date DESC, {posts}.ID DESC' ); } public function test_get_items_with_orderby_relevance_two_terms() { @@ -1148,7 +1148,7 @@ public function test_get_items_with_orderby_relevance_two_terms() { $this->assertCount( 2, $data ); $this->assertSame( $id1, $data[0]['id'] ); $this->assertSame( $id2, $data[1]['id'] ); - $this->assertPostsOrderedBy( '(CASE WHEN {posts}.post_title LIKE \'%relevant content%\' THEN 1 WHEN {posts}.post_title LIKE \'%relevant%\' AND {posts}.post_title LIKE \'%content%\' THEN 2 WHEN {posts}.post_title LIKE \'%relevant%\' OR {posts}.post_title LIKE \'%content%\' THEN 3 WHEN {posts}.post_excerpt LIKE \'%relevant content%\' THEN 4 WHEN {posts}.post_content LIKE \'%relevant content%\' THEN 5 ELSE 6 END), {posts}.post_date DESC' ); + $this->assertPostsOrderedBy( '(CASE WHEN {posts}.post_title LIKE \'%relevant content%\' THEN 1 WHEN {posts}.post_title LIKE \'%relevant%\' AND {posts}.post_title LIKE \'%content%\' THEN 2 WHEN {posts}.post_title LIKE \'%relevant%\' OR {posts}.post_title LIKE \'%content%\' THEN 3 WHEN {posts}.post_excerpt LIKE \'%relevant content%\' THEN 4 WHEN {posts}.post_content LIKE \'%relevant content%\' THEN 5 ELSE 6 END), {posts}.post_date DESC, {posts}.ID DESC' ); } public function test_get_items_with_orderby_relevance_missing_search() {