search

Automattic / woocommerce-subscriptions-core

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

`woocommerce_update_order` action lacks second argument #421

Closed dmvrtx closed 1 year ago

dmvrtx commented 1 year ago

Describe the bug

Developers of the Wholesale Pro plugin had investigated the reason for the following error on the user's site (see details):

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

They believe the culprit is this line where woocommerce_update_action is called without passing a second argument, which shuld be $subscription.

Their reasoning is below:

From April 17, 2019, the do_action calls for the woocommerce_new_order and woocommerce_update_order hooks (as well as many other similar hooks, such as new_coupon, new_customer, etc) were changed to include the correspondent object as a second parameter (this is aimed at avoiding redundant and expensive calls to get an object from its ID). When the WooCommerce Subscriptions plugin runs woocommerce_update_order on line 206 (of their code), the second parameter is omitted (which should be $subscription in their case). Their use of that hook breaks any plugin that might use both parameters (as WooCommerce Wholesale Pro does).

This started happening in WooCommerce Wholesale Pro version 2.0.0 because we started taking advantage of the second parameter to further improve plugin performance.

Please also note that the only occurrences where the same hook is used with one parameter in the whole WooCommerce source code are in the legacy API calls (both v2 and v3). In this comment, WooCommerce core developer Vedanshu Jain notes that even those calls, albeit deprecated, should be updated to pass two parameters for consistency and accuracy in the WooCommerce code base.

Once they have updated their plugin to follow WooCommerce's best practices, the issue should be resolved.

Thank you for your attention to this.

While I disagree that the line from the year 2018 shouldn't start causing issues now and there are more calls of woocommerce_update_order scattered across various plugins without passing second argument, I believe this specific case is worth being fixed.

Expected behavior

woocommerce_update_order action is called with both order ID and order object.

Actual behavior

woocommerce_update_order action is called with order ID alone.

Product impact

Additional context

WillBrubaker commented 1 year ago

6064183-zen

LuigiPulcini commented 1 year ago

While I disagree that the line from the year 2018 shouldn't start causing issues now and there are more calls of woocommerce_update_order scattered across various plugins without passing second argument, I believe this specific case is worth being fixed.

I am the developer who wrote the explanation you quoted and would be glad to clarify this aspect since you seem to partially disagree with my rationale.

The WooCommerce change is from 2019 but that doesn't mean that every plugin handling WooCommerce orders started adopting that change right away. This problem started happening now with our plugin just because we decided to take advantage of the second parameter passed to the callback function by the woocommerce_update_order hook. By the way, thanks to that use, we avoid a rather expensive call to wc_get_order() with the $order_id, which is the exact reason why WooCommerce added that second parameter, to begin with.

Now, when you call do_action on a hook that is originally defined in another plugin, it is your responsibility to comply with how that hook is originally defined. Otherwise, all the plugins that follow the original definition and want to take advantage of all its parameters – which is perfectly admissible by the original definition – will inevitably trigger a fatal error when used in combination with your plugin as well as all the various plugins that call do_action on that hook with any missing parameter. It is just a matter of time before more and more plugins start using the second parameter to improve their performance – as we did with ours – and end up triggering the same error with your plugin.

Fortunately, in this case, the solution is just one variable away, on a single line of code.

I hope this better clarifies the situation and am glad I was able to bring this problem to your attention.

ldr2273 commented 1 year ago

6062887-zen

Hi @dmvrtx - are we able to put a priority label on this? This would be helpful for our communications with customers around expectations. Thx!

melek commented 1 year ago

38028383-hc - in this incident, the third party developer had this to say to the merchant who had an issue with their product (WooCommerce Wholesale Pro):

From April 17, 2019, the do_action calls for the woocommerce_new_order and woocommerce_update_order hooks (as well as many other similar hooks, such as new_coupon, new_customer, etc) were changed to include the correspondent object as a second parameter (this is aimed at avoiding redundant and expensive calls to get an object from its ID). When the WooCommerce Subscriptions plugin runs woocommerce_update_order on line 206 (of their code), the second parameter is omitted (which should be $subscription in their case). Their use of that hook breaks any plugin that might use both parameters (as WooCommerce Wholesale Pro does).

I've gone ahead and put a 'High' priority issue on this for now since it appears to be a small fix that would support other developers who are using both parameters correctly in this common hook.