-
Notifications
You must be signed in to change notification settings - Fork 217
Implement cache prefetch for payment method configuration #4637
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 implements a cache prefetch mechanism for payment method configurations to reduce user delays caused by cache expiration. When the cache expires, multiple users could experience slow responses while the data is refetched from the API.
- Implements a prefetch window system that triggers cache refresh before expiration
- Adds tracking mechanisms using WordPress options to prevent duplicate prefetch operations
- Integrates with Action Scheduler for asynchronous cache repopulation
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
includes/class-wc-stripe-database-cache-prefetch.php |
New class implementing the core prefetch logic with configurable windows and async handling |
includes/class-wc-stripe-database-cache.php |
Adds prefetch triggering logic and refactors expiry time calculation |
includes/class-wc-stripe-payment-method-configurations.php |
Changes cache key visibility to support prefetch configuration |
includes/class-wc-stripe.php |
Registers the new prefetch class and async action handler |
tests/phpunit/WC_Stripe_Database_Cache_Prefetch_Test.php |
Comprehensive test coverage for prefetch functionality |
Comments suppressed due to low confidence (1)
includes/class-wc-stripe-database-cache.php:1
- Use strict comparison (
===
) instead of loose comparison (==
) for null checks to avoid type coercion issues.
<?php
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
* @return bool True if the cache key can be prefetched, false otherwise. | ||
*/ | ||
public function should_prefetch_cache_key( string $key ): bool { | ||
return isset( self::PREFETCH_CONFIG[ $key ] ) && self::PREFETCH_CONFIG[ $key ] > 0; |
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.
Optional:
return isset( self::PREFETCH_CONFIG[ $key ] ) && self::PREFETCH_CONFIG[ $key ] > 0; | |
return ( self::PREFETCH_CONFIG[ $key ] ?? 0 ) > 0; |
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 code looks good, and tests well.
For now, the window for the PMC cache key is 10 seconds, which is pretty generous, as it gives us time to complete the API calls for the refetch before the main entry expires. Should this be smaller?
10 seconds sounds like a reasonable amount.
In my mind, I think it's worth it for the PMC data, as that can impact a number of callers and slow down their experience pretty regularly.
I agree, and we should also consider changing the current 10-minute TTL to a 60- or 120-minute TTL (similar to the account cache).
A merchant enabling/disabling payment methods directly from the Stripe Dashboard is not that frequent.
It might be worth it for account data as well, but that expires every 120 minutes (2 hours), so the volume impact is smaller.
I think we should use it for the account data too.
--
Not a blocker by any means, but I'm wondering if we should add a filter to allow merchants to opt out of prefetching?
The likely candidate is in should_prefetch_cache_key()
, but that might give the impression that they can return true in the filter for any key and it will be prefetched.
Another approach would be to use the filter to allow overriding PREFETCH_CONFIG
, which has the benefit of also allowing merchants to customize the prefetch window (or set it to 0 to disable).
* @return bool True if the cache key can be prefetched, false otherwise. | ||
*/ | ||
public function should_prefetch_cache_key( string $key ): bool { | ||
return isset( self::PREFETCH_CONFIG[ $key ] ) && self::PREFETCH_CONFIG[ $key ] > 0; |
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 add a filter here to provide a way for merchants to disable the prefetch for a specific key if they want.
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.
Change looks good to me and works as expected 🚢
@diegocurbelo, apologies for the delay! Thanks for the excellent feedback and discussion about future options. For now, I think it would be better to get the core changes in this PR merged (so we don't need a retest), and then ship the following changes in follow-up PRs:
|
Changes proposed in this Pull Request:
This PR explores a cache prefetch implementation for a site's payment method configurations, as we are seeing busy sites generate multiple API calls immediately after the cache expires, which means that we have multiple users getting delayed responses every 10 minutes when the payment method configuration cache expires.
The PR handles this as follows:
wc_stripe_database_cache_prefetch_async
action that accepts the cache key to prefetchpayment_method_configuration
key, which callsWC_Stripe_Payment_Method_Configurations::get_upe_enabled_payment_method_ids( true );
to force a refetch of the PMC.Note that we can't completely prevent multiple refetches, but relying on a local database option means the timing window is much smaller than for the case where the cache entry expires and the second and subsequent callers need to hit the site before the PMC API call returns.
Concerns and questions
Testing instructions
CONFIGURATION_CACHE_EXPIRATION
constant inincludes/class-wc-stripe-payment-method-configurations.php
from10 * MINUTE_IN_SECONDS
to2 * MINUTE_IN_SECONDS
woocommerce-gateway-stripe
file from today.payment_method_configurations
Enqueued cache prefetch
entry, followed by a fewCache prefetch already pending
entries, and then a successfulpayment_method_configurations
API call and aSuccessfully prefetched cache key
entry.Unable to enqueue cache prefetch: Action Scheduler is not initialized or available
entries as well -- in these cases, I think it's OK to simply skip the refetch logic if we can't actually queue up the relevant deferred actions.Changelog entry
Changelog Entry Comment
Comment
Post merge