Automattic / woocommerce-subscriptions-core

Subscriptions core package for WooCommerce
Other
85 stars 31 forks source link

Replace wp_count_posts() uses with subscriptions data store equivalent to support HPOS #382

Closed mattallan closed 1 year ago

mattallan commented 1 year ago

Description

In https://github.com/Automattic/woocommerce-subscriptions-core/pull/372 we introduced a new subscriptions data store function to count subscriptions and group them by their statuses.

This issue is to look at replacing all instances of wp_count_posts( 'shop_subscription' ) with the new WC_Data_Store::load( 'subscription' )->get_subscriptions_count_by_status();.


Note: There is one instance of wp_count_posts( 'shop_order' ) used in the wcs_is_large_site() which won't work on stores with HPOS enabled, but there's no replacement function for this.

For now, I'm thinking we only worry about counting orders when the store has HPOS disabled since the idea behind this function is that we throttle the store based on how many orders and subscriptions they have, but performance issues might not be a think on HPOS sites.

So the logic in wcs_is_large_site() would change to something like:

// If an option has been set previously, convert it to a bool.
if ( false !== $is_large_site ) {
    $is_large_site = wc_string_to_bool( $is_large_site );
} elseif ( array_sum( (array) wp_count_posts( 'shop_subscription' ) ) > 3000 || ( ! wcs_is_custom_order_tables_usage_enabled() && array_sum( (array) wp_count_posts( 'shop_order' ) ) > 25000 ) ) {
    $is_large_site = true;
    update_option( 'wcs_is_large_site', wc_bool_to_string( $is_large_site ), false );
} else {
    $is_large_site = false;
}

Product impact

Dev notes

Additional context

Jinksi commented 1 year ago

Will WCS_Admin_System_Status::get_subscription_statuses() be included in this issue?

The sibling method WCS_Admin_System_Status::get_subscriptions_by_gateway() is also not HPOS-compatible. Should I log a new issue for it?

https://github.com/Automattic/woocommerce-subscriptions-core/blob/a554bd28e7dd3e70e6d108a70e379b9ae28e75d7/includes/admin/class-wcs-admin-system-status.php#L364-L372

mattallan commented 1 year ago

Will WCS_Admin_System_Status::get_subscription_statuses() be included in this issue?

@Jinksi Yeah i'll update all instances of wp_count_posts() with this issue.

Thanks for bringing up get_subscriptions_by_gateway() as well as I haven't come across this one yet. We can handle that in a separate issue I think as it's not related to wp_count_posts(). I'm not even sure what the right way to fix that would be, maybe just run a different DB query or something when HPOS is enabled. I don't know if going down the route of having a new data store function is necessary since it's not widely used.

Jinksi commented 1 year ago

We can handle that in a separate issue I think as it's not related to wp_count_posts().

384 created 👍