Refactor WC_Stripe_Subscriptions_Trait->maybe_render_subscription_payment_method() #4568

daledupreez · 2025-08-08T15:33:12Z

Changes proposed in this Pull Request:

This PR refactors the code in WC_Stripe_Subscriptions_Trait->maybe_render_subscription_payment_method(). There are a few aspects to the refactor that are worth capturing in more detail:

We now fetch only the specific payment method saved with the current subscription via the internal WC_Stripe_API::get_payment_method() API so we can handle older src_ IDs.
We use a static variable cache to avoid unnecessary API calls within the same request, as I saw two API requests triggered when viewing the Subscription details page
The logic to translate the payment method details to a string has been extracted into a helper function that is simpler than what we had.
I've also expanded the unit tests to cover more payment method types

Open questions

~~I am not sure how/if we need to handle older src_* sources. I don't know how Stripe handles those via the payment_methods APIs.~~ As per this suggestion from @Mayisha, I've switched to calling WC_Stripe_API::get_payment_method() and then check the customer ID afterwards.
I don't know if it's worth using a transient for the payment method caching, or whether we should just cache the payment method without the customer ID dimension. I think both should be unique.

Testing instructions

Set up a store with WooCommerce Subscriptions installed and Stripe is connected
Ensure you have multiple payment methods that support recurring payments enabled
Ensure that you have a plugin like Debug Bar installed or that you have logging enabled for the Stripe plugin
While logged in, purchase a subscription
While running on develop, navigate to My account > Subscriptions > Your just-purchased subscription (or My Subscription if you only have one subscription)
Note what is displayed for the "Payment" row (see the screenshot below)
Check your logs and verify that you see multiple API calls to the Stripe /v1/payment_methods API
Switch to this branch -- git checkout refactor/subscriptions-trait-payment-method-rendering
Reload the subscription page
Verify that you have the same value shown for the "Payment" element
Verify that only one API call was made to the Stripe /v1/payment_methods/:payment_method_id endpoint
If you have an older src_* payment method ID, it would be good to test that, too!

Covered with tests (or have a good reason not to test in description ☝️)
[N/A] Tested on mobile (or does not apply)

Changelog entry

This Pull Request does not require a changelog entry. (Comment required below)

Changelog Entry Comment

Comment

Post merge

Added testing instructions to the Release Testing Instructions wiki page (or does not apply)
Added the needs docs label (or does not apply)
Included this PR in the Release Thread scope (or does not apply)

…ment_method() to use get_all_payment_methods()

…tion_payment_method() to use get_all_payment_methods()" This reverts commit c622d76.

…ption_payment_method() to use get_all_payment_methods()" This reverts commit 2e20c28.

includes/compat/trait-wc-stripe-subscriptions.php

…od-rendering

… cache

…od-rendering

Copilot

Pull Request Overview

This PR refactors the subscription payment method rendering logic to reduce unnecessary Stripe API calls. Instead of iterating through all payment method types and making multiple API calls to find a matching payment method, it now makes a single direct API call to retrieve the specific payment method details.

Replaces loop-based payment method retrieval with direct API call using customer ID and payment method ID
Extracts payment method display logic into separate helper methods for better maintainability
Adds comprehensive test coverage for various payment method types

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
includes/compat/trait-wc-stripe-subscriptions.php	Refactors `maybe_render_subscription_payment_method()` to use direct API calls and extracts display logic into helper methods
tests/phpunit/WC_Stripe_Payment_Gateway_Test.php	Replaces simple test with comprehensive data provider covering multiple payment method types
readme.txt	Adds changelog entry for the optimization
changelog.txt	Adds changelog entry for the optimization

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-08-15T10:35:04Z

includes/compat/trait-wc-stripe-subscriptions.php

+		}
+
+		if ( isset( $cached_payment_methods[ $stripe_customer_id ][ $payment_method_id ] ) ) {
+			return $cached_payment_methods[ $stripe_customer_id ][ $payment_method_id ];


Using a static variable for caching in a web environment can lead to data leakage between requests or users. Consider using WordPress transients or object cache instead for safer caching that respects request boundaries.

Suggested change

return $cached_payment_methods[ $stripe_customer_id ][ $payment_method_id ];

if ( empty( $stripe_customer_id ) || empty( $payment_method_id ) ) {

return null;

}

$cache_key = 'stripe_sub_pm_' . md5( $stripe_customer_id . '|' . $payment_method_id );

$cache_group = 'stripe_payment_methods';

$saved_payment_method = wp_cache_get( $cache_key, $cache_group );

if ( false !== $saved_payment_method ) {

return $saved_payment_method;

For this use case, I think it's OK to use a static variable, due to us using the customer ID and payment method ID in the path, but I'd be happy to switch to a transient.

includes/compat/trait-wc-stripe-subscriptions.php

tests/phpunit/WC_Stripe_Payment_Gateway_Test.php

…od-rendering

Mayisha

Functionally it is working as described. I see only one request to customers/cus_abc/payment_methods/pm_xyz in my logs 🎉

Mayisha · 2025-08-18T07:52:07Z

includes/compat/trait-wc-stripe-subscriptions.php

+	/**
+	 * Helper function to get the descriptive text for a payment method or source.
+	 *
+	 * @param object $source The payment method or source object.


Optional: My preference would be to call it a payment_method in all the new methods.

Good call. Updated in 9e6ebdd

Mayisha · 2025-08-18T07:58:41Z

includes/compat/trait-wc-stripe-subscriptions.php

+
+		$encoded_customer_id       = rawurlencode( $stripe_customer_id );
+		$encoded_payment_method_id = rawurlencode( $payment_method_id );
+		$api_path                  = "customers/{$encoded_customer_id}/payment_methods/{$encoded_payment_method_id}";


As an alternate approach which would cover this related to the open question as well,

I am not sure how/if we need to handle older src_* sources. I don't know how Stripe handles those via the payment_methods APIs.

we can retrieve the payment method or source using the get_payment_method method and check if payment_method->customer matches the $stripe_customer_id.

AFAIK, /payment_methods/src_xyz fails so customers/customer_abc/payment_methods/src_xyz might also fail.

Good call. I've switched that over in 3aecb6c.

Mayisha · 2025-08-18T08:00:37Z

includes/compat/trait-wc-stripe-subscriptions.php

+	}
+
+	/**
+	 * Helper function to get and temporarily cache the payment method details for a customer and payment method ID.


Optional: we can move this helper method to WC_Stripe_Subscriptions_Helper.

I think this is also a good suggestion. Done in 2920871.

diegocurbelo

The code changes look good, and it tests well 🚢

I don't know if it's worth using a transient for the payment method caching, or whether we should just cache the payment method without the customer ID dimension. I think both should be unique.

I like the transient idea, I think is consistent with how we are caching other things.. but we cal also improve this later, after we add a auto-cleanup mechanism to the DB cache.

daledupreez added 7 commits August 7, 2025 13:40

Basic get_all_payment_methods() implementation

91d638b

Ensure we clear the new _all payment methods cache

35837db

Always send expand flags for SEPA

9cfccaf

Refactor logic to get customer UPE tokens

68bee81

Refactor WC_Stripe_Subscriptions_Trait->maybe_render_subscription_pay…

c622d76

…ment_method() to use get_all_payment_methods()

Revert "Refactor WC_Stripe_Subscriptions_Trait->maybe_render_subscrip…

2e20c28

…tion_payment_method() to use get_all_payment_methods()" This reverts commit c622d76.

Reapply "Refactor WC_Stripe_Subscriptions_Trait->maybe_render_subscri…

5dc3c1e

…ption_payment_method() to use get_all_payment_methods()" This reverts commit 2e20c28.

daledupreez requested a review from Copilot August 8, 2025 15:33

This comment was marked as outdated.

Sign in to view

daledupreez commented Aug 8, 2025

View reviewed changes

includes/compat/trait-wc-stripe-subscriptions.php Show resolved Hide resolved

Update unit test to use new cache

80bfbbf

Base automatically changed from refactor/fetching-all-customer-payment-methods to develop August 11, 2025 14:52

daledupreez and others added 8 commits August 11, 2025 16:54

Merge branch 'develop' into refactor/subscriptions-trait-payment-meth…

0ddecfb

…od-rendering

Merge branch 'develop' into refactor/subscriptions-trait-payment-meth…

58e88e5

…od-rendering

Merge branch 'develop' into refactor/subscriptions-trait-payment-meth…

e87b5c9

…od-rendering

Switch to single call to fetch payment method and add static variable…

7511081

… cache

Make API construction more declarative

9af152c

Merge branch 'develop' into refactor/subscriptions-trait-payment-meth…

83cd681

…od-rendering

Changelog

5aabff5

Update and expand unit tests

b0f5b3a

daledupreez requested a review from Copilot August 15, 2025 10:34

Copilot AI reviewed Aug 15, 2025

View reviewed changes

daledupreez marked this pull request as ready for review August 15, 2025 10:54

daledupreez requested review from a team, diegocurbelo and Mayisha and removed request for a team August 15, 2025 10:54

daledupreez self-assigned this Aug 15, 2025

Merge branch 'develop' into refactor/subscriptions-trait-payment-meth…

3a2a111

…od-rendering

Mayisha reviewed Aug 18, 2025

View reviewed changes

daledupreez added 5 commits August 18, 2025 11:13

Change parameter to $payment_method instead of $source

9e6ebdd

Move payment method fetch to WC_Stripe_Subscriptions_Helper

2920871

Switch to WC_Stripe_API::get_payment_method() to support src_ IDs

3aecb6c

Update unit tests

8d2077e

Use counter for IDs

7522668

daledupreez mentioned this pull request Aug 18, 2025

Reduce number of payment_methods API calls #4560

Closed

6 tasks

diegocurbelo approved these changes Aug 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor WC_Stripe_Subscriptions_Trait->maybe_render_subscription_payment_method() #4568

Refactor WC_Stripe_Subscriptions_Trait->maybe_render_subscription_payment_method() #4568

daledupreez commented Aug 8, 2025 •

edited

Loading

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Aug 15, 2025

Uh oh!

daledupreez Aug 18, 2025

Uh oh!

Uh oh!

Uh oh!

Mayisha left a comment

Uh oh!

Mayisha Aug 18, 2025

Uh oh!

daledupreez Aug 18, 2025

Uh oh!

Mayisha Aug 18, 2025

Uh oh!

daledupreez Aug 18, 2025

Uh oh!

Mayisha Aug 18, 2025

Uh oh!

daledupreez Aug 18, 2025

Uh oh!

diegocurbelo left a comment •

edited

Loading

Uh oh!

Uh oh!

-			return $cached_payment_methods[ $stripe_customer_id ][ $payment_method_id ];
+		if ( empty( $stripe_customer_id ) || empty( $payment_method_id ) ) {
+			return null;
+		}
+		$cache_key = 'stripe_sub_pm_' . md5( $stripe_customer_id . '|' . $payment_method_id );
+		$cache_group = 'stripe_payment_methods';
+		$saved_payment_method = wp_cache_get( $cache_key, $cache_group );
+		if ( false !== $saved_payment_method ) {
+			return $saved_payment_method;

Refactor WC_Stripe_Subscriptions_Trait->maybe_render_subscription_payment_method() #4568

Are you sure you want to change the base?

Refactor WC_Stripe_Subscriptions_Trait->maybe_render_subscription_payment_method() #4568

Conversation

daledupreez commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Open questions

Testing instructions

Changelog entry

Comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

daledupreez Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Mayisha left a comment

Choose a reason for hiding this comment

Uh oh!

Mayisha Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

daledupreez Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

Mayisha Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

daledupreez Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

Mayisha Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

daledupreez Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

diegocurbelo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

daledupreez commented Aug 8, 2025 •

edited

Loading

diegocurbelo left a comment •

edited

Loading