Ticket #6772 (reopened defect)

Opened 1 month ago

Last modified 1 week ago

get_posts Should Use WP_Query

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

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

get_posts_rework.diff (5.9 kB) - added by filosofo on 04/18/08 19:17:50.
test_get_posts.php (6.8 kB) - added by filosofo on 04/18/08 19:19:02.
6772.diff (0.6 kB) - added by DD32 on 05/05/08 06:42:08.
converted function to use array format for passing at the same time
6772.2.diff (0.5 kB) - added by DD32 on 05/05/08 07:07:57.
fix_get_children_get_posts.diff (2.0 kB) - added by filosofo on 05/05/08 07:22:33.
6772-absint.diff (6.5 kB) - added by mdawaffe on 05/08/08 00:45:55.
6772-absint-concat-and-parent.diff (6.9 kB) - added by mdawaffe on 05/08/08 01:03:58.

Change History

04/18/08 19:17:50 changed by filosofo

  • attachment get_posts_rework.diff added.

04/18/08 19:19:02 changed by filosofo

  • attachment test_get_posts.php added.

04/18/08 19:55:26 changed by ryan

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()?

04/18/08 20:27:55 changed by filosofo

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.

05/02/08 19:30:04 changed by ryan

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

05/02/08 19:45:12 changed by filosofo

I think it's ready to go. Thanks!

05/03/08 20:08:33 changed by ryan

  • status changed from new to closed.
  • resolution set to fixed.

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

(follow-up: ↓ 7 ) 05/05/08 06:14:05 changed by matt

  • status changed from closed to reopened.
  • resolution deleted.
  • severity changed from normal to blocker.

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

(in reply to: ↑ 6 ) 05/05/08 06:40:26 changed by DD32

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.

05/05/08 06:42:08 changed by DD32

  • attachment 6772.diff added.

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

05/05/08 06:43:38 changed by filosofo

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

05/05/08 07:07:51 changed by DD32

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.

05/05/08 07:07:57 changed by DD32

  • attachment 6772.2.diff added.

05/05/08 07:22:33 changed by filosofo

  • attachment fix_get_children_get_posts.diff added.

05/05/08 07:28:03 changed by filosofo

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"

05/05/08 15:46:33 changed by ryan

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

05/05/08 22:22:30 changed by xknown

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

05/08/08 00:45:40 changed by mdawaffe

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.

05/08/08 00:45:55 changed by mdawaffe

  • attachment 6772-absint.diff added.

05/08/08 01:03:45 changed by mdawaffe

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.

05/08/08 01:03:58 changed by mdawaffe

  • attachment 6772-absint-concat-and-parent.diff added.

05/08/08 05:17:29 changed by ryan

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