Make WordPress Core

Opened 10 years ago

Last modified 7 years ago

#27282 new defect (bug)

WP_Query returns more results when there are sticky posts

Reported by: markoheijnen's profile markoheijnen Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Query Keywords: has-patch
Focuses: Cc:

Description

When doing a WP_Query like the one below it can return more then 3 post depending if there are sticky posts and if there are returned.

		$query = new WP_Query( array(
			'orderby'        => 'post__in',
			'post__in'       => $post_ids,
			'posts_per_page' => 3
		) );

Attachments (6)

27282.diff (763 bytes) - added by markoheijnen 10 years ago.
trac-27872-on-31418.diff (2.9 KB) - added by bendoh 9 years ago.
Patch to query.php to consolidate sticky post results into the main query
trac-27872-on-31418-1.diff (3.0 KB) - added by bendoh 9 years ago.
Update to previous patch that uses WP_Query object for cachability and only manipulates query clauses when there published sticky posts are discovered
27282.tests.diff (1.4 KB) - added by boonebgorges 9 years ago.
27282-tests-1.diff (2.3 KB) - added by bendoh 9 years ago.
Updated tests, including test_stickies_more_than_posts_per_page_second_page
27282-passes-tests.diff (3.1 KB) - added by bendoh 9 years ago.
Updated patch that passes all tests

Download all attachments as: .zip

Change History (23)

@markoheijnen
10 years ago

#1 @markoheijnen
10 years ago

Added an unit test for this

#2 @SergeyBiryukov
10 years ago

  • Component changed from General to Query

#3 @SergeyBiryukov
10 years ago

As noted in IRC, this depends on ignore_sticky_posts.

Appears to be a duplicate of #9300 and #11950.

#4 @markoheijnen
10 years ago

I missed #9300. I was unsure about #11950 if it was a duplicate or related one. We could close this ticket to reopen #9300. I do believe we should evaluate it this behavior is really something we want to have.

#5 @wonderboymusic
10 years ago

  • Keywords stickies needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

There are a handful of tickets related to stickies I would like to look at - if we get around to it, can move to 4.0

@bendoh
9 years ago

Patch to query.php to consolidate sticky post results into the main query

#6 @bendoh
9 years ago

  • Keywords has-patch added; needs-patch removed

There isn't a lot of traction on this ticket, but I would definitely call this a bug. It doesn't seem like a lot of people care, I'll venture a guess that in general, sticky posts have fallen out of favor. However this is acutely affecting me, so I'll take a crack at it.

My read of the code in tells me that:

  • (query.php:1738) The query given in the unit test will be considered (illogically?) to be is_home, so sticky posts will be added to it.
  • (query.php:3539-3577) Sticky posts will ever only show up if the query is_home, paged == 1, ignore_sticky_posts false.

So when you ask for 3 posts, you might get more than 3 if there are sticky posts.

The problem isn't exactly scalable either, if you have more than $posts_per_page sticky posts, you'll always get a full page of just stickies for paged=1, then paged > 1 won't have any of the sticky posts. Maybe I'm overthinking it here, and I realize this is an old feature that's falling out of favor anyway, so maybe it's entirely moot, and it looks like this has been discussed to death in #23336.

I've attached a patch against the latest trunk which processes the sticky posts earlier on in the process.

If ignore_sticky_posts is false, then the sticky posts are actually being queried are selected using the same rules that were there before: post_status='publish' and post_type=$post_type. Once those IDs are resolved, the WHERE clause is updated to add the selected sticky posts explictly, and the ORDER BY clause is prefixed to push the sticky posts to the front.

This moves all of the work for aggregating the sticky posts into the database query. An EXPLAIN on this query doesn't show anything too scary.

A bonus with this technique is that paging will work correctly with any number of sticky posts. Will need some further testing, though.

@bendoh
9 years ago

Update to previous patch that uses WP_Query object for cachability and only manipulates query clauses when there published sticky posts are discovered

#7 @bendoh
9 years ago

I've also created a gist at https://gist.github.com/bendoh/bb3d62e5e0d470f00f9a that implements this fix in a way that doesn't affect core, instead using simple query filters. This can be dropped into any theme or plugin and will fix queries with stickies so that they page correctly and don't add to the post result count.

#8 @boonebgorges
9 years ago

In 31439:

Add tests for some of WP_Query's sticky post logic.

See #27282.

#9 @boonebgorges
9 years ago

  • Keywords needs-patch added; stickies needs-unit-tests has-patch removed

bendoh - Thanks for restarting this conversation. The sticky logic in WP_Query is not great, and your patch makes a couple of important improvements. After writing some tests for the existing functionality [31439], as well as tests for what I see as the primary bugs in the current logic 27282.tests.diff, I have a couple thoughts and pieces of feedback on your patch.

First, a clearer enumeration of the bugs. The tests in [27282.tests.diff] suggest three distinct but related problems with the current implementation:

  1. When looking at the home page, stickies are jammed onto the top of the page. This increases the total number of items on the page. Say you have posts_per_page=10 and you have 3 stickies, one of which would have naturally appeared on the first page of results, and the other two of which would not have. The home query will return 12 posts. If I understand correctly, this is the particular manifestation of the bug being reported in this ticket.
  2. Directly related: Results on page 2 should be offset by the total number of stickies displayed on page 1. So if posts_per_page=10, and 2 stickies are added to the front page, the first post on the second page should be the 9th oldest post, *not* the 11th oldest.
  3. If a post is stickied which would have appeared on the first page anyway (say, the 5th oldest post), it will be removed from its normal order and put at the top of the page. If a stickied post would normally appear on page 2, it currently appears in two places: at the top of page 1, and in its normal order on page 2. This seems incorrect at a glance - posts should only appear once in the paged results.

The approach in your patch fixes all three issues. However, it fails two other tests added in [31459]:

  1. It doesn't respect 'post__not_in'. See test_stickies_should_obey_post__not_in().
  2. The stickies are being ordered differently. In the current implementation, stickies are ordered by 'post_date'. See test_stickies_should_be_included_when_home_is_true().

I'm somewhat concerned about what happens when the number of stickies is more than posts_per_page. On the current implementation, all stickies will be shown on the front page, no matter what. (See #23336 for some discussion.) With your patch, stickies will fall into the normal pagination flow, which may mean that stickies get sent to the second page and beyond. While I think that this behavior is more logical, I also think it's bound to cause some consternation. Do you have a sense of other software handles this problem? What do popular forum packages do?

@bendoh
9 years ago

Updated tests, including test_stickies_more_than_posts_per_page_second_page

@bendoh
9 years ago

Updated patch that passes all tests

#10 @bendoh
9 years ago

  • Keywords has-patch added; needs-patch removed

Heya! Thanks for writing back. I was hoping this ticket wasn't completely dead.

I went ahead and ran the unit tests and did find those failures. I went ahead and fixed them and all the tests pass now. I also added one other test, test_stickies_more_than_posts_per_page_second_page in attachment:27282-tests-1.diff which demonstrates the behavior for when the number of stickies is greater than posts_per_page, which with this patch, causes them to spill over onto the second page. I personally think this is correct behavior, but I agree that this topic definitely needs a bit more discussion as blindly implementing it could break some (very few?) sites. I'm not too familiar with what existing BB / Forum software does for this situation, so I would have to spend a bit of time investigating to get a feel for it.

See attachment:27282-tests-1.diff (am I doing this wrong, how do I drop the 'attachment:' prefix?) for the extended patch with your tests as well as the new test, and attachment:27282-passes-tests.diff for the updated patch against the wp-test repo @31448.

#11 @boonebgorges
9 years ago

Thanks for looking into it! I think you forgot to include the new test, though - I don't see it in 27282-passes-tests.diff (surround with square brackets to get the formatting)

#12 @bendoh
9 years ago

Woops, I attached it as a separate file in 27282-tests-1.diff (thanks!), the new test is called test_stickies_more_than_posts_per_page_second_page.

This ticket was mentioned in Slack in #core by bendoh. View the logs.


9 years ago

#14 follow-up: @Trimension
9 years ago

I think the whole discussion is very strange and you make the thinks much more complicate than it really is. All "solutions" are more or less workarounds but not really solutions.

The Bug(s) is(are) caused by a design error. The sticky-Bit is a property of the Post !
But it is not designed as a Property but is saved in a separate Options-List. This makes things very complicate and buggy. Hence it is impossible to consider the Flag during the Query.

Query ignores the flag and the query-method tries to put the separate sticky-information afterwards into the result. This cannot really work.

Changing the Design (add a sticky column to the post-Table) would solve all the problems discussed here. The Query itself could filter the results on the sticky bit in the right way and could also order it correctly and the pagination could do its job.

This simple design change would also bring some more benefits, because as a Post-Property it doesn't need the restriction to be only available in normal Posts but also in CPT's and it would also improve the performance because one query is better than two, Right ?!

The sticky-Information would be always present and could be used in any other logic without always querying against the Options and merging the result.

This Solution would be a great improvement of Quality and Usability with the next release.

Last edited 9 years ago by Trimension (previous) (diff)

#15 @swissspidy
8 years ago

#36720 was marked as a duplicate.

#16 in reply to: ↑ 14 @bastho
8 years ago

Replying to Trimension:

This simple design change would also bring some more benefits, because as a Post-Property it doesn't need the restriction to be only available in normal Posts but also in CPT's and it would also improve the performance because one query is better than two, Right ?!

It means a migration script which updates all concerned posts before cleaning the options table.

Maybe @bendoh 's patch can be applied while a new ticket is created to change the design.

  1. stop the bug
  2. redesign and migrate

#17 @dd32
7 years ago

#39431 was marked as a duplicate.

Note: See TracTickets for help on using tickets.