Reduce number of payment_methods API calls #4560
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -704,32 +704,90 @@ public function get_payment_methods( $payment_method_type ) { | |||||||
| return []; | ||||||||
| } | ||||||||
|
|
||||||||
| return $this->get_all_payment_methods( [ $payment_method_type ] ); | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| * Get all payment methods for a customer. | ||||||||
| * | ||||||||
| * @param string[] $payment_method_types The payment method types to look for using Stripe method IDs. If the array is empty, it implies all payment method types. | ||||||||
| * @param int $limit The maximum number of payment methods to return. If the value is -1, no limit is applied. | ||||||||
| * @return array | ||||||||
| */ | ||||||||
| public function get_all_payment_methods( array $payment_method_types = [], int $limit = -1 ) { | ||||||||
| if ( ! $this->get_id() ) { | ||||||||
| return []; | ||||||||
| } | ||||||||
|
|
||||||||
| $cache_key = self::PAYMENT_METHODS_TRANSIENT_KEY . '__all_' . $this->get_id(); | ||||||||
| $all_payment_methods = get_transient( $cache_key ); | ||||||||
|
|
||||||||
| if ( false === $all_payment_methods || ! is_array( $all_payment_methods ) ) { | ||||||||
| $all_payment_methods = []; | ||||||||
| $last_payment_method_id = null; | ||||||||
|
|
||||||||
| do { | ||||||||
| $request_params = [ | ||||||||
| 'customer' => $this->get_id(), | ||||||||
| 'limit' => 100, | ||||||||
|
There was a problem hiding this comment. 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.
Suggested change
Copilot uses AI. Check for mistakes. |
||||||||
| ]; | ||||||||
|
|
||||||||
| if ( $last_payment_method_id ) { | ||||||||
| $request_params['starting_after'] = $last_payment_method_id; | ||||||||
| } | ||||||||
|
|
||||||||
| $response = WC_Stripe_API::request( $request_params, 'payment_methods', 'GET' ); | ||||||||
|
|
||||||||
| if ( ! empty( $response->error ) ) { | ||||||||
| if ( | ||||||||
| isset( $response->error->code ) | ||||||||
| && isset( $response->error->param ) | ||||||||
| && 'customer' === $response->error->param | ||||||||
| && '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. Should we use a smaller TTL for this negative cache? |
||||||||
| } | ||||||||
| return []; | ||||||||
| } | ||||||||
|
|
||||||||
| if ( ! is_array( $response->data ) || [] === $response->data ) { | ||||||||
| break; | ||||||||
| } | ||||||||
|
|
||||||||
| $all_payment_methods = array_merge( $all_payment_methods, $response->data ); | ||||||||
|
|
||||||||
| // Reset the last payment method ID so we can paginate correctly. | ||||||||
| $last_payment_method_id = null; | ||||||||
| if ( isset( $response->has_more ) && $response->has_more ) { | ||||||||
| $last_payment_method = end( $response->data ); | ||||||||
|
|
||||||||
| if ( $last_payment_method && ! empty( $last_payment_method->id ) ) { | ||||||||
| $last_payment_method_id = $last_payment_method->id; | ||||||||
| } | ||||||||
| } | ||||||||
| } while ( null !== $last_payment_method_id ); | ||||||||
|
|
||||||||
| // Always cache the result without any filters applied. | ||||||||
| set_transient( $cache_key, $all_payment_methods, DAY_IN_SECONDS ); | ||||||||
| } | ||||||||
|
|
||||||||
| // Note that we only apply the limit and type filters after fetching and caching all payment methods. | ||||||||
| $filtered_payment_methods = $all_payment_methods; | ||||||||
| if ( [] !== $payment_method_types ) { | ||||||||
| $filtered_payment_methods = array_filter( | ||||||||
| $filtered_payment_methods, | ||||||||
| function ( $payment_method ) use ( $payment_method_types ) { | ||||||||
| return in_array( $payment_method->type, $payment_method_types, true ); | ||||||||
| } | ||||||||
| ); | ||||||||
| } | ||||||||
|
|
||||||||
| if ( $limit > 0 ) { | ||||||||
| return array_slice( $filtered_payment_methods, 0, $limit ); | ||||||||
| } | ||||||||
|
|
||||||||
| return $filtered_payment_methods; | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
|
|
@@ -837,6 +895,7 @@ public function clear_cache( $payment_method_id = null ) { | |||||||
| foreach ( self::STRIPE_PAYMENT_METHODS as $payment_method_type ) { | ||||||||
| delete_transient( self::PAYMENT_METHODS_TRANSIENT_KEY . $payment_method_type . $this->get_id() ); | ||||||||
| } | ||||||||
|
Comment on lines
895
to
897
There was a problem hiding this comment. 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.
Suggested change
Copilot uses AI. Check for mistakes. |
||||||||
| delete_transient( self::PAYMENT_METHODS_TRANSIENT_KEY . '__all_' . $this->get_id() ); | ||||||||
| // Clear cache for the specific payment method if provided. | ||||||||
| if ( $payment_method_id ) { | ||||||||
| WC_Stripe_Database_Cache::delete( 'payment_method_for_source_' . $payment_method_id ); | ||||||||
|
|
||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1016,69 +1016,69 @@ public function maybe_render_subscription_payment_method( $payment_method_to_dis | |
| $payment_method_to_display = __( 'N/A', 'woocommerce-gateway-stripe' ); | ||
|
|
||
| try { | ||
| // Retrieve all possible payment methods for the customer so we make minimal API calls to Stripe. | ||
| $customer_payment_methods = $stripe_customer->get_all_payment_methods(); | ||
|
|
||
| foreach ( $customer_payment_methods as $source ) { | ||
| if ( $source->id !== $stripe_source_id ) { | ||
| continue; | ||
| } | ||
|
|
||
| // 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 ) { | ||
| /* 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 ); | ||
| break; | ||
| } | ||
|
|
||
| switch ( $source->type ) { | ||
|
There was a problem hiding this comment. I think we have the opportunity to move this to a new method. |
||
| case WC_Stripe_Payment_Methods::CARD: | ||
| /* 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->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. [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. |
||
| case WC_Stripe_Payment_Methods::SEPA_DEBIT: | ||
| /* translators: 1) last 4 digits of SEPA Direct Debit */ | ||
| $payment_method_to_display = sprintf( __( 'Via SEPA Direct Debit ending in %1$s', 'woocommerce-gateway-stripe' ), $source->sepa_debit->last4 ); | ||
| break 2; | ||
| case WC_Stripe_Payment_Methods::CASHAPP_PAY: | ||
| /* translators: 1) Cash App Cashtag */ | ||
| $payment_method_to_display = sprintf( __( 'Via Cash App Pay (%1$s)', 'woocommerce-gateway-stripe' ), $source->cashapp->cashtag ); | ||
| break 2; | ||
| case WC_Stripe_Payment_Methods::LINK: | ||
| /* translators: 1) email address associated with the Stripe Link payment method */ | ||
| $payment_method_to_display = sprintf( __( 'Via Stripe Link (%1$s)', 'woocommerce-gateway-stripe' ), $source->link->email ); | ||
| break 2; | ||
| case WC_Stripe_Payment_Methods::ACH: | ||
| $payment_method_to_display = sprintf( | ||
| /* translators: 1) account type (checking, savings), 2) last 4 digits of account. */ | ||
| __( 'Via %1$s Account ending in %2$s', 'woocommerce-gateway-stripe' ), | ||
| ucfirst( $source->us_bank_account->account_type ), | ||
| $source->us_bank_account->last4 | ||
| ); | ||
| break 2; | ||
| case WC_Stripe_Payment_Methods::BECS_DEBIT: | ||
| $payment_method_to_display = sprintf( | ||
| /* translators: last 4 digits of account. */ | ||
| __( 'BECS Direct Debit ending in %s', 'woocommerce-gateway-stripe' ), | ||
| $source->au_becs_debit->last4 | ||
| ); | ||
| break 2; | ||
| case WC_Stripe_Payment_Methods::ACSS_DEBIT: | ||
| $payment_method_to_display = sprintf( | ||
| /* translators: 1) bank name, 2) last 4 digits of account. */ | ||
| __( 'Via %1$s ending in %2$s', 'woocommerce-gateway-stripe' ), | ||
| $source->acss_debit->bank_name, | ||
| $source->acss_debit->last4 | ||
| ); | ||
| break 2; | ||
| case WC_Stripe_Payment_Methods::BACS_DEBIT: | ||
| /* translators: 1) the Bacs Direct Debit payment method's last 4 numbers */ | ||
| $payment_method_to_display = sprintf( __( 'Via Bacs Direct Debit ending in (%1$s)', 'woocommerce-gateway-stripe' ), $source->bacs_debit->last4 ); | ||
| break 2; | ||
| case WC_Stripe_Payment_Methods::AMAZON_PAY: | ||
| /* translators: 1) the Amazon Pay payment method's email */ | ||
| $payment_method_to_display = sprintf( __( 'Via Amazon Pay (%1$s)', 'woocommerce-gateway-stripe' ), $source->billing_details->email ?? '' ); | ||
| break 2; | ||
| } | ||
| } | ||
| } catch ( WC_Stripe_Exception $e ) { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.Copilot uses AI. Check for mistakes.