Automattic / woocommerce-subscriptions-core

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

Pass the subscription object as the second param on data store update hooks to match WC Core #433

Closed mattallan closed 1 year ago

mattallan commented 1 year ago

Fixes #421

Description

Third-party developers that hook onto the woocommerce_update_order hook and try to use the second parameter are receiving fatal errors with WC Subscriptions installed.

From the issue, the developers of Wholesale Pro Plugin have been getting the following fatal errors which is preventing renewals from processing:

02-28-2023 @ 19:42:10 - scheduled action 164649 (subscription payment) failed to finish processing due to the following error: Uncaught ArgumentCountError: Too few arguments to function Barn2\Plugin\WC_Wholesale_Pro\Order_Handler::add_wholesale_metadata(), 1 passed in /wordpress/wp-includes/class-wp-hook.php on line 308 and exactly 2 expected in /www/wp-content/plugins/woocommerce-wholesale-pro/src/Order_Handler.php:33
Stack trace:
#0 /wordpress/wp-includes/class-wp-hook.php(308): Barn2\Plugin\WC_Wholesale_Pro\Order_Handler->add_wholesale_metadata(10926)
#1 /wordpress/wp-includes/class-wp-hook.php(332): WP_Hook->apply_filters(NULL, Array)
#2 /wordpress/wp-includes/plugin.php(517): WP_Hook->do_action(Array)
#3 /www/wp-content/plugins/woocommerce-subscriptions/vendor/woocommerce/subscriptions-core/includes/data-stores/class-wcs-subscription-data-store-cpt.php(206): do_action('woocommerce_upd...', 10926)
#4 /www/wp-content/plugins/woocommerce/includes/class-wc-data-store.php(196): WCS_Subscription_Data_Store_CPT->update(Object(WC_Subscription))
#5 /www/wp-content/plugins/woocommerce/includes/abstracts/abstract-wc-order.php(199): WC_Data
02-28-2023 @ 19:42:10 - action args: subscription_id: 10926
03-01-2023 @ 13:46:44 - scheduled action 162670 (subscription payment) failed to finish processing due to the following error: Uncaught ArgumentCountError: Too few arguments to function Barn2\Plugin\WC_Wholesale_Pro\Order_Handler::add_wholesale_metadata(), 1 passed in /wordpress/wp-includes/class-wp-hook.php on line 308 and exactly 2 expected in /www/wp-content/plugins/woocommerce-wholesale-pro/src/Order_Handler.php:33
Stack trace:
#0 /wordpress/wp-includes/class-wp-hook.php(308): Barn2\Plugin\WC_Wholesale_Pro\Order_Handler->add_wholesale_metadata(5523)
#1 /wordpress/wp-includes/class-wp-hook.php(332): WP_Hook->apply_filters(NULL, Array)
#2 /wordpress/wp-includes/plugin.php(517): WP_Hook->do_action(Array)
#3 /www/wp-content/plugins/woocommerce-subscriptions/vendor/woocommerce/subscriptions-core/includes/data-stores/class-wcs-subscription-data-store-cpt.php(206): do_action('woocommerce_upd...', 5523)
#4 /www/wp-content/plugins/woocommerce/includes/class-wc-data-store.php(196): WCS_Subscription_Data_Store_CPT->update(Object(WC_Subscription))
#5 /www/wp-content/plugins/woocommerce/includes/abstracts/abstract-wc-order.php(199): WC_Data_S
03-01-2023 @ 13:46:44 - action args: subscription_id: 5523

To resolve these issues, this PR updates our woocommerce_update_order action hook which is called inside of our Subscription data store classes to match the same hooks in the latest WooCommerce Core version.

There are no concerns with this breaking backwards compatibility as this is just adding a parameter to a hook. So all existing code using the previous version of this hook will operate normally:

add_action( 'woocommerce_update_order', 'my_awesome_callback', 10, 1 );

add_action( 'woocommerce_update_order', 'use_default' );

How to test this PR

The woocommerce_update_order hook is called every time we call $subscription->save(), therefore I used the following snippet of code to test these changes:

// Existing use of this hook -- backwards compatibility test
add_action( 'woocommerce_update_order', function( $subscription_id ) {
    echo "---- 'woocommerce_update_order' hook with 1 parameter ----\n";
    echo 'One param: [subscription_id] ' . $subscription_id . "\n";
    echo "----\n\n";
} );

// Hook with 2 params
add_action( 'woocommerce_update_order', function( $subscription_id, $subscription ) {
    echo "---- 'woocommerce_update_order' hook with 2 parameters ----\n";
    echo 'First param: [subscription_id] ' . $subscription_id . "\n";
    echo 'Second param: [subscription] ' . print_r( $subscription, true );
}, 10, 2 );

// trigger the woocommerce_update_order hook
$subscription = wcs_get_subscription( 1040 );
$subscription->save();
  1. With HPOS disabled, run the above snippet on trunk in WP Console.
  2. You will get a fatal error similiar to the above reported issue (1 passed, expected 2)
  3. Checkout this branch and re-run the code snippet, notice no issues and correct output
  4. Since these changes impact both data stores, turn on HPOS and re-run the test.

You can also test this PR in a similar way to how the issue was reported to us:

  1. Add the following code snippet to a test plugin or your local themes functions.php
    add_action( 'woocommerce_update_order', function( $order_id, $order ) {
    error_log( 'woocommerce_update_order hook fired for ID: ' . $order_id );
    }, 10, 2 );
  2. On trunk attempt to process a process renewal with Action Scheduler.
  3. Confirm the renewal scheduled action failed and subscription is left on-hold.

Product impact

james-allan commented 1 year ago

Changes look good to me. I tested the changes with the code snippets provided. I replicated the error and confirmed the fix.

I also ran the code snippets with an order and confirmed that the 2 params are passed etc.