-
Notifications
You must be signed in to change notification settings - Fork 216
Refactor webhook handler to compute order or refund upfront #4546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Refactor webhook handler to compute order or refund upfront #4546
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the webhook handler to centralize order lookup logic and pass the resolved order directly to webhook processing methods. This change aims to improve maintainability and supports the development of WordPress hooks for webhook processing.
- Centralized order lookup logic into a single
get_order_from_notification()
method - Modified all webhook processing methods to accept an optional
$order
parameter - Updated
process_webhook()
to resolve the order upfront and pass it to each handler
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
includes/class-wc-stripe-webhook-handler.php | Refactored webhook handler methods to accept order parameter and added centralized order lookup logic |
tests/phpunit/WC_Stripe_Webhook_Handler_Test.php | Added missing notification type field to test data |
@@ -869,23 +888,19 @@ public function process_webhook_refund_updated( $notification ) { | |||
* Process webhook reviews that are opened. i.e Radar. | |||
* | |||
* @since 4.0.6 | |||
* @param object $notification | |||
* @param object $notification The notification data from Stripe. | |||
* @param WC_Order|false|null $refund The related order. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @param documentation incorrectly states '$refund' as the parameter name when it should be '$order'. Also, the description 'The related order' is inconsistent with other methods that use 'The related order.'
Copilot uses AI. Check for mistakes.
break; | ||
|
||
case 'setup_intent.succeeded': | ||
case 'setup_intent.setup_failed': | ||
$this->process_setup_intent( $notification ); | ||
$this->process_setup_intent( $notification, $order ); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing 'break;' statement after the setup_intent case block. This will cause the code to fall through to the next statement, which could lead to unexpected behavior.
break; |
Copilot uses AI. Check for mistakes.
* @return WC_Order|WC_Order_Refund|false The order or refund object, or false if not found. | ||
*/ | ||
protected function get_order_from_notification( $notification ) { | ||
$object = $notification->data->object ?? new stdClass(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating an empty stdClass as a fallback could lead to unexpected behavior when the object properties are accessed later. Consider logging a warning or returning false early when the notification data object is missing.
$object = $notification->data->object ?? new stdClass(); | |
if ( ! isset( $notification->data ) || ! isset( $notification->data->object ) ) { | |
if ( function_exists( 'wc_get_logger' ) ) { | |
wc_get_logger()->warning( | |
'Stripe webhook notification missing data object: ' . wp_json_encode( $notification ), | |
array( 'source' => 'wc-stripe-webhook-handler' ) | |
); | |
} else { | |
error_log( 'Stripe webhook notification missing data object: ' . print_r( $notification, true ) ); | |
} | |
return false; | |
} | |
$object = $notification->data->object; |
Copilot uses AI. Check for mistakes.
Towards STRIPE-564
Related to #4466
Changes proposed in this Pull Request:
This PR explores refactoring our webhook handler in two specific ways:
process_webhook()
and then pass in the order that was identifiedThe refactor stems from a discussion in #4466, where we're trying to build WordPress hooks for developers to process webhooks more easily.
Testing instructions
Changelog entry
Changelog Entry Comment
Comment
Post merge