WP_Query.query with invalid post_status will return all

[carsonreinke]

Hello, I noticed an issue with WooCommerce method wc_get_products and dove into the issue.

The problem was wc_get_products(array('status' => 'published')); would return ALL posts, even trashed ones because of the "published" status is not a valid post status.

I believe that behavior is unexpected that it should return NO posts.

The issue appears to be this portion of WP_Query.get_posts that only known post statuses are added to the query:

<?php
                                foreach ( get_post_stati() as $status ) {
                                        if ( in_array( $status, $q_status, true ) ) {
                                                if ( 'private' === $status ) {
                                                        $p_status[] = "{$wpdb->posts}.post_status = '$status'";
                                                } else {
                                                        $r_status[] = "{$wpdb->posts}.post_status = '$status'";
                                                }
                                        }
                                }

I believe it would best if this method checks for any unrequested post status supplied in the query and either applies them to the query or produces an error. I would imagine this is a pretty major change.

[kishanjasani]

Hey @carsonreinke, Welcome to WordPress Core Trac! This issue seems like issue of the WooCommerce.
So you have to report it here: ​https://github.com/woocommerce/woocommerce/issues

[carsonreinke]

@kishanjasani Sorry for the confusion, my reference to WooCommerce was simply how I came to this area in WordPress.

The following below seems wrong to me, since instead of returning NOTHING, it will return ALL posts:

<?php
$q = new WP_Query( array( 'post_status' => 'publish' ) );
$q->found_posts; //All published posts

$q = new WP_Query( array( 'post_status' => 'fake status' ) );
$q->found_posts; //Should be zero, instead ALL posts

[xParham]

I also believe this is a bug in WP_Query and actually one with potentially serious consequences. I came across the bug running a wp post CLI command, in my case passing an unregistered custom post status.

But here is another case, if you simply have a typo in your command, passing pendin instead of pending here:
wp post delete $(wp post list --post_type='post' --post_status=pendin --format=ids) --force

You will end up having ALL of your posts deleted.

[metalandcoffee]

I've also confirmed this issue. I tested the bug all the way back to WordPress version 3.9 and it was still an issue. Maybe this has always been a thing?

It looks like the logic inside of WP_Query's get_posts method (wp-includes/class-wp-query.php) only accommodates for the following cases:

1. No post_status argument was provided in the new WP_query object so it generates the SQL statement with the default values ('publish' or 'private'):

Example: new WP_Query( array( 'author' => '1') );

Generated SQL:
SELECT SQL_CALC_FOUND_ROWS wp_posts.ID FROM wp_posts WHERE 1=1 AND wp_posts.post_author IN (1) AND wp_posts.post_type = 'post' AND (wp_posts.post_status = 'publish' OR wp_posts.post_status = 'private') ORDER BY wp_posts.post_date DESC LIMIT 0, 10

2. A post_status argument is provided in the new WP_query object and matches one of the available post statuses.

Example: new WP_Query( array( 'post_status' => 'publish') );

Generated SQL:
SELECT SQL_CALC_FOUND_ROWS wp_posts.ID FROM wp_posts WHERE 1=1 AND wp_posts.post_type = 'post' AND ((wp_posts.post_status = 'publish')) ORDER BY wp_posts.post_date DESC LIMIT 0, 10

It doesn't accommodate for the case where a post_status argument is provided but it doesn't match any of the available post statuses.

So in that case, because none of the conditionals are met in order to append one of the relevant post_status SQL conditonals, the following SQL is generated:
SELECT SQL_CALC_FOUND_ROWS wp_posts.ID FROM wp_posts WHERE 1=1 AND wp_posts.post_type = 'post' ORDER BY wp_posts.post_date DESC LIMIT 0, 10

Which obviously results in all posts being returned.

[metalandcoffee]

Hi again! I'd like to propose a patch although this would require some major dev feedback since I do not want to risk breaking other aspects of the WP_Query class.

Like @carsonreinke pointed out, this bug does result in posts with any post stati being returned (including trashed and auto-draft posts) so I think it's worth fixing.

In my patch, I changed from looping through the available post stati to looping through the post stati provided in the custom query. This results in the following generated SQL statement if a custom query has invalid post stati:

SELECT SQL_CALC_FOUND_ROWS wp_posts.ID FROM wp_posts WHERE 1=1 AND wp_posts.post_type = 'post' AND ((wp_posts.post_status = 'fakestatus' OR wp_posts.post_status = 'publishe')) ORDER BY wp_posts.post_date DESC LIMIT 0, 10

This results in 0 posts being returned which is what we want if we enter something invalid.

I set the milestone to 5.7 because it'd be nice to get this fix in there :)

[leogermani]

Hi, I just want to bring this other ticket to attention here:

https://core.trac.wordpress.org/ticket/48556

It's changing the behavior in the same place. It was merged into trunk but reverted due to a regression already addressed.

If you agree, I could add some unit tests there and address this issue in the same patch so we don't conflict.

Otherwise, it's better if we decide which patch goes first so the other can adapt.

[peterwilsoncc]

@leogermani I think it's a good idea to work on this and #48556 in the same patch. The code is intertwined and the bugs are similar enough.

cc @boonebgorges as you've been involved in the other ticket.

[leogermani]

Actually, I made a small confusion when I first looked at this.

This is not directly related to #48556, but rather with its counterpart #52094.

In #52094 I surface a series of inconsistencies in the status query, and I think I include this bug here as well. I will try to work on that this week.

Since it's not a small change, I'm not sure we will have it ready and tested by the next release, so I'm not sure if you guys want to go with the simple fix proposed here that addresses at least part of the problem. But as I show in the other ticket, this is just one of the several issues.

[peterwilsoncc]

With 5.7's RC1 due next week, I'm going to move this off the milestone for a later release.

[leogermani]

I've worked on a fix for this in #52094

[leogermani]

I've updated the patch in https://core.trac.wordpress.org/ticket/52094 that fixes this issue (among others)

I'd appreciate some testing and feedback