-
Couldn't load subscription status.
- Fork 3.1k
WP_Query: force deterministic ordering #10262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
WP_Query: force deterministic ordering #10262
Conversation
8f83b7e to
85339ff
Compare
src/wp-includes/class-wp-query.php
Outdated
| 'post_date', | ||
| 'post_title', | ||
| 'post_modified', | ||
| 'post_mime_type', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to test #10135
| $orderby = ''; | ||
| } else { | ||
| $orderby_array = array(); | ||
| /* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterwilsoncc When you get a spare moment (can be after 6.9 or whenever you have head space) could you sanity check this approach for me?
The TL;DR is:
When multiple posts have identical values for the primary sort field (like post_date, post_title, menu_order), the database doesn't guarantee consistent ordering across pagination.
This causes inconsistent pagination results, mainly in the form of dupes.
The solution here (and in all the other attempts from 6 years ago) has been to automatically add ID as a secondary sort field when ordering by fields that can have duplicate values. This ensures records with identical primary sort values always appear in the same order.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
src/wp-includes/class-wp-query.php
Outdated
| $orderby = "{$wpdb->posts}.post_date " . $query_vars['order'] . ', ' . "{$wpdb->posts}.ID " . $query_vars['order']; | ||
| } elseif ( $needs_deterministic_orderby && ! $has_id_orderby ) { | ||
| // Add ID as tie-breaker for deterministic ordering | ||
| $orderby .= ", {$wpdb->posts}.ID " . $query_vars['order']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO - don't forget search below. Might need it as well.
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
| $new_request = $this->request; | ||
| // Split SQL into parts. | ||
| $parts = explode( 'ORDER BY', $new_request ); | ||
| if ( count( $parts ) === 2 ) { | ||
| // Replace only in the SELECT part, preserve ORDER BY. | ||
| $select_part = str_replace( $fields, "{$wpdb->posts}.*", $parts[0] ); | ||
| $new_request = $select_part . 'ORDER BY' . $parts[1]; | ||
| } else { | ||
| // No ORDER BY clause, safe to replace. | ||
| $new_request = str_replace( $fields, "{$wpdb->posts}.*", $new_request ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current str_replace is too greedy in that it will replace strings in the ORDER BY clause as well.
…D as a secondary sort field. This change addresses potential duplicate records across pages when multiple posts share the same value for a field. A list of fields requiring deterministic ordering has been introduced to improve query consistency
…nding ID as a secondary sort field. Update related unit tests to reflect changes in expected SQL output for various orderby scenarios
…unit tests for orderby scenarios.
…and ensure ID is consistently used as a tie-breaker. Update unit tests to reflect changes in expected SQL output for various orderby cases.
…secondary sort field in various scenarios. Update unit tests to reflect changes in expected SQL output for orderby cases, enhancing determinism in query results.
…or consistent cache key generation. This change ensures deterministic results when 'date' is specified as the orderby value, improving query consistency.
9c77315 to
03ff908
Compare
…osts with identical dates, ensuring deterministic ordering in adjacent post queries.
| * @param WP_Post $post WP_Post object. | ||
| */ | ||
| $where = apply_filters( "get_{$adjacent}_post_where", $wpdb->prepare( "WHERE p.post_date $op %s AND p.post_type = %s $where", $current_post_date, $post->post_type ), $in_same_term, $excluded_terms, $taxonomy, $post ); | ||
| $where = apply_filters( "get_{$adjacent}_post_where", $wpdb->prepare( "WHERE (p.post_date $op %s OR (p.post_date = %s AND p.ID $op %d)) AND p.post_type = %s $where", $current_post_date, $current_post_date, $post->ID, $post->post_type ), $in_same_term, $excluded_terms, $taxonomy, $post ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a separate patch.
I'm just testing things out for https://core.trac.wordpress.org/ticket/8107
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| $post_ids[] = self::factory()->post->create( | ||
| array( | ||
| 'post_title' => $identical_title, | ||
| 'post_date' => "2023-01-0$i 10:00:00", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will end up with the dates 2023-01-010 through 2023-01-015 -- you could use str_pad( (string) $i, 2, '0', STR_PAD_LEFT ) instead
| * When multiple posts have the same value for a field, add ID as secondary sort to guarantee consistent ordering. | ||
| * Note: this is to circumvent a bug that is currently being tracked in https://core.trac.wordpress.org/ticket/44349. | ||
| */ | ||
| $fields_requiring_deterministic_orderby = array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meta data (both values and keys) can be identical too. That will be a little messy because orderby also accepts the array keys in $meta_query.
The tickets are very long, so I'm wondering if it was considered to add the ID to the ordering if it wasn't included in the parameter already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so I'm wondering if it was considered to add the ID to the ordering if it wasn't included in the parameter already.
Thanks, do you mean just include ID by default?
The whitelist is a bit janky and will add maintenance burden. I just did it as a very naive, first version to get a discussion going.
A previous, much older patch used a black list 'ID', 'rand', 'relevance'.
Not sure it would address the maintenance burden side of things, but it'd be more permissive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use some shared fixtures in this class to avoid creating the posts in each test for the ascending and descending equivalents.
For ease, you might want to register some post types wptests_time_ident, wptests_title_ident
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh! I'll look into it, cheers.
I'm testing out an approach to fix a long-standing issue:
https://core.trac.wordpress.org/ticket/47642
https://core.trac.wordpress.org/ticket/52907
https://core.trac.wordpress.org/ticket/64042
https://core.trac.wordpress.org/ticket/44349
https://core.trac.wordpress.org/ticket/8107 (17 years! 🏅 )
Unit tests to come once I've validated the approach.
This change addresses potential duplicate records across pages when multiple posts share the same value for a field.
It updates WP_Query ordering to ensure deterministic results by adding ID as a secondary sort field.