Update wcs_get_orders_with_meta_query() to mimic the post_status handling like WP_Query would

[prettyboymp]

Fixes #453 Fixes https://github.com/woocommerce/woocommerce-subscriptions/issues/4531

Description

This PR addresses the differences in how the status argument is handled from within wcs_get_orders_with_meta_query() and wcs_get_subscriptions() in HPOS and non-HPOS environments.

These differences are identified in the WCS_Functions_Test:: test_2_wcs_get_subscriptions() test.

There are three different behaviors that have to be address for HPOS.

• With HPOS enabled, Woocommerce will map the any status argument will to order statuses, not subscription statuses. [Previously fixed]

• In non-HPOS environments, WP_Query would simply ignore any invalid status when building the where clauses. The result meant that if only invalid statuses arguments were passed in, no filtering of status would be applied, but if any valid status argument was included, only subscriptions of those valid status would get returned. Mimicking this for HPOS environments requires filtering the status to only valid stati, but then passing in all valid stati if no status arguments remain.

• wcs_get_subscriptions() will convert a falsy value for $args['status'] to an empty array. In a non-HPOS environment, WP_Query will force the query to run with the default status of 'publish'. This is an invalid status for subscriptions and unless being filtered, would always return an empty result. To mimic this in HPOS environments, we're simply setting the status to 'publish' so the end argument ends up the same.

Discussion

The fixes included in this PR are somewhat hack-ish and are being used to mimic historical behavior even though that behavior may not be ideal. So I question whether we should be fixing these by making HPOS mimic the old WP_Query handling or document an expected change and have the behavior when running through WP_Query mimic the more expected behavior.

I'm not currently sure how complex it would be to the original WP_Query handling to be changed since there are some permission checks included in the handling of post_status within WP_Query.

How to test this PR

WCS_Functions_Test:: test_2_wcs_get_subscriptions() was extended to cover some more edge cases

Product impact

Fixes differences in how the status argument is handled from within wcs_get_orders_with_meta_query() and wcs_get_subscriptions() in HPOS and non-HPOS environments

• [ ] Added changelog entry (or does not apply)
• [ ] Will this PR affect WooCommerce Subscriptions? yes/no/tbc, add issue ref
• [ ] Will this PR affect WooCommerce Payments? yes/no/tbc, add issue ref
• [ ] Added deprecated functions, hooks or classes to the spreadsheet

[prettyboymp]

Overall, I'm okay with introducing these changes to make using $args['status'] with wcs_get_orders_by_meta_query() consistent across stores using HPOS and non-HPOS, however, I'm interested in learning how you ran into these problems. Was it found out by failing unit tests or some other extension code somewhere?

I came across this trying to get all the current tests to pass with HPOS enabled (without syncing). The final two sets of assertions in test_2_wcs_get_subscriptions() currently fail on HPOS in trunk.

My expectation around the status argument would be that whatever values are passed would be applied as IN clauses to the final query.

https://github.com/Automattic/woocommerce-subscriptions-core/blob/ca79702f3328d19db3c7473eb13ffdd3c4a8b67e/tests/unit/test-wcs-functions.php#L1054-L1057

The test currently expects all subscriptions to be returned because WP_Query simply ignores the invalid status so that there is no status filtering applied to the query. My expectation here would be how HPOS currently handles it, which includes the invalid status in the WHERE clause, resulting in no matching subscriptions.

I think this one is pretty edge-case and it would be okay with the difference in behavior here and document it in the test. The invalid status getting passed in could be considered a bug in the integration since it isn't validating the input prior to sending it to lower layers.

https://github.com/Automattic/woocommerce-subscriptions-core/blob/ca79702f3328d19db3c7473eb13ffdd3c4a8b67e/tests/unit/test-wcs-functions.php#L1069-L1072

This one is a bit more nuanced. Within wcs_get_subscriptions(), if a subscription_status isn't passed in, wp_parse_args() sets the default to any, and we already have handling to make the WP_Query/HPOS handling behave the same for this case.

If a falsy status gets passed in, the status argument gets turned into an empty array. In this instance, WP_Query will just default to setting the status value as publish. HPOS will currently default to setting the status as the valid order status from WooCommerce core, so there are 4 valid statuses that overlap with WCS.

I don't like the way it works with WP_Query, but I think the result of returning no matching subscriptions is correct.

The HPOS handling definitely needs to change here, though. I think we should make it so that passing in a falsy value for status is the same as passing in an invalid value for status.

However, I would also expect that if you pass in a falsy status along with a valid status, e.g. [ '', 'wc-active'], that only results matching the valid status would be returned.

I would consider this less of an edge-case situation as there may be cases where integrations default to an empty value for status instead of removing the argument from the array.

[prettyboymp]

I thought this may be easier to discuss if we hard a chart of the current (trunk) behavior around invalid status arguments:

+-----------------------------------------------------+--------------------------------------+---------------------------------------+---------------------------------------------------------------------------------------------------------------------+
| args                                                | Expected Clause                      | Current non-HPOS                      | Current HPOS                                                                                                        |
+-----------------------------------------------------+--------------------------------------+---------------------------------------+---------------------------------------------------------------------------------------------------------------------+
| [ 'subscription_status' => 'rubbish' ]              | status IN ('rubbish')                | NO STATUS CLAUSE APPLIED              | status IN ('rubbish')                                                                                               |
| [ 'subscription_status' => [ 'rubbish' ] ]          | status IN ('rubbish')                | NO STATUS CLAUSE APPLIED              | status IN ('rubbish')                                                                                               |
| [ 'subscription_status' => [ 'rubbish', 'active ] ] | status IN ( 'rubbish', 'wc-active' ) | status IN ( 'wc-active' ) *equivalent | status IN ( 'rubbish', 'wc-active' )                                                                                |
| [ 'subscription_status' => '' ]                     | status IN ('') or 'any'              | status IN ('publish')                 | status IN ('wc-pending', 'wc-processing', 'wc-on-hold', 'wc-completed', 'wc-cancelled', 'wc-refunded', 'wc-failed') |
| [ 'subscription_status' => [ '' ] ]                 | status IN ('') or 'any'              | NO STATUS CLAUSE APPLIED              | NO STATUS CLAUSE APPLIED                                                                                            |
| [ 'subscription_status' => [ '', 'active ] ]        | status IN ( '', 'wc-active' )        | status IN ( 'wc-active' ) *equivalent | status IN ( 'wc-active' ) *equivalent                                                                               |
+-----------------------------------------------------+--------------------------------------+---------------------------------------+---------------------------------------------------------------------------------------------------------------------+

Out of these, I would consider them all to be edge use-cases with the exception of 'subscription_status' => '', which I think could easily happen in an integration where status was a parameter of a wrapping function that defaulted to null or ''.

[mattallan]

Thanks very much for the detailed comment @prettyboymp!

---

For the first case:

subscriptions = wcs_get_subscriptions( array( 'subscription_status' => 'rubbish' ) ); 

I agree with your comment:

My expectation here would be how HPOS currently handles it, which includes the invalid status in the WHERE clause, resulting in no matching subscriptions.

I think this one is pretty edge-case and it would be okay with the difference in behavior here and document it in the test.

I think querying an invalid status should result in no matching subscriptions.

For the second case:

$subscriptions = wcs_get_subscriptions( array( 'subscription_status' => '' ) ); 

This is the one that feels the most incorrect given the results in your table above.

So that I'm understanding correctly, on non-HPOS stores the query is searching for subscriptions with published status which I'm guessing returns an empty result. Then on HPOS stores, it's querying all order statuses?

Definitely shouldn't be querying order statuses 🙈

I think when wcs_get_subscriptions() is passed 'subscription_status' => '', we should return an empty result when HPOS is enabled to maintain backward compatibility.

However, I think we also need to update wcs_get_orders_with_meta_query() so that it returns consistent results when 'status' => '' and the type is shop_order or shop_subscription.

For instance on trunk I'm noticing:

• wcs_get_orders_with_meta_query( [ 'type' => 'shop_order', 'status' => '' ] ); returns all orders
• wcs_get_orders_with_meta_query( [ 'type' => 'shop_subscription', 'status' => '' ] ); returns subscriptions with statuses that match order statuses. This should be fixed to return subscriptions with every subscription status.

The HPOS handling definitely needs to change here, though. I think we should make it so that passing in a falsy value for status is the same as passing in an invalid value for status.

Agreed.

---

On a side note, something I just thought about is we should be careful changing the behavior too much inside wcs_get_orders_with_meta_query() with how it handles querying shop_subscription statuses because the purpose of this function is to be an extension of wc_get_orders() but with built-in handling for querying metadata.

So we should avoid having specific query handling for the shop_subscription type because it could be confusing if running these two lines queried things differently:

wcs_get_orders_with_meta_query( [ 'type' => 'shop_order', 'status' => [ 'rubbish' ] ] );

wcs_get_orders_with_meta_query( [ 'type' => 'shop_subscription', 'status' => [ 'rubbish' ] ] );

[prettyboymp]

On a side note, something I just thought about is we should be careful changing the behavior too much inside wcs_get_orders_with_meta_query() with how it handles querying shop_subscription statuses because the purpose of this function is to be an extension of wc_get_orders() but with built-in handling for querying metadata.

So we should avoid having specific query handling for the shop_subscription type because it could be confusing if running these two lines queried things differently:

Thinking about this more, I decided that it would be worth diving into how WooCommerce core deals with some of these statuses and I found that these diverge when it comes to HPOS and non-HPOS environments there as well. Specifically, these cases:

Invalid statuses

wc_get_orders( [ 'status' => 'invalid_status' ] );

non-HPOS returns all results because the invalid status is ignored by WP_Query HPOS returns no results

This matches what we previously agreed with.

Empty status

wc_get_orders( [ ' status' => '' ] );

non-HPOS returns empty because WP_Query defaults to 'publish' as the filter status. HPOS treats it as an equivalent of 'any' and applies the valid order statuses.

This is different than what we previously discussed about treating an empty string like an invalid status. Given the goal of wcs_get_orders_with_meta_query(), it is probably best to convert to that handling.

In this latest commit, I changed the strategy of working the issue with WooCommerce core's handling of valid statuses and decided that it would be less complex and fix other areas like https://github.com/woocommerce/woocommerce-subscriptions/issues/4531 by filtering wc_order_statuses to include subscription statuses.

[mattallan]

Hey @prettyboymp, the latest changes with filtering order statuses to include Subscription statuses unfortunately won't work.

This filter is applying to all wc_get_order_statuses() calls which is used in a lot of places throughout WC & WC Subscriptions where it's expected to only include order statuses. One example of this breaking is if you go to create a new manual order, the drop-down for order statuses now also has Subscription statuses (see Active, Expired etc) 😅

How far off is the existing logic inside wcs_get_orders_with_meta_query() to matching wc_get_orders() behaviour when HPOS is enabled?

I think the only difference is just the handling of the '' status right? i.e. wc_get_orders_with_meta_query( [ 'type' => 'shop_subscription', 'status' => '' ] ); currently returns an empty list while the same request with 'type' => 'shop_order' returns a list of any status.

We could update the previous logic to handle this case for shop_subscription i.e.:

/**
 * Map the 'any' status to wcs_get_subscription_statuses() in HPOS environments.
 *
 * In HPOS environments, the 'any' status now maps to wc_get_order_statuses() statuses. Whereas, in
 * WP Post architecture 'any' meant any status except for ‘inherit’, ‘trash’ and ‘auto-draft’.
 *
 * If we're querying for subscriptions, we need to map 'any' to be all valid subscription statuses otherwise it would just search for order statuses.
 */
if ( isset( $args['status'], $args['type'] ) &&
    ( [ 'any' ] === (array) $args['status'] || '' === $args['status'] ) &&
    'shop_subscription' === $args['type'] &&
    $is_hpos_in_use
) {
    $args['status'] = array_keys( wcs_get_subscription_statuses() );
}

[prettyboymp]

Related: https://github.com/woocommerce/woocommerce/issues/39143

[mattallan]

Hey @prettyboymp, apologies for being slow to respond to the latest changes. I've looked over the most recent changes and I like the new direction! Especially since this fixes the issue for everyone that uses wc_get_orders() to fetch subscriptions, not just wcs_get_orders_with_meta_query().

I plan to do some testing on this PR tomorrow and will get back to you!