Stop using dynamic keys with the database cache #11092
Conversation
… & locale in the cached value, rather than different prefix.
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: 0 B Total Size: 873 kB ℹ️ View Unchanged
|
Some functionality (ex. clearing payment methods) was moved to the token service, and the service was added where needed. This left the database cache clear of all methods that have dynamic keys. The token service still needs cache and some improvements.
…rom the DB cache with prefix
leonardola
left a comment
leonardola
left a comment
There was a problem hiding this comment.
My understanding is that we only remove cached payment methods when there is an merchant account update, the customer deletes a payment method, the customer adds a new payment method or the customer sets the default payment method.
Do you think it would be beneficial to add a scheduled action to proactively remove stale cached methods? reducing the size of the wp_usermeta table?
|
Thanks for the review @leonardola!
Yes and no. It could potentially be beneficial, but I'd say that the effect would be rather negligible. The way metadata is loaded (altogether at the first load of the object, user in this case), up to a single entry per user does not constitute a significant slow-down in any case. On the other hand, even with just Card and Link enabled, not having the data handy, slows down the initial checkout page by more than a second for me. I think this is worth the sacrifice. Regarding tests, I might have decided to over-utilize Cursor at the end of my work day. I'll optimize them first thing tomorrow. |
leonardola
left a comment
leonardola
left a comment
There was a problem hiding this comment.
I tried different methods to trigger the removal of the wp_options data but it never happened. See comments below
| if ( | ||
| is_array( $cached_data ) | ||
| && isset( $cached_data[ $cache_key ] ) | ||
| && is_array( $cached_data[ $cache_key ] ) |
There was a problem hiding this comment.
When I visit /my-account > Payment Methods the $cache_key is set to payment_methods_ instead of payment_methods_woocommerce_payments so the cache is not used for the first page load.
Steps followed:
- Remove the
_wcpay_payment_methodsmeta from the database - Add breakpoint here
- Visit /my-account > Payment Methods
- First time the function is called the cache does not exist
- Second time it does but uses the wrong key so it has to recreate the cache again
- Same happens when I change the default payment method
There was a problem hiding this comment.
Great catch! We are using an undocumented filter from core and assuming that the gateway ID is always a string, even though that is not the case.
Seems like this will require me to rewrite the get_payment_methods_from_stripe method and particularly the format of the cached data. Right now, it follows this format:
{
"customer_id": "cus_XYZ",
"payment_methods_{gateway_id}": [
// payment methods here
]
}However, with the gateway ID possibly missing, I can't use it. I'll switch back to using the $type from $retrievable_payment_method_types instead of the gateway ID. The data would look pretty similar with {type} instead of {gateway_id} in the key, but instead of checking the cache and returning it at once, I'll always go through the foreach loop for each payment method and check the cache there.
I'll implement this on Monday, but I'm curious what you think in the meantime.
There was a problem hiding this comment.
I'm not sure which option is the fastest but caching something with the wrong name is prone to other problems. Let's try the loop version.
| * Clear all cached payment methods. | ||
| * Used when account data is updated and all payment method caches need to be cleared. | ||
| */ | ||
| public function clear_all_cached_payment_methods() { |
There was a problem hiding this comment.
I updated the merchant account via stripe dashboard. The account.updated event got to the transact server but this function was never called. What's the proper way to trigger this flow?
Steps taken:
- Visit Stripe account dashboard
- Make sure your transact server is listening to Stripe webhooks
- Add a breakpoint here
- Update the
Personal details - Notice the function is never called
There was a problem hiding this comment.
The account.updated webhook for the client seems not to be a mirror image of Stripe's account.updated, nor sent at the same time.
I assume that you were expecting this to happen through the changed webhook processing service. However, after this PR (very recent), the account.updated seems to be sent upon specific conditions, ex. hitting some account-based limit.
I added 97d1540 to make sure that we use the new account.deleted webhook to clean up payment methods.
Now, you should be able to hit the breakpoint shortly after starting the account reset process, even with a test drive account.
There was a problem hiding this comment.
I don't see the account.deleted webhook being triggered after reseting and re-onboarding the test account and it didn't hit the breakpoint either.
leonardola
left a comment
leonardola
left a comment
There was a problem hiding this comment.
Overall the changes look good. Left a few comments about backward compatibility and the old options still not being deleted
| * @return ?array Fields data, or NULL if failed to retrieve. | ||
| */ | ||
| public function get_fields_data( string $locale = '', bool $force_refresh = false ): ?array { | ||
| public function get_fields_data( string $locale = '' ): ?array { |
There was a problem hiding this comment.
This is public method. I'm not sure about the backward compatibility policy for WooPayments but any other 3rd party plugin using this method will break after this change
| $payment_methods = $this->payments_api_client->get_recommended_payment_methods( $country_code, $locale ); | ||
|
|
||
| // Indicate that the cached value is specific for the given locale and country code. | ||
| return [ |
There was a problem hiding this comment.
Changing the returned data also breaks backward compatibility
Fixes #10617
Changes proposed in this Pull Request
WC_Payments_Onboarding_Servicein favor of storing the locale (& country code) within the array. This is done under the assumption that those change, but not frequently.The latter is the bigger part of this PR, and replaces the current storage of payment methods as options with dynamic keys to user meta under a single key (
_wcpay_payment_methods). The stored value looks like this:{ "customer_id": "cus_TEwTgNq3kg25ff", "payment_methods_woocommerce_payments": [ { "payment_method" }, { "payment_method" } ] }Upon follow-up calls, we validate that the customer ID is the same as expected and if so, use the cached value.
Considerations:
Testing instructions
I recommend connecting to xDebug within
get_payment_methods_from_stripe()to confirm whether the cache is hit, busted, or updated properly.npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.