Automattic / woocommerce-subscriptions-core

Subscriptions core package for WooCommerce
Other
87 stars 33 forks source link

Saving dates on a WC_Subscription doesn't clear cache with HPOS enabled #444

Closed prettyboymp closed 4 months ago

prettyboymp commented 1 year ago

Issue:

With HPOS enabled (and hence OrderCache), any updates to dates on a WC_Subscription object will automatically save back to the database, however, the order cache will still hold onto the old data unless ::save() is called. See WC_Subscription::save_dates()

https://github.com/Automattic/woocommerce-subscriptions-core/blob/12402c886760d71a67f89ccea1d6d70279456098/includes/class-wc-subscription.php#L1524-L1540

Pseudo code to reproduce:

$original_subscription = wcs_get_subscription( $subscription_id );
$original_next_payment_time = $original_subscription->get_time( 'next_payment' );
$new_next_payment_time = $original_next_payment_time + YEAR_IN_SECONDS;

$original_subscription->update_dates( [ 'next_payment' => date( 'Y-m-d H:i:s', $new_next_payment_time ) ] );

$cached_subscription =  wcs_get_subscription( $subscription_id );

// clear the cache before reloading
$order_cache = wc_get_container()->get( \Automattic\WooCommerce\Caches\OrderCache::class );
$order_cache->remove( $subscription_id );

$db_loaded_subscription = wcs_get_subscription( $subscription_id );

print $original_subscription->get_time( 'next_payment' ) === $new_next_payment_time ? 'The original subscription object was updated to the new next payment time' : 'The original subscription object was not updated';
print "\n";
print $cached_subscription->get_time( 'next_payment' ) === $new_next_payment_time ? 'The cached subscription object was updated to the new next payment time' : 'The cached subscription object was not updated';
print "\n";
print $db_loaded_subscription->get_time( 'next_payment' ) === $new_next_payment_time ? 'The db_loaded subscription object was updated to the new next payment time' : 'The db_loaded subscription object was not updated';
print "\n";
prettyboymp commented 1 year ago

My personal thoughts on this is that we should be moving away from automatically writing back to any storage (cache or db) unless ::save() is called on the root object after updates. This likely creates a lot of backwards compatibility issues that will need to be worked through, though. Updating the comments/expectations around the usage of ::save_dates() is at least a start.

contemplate commented 6 months ago

So glad I found this and wish this could be resolved. After enabling HPOS we began seeing our custom snippet which expires a WC Subscription after the last failed payment retry rule begin to fail and expire subscriptions immediately as the comparison rule had an old date. I couldn't find anything in the documentation about this so your report here helped. Now I need to evaluate other snippets that might be impacted.

Snippet that worked before: ` // Cancel the subscription after the last retry. function tbl_after_payment_retry( $retry, $order ) {

// If payment completed, nothing to do.
if ( ! $order->needs_payment() ) {
    return;
}
$order_note    = __( 'All payment retry attempts failed:', 'tbl-payment-retry' );
$cancel_order  = false;
$subscriptions = wcs_get_subscriptions_for_renewal_order( $order );
foreach ( $subscriptions as $subscription ) {

    // The subscription will only have retry date in future if this isn't the last retry.
    if ( $subscription->get_time( 'payment_retry' ) <= gmdate( 'U' ) ) {
        $subscription->update_status( 'expired', $order_note );
        $cancel_order = true;
    }
}
if ( $cancel_order ) {
    $order->update_status( 'failed', $order_note );
    do_action( 'woocommerce_subscriptions_cancelled_after_retry', $order, $subscription, $retry );
}

} add_filter( 'woocommerce_subscriptions_after_payment_retry', 'tbl_after_payment_retry', 10, 2 ); `

Changed to the following: `// Cancel the subscription after the last retry. function tbl_after_payment_retry( $retry, $order ) {

// If payment completed, nothing to do.
if ( ! $order->needs_payment() ) {
    return;
}
$order_note    = __( 'All payment retry attempts failed:', 'tbl-payment-retry' );
$cancel_order  = false;
$subscriptions = wcs_get_subscriptions_for_renewal_order( $order );
foreach ( $subscriptions as $subscription ) {

    //HPOS
    $subscription_id = $subscription->get_id();

    // clear the cache before reloading
    $order_cache = wc_get_container()->get( \Automattic\WooCommerce\Caches\OrderCache::class );
    $order_cache->remove( $subscription_id );

    $cleared_subscription = wcs_get_subscription( $subscription_id );

    // The subscription will only have retry date in future if this isn't the last retry.
    if ( $cleared_subscription->get_time( 'payment_retry' ) <= gmdate( 'U' ) ) {
        $cleared_subscription->update_status( 'expired', $order_note );
        $cancel_order = true;
    }
}
if ( $cancel_order ) {
    $order->update_status( 'failed', $order_note );
    do_action( 'woocommerce_subscriptions_cancelled_after_retry', $order, $cleared_subscription, $retry );
}

} add_filter( 'woocommerce_subscriptions_after_payment_retry', 'tbl_after_payment_retry', 10, 2 );`

mattallan commented 4 months ago

I'm only finding this issue now :( but just wanted to say we shipped a quick fix in #577 (WooCommerce Subscriptions 6.1).