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.
…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>
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>
- 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>
📝 WalkthroughWalkthroughThe pull request replaces the Symfony Cache dependency with a custom WordPress-based PSR-16 Simple Cache implementation for SSO functionality. Updates include removing the Symfony Cache package, refactoring the SSO cache layer to use the new WordPress_Simple_Cache class, adding type hints for improved code clarity, and introducing comprehensive tests for the new cache implementation. Changes
Sequence Diagram(s)sequenceDiagram
participant SSO as SSO Class
participant Cache as WordPress_Simple_Cache
participant Transient as WordPress Transients API
rect rgba(100, 150, 200, 0.5)
Note over SSO,Transient: Cache Initialization & Retrieval
SSO->>Cache: cache() - request cache instance
Cache->>Cache: lazy initialize with prefix 'wu_sso_'
end
rect rgba(150, 100, 200, 0.5)
Note over Cache,Transient: Cache Operations
Cache->>Transient: get_site_transient(prefixed_key)
Transient-->>Cache: cached value or default
Cache-->>SSO: return PSR-16 CacheInterface
end
rect rgba(200, 100, 150, 0.5)
Note over Cache,Transient: Store Operation
SSO->>Cache: set(key, value, ttl)
Cache->>Cache: validate key & calculate expiration
Cache->>Transient: set_site_transient(prefixed_key, value, ttl)
Transient-->>Cache: confirmation
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
🔨 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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.php (1)
103-103:test_ttl_handling()never observes the TTL.Line 103 discards
$expiration, so these assertions only prove thatset()returnstrue. Recording the passed expiration in the mock would let this file catch timeout-calculation bugs instead of masking them.Also applies to: 329-345
🤖 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` at line 103, The mock helper set_site_transient currently ignores the $expiration param so tests like test_ttl_handling() can't verify TTL behavior; modify the mock implementation (function set_site_transient) to record the passed $expiration alongside the key and value (e.g., store ['value'=> $value, 'expiration'=> $expiration] in the same mock storage used by get_site_transient) so assertions can inspect the recorded expiration; apply the same change to the other mock implementations referenced around lines 329-345 so all tests can validate timeout calculations rather than just set() return values.
🤖 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 99-117: clear() only scans sitemeta and misses transients stored
solely in an external object cache; update the implementation so clearing covers
cache-only keys by using a versioned namespace or an explicit key index: either
(A) implement a namespace/version value (e.g., store "{$this->prefix}_version"
as a site option and prepend it to transient names set via set_site_transient)
and have clear() bump that version to invalidate all cached keys, or (B)
maintain an index (e.g., site option "{$this->prefix}_index") that
set_site_transient records each transient name into via
wp_cache_set/wp_cache_get/wp_cache_delete and
set_site_option/update_site_option, and have clear() iterate that index to call
delete_site_transient and wp_cache_delete for each entry and then clear the
index; locate changes around the clear() method and corresponding set/save code
that calls set_site_transient to ensure both database and object-cache-only
transients are tracked and invalidated.
- Around line 71-80: The set() method currently treats zero/negative TTLs by
forcing a 1-second expiry and convert_ttl_to_seconds() mutates DateTime via
DateTime::add(), collapsing DateInterval TTLs to one second; change
convert_ttl_to_seconds() to avoid mutating $now (use (clone
$now)->add($interval) or DateTimeImmutable->add) so future time is computed
correctly, and update set($key, $value, $ttl = null) to treat null !== $ttl &&
$expiration <= 0 by deleting the key with delete_site_transient($this->prefix .
$key) and returning false (PSR-16 requires deletion of expired items) instead of
forcing a 1-second TTL. Ensure references are to convert_ttl_to_seconds(),
set(), DateInterval, DateTime::add(), and delete_site_transient().
In `@tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.php`:
- Around line 77-132: The test runner fails because the plugin file early-exits
when ABSPATH is missing and clear() uses global $wpdb which doesn't exist; fix
by defining a minimal ABSPATH constant in the standalone CLI runner before
requiring inc/sso/class-wordpress-simple-cache.php and/or update the guard in
class-wordpress-simple-cache.php to allow a test mode, and in tests mock a
minimal $wpdb object with an sitemeta property or refactor clear() to use
set_site_transient/delete_site_transient when global $wpdb is not present (refer
to the clear() method and class-wordpress-simple-cache.php). For TTL testing,
update the test mocks for set_site_transient/get_site_transient to record
expiration timestamps (store both value and expiry) and make get_site_transient
return false when expired, then enhance test_ttl_handling() to assert the
expiration behavior (refer to mock_wordpress_functions(), set_site_transient,
get_site_transient, and test_ttl_handling()).
---
Nitpick comments:
In `@tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.php`:
- Line 103: The mock helper set_site_transient currently ignores the $expiration
param so tests like test_ttl_handling() can't verify TTL behavior; modify the
mock implementation (function set_site_transient) to record the passed
$expiration alongside the key and value (e.g., store ['value'=> $value,
'expiration'=> $expiration] in the same mock storage used by get_site_transient)
so assertions can inspect the recorded expiration; apply the same change to the
other mock implementations referenced around lines 329-345 so all tests can
validate timeout calculations rather than just set() return values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c596385b-8f96-471a-a450-2774b73d46cf
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
composer.jsoninc/class-maintenance-mode.phpinc/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
💤 Files with no reviewable changes (1)
- composer.json
| public function set($key, $value, $ttl = null) { | ||
| $this->validate_key($key); | ||
| $expiration = $this->convert_ttl_to_seconds($ttl); | ||
|
|
||
| // Non-null TTL <= 0 means item should expire immediately (PSR-16). | ||
| if (null !== $ttl && $expiration <= 0) { | ||
| $expiration = 1; | ||
| } | ||
|
|
||
| return set_site_transient($this->prefix . $key, ['v' => $value], $expiration); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PHP, does DateTime::add() mutate the original object, and what does PSR-16 require a cache implementation to do when set() receives a zero or negative TTL?
💡 Result:
-
PHP
DateTime::add(): Yes—it mutates the originalDateTimeobject (it “modifies a DateTime object”) and returns the same (now-modified) object for chaining. If you need non-mutating behavior, useDateTimeImmutable::add(), which returns a new instance. [1] -
PSR-16 (
CacheInterface::set) TTL ≤ 0: Ifset()receives a zero or negative TTL, the cache implementation MUST delete the item from the cache (if it exists) because it is already expired. [2]
Sources: [1] PHP Manual: DateTime::add(); [2] PHP-FIG PSR-16 specification.
🏁 Script executed:
# Check the set() method at lines 71-80
sed -n '71,80p' inc/sso/class-wordpress-simple-cache.phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 432
🏁 Script executed:
# Check the convert_ttl_to_seconds() method at lines 226-238
sed -n '226,238p' inc/sso/class-wordpress-simple-cache.phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 376
🏁 Script executed:
# Get context around these lines to verify structure
sed -n '60,85p' inc/sso/class-wordpress-simple-cache.phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 913
🏁 Script executed:
# Get context for convert_ttl_to_seconds
sed -n '215,245p' inc/sso/class-wordpress-simple-cache.phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 779
DateInterval TTLs collapse to one second due to DateTime::add() mutation; PSR-16 requires deleting expired items, not caching them.
Line 233 mutates $now in place via DateTime::add(), causing $future and $now to reference the same object. Line 234 then computes 0, forcing every DateInterval TTL through the 1-second fallback in Lines 75-78. Additionally, zero or negative TTLs should be deleted per PSR-16, not cached for an extra second.
Proposed fix
public function set($key, $value, $ttl = null) {
$this->validate_key($key);
$expiration = $this->convert_ttl_to_seconds($ttl);
- // Non-null TTL <= 0 means item should expire immediately (PSR-16).
+ // Non-null TTL <= 0 means the item is already expired (PSR-16).
if (null !== $ttl && $expiration <= 0) {
- $expiration = 1;
+ delete_site_transient($this->prefix . $key);
+ return true;
}
return set_site_transient($this->prefix . $key, ['v' => $value], $expiration);
} protected function convert_ttl_to_seconds($ttl) {
if (null === $ttl) {
return 0; // No expiration.
}
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());
}
return (int) $ttl;
}Also applies to: 226-238
🤖 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 71 - 80, The set()
method currently treats zero/negative TTLs by forcing a 1-second expiry and
convert_ttl_to_seconds() mutates DateTime via DateTime::add(), collapsing
DateInterval TTLs to one second; change convert_ttl_to_seconds() to avoid
mutating $now (use (clone $now)->add($interval) or DateTimeImmutable->add) so
future time is computed correctly, and update set($key, $value, $ttl = null) to
treat null !== $ttl && $expiration <= 0 by deleting the key with
delete_site_transient($this->prefix . $key) and returning false (PSR-16 requires
deletion of expired items) instead of forcing a 1-second TTL. Ensure references
are to convert_ttl_to_seconds(), set(), DateInterval, DateTime::add(), and
delete_site_transient().
| public function clear() { | ||
| global $wpdb; | ||
|
|
||
| // Get all transient keys with our prefix and delete each via the WordPress API, | ||
| // which handles both DB and object cache correctly. | ||
| $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) { | ||
| $transient_name = substr($meta_key, strlen('_site_transient_')); | ||
| delete_site_transient($transient_name); | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In WordPress core, when wp_using_ext_object_cache() is true, does set_site_transient() write to the sitemeta table or only to the object cache?
💡 Result:
When wp_using_ext_object_cache() is true, set_site_transient() does not write to the database (wp_sitemeta) in WordPress core—it short-circuits to wp_cache_set( $transient, $value, 'site-transient', $expiration ), i.e. object cache only. [1]
It only uses the DB path (via add_site_option() / update_site_option(), which stores network options in wp_sitemeta) when no external object cache is in use. [1], [2]
clear() misses entries stored in the object cache when an external object cache is active.
The implementation discovers keys by scanning $wpdb->sitemeta, but set_site_transient() bypasses the database entirely when an external object cache is active—it writes only to the cache layer. Consequently, clear() returns true without invalidating any transients for this prefix in that configuration. A versioned namespace or explicit key index is necessary to reliably clear all entries.
🤖 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 99 - 117, clear() only
scans sitemeta and misses transients stored solely in an external object cache;
update the implementation so clearing covers cache-only keys by using a
versioned namespace or an explicit key index: either (A) implement a
namespace/version value (e.g., store "{$this->prefix}_version" as a site option
and prepend it to transient names set via set_site_transient) and have clear()
bump that version to invalidate all cached keys, or (B) maintain an index (e.g.,
site option "{$this->prefix}_index") that set_site_transient records each
transient name into via wp_cache_set/wp_cache_get/wp_cache_delete and
set_site_option/update_site_option, and have clear() iterate that index to call
delete_site_transient and wp_cache_delete for each entry and then clear the
index; locate changes around the clear() method and corresponding set/save code
that calls set_site_transient to ensure both database and object-cache-only
transients are tracked and invalidated.
| private static function mock_wordpress_functions() { | ||
| if (! function_exists('get_site_transient')) { | ||
| /** | ||
| * Mock get_site_transient for standalone testing. | ||
| * | ||
| * @param string $key Transient key. | ||
| * @return mixed Value or false if not set. | ||
| */ | ||
| function get_site_transient($key) { | ||
| global $_site_transients; | ||
| if (! isset($_site_transients)) { | ||
| $_site_transients = array(); | ||
| } | ||
| return isset($_site_transients[ $key ]) ? $_site_transients[ $key ] : false; | ||
| } | ||
| } | ||
|
|
||
| if (! function_exists('set_site_transient')) { | ||
| /** | ||
| * Mock set_site_transient for standalone testing. | ||
| * | ||
| * @param string $key Transient key. | ||
| * @param mixed $value Value to store. | ||
| * @param int $expiration Expiration in seconds (unused in mock). | ||
| * @return bool Always true. | ||
| */ | ||
| function set_site_transient($key, $value, $expiration = 0) { // phpcs:ignore Generic.CodeAnalysis.UnusedFunctionParameter.FoundAfterLastUsed | ||
| global $_site_transients; | ||
| if (! isset($_site_transients)) { | ||
| $_site_transients = array(); | ||
| } | ||
| $_site_transients[ $key ] = $value; | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| if (! function_exists('delete_site_transient')) { | ||
| /** | ||
| * Mock delete_site_transient for standalone testing. | ||
| * | ||
| * @param string $key Transient key. | ||
| * @return bool True if deleted, false if not found. | ||
| */ | ||
| function delete_site_transient($key) { | ||
| global $_site_transients; | ||
| if (! isset($_site_transients)) { | ||
| $_site_transients = array(); | ||
| } | ||
| if (isset($_site_transients[ $key ])) { | ||
| unset($_site_transients[ $key ]); | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '1,20p' inc/sso/class-wordpress-simple-cache.php
sed -n '74,132p' tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.php
sed -n '350,356p' tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 2548
🏁 Script executed:
# Check the clear() method implementation in WordPress_Simple_Cache
sed -n '99,117p' inc/sso/class-wordpress-simple-cache.php
# Find and show the TTL test
rg -n 'test_ttl|test_set_with_expiration' tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.php -A 15
# Check if there's any ABSPATH or WordPress bootstrap provided before the require
sed -n '340,360p' tests/WP_Ultimo/SSO/WordPress_Simple_Cache_Integration_Test.phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 2632
The advertised standalone runner cannot execute as written due to missing WordPress bootstrap.
The main class file (inc/sso/class-wordpress-simple-cache.php) contains a guard that exits when ABSPATH is undefined (line 15). When the CLI runner (lines 350–356) attempts to require this file in standalone mode, the execution stops before any tests run. Additionally, the clear() method (lines 99–117) directly references global $wpdb to query $wpdb->sitemeta, which is unavailable outside a WordPress environment.
The TTL test does not validate expiration handling.
The test_ttl_handling() method (lines 329–345) only asserts that set() returns true. It never verifies that the TTL values are actually stored or respected. The mock set_site_transient() function explicitly ignores the $expiration parameter (marked with a phpcs:ignore comment), so no TTL data is ever captured or tested.
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 103-103: Avoid unused parameters such as '$expiration'. (undefined)
(UnusedFormalParameter)
🤖 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 fails because the plugin file early-exits when ABSPATH
is missing and clear() uses global $wpdb which doesn't exist; fix by defining a
minimal ABSPATH constant in the standalone CLI runner before requiring
inc/sso/class-wordpress-simple-cache.php and/or update the guard in
class-wordpress-simple-cache.php to allow a test mode, and in tests mock a
minimal $wpdb object with an sitemeta property or refactor clear() to use
set_site_transient/delete_site_transient when global $wpdb is not present (refer
to the clear() method and class-wordpress-simple-cache.php). For TTL testing,
update the test mocks for set_site_transient/get_site_transient to record
expiration timestamps (store both value and expiry) and make get_site_transient
return false when expired, then enhance test_ttl_handling() to assert the
expiration behavior (refer to mock_wordpress_functions(), set_site_transient,
get_site_transient, and test_ttl_handling()).
1 participant
Summary by CodeRabbit
Refactor
Tests