Make WordPress Core

Opened 9 years ago

Last modified 5 years ago

#30911 new defect (bug)

Overhaul post_status logic in WP_Query

Reported by: boonebgorges's profile boonebgorges Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Query Keywords: needs-unit-tests needs-patch
Focuses: Cc:

Description

WP_Query filters based on post_status in two different places:

  1. When building the main SQL query (SELECT ID FROM $wpdb->posts...). https://core.trac.wordpress.org/browser/tags/4.1/src/wp-includes/query.php#L2946 This block is responsible for parsing the 'post_status' query var, and rectifying these passed post_statuses with the 'perm' param, the logged-in user's permissions, and whether is_admin(). (The ability to query by 'post_status' dates from [5575].)
  1. On single queries (is_page(), is_single()), additional filtering happens *after* the query. https://core.trac.wordpress.org/browser/tags/4.1/src/wp-includes/query.php#L3511 The intended purpose of this block is to ensure that Preview works (since posts being previewed generally are not published). The current incarnation of the preview logic (ie, living in WP_Query) was introduced in [2523].

These two 'post_status' blocks have two different purposes and two different histories, but they interact in a number of problematic ways. Just a few of the problems:

  1. Querying posts with 'fields=ids' means that the post ids are returned before the second filter block gets a chance to run. As a result, certain WP_Query objects (eg 'p=123&post_status=trash') can return different post IDs depending on whether you request 'fields=ids'.
  2. Values passed to the post_status parameter of WP_Query are sometimes overridden forcibly, based on logged-in user status. In an ideal world, WP_Query would not make any essential reference to the logged-in user. Realistically, we can't change some of this behavior for reasons having to do with backward compatibility. But there are places where the current user logic could be reworked so that it provides defaults, which can then be overridden by the params passed to WP_Query. Some relevant tickets: #28568, #29167
  3. Filtering out non-previewable posts after the main query has taken place means doing more SQL queries than necessary in cases where post_status=draft and is_single().
  4. Filtering out non-previewable posts after the main query has taken place can result in certain sorts of data leakage. See eg #30910.
  5. Having two separate blocks that filter results based on post_status makes unnecessarily complicated to fix post_status bugs. See eg #30530, #22001, #23309.

My initial thought is that the second block should be removed. Preview logic should either be merged with the first block of post_status parsing logic that runs when building the main post query, or it should be moved into a separate query_var, which would then be passed when making a Preview request. In general, the challenge here is to ensure that the default post_status whitelist is properly sensitive to the permissions of the logged-in user - which sometimes means building clauses like (post_status = 'publish' OR ( post_status = 'private' AND post_author = 123 )).

I've started to write some unit tests that describe the (weird and complicated) existing behavior. See #29167 [31047].

Change History (2)

#1 @boonebgorges
9 years ago

In 31302:

In get_adjacent_post(), return private post if the current user has the capacity to read it.

This mirrors the check that happens post-query in WP_Query. See #30911.

Props bswatson.
Fixes #30287.

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


8 years ago

Note: See TracTickets for help on using tickets.