Implement custom database cache #4331

diegocurbelo · 2025-05-16T21:41:52Z

Fixes STRIPE-<linear_issue_id>
Fixes #4247, #4182

Changes proposed in this Pull Request:

This PR implements a custom database cache layer for persistent caching with in-memory optimization. It's based on the WooPayments Database Cache implementation class.

It's intended as a drop-in replacement of the current transient-based implementation; future PRs will move the live/test logic to the cache layer.

NOTE: It keeps the Fetch cooldown (but adds a key suffix) for now; it will be replaced with a 5-minute cache for admin pages in a future PR.

Testing instructions

To easily test the cache edit the file includes/class-wc-stripe-api.php add a WC_Stripe_Logger::log( 'get_payment_method_configurations' ) to the function get_payment_method_configurations (line 505).

Or add a breakpoint on line 506 and enable Xdebug.

Go to the plugin settings: WP-Admin > WooCommerce > Settings
Go to Payments and open the Stripe Gateway details page
Go to the Payment Methods tab
Check that only one call to the Configurations API was made
Reload the page, and check that a new Configurations API call was made (always get the latest data for the settings page)
Go to the Store, add any product to the cart, go to the cart, and then go to the checkout
Check that no new Configurations API call was made

The new Cache implementation uses an option, you can check it with:
wp option get wcstripe_test_payment_method_configuration_cache
It should show something like this:

% wp option get wcstripe_test_payment_method_configuration_cache
array (
  'data' =>
  (object) array(
     'id' => 'pmc_1P9Z7lH8j9v0pLPbZq3PvAta',
     'object' => 'payment_method_configuration',
     'active' => true,
     'affirm' =>
    (object) array(
       'available' => false,
       'display_preference' =>
      (object) array(
         'overridable' => true,
         'preference' => 'off',
         'value' => 'off',
      ),
    ),
  ...
  'card' =>
    (object) array(
       'available' => true,
       'display_preference' =>
      (object) array(
         'overridable' => true,
         'preference' => 'on',
         'value' => 'on',
      ),
    ),
  ...
  ),
  'updated' => 1747680270,
  'ttl' => 600,
)

Covered with tests (or have a good reason not to test in description ☝️)
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)

…om-cache

annemirasol

Verified no excessive PMC fetches
Verified able to update payment methods in wp-admin and immediately see them in the store front

Found a possibly redundant call to delete_option().

includes/class-wc-stripe-payment-method-configurations.php

Co-authored-by: Anne Mirasol <anne.mirasol@automattic.com>

daledupreez

This is testing well with one minor note: we've kept the cooldown logic in effect, so we won't re-fetch data while we're in the cooldown. (This impacts step 5 in the test instructions.)

I have a lot of smaller feedback items, but I want to call out two:

We need to make a call on whether we want prefixing for the options, and how that should work.
We should add some guards for bad data in is_expired().

It's probably worth adding an explicit link to the WooCommerce Payments code we're using to start with: https://github.com/Automattic/woocommerce-payments/blob/4b084af108cac9c6bd2467e52e5cdc3bc974a951/includes/class-database-cache.php

includes/class-wc-stripe-database-cache.php

daledupreez · 2025-05-20T07:59:12Z

includes/class-wc-stripe-database-cache.php

+		// Yes, there is the possibility that we attempt to write the same data multiple times within the SAME second,
+		// and we will mistakenly think that the DB write failed. We are OK with this false positive,
+		// since the actual data is the same.
+		$result = update_option( $key, $cache_contents, 'no' );


I have multiple comments here:

I am not sure why we're using autoload = 'no', as I believe we're expecting to need this cache regularly, so not autoloading the value is unexpected for me.

It may have to do with other processes being more likely to fetch the value from the database later, but I still feel we should be OK with there being small timing gaps.

It looks like we should use a boolean value in more recent versions of WordPress per the PHPDoc for update_option:
https://github.com/WordPress/wordpress-develop/blob/9ebba7a9aedc57d5492e409cd0aae0b6958d3642/src/wp-includes/option.php#L831-L847

I think we should explicitly comment on why we're making whatever choice we want for this flag.

Autoloading too many options can lead to performance problems, especially if the
options are not frequently used. For options which are accessed across several places
in the frontend, it is recommended to autoload them, by using true.
For options which are accessed only on few specific URLs, it is recommended
to not autoload them, by using false.

The behavior was copied from the WooPayments implementation. I think it makes sense to set the autoload = false, as we are implementing this to be a general cache for the plugin; some values like the PMC are going to be needed frequently, but it might be better to allow specific values to set it to true (we can do this when we implement the prefix and test/live mode detection).
I don't have a strong opinion either way. I replaced the 'no' with false for now, but I'm happy to remove the parameter and let WordPress determine the autoload value.

I don't have a super-strong opinion either, but it feels like we're fetching the payment method configurations a lot, so they may qualify for a true value. But that does feel like something that is worth tackling in a follow-up.

includes/class-wc-stripe-database-cache.php

tests/phpunit/test-class-wc-stripe-database-cache.php

daledupreez · 2025-05-20T08:24:13Z

tests/phpunit/test-class-wc-stripe-database-cache.php

+	/**
+	 * Setup test.
+	 */
+	public function setUp(): void {
+		parent::setUp();
+	}


We should remove this -- it's a no-op.

Suggested change

/**

* Setup test.

*/

public function setUp(): void {

parent::setUp();

}

We should still remove this method (but it's not blocking).

daledupreez · 2025-05-20T09:07:38Z

includes/class-wc-stripe-database-cache.php

+	public static function set( $key, $data, $ttl = HOUR_IN_SECONDS ) {
+		self::write_to_cache( $key, $data, $ttl );
+	}


I think we may need to require or add a key prefix so we don't unintentionally overwrite or delete "real" options in the database. Requiring callers to provide valid keys would make it more work for callers, while adding a prefix would reduce the key length that we can support, and could lead to redundant option key names. That said, having a key prefix would allow for clearing all persistent cached data via a database query.

Suggested change

public static function set( $key, $data, $ttl = HOUR_IN_SECONDS ) {

self::write_to_cache( $key, $data, $ttl );

}

public static function set( $key, $data, $ttl = HOUR_IN_SECONDS ) {

// Requiring a key prefix

if ( ! self::is_permitted_key( $key ) ) {

// Either return early and do nothing, or throw an exception

return;

throw new InvalidArgumentException( 'Unsupported key' );

}

// Add key prefix -- that function would simply do something like return 'wc_stripe_db_cache_' . $key;

$db_key = self::get_db_key( $key );

self::write_to_cache( $key, $data, $ttl );

}

To reduce the changes needed in the WC_Stripe_Payment_Method_Configurations class, I just kept the prefixes we had in place; I agree we should add a prefix for the cache (wcstripe_cache_*) but we should also handle the test/live mode internally, so we can remove the multiple WC_Stripe_Mode::is_test() ternary ifs we have everytime we use the cache. I'll make a new PR for that improvement.

Co-authored-by: daledupreez <dale.du.preez@automattic.com>

annemirasol

Looks overall solid to me.

Tested, while logging when fallback cache is used:

when cache is expired, we fetch the PMC
within cooldown, no fetches observed, even when reloading the payment methods page
when cache is not expired, storefront does not trigger a PMC fetch
after updating payment methods, fetch is observed
no indication fallback cache had to be used

daledupreez

I think we can get this shipped as-is, and then iterate from there.

Noting that I didn't retest, but I did check the changes since my last review and they look good. Also, Anne (re)tested, so I feel confident in the code.

includes/class-wc-stripe-database-cache.php

daledupreez · 2025-05-21T10:50:07Z

tests/phpunit/test-class-wc-stripe-database-cache.php

+	/**
+	 * Setup test.
+	 */
+	public function setUp(): void {
+		parent::setUp();
+	}


We should still remove this method (but it's not blocking).

* Add database cache class * Replace PMC cache transients with custom database cache Co-authored-by: Anne Mirasol <anne.mirasol@automattic.com> Co-authored-by: Dale du Preez <dale@automattic.com>

diegocurbelo added 5 commits May 16, 2025 17:26

Add database cache class

042c4fb

Add tests

d291371

Replace PMC cache transients with custom database cache

b40e271

Fix typos

86fde85

Add changelog entry

5182536

diegocurbelo self-assigned this May 16, 2025

diegocurbelo added 2 commits May 19, 2025 14:56

Merge remote-tracking branch 'origin/develop' into try/implement-cust…

d3f569c

…om-cache

Merge back the api cooldown fallback

1b00e37

diegocurbelo requested review from daledupreez, a team and annemirasol May 19, 2025 19:03

diegocurbelo marked this pull request as ready for review May 19, 2025 19:04

diegocurbelo changed the title ~~[WIP] Implement custom database cache~~ Implement custom database cache May 19, 2025

annemirasol reviewed May 19, 2025

View reviewed changes

includes/class-wc-stripe-payment-method-configurations.php Show resolved Hide resolved

includes/class-wc-stripe-payment-method-configurations.php Outdated Show resolved Hide resolved

includes/class-wc-stripe-payment-method-configurations.php Outdated Show resolved Hide resolved

diegocurbelo and others added 2 commits May 19, 2025 17:49

Apply suggestions from code review

4dd15ba

Co-authored-by: Anne Mirasol <anne.mirasol@automattic.com>

Merge branch 'develop' into try/implement-custom-cache

7d89bb5

daledupreez reviewed May 20, 2025

View reviewed changes

diegocurbelo and others added 4 commits May 20, 2025 12:24

Fix review comments

9762623

Update tests/phpunit/test-class-wc-stripe-database-cache.php

4f1bc6c

Co-authored-by: daledupreez <dale.du.preez@automattic.com>

Improve tests

5dc84ec

Fix deprecated param in update_option

60532d4

diegocurbelo requested review from annemirasol and daledupreez May 20, 2025 21:13

annemirasol approved these changes May 20, 2025

View reviewed changes

daledupreez approved these changes May 21, 2025

View reviewed changes

daledupreez and others added 4 commits May 21, 2025 14:18

Small comment fixes

cc434a2

Merge branch 'develop' into try/implement-custom-cache

31fa2eb

Code cleanup

d81e9fe

Merge branch 'develop' into try/implement-custom-cache

ba7de12

diegocurbelo enabled auto-merge (squash) May 21, 2025 13:18

diegocurbelo merged commit 428a745 into develop May 21, 2025
52 checks passed

diegocurbelo deleted the try/implement-custom-cache branch May 21, 2025 13:28

daledupreez mentioned this pull request May 23, 2025

UI is being collapse in the latest version of this plugin on wordress #4353

Closed

-	public static function set( $key, $data, $ttl = HOUR_IN_SECONDS ) {
-		self::write_to_cache( $key, $data, $ttl );
-	}
+	public static function set( $key, $data, $ttl = HOUR_IN_SECONDS ) {
+		// Requiring a key prefix
+		if ( ! self::is_permitted_key( $key ) ) {
+		 	// Either return early and do nothing, or throw an exception
+		 	return;
+		 	throw new InvalidArgumentException( 'Unsupported key' );
+		}
+		// Add key prefix -- that function would simply do something like return 'wc_stripe_db_cache_' . $key;
+		$db_key = self::get_db_key( $key );
+		self::write_to_cache( $key, $data, $ttl );
+	}

Implement custom database cache #4331

Implement custom database cache #4331

Uh oh!

Conversation

diegocurbelo commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Testing instructions

Changelog entry

Comment

Uh oh!

annemirasol left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daledupreez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

daledupreez May 20, 2025

Choose a reason for hiding this comment

Uh oh!

diegocurbelo May 20, 2025 • 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!

daledupreez May 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daledupreez May 20, 2025

Choose a reason for hiding this comment

Uh oh!

daledupreez May 21, 2025

Choose a reason for hiding this comment

Uh oh!

daledupreez May 20, 2025

Choose a reason for hiding this comment

Uh oh!

diegocurbelo May 20, 2025

Choose a reason for hiding this comment

Uh oh!

annemirasol left a comment

Choose a reason for hiding this comment

Uh oh!

daledupreez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

daledupreez May 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

diegocurbelo commented May 16, 2025 •

edited

Loading

diegocurbelo May 20, 2025 •

edited

Loading