Query: prevent invalidated query caches from accumulating in 3.0. - #255
Conversation
Port the 2.0.3 result-cache fix to the 3.0 maintenance line. Store the cache generation with each result, validate it before reuse, and replace stale results at the same key. Capture the generation before reading the database. Remove the unused private cache-key parameter. Add regression coverage for repeated mutations, missing and stale generations, empty results, and invalidation during a database read. Props hellofromahmed for reporting the persistent-cache growth. Props robincornett for the existing unset-value cache-key correction. See #253, #254, #185.
There was a problem hiding this comment.
🟡 Changes recommended
The newly added tests invoke private methods via ReflectionMethod without the existing PHP-version guard needed for compatibility with PHP < 8.1, which can break the test suite on supported older runtimes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Ports the 2.0.3 query-result cache fix to the 3.0 release line so repeated invalidations no longer accumulate generation-suffixed cache keys; instead, results are stored under a stable key and validated against a captured cache-generation value.
Changes:
- Make query-result cache keys generation-independent and store
last_changedinside the cached payload for stale-entry detection. - Replace cache writes with
cache_set()so stale/missing-generation entries can be overwritten at the stable key. - Add regression tests covering stable-key replacement, stale/missing generations, and “generation captured before DB read”.
File summaries
| File | Description |
|---|---|
| tests/Database/Query/QueryCacheTest.php | Adds regression coverage for stable cache keys and generation-aware cached payloads. |
| src/Database/Kern/Query.php | Implements stable cache keys and generation validation/replacement in query-result caching. |
Review details
Suppressed comments (2)
tests/Database/Query/QueryCacheTest.php:302
- This test invokes private get_cache_key()/get_cache_group() via ReflectionMethod without the PHP_VERSION_ID < 80100 setAccessible(true) guard used elsewhere in this file, which will break the suite on PHP < 8.1.
$query = new TestQuery( $args );
$get_key = new \ReflectionMethod( TestQuery::class, 'get_cache_key' );
$group = ( new \ReflectionMethod( TestQuery::class, 'get_cache_group' ) )->invoke( self::$query );
$key = $get_key->invoke( $query );
tests/Database/Query/QueryCacheTest.php:337
- Same reflection-access issue here: for PHP < 8.1, invoking private methods via ReflectionMethod requires setAccessible(true) (as done in the earlier tests), otherwise the test will fail.
$query = new TestQuery( $args );
$get_key = new \ReflectionMethod( TestQuery::class, 'get_cache_key' );
$group = ( new \ReflectionMethod( TestQuery::class, 'get_cache_group' ) )->invoke( self::$query );
$key = $get_key->invoke( $query );
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| $get_key = new \ReflectionMethod( TestQuery::class, 'get_cache_key' ); | ||
| $group = ( new \ReflectionMethod( TestQuery::class, 'get_cache_group' ) )->invoke( self::$query ); | ||
| $query = new TestQuery( $args ); |
Port the 2.0.3 query-result cache fix to the released 3.0 line. Repeated invalidations now replace the result at one stable key instead of accumulating generation-suffixed keys. Each entry carries the generation captured before its database read; entries with missing or stale generations are replaced.
Robin's unset-value cache-key correction already exists in 3.0.0. Remove the now-unused private
get_cache_key()parameter, as noted in the review of #254. No public API or runtime requirement changes.Validation:
Existing orphaned entries are not removed, and distinct query entries do not gain an expiration policy. The development-branch up-port is separate.
Props @hellofromahmed for reporting #253 and @robincornett for the existing sentinel correction (#185).
See #253, #254, #185.