-
Notifications
You must be signed in to change notification settings - Fork 216
Add method to fetch and cache all saved payment methods for a customer #4567
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
Changes from 9 commits
91d638b
35837db
9cfccaf
68bee81
c622d76
2e20c28
b733cd3
243d766
341f885
20ade01
6c9eb2f
0b12464
b6e1e68
aaa7ba7
188098a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -30,6 +30,11 @@ class WC_Stripe_Customer { | |||||
| WC_Stripe_UPE_Payment_Method_Becs_Debit::STRIPE_ID, | ||||||
| ]; | ||||||
|
|
||||||
| /** | ||||||
| * The maximum value for the `limit` argument in the Stripe payment_methods API. | ||||||
| */ | ||||||
| protected const PAYMENT_METHODS_API_LIMIT = 100; | ||||||
|
|
||||||
| /** | ||||||
| * Stripe customer ID | ||||||
| * | ||||||
|
|
@@ -712,7 +717,7 @@ public function get_payment_methods( $payment_method_type ) { | |||||
| [ | ||||||
| 'customer' => $this->get_id(), | ||||||
| 'type' => $payment_method_type, | ||||||
| 'limit' => 100, // Maximum allowed value. | ||||||
| 'limit' => self::PAYMENT_METHODS_API_LIMIT, | ||||||
| ], | ||||||
| 'payment_methods' . $params, | ||||||
| 'GET' | ||||||
|
|
@@ -741,6 +746,94 @@ public function get_payment_methods( $payment_method_type ) { | |||||
| return empty( $payment_methods ) ? [] : $payment_methods; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * 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(); | ||||||
|
||||||
| $cache_key = self::PAYMENT_METHODS_TRANSIENT_KEY . '__all_' . $this->get_id(); | |
| $cache_key = self::PAYMENT_METHODS_TRANSIENT_KEY . self::PAYMENT_METHODS_ALL_SEPARATOR . $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.
I think our long-term plan here should just be to use one cache, as most users don't have a large number of saved payment methods, and I think most of our use cases revolve around all saved payment methods rather than saved payment methods for only one particular type of payment.
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.
Shouldn't we use the WC_Stripe_Database_Cache here?
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.
We could, but I was wary of adding new code and adding new data to the database cache before we have a cleanup mechanism (which I created an issue for earlier today in #4569).
For large stores with regular customers, using the database cache would result in a big jump in the options size, which could easily drive other performance issues on sites.
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 also want to pull @diegocurbelo into this thread as he flagged the persistent cache usage as well in his review.
I've implemented calling WC_Stripe_Database_Cache in #4570, mostly so we can merge that change into this branch if we decide to go in that direction.
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.
For large stores with regular customers, using the database cache would result in a big jump in the options size, which could easily drive other performance issues on sites.
Expired transients are not automatically removed from the DB; that requires custom code (calling delete_expired_transients ()) or using a plugin... and each transient adds two items to the options table (one with the data, and one with the expiration), the DB cache uses one so it would be half the amount of entries than using transients.
I can create a quick PR that adds an additional options entry for the expiration timestamp in the private write_to_cache() method, and then add a new public method delete_expired() to WC_Stripe_Database_Cache similar to this one. And schedule it to run every 24 hours (filterable).
This would generate the same number of options entries as using transients, and provide a way for custom code to remove expired cache items, what do you think @daledupreez?
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.
For now, I am concerned about timelines and stability, and what changes should (and should not) be included in 9.8.0. Maybe it makes sense to have a quick sync with the team to work things out?
Copilot
AI
Aug 8, 2025
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 hardcoded expand parameters in the API request URL make the code inflexible. Consider extracting these parameters into a constant or making them configurable to improve maintainability.
| $response = WC_Stripe_API::request( $request_params, 'payment_methods?expand[]=data.sepa_debit.generated_from.charge&expand[]=data.sepa_debit.generated_from.setup_attempt', 'GET' ); | |
| $response = WC_Stripe_API::request( $request_params, 'payment_methods?' . self::PAYMENT_METHODS_EXPAND_PARAMS, 'GET' ); |
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 would love to, but that should be tackled in a follow-up PR.
daledupreez marked this conversation as resolved.
Show resolved
Hide resolved
wjrosa marked this conversation as resolved.
Show resolved
Hide resolved
wjrosa marked this conversation as resolved.
Show resolved
Hide resolved
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.
Since this is getting lengthy, I would consider creating new private methods to handle different parts of the logic.
Uh oh!
There was an error while loading. Please reload this page.