Automattic / woocommerce-subscriptions-core

Subscriptions core package for WooCommerce
Other
80 stars 29 forks source link

When populating the checkout with renewal order checkout values, limit it to only fetch address fields #596

Closed james-allan closed 2 months ago

james-allan commented 2 months ago

Fixes https://github.com/woocommerce/woocommerce-subscriptions/issues/4627

Description

When a customer pays for a failed/pending renewal order, we pre-populate the checkout fields with address values from the order.

The function responsible for doing that is WCS_Cart_Renewal::checkout_get_value().

This function, prior to this PR, used wcs_get_objects_property() to pull these values for the checkout. That function is wide open in terms of what it could fetch and return. ie it attempts to call getters and then meta data and so any order property or meta is returnable in this case.

This level of openness can lead to issues when stores use custom checkout fields. Eg in the issue description, the customer has a user field which leads wcs_get_objects_property() to pull the WP_User object from the order and attempts to put that into the checkout field.

This PR fixes this by limiting this function to only fetch address values. From the function description itself, I don't believe this broad nature was intended.

How to test this PR

  1. Add the following code snippet to your site to mimic a custom checkout field being added by a plugin like Checkout Field Editor.
  2. Purchase a subscription.
  3. Create a pending renewal order for the subscription.
  4. Attempt to pay for the renewal order.
    1. On this trunk notice the PHP error notice indicating that an WP_User object cannot be converted etc.
    2. On this branch there should be no issue.

Testing intended functionality.

  1. Change the default checkout page to a shortcode checkout.
    • The Block checkout has some strange caching or something. Sometimes the block checkout would honour the values and other times it wouldn't 🤷‍♂️. It seemed to bypass this function all together.
  2. Edit the renewal order you created at step 3 above.
  3. Change the billing and shipping address in a way that you'll recognise. You want it to be a completely different address.
  4. Attempt to pay for the order again. Confirm that the address fields on the checkout correctly reflect the changes you made.
add_filter( 'woocommerce_checkout_fields', function ( $fields ) {
    $fields['billing']['user'] = [
        'label'    => 'Custom user',
        'required' => false,
    ];
    return $fields;
} );

Product impact

james-allan commented 2 months ago

I was also generally curious about why the change from using get_checkout_fields() because I thought generating the getter function and checking is_callable against the order would be enough to filter out these errors.

Yeah so the reason for changing it to use get_address_fields() is because the old function returned the filtered list of fields that are displayed on the checkout. Anyone using a plugin that adds custom fields to the checkout would have their fields returned.

So, in the end using get_checkout_fields() wouldn't fix the issue. If anyone hooked onto woocommerce_checkout_fields and retuned a field with a key that also maps to an order getter (eg user) it would still call $order->get_user().

In this PR I've limited it to only actually return address values. To do that I had to change the source of address fields and that's the WC()->countries->get_address_fields() function.