-
Notifications
You must be signed in to change notification settings - Fork 216
Reduce number of payment_methods API calls #4560
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
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 optimizes Stripe payment methods API calls by implementing intelligent caching and batching strategies to reduce the number of requests made to Stripe's payment_methods endpoint.
Key changes include:
- Implementing a unified
get_all_payment_methods()
method that fetches all payment methods at once and caches the complete result - Adding error handling for non-existent customers with appropriate caching of empty results
- Refactoring callers to use the new batched approach instead of making separate API calls per payment method type
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
includes/class-wc-stripe-customer.php | Adds new get_all_payment_methods() method with pagination, error handling, and unified caching strategy |
includes/payment-tokens/class-wc-stripe-payment-tokens.php | Refactors to use single batched API call instead of multiple type-specific calls |
includes/compat/trait-wc-stripe-subscriptions.php | Updates subscription payment method rendering to use batched approach with corrected break statements |
tests/phpunit/WC_Stripe_Payment_Gateway_Test.php | Updates test to reflect new cache key format for all payment methods |
@@ -704,32 +704,90 @@ public function get_payment_methods( $payment_method_type ) { | |||
return []; | |||
} | |||
|
|||
$payment_methods = get_transient( self::PAYMENT_METHODS_TRANSIENT_KEY . $payment_method_type . $this->get_id() ); | |||
return $this->get_all_payment_methods( [ $payment_method_type ] ); |
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.
This implementation may cause performance issues when only one payment method type is needed. The original method cached results per payment method type, but now every call to get_payment_methods()
will fetch and cache ALL payment methods, even when only one type is requested. Consider maintaining type-specific caching for backward compatibility and performance.
return $this->get_all_payment_methods( [ $payment_method_type ] ); | |
$transient_key = self::PAYMENT_METHODS_TRANSIENT_KEY . $this->get_id() . '_' . $payment_method_type; | |
$methods = get_transient( $transient_key ); | |
if ( false === $methods ) { | |
$response = WC_Stripe_API::request( | |
[ | |
'customer' => $this->get_id(), | |
'type' => $payment_method_type, | |
'limit' => 100, | |
], | |
'payment_methods', | |
'GET' | |
); | |
if ( ! empty( $response->error ) || ! is_array( $response->data ) ) { | |
return []; | |
} | |
$methods = $response->data; | |
set_transient( $transient_key, $methods, DAY_IN_SECONDS ); | |
} | |
return empty( $methods ) ? [] : $methods; |
Copilot uses AI. Check for mistakes.
'payment_methods' . $params, | ||
'GET' | ||
); | ||
'limit' => 100, |
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 magic number 100 should be extracted to a class constant for better maintainability. This appears to be the Stripe API maximum limit and should be documented as such.
'limit' => 100, | |
'limit' => self::STRIPE_API_MAX_LIMIT, |
Copilot uses AI. Check for mistakes.
/* translators: 1) card brand 2) last 4 digits */ | ||
$payment_method_to_display = sprintf( __( 'Via %1$s card ending in %2$s', 'woocommerce-gateway-stripe' ), ( isset( $source->brand ) ? wc_get_credit_card_type_label( $source->brand ) : __( 'N/A', 'woocommerce-gateway-stripe' ) ), $source->last4 ); | ||
$payment_method_to_display = sprintf( __( 'Via %1$s card ending in %2$s', 'woocommerce-gateway-stripe' ), ( isset( $source->card->brand ) ? wc_get_credit_card_type_label( $source->card->brand ) : __( 'N/A', 'woocommerce-gateway-stripe' ) ), $source->card->last4 ); | ||
break 2; |
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.
[nitpick] The numbered break statements (break 2) are harder to maintain and understand than using labeled breaks or restructuring the code. Consider refactoring to use a flag variable or extracting the logic into a separate method to improve readability.
Copilot uses AI. Check for mistakes.
foreach ( self::STRIPE_PAYMENT_METHODS as $payment_method_type ) { | ||
delete_transient( self::PAYMENT_METHODS_TRANSIENT_KEY . $payment_method_type . $this->get_id() ); | ||
} |
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 cache clearing logic is incomplete. While the new '_all' cache key is being cleared, the old individual payment method type cache keys are still being cleared in the loop above, but these are no longer being set by the new implementation. This could lead to memory leaks if old cache keys remain.
foreach ( self::STRIPE_PAYMENT_METHODS as $payment_method_type ) { | |
delete_transient( self::PAYMENT_METHODS_TRANSIENT_KEY . $payment_method_type . $this->get_id() ); | |
} |
Copilot uses AI. Check for mistakes.
|
||
// Legacy handling for Stripe Card objects. ref: https://docs.stripe.com/api/cards/object | ||
if ( isset( $source->object ) && WC_Stripe_Payment_Methods::CARD === $source->object ) { | ||
switch ( $source->type ) { |
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.
I think we have the opportunity to move this to a new method. maybe_render_subscription_payment_method
is exceptionally long.
I think it is worth creating specific issues for your "Next steps and concerns", so we can tackle those separately. |
@wjrosa, part of what I am looking for in terms of feedback is finding the right scope across the issues so we tackle the work appropriately. I've already split out the basic check for a missing customer in #4561, and I'll look into adding a new |
&& 'resource_missing' === $response->error->code | ||
) { | ||
// If the customer doesn't exist, cache an empty array. | ||
set_transient( $cache_key, [], DAY_IN_SECONDS ); |
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.
Should we use a smaller TTL for this negative cache?
I am going to close this PR, as I have since split out the various implementation aspects into the following smaller PRs: |
Fixes STRIPE-<linear_issue_id>
Fixes #<github_issue_id>
Changes proposed in this Pull Request:
This PR tackles multiple pieces of code to try and reduce the number of API calls we make to the Stripe
payment_methods
API, which we may want to split up into smaller PRs:Next steps and concerns
expand
URL parameter (Stripe API docs for the API) -- I am not sure how this would (or would not) work across other payment methodsget_payment_methods()
API and the changes to callers that I have implemented in this change, but leaveget_payment_method()
untouched for now.Testing instructions
Changelog entry
Changelog Entry Comment
Comment
Post merge