WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#6772 closed defect (bug) (fixed)

get_posts Should Use WP_Query

Reported by: filosofo Owned by:
Milestone: 2.6 Priority: normal
Severity: blocker Version: 2.6
Component: General Keywords: wp_query get_posts wpdb has-patch
Focuses: Cc:

Description

get_posts currently runs its own db queries based on the parameters it receives. Instead, it should query posts using WP_Query. Advantages:

  • It avoids redundancy. query_posts and get_posts will have similar behavior, and a change in one place won't have to be doubled in the other.
  • It adds functionality to both means of querying posts: all parameters passed to get_posts can be passed to query_posts, and vice-versa.
  • It fixes some of get_posts problems, such as those for meta_key and meta_value, which currently are broken.
  • It allows get_posts queries and results to be filtered.

To address the concern that this might be making significant changes to behavior, I've made a page template that lets you try queries in both the old get_posts and my proposed get_posts, simultaneously, with the results shown side-by-side. Just assign the attached template to a page, apply the patch, and you can see how my proposed get_posts query will work.

Attachments (7)

get_posts_rework.diff (5.9 KB) - added by filosofo 7 years ago.
test_get_posts.php (6.8 KB) - added by filosofo 7 years ago.
6772.diff (569 bytes) - added by DD32 7 years ago.
converted function to use array format for passing at the same time
6772.2.diff (522 bytes) - added by DD32 7 years ago.
fix_get_children_get_posts.diff (2.0 KB) - added by filosofo 7 years ago.
6772-absint.diff (6.5 KB) - added by mdawaffe 7 years ago.
6772-absint-concat-and-parent.diff (6.9 KB) - added by mdawaffe 7 years ago.

Download all attachments as: .zip

Change History (23)

@filosofo7 years ago

@filosofo7 years ago

comment:1 @ryan7 years ago

My concern is that this subjects get_posts() to all of the filters run in WP_Query. get_posts() has been a way to get what's actually in the posts table without plugin interference. Do we need to skip filters when being called from get_posts()?

comment:2 @filosofo7 years ago

In my view, filter-less queries should be opt-in, not default. Suppose plugin A is using get_posts naively, and plugin B is trying to filter posts, say to restrict certain ones from public view. Plugin A is going to be returning the posts Plugin B doesn't want it to, and there's nothing B can do about it except, perhaps, a regex hack on the db query.

Instead, if something really needs unfiltered posts, it should remove the filters prior to calling get_posts.

comment:3 @ryan7 years ago

Okay, I'm down with this. Any more changes you want to make before commit?

comment:4 @filosofo7 years ago

I think it's ready to go. Thanks!

comment:5 @ryan7 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [7880]) Use WP_query in get_posts(). Props filosofo. fixes #6772

comment:6 follow-up: @matt7 years ago

  • Resolution fixed deleted
  • Severity changed from normal to blocker
  • Status changed from closed to reopened

With [7879] all my galleries work perfectly, with [7880] they disappear.

comment:7 in reply to: ↑ 6 @DD327 years ago

Replying to matt:

With [7879] all my galleries work perfectly, with [7880] they disappear.

Example Query created:

SELECT   wp_posts.* FROM wp_posts  WHERE 1=1 AND wp_posts.post_parent = 137  AND (post_mime_type LIKE 'image/%')  AND wp_posts.post_type = 'attachment'  AND (wp_posts.post_status = 'publish' OR wp_posts.post_author = 2 AND wp_posts.post_status = 'private') ORDER BY menu_order,wp_posts.ID DESC

Which fails for The Gallery as attachments have a post_status of 'inherit'

I've attached a patch which causes the gallery to specifically set the post_status.

@DD327 years ago

converted function to use array format for passing at the same time

comment:8 @filosofo7 years ago

I can confirm that DD32's patch fixes the problem.

comment:9 @DD327 years ago

Also note, adjacent_image_link() is affected too.

WP_Query::is_attachment and other flags are also not available in the current case as WP_Query::parse_query() is never called.

I'm honestly not sure if theres a better place for the code addition, It looks a bit out of place to me.

@DD327 years ago

comment:10 @filosofo7 years ago

Talking with DD32 and photomatt in IRC, I've attached the fix_get_children_get_posts.diff patch, which combines DD32's 6772.diff patch with a couple of other changes:

  • The patch specifies "inherit" as the post_status for the media get_children requests
  • The patch also specifies "order" to make sure that it's preserved
  • but for backwards-compat with old queries, if the post_type is "attachment" and post_status is not defined, get_posts (the function) sets post_status to "inherit"

comment:11 @ryan7 years ago

(In [7892]) get_posts fixes from DD32 and filosofo. see #6772

comment:12 @xknown7 years ago

On a side note, I think post__not_in and post__in should be properly sanitized to avoid future SQL injection bugs.

comment:13 @mdawaffe7 years ago

6772-absint.diff

  1. Sanitizes post__in and post__not_in.
  2. Converts most of the (int) and intval() sanitation to absint(). I'm pretty sure I left all the arguments that can be negative (like cat, posts_per_page, etc.). I'm not sure if the time arguments should be allowed to be negative or not.

@mdawaffe7 years ago

comment:14 @mdawaffe7 years ago

6772-absint-concat-and-parent.diff

  1. Sanitizes post__in and post__not_in.
  2. Converts most of the (int) and intval() sanitation to absint(). I'm pretty sure I left all the arguments that can be negative (like cat, posts_per_page, etc.). I'm not sure if the time arguments should be allowed to be negative or not.
  3. Prevents WP_Query from stomping on $where variable when p, post_parent, post__in, or post__not_in argument is passed.
  4. Pulls post_parent out from a case in an elseif chain so that it can be used independently.

comment:15 @ryan7 years ago

(In [7906]) Query cleanups. Use absint, concat where instead of overwrite, make post_parent independent, sanitize postin and postnot_in. Props mdawaffe. see #6772

comment:16 @ryan7 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.