Duplicate query in WP_Query related to sticky post. #5295
Conversation
|
One nice side effect of this change, is this line will not have an empty string. |
kt-12
changed the title
| * @covers WP_Query::generate_cache_key | ||
| * @dataProvider data_duplicate_query_when_sticky_post |
There was a problem hiding this comment.
| * @covers WP_Query::generate_cache_key | |
| * @dataProvider data_duplicate_query_when_sticky_post | |
| * @covers WP_Query::generate_cache_key | |
| * | |
| * @dataProvider data_duplicate_query_when_sticky_post |
There was a problem hiding this comment.
At minimum, I think we need tests that show this failing in trunk before I would feel confident making this change. However, I left a few other observations/questions. I'd love to get a 2nd set of 👀 from @peterwilsoncc or someone else who has spent more time in this part of WP_Query.
| if ( empty( $post_type ) ) { | ||
| $post_type_prime = 'any'; | ||
| if ( $this->is_attachment ) { | ||
| $post_type = 'attachment'; |
There was a problem hiding this comment.
Are we sure that altering the value of the $post_type variable here is safe? Seems to be used elsewhere in this function. For example, it looks like it could cause side effects for the capabilities checks @peterwilsoncc added in 51276.
There was a problem hiding this comment.
In my testing, it wasn't a issue. If anything it would be a posative, as post type being empty doesn't really help ina lot of code paths.
| @@ -3533,7 +3535,7 @@ public function get_posts() { | |||
|
|
|||
| if ( $q['cache_results'] ) { | |||
| if ( $is_unfiltered_query && $unfiltered_posts === $this->posts ) { | |||
| update_post_caches( $this->posts, $post_type, $q['update_post_term_cache'], $q['update_post_meta_cache'] ); | |||
| update_post_caches( $this->posts, $post_type_prime, $q['update_post_term_cache'], $q['update_post_meta_cache'] ); | |||
There was a problem hiding this comment.
This will cause any previous cases that would have passed a null value for $post_type (which would evaluate as post) to now be evaluated as any. This is probably fine, but want to ensure this is an intentional change.
There was a problem hiding this comment.
@joemcgill Before, any empty value would have been converted to 'any' in update_post_caches.
See
wordpress-develop/src/wp-includes/post.php
Lines 7331 to 7333 in fe1df40
There was a problem hiding this comment.
For most empty values that's true. But null would have been converted to the default param value of 'post'. Again, probably fine, just wanted to make that observation clear.
| * @covers WP_Query::generate_cache_key | ||
| * @dataProvider data_duplicate_query_when_sticky_post | ||
| */ | ||
| public function test_duplicate_query_when_sticky_post( $is_cached_prior, $expected_num_queries ) { |
There was a problem hiding this comment.
I tried running these tests in trunk without the rest of the changes applied and they're already passing, which makes me think that this isn't an effective test.
There was a problem hiding this comment.
This case should pass before and after. That is the point, to show the behaviour didn't change. This test just covers a code path that was not covered before.
There was a problem hiding this comment.
Thanks, Jonny. In that case, I think we need another test case that demonstrates the bug and confirms that this change fixes the issue.
|
Committed in https://core.trac.wordpress.org/changeset/58122 |
Trac ticket: https://core.trac.wordpress.org/ticket/59442
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.