Automattic / woocommerce-subscriptions-core

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

Make sure subscriptions are created with the standard default so we can avoid reloading the subscription #580

Closed james-allan closed 5 months ago

james-allan commented 6 months ago

Description

When we create a subscription in wcs_create_subscription() we set a bunch of properties based on the args provided and a set of defaults, and then we save the subscription and fetch a fresh copy via wcs_get_subscription().

This fetching of the subscription is a failure point for some sites because the subscription fails to load from the database because they have some database load balancing and it cannot receive the subscription immediately or the subscription data is corrupted some how and calling wc_get_order() returns false.

As @mattallan pointed on in this Slack message(p1710811340613109/1710755100.597449-slack-C055WHLA98D) if we don't call wcs_get_subscription() some of the subscription's properties are empty (eg status) or malformed (eg requires_manual_renewal is a string and the scheduled_start date is a mysql string, not a datetime object) or missing entirely (eg related order caches).

This PR fixes that in a few ways:

Status

Screenshot 2024-03-20 at 4 27 05 pm
Prior to $subscription->save() (left) after fetching a fresh copy (right)

If no status arg is provided to wcs_create_subscription() it will default to 'pending'. If no status was provided previously it would be default to pending anyway on the saving of the subscription so this change only means were setting it actively before the save.

requires_manual_renewal property

Screenshot 2024-03-20 at 4 31 07 pm
Prior to $subscription->save() (left) after fetching a fresh copy (right)

The requires_manual_renewal property on the subscription prior to saving was a string (ie 'true') and a bool after fetching. I've fixed this by making sure the default property in the subscription definition is not a string. For some background, the requires_manual_renewal meta is stored in the database as a string, however it in memory it should be a bool. Prior to this change, if you did something like this you'd get true or 'true'. On this branch it should always be true (bool).

$subscription = new WC_Subscription();
var_export( $subscription->get_requires_manual_renewal() ); // 'true'
$subscription->save();

$subscription = wcs_get_subscription( $subscription->get_id() );
var_export( $subscription->get_requires_manual_renewal() ); // true

Start date

[!note] This change is technically optional because while the data looks different between the two subscription instances when printed out like this, I couldn't actually run into an issue. In order to fetch the subscription's start date you need to call get_time() or get_date() and both of those functions are capable of handling this kind of format.

Screenshot 2024-03-20 at 5 04 44 pm
Prior to $subscription->save() (left) after fetching a fresh copy (right)

Prior to this PR, the start date was set to a mysql string rather than a WC_DateTime. This didn't appear to cause any issues because when dates are written to the database, we use get_date() (ref) and it is capable of converting WC_DateTime date values into mysql strings or just returning whatever value is stored there - in this case a raw mysql string.

I've fixed this by making sure when we call set_start_date() we use set_date_prop() which handles setting dates and storing them as WC_DateTime objects. This is also inline with how we set dates via update_dates() (ref).

Related order caches

I attempted to fix this by making sure we set the empty meta cache on the subscription in memory (see https://github.com/Automattic/woocommerce-subscriptions-core/pull/580/commits/5892a982b683e3b0617b9a9df33d63b97fa624f1) however that broke unit tests which run on wp post tables because saving the meta directly in the database and then storing a value in memory too will trick WC into adding the meta resulting in double _subscription_renewal_order_ids_cache values.

These caches aren't supposed to be accessed via get_meta_data() functions anyway so I think this minor difference isn't worth trying to "fix" which risks breaking something else.

How to test this PR

  1. Using a plugin like WP Console run the following code snippet.
$subscription = new WC_Subscription();
$subscription->save();

print_r( $subscription );
print_r( wcs_get_subscription( $subscription ) );
  1. Put the two subscription results into a tool like https://www.diffchecker.com/
  2. On trunk you'll notice there are a few differences.
    • status empty vs 'pending'
    • total 0 vs 0.00
    • requires_manual_renewal true vs 1
    • schedule_start empty vs a WC_DateTime object.
    • ..._order_ids_cache not set vs empty arrays.
  3. Repeat on this branch and requires_manual_renewal should now be the same. Every other property cannot be set on a simple new WC_Subscription(). See note below

[!note] The status and total values being empty and 0 are inherited from when a WC_Order is instantiated. See here. I don't think it would be appropriate to set the start date of a subscription by simply doing new WC_Subscription().

  1. Change the code snippet to something like this:
add_filter( 'wcs_created_subscription', function( $subscription ) {
    print_r( $subscription );
    return $subscription;
} );

$subscription = wcs_create_subscription(
    [
        'customer_id'      => 1,
        'billing_period'   => 'week',
        'billing_interval' => 4,
    ]
);

print_r(wcs_get_subscription( $subscription->get_id() ) );
  1. Put the two subscription results into a tool like https://www.diffchecker.com/
  2. On this branch the two subscriptions should be identical except for the total which is just an int vs float and the _order_ids_cache are still not set either.

Product impact