Fix: Replace symfony/cache with WordPress-native PSR-16 implementation for SSO#357
Fix: Replace symfony/cache with WordPress-native PSR-16 implementation for SSO#357superdav42 wants to merge 4 commits intomainfrom
Conversation
…n for SSO This commit resolves a fatal error caused by incompatibility between symfony/cache v5.4 and psr/simple-cache v3.0+. Changes: - Add WordPress_Simple_Cache class implementing PSR-16 CacheInterface using WordPress transients (get_site_transient, set_site_transient) - Update SSO class to use WordPress_Simple_Cache instead of symfony/cache - Remove symfony/cache dependency from composer.json (reduces 3 packages) - Add comprehensive unit tests for WordPress_Simple_Cache - Add standalone integration tests for PSR-16 compliance Benefits: - Eliminates PSR-16 compatibility error in SSO functionality - Reduces dependency footprint (removed symfony/cache, symfony/cache-contracts, symfony/var-exporter) - Better WordPress integration using native transient APIs - Maintains full PSR-16 compatibility required by jasny/sso library Fixes: Fatal Error 'psr/simple-cache 3.0+ is not compatible with symfony/cache v5.4' Note: PHPCS warnings about 'default' parameter name and direct database calls are acceptable as they follow PSR-16 standard and are necessary for cache clearing functionality.
📝 WalkthroughWalkthroughRemoved the symfony/cache dependency and replaced the Symfony-based cache initialization with a new WordPress-backed PSR-16 Simple Cache used by the SSO class; added the cache implementation and unit/integration tests; added an explicit void return type to toggle_maintenance_mode(). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
composer.json (1)
32-68:⚠️ Potential issue | 🔴 CriticalRe-add
hashids/hashidsbefore merge.
inc/helpers/class-hash.phpimports and instantiatesHashids\Hashidson lines 45 and 61, but the dependency is missing fromcomposer.jsonrequire. Production will fail with a fatal class-not-found error whenHash::encode()orHash::decode()are called.Suggested fix
"require": { "php": ">=7.4.1", + "hashids/hashids": "^4.1.0", "automattic/jetpack-autoloader": "^5.0.0", "wp-ultimo/autoloader-plugin": "dev-main",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composer.json` around lines 32 - 68, The composer.json is missing the hashids/hashids package used by inc/helpers/class-hash.php (it instantiates Hashids\Hashids and calls Hash::encode() and Hash::decode()); add the hashids/hashids requirement to composer.json's "require" (pick a compatible semver such as ^4.0) and run composer update/install so the Hashids\Hashids class is available at runtime.
🧹 Nitpick comments (2)
tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Test.php (1)
216-230: TTL tests don’t validate expiration behavior.Current assertions only check “set + immediate get.” They won’t catch TTL conversion bugs (especially
DateInterval). Add assertions against timeout metadata or time-controlled expiry behavior.Also applies to: 250-261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Test.php` around lines 216 - 230, The TTL tests (e.g., test_ttl_with_integer_seconds) only assert set+get and miss verifying the actual expiration metadata, so update these tests to assert the stored transient timeout matches the expected expiry and/or simulate time progression to confirm expiration; specifically, after calling $this->cache->set($key, $value, $ttl) assert that the underlying timeout option (the _transient_timeout_{key} option or equivalent returned by wp_get_transient_timeout / get_option) equals time() + $ttl (or within a 1-second tolerance), and add a complementary assertion that the cached value is no longer returned after advancing simulated time or by manually setting the timeout to a past timestamp and calling $this->cache->get($key) to ensure expiration behavior is enforced (apply the same changes to the other TTL test referenced around lines 250-261).tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.php (1)
87-93: TTL integration tests are non-functional with current mock storage.
$expirationis ignored inset_site_transient()mock, so TTL assertions can’t validate expiry behavior.Also applies to: 304-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.php` around lines 87 - 93, The mock set_site_transient currently ignores $expiration so TTL tests fail; update set_site_transient to store both the value and an expiry timestamp in the global $_site_transients (e.g. store an array ['value'=>..., 'expires_at'=> time() + $expiration] or null for no TTL) and update the corresponding get_site_transient (and any delete_site_transient logic) to check $_site_transients[$key]['expires_at'] and treat expired entries as missing (remove them and return false/null) so TTL assertions work; change references to $_site_transients in both set_site_transient and get_site_transient to the new shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@inc/sso/class-wordpress-simple-cache.php`:
- Around line 83-92: The clear() method currently returns true unconditionally;
change it so it checks the result of $wpdb->query and returns failure when the
DB operation errored: capture the query call (the DELETE using $wpdb->prepare
and $wpdb->esc_like) then if $wpdb->last_error is non-empty return false,
otherwise return true (this ensures clear() reports failure on DB errors while
still treating zero affected rows as success).
- Around line 169-173: The DateInterval handling mutates $now because
DateTime::add() returns the same instance, so $future and $now end up identical
and the TTL becomes 0; fix this in class-wordpress-simple-cache.php by creating
a separate DateTime instance (e.g., clone $now or instantiate a new DateTime)
before calling add() so $future != $now, then compute and return
$future->getTimestamp() - $now->getTimestamp(); ensure this change is applied to
the code handling $ttl when it's an instance of \DateInterval (references: $ttl,
$now, $future, DateTime::add).
- Around line 46-49: The get() and has() methods currently treat WordPress
transient return value false as a cache miss, so legitimately cached boolean
false cannot be stored; update the caching layer (methods get(), has(), and the
corresponding set()/set_site_transient usage) to wrap stored values in a
sentinel structure (e.g., ['hit' => true, 'value' => ...]) before calling
set_site_transient, and on retrieval in get() unwrap that structure to return
the original value (including false) while has() should check for the wrapper
and return true only when the wrapper is present; ensure existing consumers of
get() keep receiving the raw unwrapped value and that miss handling still
returns the $default.
In `@tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.php`:
- Around line 325-331: The test runner fails because the included class file
exits when ABSPATH is undefined and the cache clear() method expects a global
$wpdb that the test doesn't provide; before requiring
'inc/sso/class-wordpress-simple-cache.php' (in
WordPress_Simple_Cache_Integration_Test runner) define a minimal ABSPATH
constant and inject a mocked global $wpdb with stubbed methods query(),
prepare(), and esc_like() (and any used properties) so clear() can run; also
ensure mock_wordpress_functions() remains in place and is called before
inclusion so get_site_transient()/set_site_transient()/delete_site_transient()
and the $wpdb mock exist when the class file is loaded and when clear() is
invoked during tests.
In `@tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Test.php`:
- Around line 332-335: The local variable $user_id is declared but never used;
remove the unused declaration (the line setting $user_id = 42) from the test to
avoid dead code, or if the test should assert on the user id, instead use
$user_id in the relevant assertion or call; locate the declaration near
$broker_id and $session_id in the test and either delete the $user_id line or
wire it into the test expectations where appropriate.
---
Outside diff comments:
In `@composer.json`:
- Around line 32-68: The composer.json is missing the hashids/hashids package
used by inc/helpers/class-hash.php (it instantiates Hashids\Hashids and calls
Hash::encode() and Hash::decode()); add the hashids/hashids requirement to
composer.json's "require" (pick a compatible semver such as ^4.0) and run
composer update/install so the Hashids\Hashids class is available at runtime.
---
Nitpick comments:
In `@tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.php`:
- Around line 87-93: The mock set_site_transient currently ignores $expiration
so TTL tests fail; update set_site_transient to store both the value and an
expiry timestamp in the global $_site_transients (e.g. store an array
['value'=>..., 'expires_at'=> time() + $expiration] or null for no TTL) and
update the corresponding get_site_transient (and any delete_site_transient
logic) to check $_site_transients[$key]['expires_at'] and treat expired entries
as missing (remove them and return false/null) so TTL assertions work; change
references to $_site_transients in both set_site_transient and
get_site_transient to the new shape.
In `@tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Test.php`:
- Around line 216-230: The TTL tests (e.g., test_ttl_with_integer_seconds) only
assert set+get and miss verifying the actual expiration metadata, so update
these tests to assert the stored transient timeout matches the expected expiry
and/or simulate time progression to confirm expiration; specifically, after
calling $this->cache->set($key, $value, $ttl) assert that the underlying timeout
option (the _transient_timeout_{key} option or equivalent returned by
wp_get_transient_timeout / get_option) equals time() + $ttl (or within a
1-second tolerance), and add a complementary assertion that the cached value is
no longer returned after advancing simulated time or by manually setting the
timeout to a past timestamp and calling $this->cache->get($key) to ensure
expiration behavior is enforced (apply the same changes to the other TTL test
referenced around lines 250-261).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92669a09-ba40-48cd-8dd8-3544b7a26bbc
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
composer.jsoninc/sso/class-sso.phpinc/sso/class-wordpress-simple-cache.phptests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.phptests/WP_Ultimo/SSO/WordPress_Simple_Cache_Test.php
| $wpdb->query( | ||
| $wpdb->prepare( | ||
| "DELETE FROM {$wpdb->sitemeta} WHERE meta_key LIKE %s OR meta_key LIKE %s", | ||
| $wpdb->esc_like('_site_transient_' . $this->prefix) . '%', | ||
| $wpdb->esc_like('_site_transient_timeout_' . $this->prefix) . '%' | ||
| ) | ||
| ); | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
clear() always returns success even on DB failure.
Line 91 returns true unconditionally. If $wpdb->query() fails, callers won’t know clear failed.
Proposed fix
- $wpdb->query(
+ $result = $wpdb->query(
$wpdb->prepare(
"DELETE FROM {$wpdb->sitemeta} WHERE meta_key LIKE %s OR meta_key LIKE %s",
$wpdb->esc_like('_site_transient_' . $this->prefix) . '%',
$wpdb->esc_like('_site_transient_timeout_' . $this->prefix) . '%'
)
);
- return true;
+ return false !== $result;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@inc/sso/class-wordpress-simple-cache.php` around lines 83 - 92, The clear()
method currently returns true unconditionally; change it so it checks the result
of $wpdb->query and returns failure when the DB operation errored: capture the
query call (the DELETE using $wpdb->prepare and $wpdb->esc_like) then if
$wpdb->last_error is non-empty return false, otherwise return true (this ensures
clear() reports failure on DB errors while still treating zero affected rows as
success).
| if ($ttl instanceof \DateInterval) { | ||
| $now = new \DateTime(); | ||
| $future = $now->add($ttl); | ||
| return $future->getTimestamp() - $now->getTimestamp(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PHP DateTime::add() mutate object return same instance
💡 Result:
Yes. DateTime::add() mutates the existing DateTime object and returns the same instance (so assignments just create another reference to the same object), mainly to allow method chaining. To avoid mutation, use DateTimeImmutable::add() (returns a new object) or clone before adding. [1], [2]
$dt = new DateTime('2026-03-05');
$dt2 = $dt->add(new DateInterval('P1D'));
var_dump($dt === $dt2); // true (same instance)$dt = new DateTime('2026-03-05');
$dt2 = (clone $dt)->add(new DateInterval('P1D')); // $dt unchanged$dt = new DateTimeImmutable('2026-03-05');
$dt2 = $dt->add(new DateInterval('P1D')); // new instanceSources: PHP manual for DateTime::add() and DateTime mutability, plus DateTimeImmutable::add() behavior. [1], [2]
[1] https://www.php.net/manual/en/datetime.add.php
[2] https://www.php.net/manual/en/class.datetime.php
🏁 Script executed:
# Check the actual file and surrounding context
find . -name "class-wordpress-simple-cache.php" -path "*/inc/sso/*" | head -5Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 123
🏁 Script executed:
# Read the file to see the complete context around lines 169-173
cat -n inc/sso/class-wordpress-simple-cache.php | sed -n '160,185p'Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 611
DateInterval TTL currently collapses to 0 (no expiration).
On line 171, DateTime::add() mutates $now and returns the same instance, so $future references the identical object. Line 172 then subtracts identical timestamps, yielding 0. This causes DateInterval entries to become non-expiring.
Proposed fix
if ($ttl instanceof \DateInterval) {
- $now = new \DateTime();
- $future = $now->add($ttl);
- return $future->getTimestamp() - $now->getTimestamp();
+ $now = new \DateTimeImmutable();
+ $future = $now->add($ttl);
+ return max(0, $future->getTimestamp() - $now->getTimestamp());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ($ttl instanceof \DateInterval) { | |
| $now = new \DateTime(); | |
| $future = $now->add($ttl); | |
| return $future->getTimestamp() - $now->getTimestamp(); | |
| } | |
| if ($ttl instanceof \DateInterval) { | |
| $now = new \DateTimeImmutable(); | |
| $future = $now->add($ttl); | |
| return max(0, $future->getTimestamp() - $now->getTimestamp()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@inc/sso/class-wordpress-simple-cache.php` around lines 169 - 173, The
DateInterval handling mutates $now because DateTime::add() returns the same
instance, so $future and $now end up identical and the TTL becomes 0; fix this
in class-wordpress-simple-cache.php by creating a separate DateTime instance
(e.g., clone $now or instantiate a new DateTime) before calling add() so $future
!= $now, then compute and return $future->getTimestamp() - $now->getTimestamp();
ensure this change is applied to the code handling $ttl when it's an instance of
\DateInterval (references: $ttl, $now, $future, DateTime::add).
| // Auto-run if this file is executed directly (not included by PHPUnit). | ||
| if (php_sapi_name() === 'cli' && !defined('PHPUNIT_COMPOSER_INSTALL')) { | ||
| // Load autoloader. | ||
| require_once dirname(dirname(dirname(__DIR__))) . '/vendor/autoload.php'; | ||
| require_once dirname(dirname(dirname(__DIR__))) . '/inc/sso/class-wordpress-simple-cache.php'; | ||
|
|
||
| WordPress_Simple_Cache_Integration_Test::run_all_tests(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify ABSPATH guard and clear() DB dependency in cache class
rg -n "defined\\('ABSPATH'\\) \\|\\| exit|function clear\\(|global \\$wpdb" inc/sso/class-wordpress-simple-cache.php
# Verify standalone runner path and clear() usage in integration test
rg -n "require_once .*class-wordpress-simple-cache.php|run_all_tests|->clear\\(" tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 1013
🏁 Script executed:
#!/bin/bash
# Check ABSPATH guard in cache class
echo "=== Checking for ABSPATH guard in class file ==="
rg "ABSPATH" inc/sso/class-wordpress-simple-cache.php -A 2 -B 2
echo ""
echo "=== Checking clear() function definition ==="
rg "function clear" inc/sso/class-wordpress-simple-cache.php -A 10
echo ""
echo "=== Checking for $wpdb usage in class file ==="
rg "wpdb" inc/sso/class-wordpress-simple-cache.php -nRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 916
🏁 Script executed:
#!/bin/bash
# Check for setUp, bootstrap, or $wpdb mocking in the test file
echo "=== Checking for setUp/init methods ==="
rg "function setUp|function __construct|global \\\$wpdb|\\$GLOBALS\[|mock|Mock" tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.php -n
echo ""
echo "=== Checking file header and class definition ==="
head -50 tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 1451
🏁 Script executed:
#!/bin/bash
# Find and display the mock_wordpress_functions method
rg -n "mock_wordpress_functions" tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.php -A 30Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 2030
🏁 Script executed:
#!/bin/bash
# Get the complete mock_wordpress_functions method
rg -n "private static function mock_wordpress_functions" tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.php -A 60Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 1846
Standalone CLI runner cannot execute due to unmet WordPress dependencies.
The included cache class file exits when ABSPATH is undefined (line 2: defined('ABSPATH') || exit;), and the clear() method requires the global $wpdb object which is not mocked in the test setup. The mock_wordpress_functions() method only provides get_site_transient, set_site_transient, and delete_site_transient—it does not mock $wpdb. Since clear() is called 21 times throughout the test suite and directly executes database queries via $wpdb->query(), $wpdb->prepare(), and $wpdb->esc_like(), the standalone runner will fail immediately upon inclusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.php` around lines
325 - 331, The test runner fails because the included class file exits when
ABSPATH is undefined and the cache clear() method expects a global $wpdb that
the test doesn't provide; before requiring
'inc/sso/class-wordpress-simple-cache.php' (in
WordPress_Simple_Cache_Integration_Test runner) define a minimal ABSPATH
constant and inject a mocked global $wpdb with stubbed methods query(),
prepare(), and esc_like() (and any used properties) so clear() can run; also
ensure mock_wordpress_functions() remains in place and is called before
inclusion so get_site_transient()/set_site_transient()/delete_site_transient()
and the $wpdb mock exist when the class file is loaded and when clear() is
invoked during tests.
| $broker_id = 'test_broker_123'; | ||
| $session_id = 'session_abc_456'; | ||
| $user_id = 42; | ||
|
|
There was a problem hiding this comment.
Remove unused local $user_id.
$user_id is declared but never used in this test.
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 334-334: Avoid unused local variables such as '$user_id'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Test.php` around lines 332 - 335,
The local variable $user_id is declared but never used; remove the unused
declaration (the line setting $user_id = 42) from the test to avoid dead code,
or if the test should assert on the user id, instead use $user_id in the
relevant assertion or call; locate the declaration near $broker_id and
$session_id in the test and either delete the $user_id line or wire it into the
test expectations where appropriate.
…e implementation bugs - Add missing patches/mpdf-psr-http-message-shim-php8-compat.patch (file was referenced in composer.json but not committed, causing composer install to fail on all PHP versions with a TypeError in cweagans/composer-patches) - Restore hashids/hashids to composer.json (was removed but still required by inc/helpers/class-hash.php — would cause fatal class-not-found in production) - Fix WordPress_Simple_Cache::get/set to wrap values in array so that storing false can be distinguished from a cache miss (get_site_transient returns false for both missing keys and stored false values) - Fix WordPress_Simple_Cache::clear() to use delete_site_transient() which properly clears the WordPress object cache, not just the DB rows (direct DB delete left stale values in the in-memory cache, causing test failures) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
inc/sso/class-wordpress-simple-cache.php (1)
181-184:⚠️ Potential issue | 🔴 CriticalClone
$nowbefore adding the interval.
DateTime::add()mutates the original object, so$futureand$nowpoint at the same timestamp here and the computed TTL collapses to0.Proposed fix
if ($ttl instanceof \DateInterval) { - $now = new \DateTime(); + $now = new \DateTimeImmutable(); $future = $now->add($ttl); - return $future->getTimestamp() - $now->getTimestamp(); + return max(0, $future->getTimestamp() - $now->getTimestamp()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inc/sso/class-wordpress-simple-cache.php` around lines 181 - 184, The code incorrectly mutates $now by calling DateTime::add(), causing $future and $now to be identical and the TTL to become 0; fix it by cloning $now before applying the DateInterval ($ttl) so you compute the difference between the original timestamp and the advanced timestamp (use a clone of the DateTime instance instead of mutating $now), updating the block that handles DateInterval to create a separate $future from $now and then return $future->getTimestamp() - $now->getTimestamp().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@inc/sso/class-wordpress-simple-cache.php`:
- Around line 46-67: Add PSR-16 validation: implement a private helper (e.g.,
validateKey(string $key): void) that throws
Psr\SimpleCache\InvalidArgumentException when a key is empty, >64 chars, or
contains any of the reserved characters {}()/\@:, and another helper (e.g.,
validateIterable($iterable): void) that throws the same exception when the
argument is not an array or Traversable; call validateKey at the start of get(),
set(), delete(), has(), and any other single-key methods (reference methods get
and set in class WordPress_Simple_Cache) and call validateIterable plus
validateKey for each item in getMultiple(), setMultiple(), and deleteMultiple()
before processing so invalid keys produce the required exception rather than
being passed to get_site_transient()/set_site_transient() or used in foreach
loops.
- Around line 84-102: The clear() method is ineffective with Redis/Memcached
because it only scans the sitemeta table; instead implement versioned namespace
invalidation: add a cache namespace/version property (e.g., $this->prefixVersion
stored in an option like "{$this->prefix}_version"), change all transient keys
(in the class methods that set/get/delete transients) to include that version
(e.g., "{$this->prefix}_v{$version}_{$name}"), and then make clear() simply
increment/update the stored version option (or regenerate the prefix) so
existing cached keys become unreachable without DB enumeration; update
references to delete_site_transient only for explicit deletes, not for global
clear().
- Around line 64-67: The set() method currently passes a 0 TTL into
set_site_transient which WordPress treats as permanent; change set() so after
calling convert_ttl_to_seconds($ttl) you detect non-positive values and replace
them with a minimal positive expiry (e.g. 1 second) so non-positive TTLs are
treated as expired rather than permanent; note setMultiple() delegates to set(),
so this single guard in set() fixes both.
---
Duplicate comments:
In `@inc/sso/class-wordpress-simple-cache.php`:
- Around line 181-184: The code incorrectly mutates $now by calling
DateTime::add(), causing $future and $now to be identical and the TTL to become
0; fix it by cloning $now before applying the DateInterval ($ttl) so you compute
the difference between the original timestamp and the advanced timestamp (use a
clone of the DateTime instance instead of mutating $now), updating the block
that handles DateInterval to create a separate $future from $now and then return
$future->getTimestamp() - $now->getTimestamp().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 538c7686-107b-45b6-b224-f1f225ca7cf0
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
composer.jsoninc/sso/class-wordpress-simple-cache.phppatches/mpdf-psr-http-message-shim-php8-compat.patch
🚧 Files skipped from review as they are similar to previous changes (1)
- composer.json
| public function clear() { | ||
| global $wpdb; | ||
|
|
||
| // Get all transient keys with our prefix to delete them properly (handles object cache). | ||
| $like_pattern = $wpdb->esc_like('_site_transient_' . $this->prefix) . '%'; | ||
| $meta_keys = $wpdb->get_col( // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching | ||
| $wpdb->prepare( | ||
| "SELECT meta_key FROM {$wpdb->sitemeta} WHERE meta_key LIKE %s", | ||
| $like_pattern | ||
| ) | ||
| ); | ||
|
|
||
| foreach ($meta_keys as $meta_key) { | ||
| // Strip the '_site_transient_' prefix to get the original transient name. | ||
| $transient_name = substr($meta_key, strlen('_site_transient_')); | ||
| delete_site_transient($transient_name); | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "\.(php|md)$" | head -20Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 699
🏁 Script executed:
find . -name "class-wordpress-simple-cache.php" -o -name "class-sso.php" | head -5Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 182
🏁 Script executed:
cat -n inc/sso/class-wordpress-simple-cache.php | head -120Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 4399
🏁 Script executed:
sed -n '980,991p' inc/sso/class-sso.phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 380
🏁 Script executed:
grep -r "set_site_transient\|get_site_transient\|delete_site_transient" inc/sso/ --include="*.php"Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 876
🏁 Script executed:
sed -n '1,100p' inc/sso/class-sso.php | grep -A 20 "cache()"Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 63
🌐 Web query:
WordPress set_site_transient behavior with Redis Memcached external object cache
💡 Result:
What set_site_transient() does with/without a persistent object cache (Redis/Memcached)
1) With an external/persistent object cache enabled (Redis, Memcached via object-cache.php)
set_site_transient( $key, $value, $expiration )stores the value in the object cache, not in the DB: it callswp_cache_set( $key, $value, 'site-transient', $expiration ). The cache group used issite-transient. [1]- The
$expirationis passed to the object-cache backend as the cache TTL (seconds). [1] - Practical implication: there will be no
_site_transient_*/_site_transient_timeout_*rows created/updated in the options/sitemeta tables by core when an external object cache is in use. [1], [2]
2) Without an external object cache (default WP behavior)
- WordPress persists site transients in the database as network options:
_site_transient_{key}= value_site_transient_timeout_{key}=time() + $expiration(only if$expirationis non-zero) [1]
- (So: DB cleanup/expiration is driven by that stored timeout, and the transient is “found” by reading those options.) [1], [3]
Important behavioral differences you’ll notice with Redis/Memcached
- Transients become “cache”, not “storage”. With a persistent object cache present, the Transients API no longer uses the Options API tables for persistence and instead relies on the object cache layer. [3]
- Eviction can happen before your TTL. Especially with Memcached (and sometimes Redis depending on policy/config), entries can disappear due to memory pressure/eviction; TTL is not a guarantee of persistence. (VIP’s Memcached notes: items expire and can also be evicted via LRU; “no expiration” items can still be evicted.) [2]
- Updating without an expiration can change semantics. Core documents that when not using an external object cache, calling
set_transient()again without$expirationkeeps the existing expiration; this guarantee does not apply with an external object cache (because the object cache is the source of truth). In practice, you should always pass an explicit expiration if you care about it. [4]
Operational gotchas (common with Redis/Memcached object-cache plugins)
- Don’t query the DB for transients (or rely on tools that do) when a persistent object cache is enabled—those transients may not be in the DB at all. [2], [3]
- Flushing Redis/Memcached flushes your transients (and can cause stampedes if many requests regenerate them at once). Treat transients as rebuildable.
Sources
[1] WordPress Developer Reference: set_site_transient() (shows wp_cache_set(..., 'site-transient', $expiration) when wp_using_ext_object_cache())
[2] WordPress VIP docs: Object cache behavior, transients stored in object cache; eviction/LRU/no-expiration notes
[3] WordPress Developer Blog: Transients API uses object cache instead of DB when persistent cache is present
[4] WordPress Developer Reference: set_transient() note about expiration retention only when not using external object cache
🏁 Script executed:
grep -r "wp_cache_flush\|flush\|clear" inc/sso/ --include="*.php" | grep -v "^\s*//"Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 313
🏁 Script executed:
rg "class WordPress_Simple_Cache" -A 200 inc/sso/class-wordpress-simple-cache.php | grep -E "(public function|private function)" | head -15Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 643
clear() becomes a no-op when using Redis/Memcached-backed object caching.
This implementation only discovers transients persisted to $wpdb->sitemeta. When Redis/Memcached is enabled, set_site_transient() stores values exclusively in the object cache, never creating database rows. As a result, clear() finds no entries and leaves all SSO state intact in the cache. A safer approach is to implement cache invalidation via a versioned namespace/prefix that doesn't depend on database enumeration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@inc/sso/class-wordpress-simple-cache.php` around lines 84 - 102, The clear()
method is ineffective with Redis/Memcached because it only scans the sitemeta
table; instead implement versioned namespace invalidation: add a cache
namespace/version property (e.g., $this->prefixVersion stored in an option like
"{$this->prefix}_version"), change all transient keys (in the class methods that
set/get/delete transients) to include that version (e.g.,
"{$this->prefix}_v{$version}_{$name}"), and then make clear() simply
increment/update the stored version option (or regenerate the prefix) so
existing cached keys become unreachable without DB enumeration; update
references to delete_site_transient only for explicit deletes, not for global
clear().
Merge changes from main (PRs #352 mpdf PHP8.5 compat, #355 lazy settings, #358 remove hashids dependency) and apply follow-up improvements: - Use main's mpdf-psr-http-message-shim-php8-compat.patch (proper PHP 8.5 type declarations for Request.php methods) - Fix TTL=0 handling: non-null TTL <= 0 now stores with 1s expiry (PSR-16) - Add PSR-16 key validation via validate_key() and validate_iterable() - Fix PHPStan: toggle_maintenance_mode() return type void Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
- Add docblocks to mock WordPress functions - Fix Yoda condition violations throughout assert calls - Suppress emoji output escaping warnings with phpcs:ignore - Suppress unused \$expiration parameter warning in mock set_site_transient Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.php (1)
77-132:⚠️ Potential issue | 🟠 MajorStandalone CLI mode still doesn't bootstrap enough WordPress state.
The direct-run path loads
inc/sso/class-wordpress-simple-cache.phpbefore any WordPress bootstrap, while this file only mocks the transient functions. Because the cache class is WordPress-guarded on load andclear()still depends on the global$wpdb,php tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.phpwill exit or fatal before the suite finishes. Please define a minimalABSPATHand$wpdbstub before requiring the cache class, or route this runner through the existing WordPress test bootstrap instead.Also applies to: 350-356
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.php` around lines 77 - 132, The test runner does not define minimal WordPress state so requiring inc/sso/class-wordpress-simple-cache.php can fatal because the cache class (WordPress_Simple_Cache) and its clear() method expect ABSPATH and the global $wpdb; update WordPress_Simple_Cache_Integration_Test::mock_wordpress_functions (or the test setup) to define('ABSPATH', __DIR__ . '/..') if not defined and create a minimal $wpdb stub global (an object with the methods/properties used by WordPress_Simple_Cache::clear) before including the cache class, or instead change the test to bootstrap via the existing WordPress test bootstrap; ensure you reference ABSPATH, global $wpdb, and the class WordPress_Simple_Cache/clear() when making the change.
🧹 Nitpick comments (1)
tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.php (1)
292-295: Add a regression check for cachedfalse.This block covers
true, but notfalse—the exact value WordPress transients also use for cache misses. SinceWordPress_Simple_Cache::get()only distinguishes a storedfalsevia the wrapped payload, skipping that case leaves the key regression in this PR unguarded here.➕ Suggested test addition
// Boolean. $cache->set('bool', true); self::assert(true === $cache->get('bool'), 'Stores and retrieves boolean'); + $cache->set('bool_false', false); + self::assert(false === $cache->get('bool_false'), 'Stores and retrieves false without treating it as a miss');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.php` around lines 292 - 295, Add a regression test that stores and retrieves a cached false value to ensure WordPress_Simple_Cache::get() distinguishes stored false from a cache miss: after the existing true check in WordPress_Simple_Cache_Integration_Test, call $cache->set('bool_false', false) and assert that $cache->get('bool_false') === false using a strict comparison so the wrapped payload path for stored false is exercised and protected against regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.php`:
- Around line 77-132: The test runner does not define minimal WordPress state so
requiring inc/sso/class-wordpress-simple-cache.php can fatal because the cache
class (WordPress_Simple_Cache) and its clear() method expect ABSPATH and the
global $wpdb; update
WordPress_Simple_Cache_Integration_Test::mock_wordpress_functions (or the test
setup) to define('ABSPATH', __DIR__ . '/..') if not defined and create a minimal
$wpdb stub global (an object with the methods/properties used by
WordPress_Simple_Cache::clear) before including the cache class, or instead
change the test to bootstrap via the existing WordPress test bootstrap; ensure
you reference ABSPATH, global $wpdb, and the class
WordPress_Simple_Cache/clear() when making the change.
---
Nitpick comments:
In `@tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.php`:
- Around line 292-295: Add a regression test that stores and retrieves a cached
false value to ensure WordPress_Simple_Cache::get() distinguishes stored false
from a cache miss: after the existing true check in
WordPress_Simple_Cache_Integration_Test, call $cache->set('bool_false', false)
and assert that $cache->get('bool_false') === false using a strict comparison so
the wrapped payload path for stored false is exercised and protected against
regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eabaf433-4cff-4cbe-9251-d5245fd1ae81
📒 Files selected for processing (1)
tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.php
1 participant
Summary
This PR resolves a fatal error in SSO functionality caused by incompatibility between
symfony/cachev5.4 andpsr/simple-cachev3.0+.Problem
The SSO feature was experiencing a fatal error:
This occurred because:
symfony/cachev5.4.x has a compatibility check that rejectspsr/simple-cache3.0+symfony/cache6.0+ requires PHP 8.1+ (not compatible with our PHP 7.4 requirement)CacheInterfaceSolution
Created a WordPress-native PSR-16 cache implementation that:
Psr\SimpleCache\CacheInterface(PSR-16 standard)get_site_transient,set_site_transient,delete_site_transient)Changes
New Files
inc/sso/class-wordpress-simple-cache.php- WordPress transient-based PSR-16 implementationtests/WP_Ultimo/SSO/WordPress_Simple_Cache_Test.php- Comprehensive PHPUnit test suite (20+ tests)tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.php- Standalone integration testsModified Files
inc/sso/class-sso.php- Updated to useWordPress_Simple_Cacheinstead ofPsr16Cachecomposer.json- Removedsymfony/cachedependencycomposer.lock- Updated (removed 3 packages: symfony/cache, symfony/cache-contracts, symfony/var-exporter)Benefits
Testing
All PSR-16 interface methods tested:
get(),set(),delete(),clear(),has()getMultiple(),setMultiple(),deleteMultiple()Notes
$defaultparameter name are acceptable (required by PSR-16 standard)clear()method are necessary for bulk transient deletionMigration
No migration needed - the change is backward compatible. Existing SSO sessions will continue to work as the cache implementation is transparent to the SSO logic.
Fixes: Fatal error "psr/simple-cache 3.0+ is not compatible with symfony/cache v5.4"
Summary by CodeRabbit
Refactor
Chores
Tests
Style