Skip to content

Set ignore_sticky_posts=true as default wp_query argument#1813

Merged
jarednova merged 12 commits intotimber:masterfrom
bartvanraaij:ignore-sticky-posts-default
Mar 26, 2019
Merged

Set ignore_sticky_posts=true as default wp_query argument#1813
jarednova merged 12 commits intotimber:masterfrom
bartvanraaij:ignore-sticky-posts-default

Conversation

@bartvanraaij
Copy link
Copy Markdown

... to prevent fetching too many posts when using stickies.

This fixes #1812 and makes sure that Timber::get_posts() produces the same output as WP get_posts() when using sticky posts.

Ticket: #1812

Issue

See my bug report: when using sticky posts; the incorrect amount of posts in returned.

Solution

I've added a method set_query_defaults to QueryIterator which is hooked at pre_get_posts. The method sets ignore_sticky_posts to true if not set otherwise.
This is the same behaviour as WP get_posts, see wp-includes/post.php:1759.

Impact

Other than fixing this bug, no impact, presumably.

Testing

I've added testGettingPostsWithStickiesReturnsCorrectAmountOfPosts in test-timber-post-getter.php
If you comment out the new add_action set_query_defaults at QueryIterator.php:26 the test will fail. With my fix, it will pass.

…fetching too many posts when using stickies
@coveralls
Copy link
Copy Markdown

coveralls commented Oct 9, 2018

Coverage Status

Coverage increased (+0.02%) to 94.34% when pulling 7740f6c on bartvanraaij:ignore-sticky-posts-default into 9bdfd92 on timber:master.

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 12, 2018

Codecov Report

Merging #1813 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1813      +/-   ##
============================================
+ Coverage     95.04%   95.05%   +0.01%     
- Complexity     1556     1563       +7     
============================================
  Files            48       48              
  Lines          3672     3682      +10     
============================================
+ Hits           3490     3500      +10     
  Misses          182      182
Impacted Files Coverage Δ Complexity Δ
lib/QueryIterator.php 93.42% <ø> (ø) 45 <0> (ø) ⬇️
lib/PostGetter.php 89.01% <100%> (+1.35%) 53 <7> (+7) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9bdfd92...7740f6c. Read the comment docs.

@jarednova
Copy link
Copy Markdown
Member

@bartvanraaij thanks for this PR. I think this reveals one of the differences between WP's get_posts vs. WP_Query.

It appears that WP's get_posts ignores stickys, while WP_Query respects it.

I (think) merging this would fix the get_posts diffs between Timber and WP, but in the process create a disparity between WP_Query and Timber\PostQuery.

Can you take a look to see if there's a way (perhaps using a $defaults array) to have both methods match WordPress default behavior?

@jarednova jarednova added the wip Work in Progress label Oct 17, 2018
@bartvanraaij
Copy link
Copy Markdown
Author

Thanks for the feedback. I will take a look at that.
I’m at a conference this week (IPC München) so it will be next week.

@palmiak
Copy link
Copy Markdown
Collaborator

palmiak commented Mar 24, 2019

@jarednova @bartvanraaij If we want to make them working the same shouldn't we also add:

  • no_found_rows
  • supress_filters
    ?

@jarednova
Copy link
Copy Markdown
Member

@palmiak agreed, while we're here we may as well get the other parts of get_posts vs WP_Query to align

@jarednova jarednova removed the wip Work in Progress label Mar 26, 2019
Copy link
Copy Markdown
Member

@jarednova jarednova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a fair amount of reorganization but @bartvanraaij's basic formation is the right one. By pulling this into PostGetter and tying it directly to the get_posts method, we've got a pretty simple framework to address #1812 and add any other get_posts vs. WP_Query quirks are out there

@jarednova
Copy link
Copy Markdown
Member

@palmiak I made some adjustments based on @bartvanraaij's code. I think this is in a good place, but I'd love your eyes in a review so we can make sure — thanks!

@palmiak
Copy link
Copy Markdown
Collaborator

palmiak commented Mar 26, 2019

It looks good @jarednova . I just saw one thing in 2.0 branch related with this, but I'll open an issue for this.

@jarednova
Copy link
Copy Markdown
Member

@palmiak yeah, good call on how/if this comes into 2.0 — let's discuss it there

@jarednova jarednova merged commit 0fc0d37 into timber:master Mar 26, 2019
@bartvanraaij
Copy link
Copy Markdown
Author

Thank you @jarednova . This totally slipped my attention.

@jarednova
Copy link
Copy Markdown
Member

all good @bartvanraaij ! Thanks again for your contribution. I'll release later this week to Composer and WP.org!

}
if ( isset($query->query) && !isset($query->query['supress_filters']) ) {
$query->set('supress_filters', true);
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be suppress_filters instead of supress_filters!!! 🚨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Timber::get_posts() return too many posts when using sticky_posts

5 participants