Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Database.php (1)
1144-1148:⚠️ Potential issue | 🟠 MajorInstance filters with same-line closures can produce identical cache signatures despite different captures.
The
computeCallableSignature()method reduces closures tofilename:startLine, ignoring captured variables and bound state. Since instance filter signatures are included directly in cache keys (viagetCacheKeys()line computation), two instance filters defined on the same source line but capturing different variables will generate identical signatures and collide in the cache, causing the wrong filter behavior to be applied.The test suite covers different-line closures but does not cover the case of same-line closures with different captures. Please either accept an explicit caller-provided signature for instance filters or fold closure captures into the computed signature, and add a test case for same-line closure collisions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` around lines 1144 - 1148, The instance filter signatures can collide because computeCallableSignature reduces closures to file:startLine; update the instance filter registration to accept an optional explicit signature and use it when storing into the instanceFilters map (reference: instanceFilters property and getInstanceFilters()), and update getCacheKeys() to prefer that provided signature over computeCallableSignature(); if no explicit signature is passed, fall back to computeCallableSignature() as before (computeCallableSignature() and getCacheKeys() are the functions to change). Add a unit test that registers two same-line closures with different captures but different explicit signatures and assert their cache keys differ to prevent collisions.
🧹 Nitpick comments (1)
tests/unit/CacheKeyTest.php (1)
77-103: Test may not behave as expected with identical closures.Both
$noopAand$noopBhave identical code bodies. If signature generation is based on the closure's serialized form or code representation, these could produce the same signature, causing the assertion to fail unexpectedly.Consider using meaningfully different closures to ensure distinct signatures:
Proposed fix
$noopA = function (mixed $value) { return $value; }; - $noopB = function (mixed $value) { - return $value; - }; + $noopB = function (mixed $value) { + return \strval($value); // Different implementation + };Alternatively, if the
Databaseallows explicit signatures, provide different signature strings directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/CacheKeyTest.php` around lines 77 - 103, The test testDifferentInstanceFilterCallablesProduceDifferentCacheKeys uses two closures ($noopA and $noopB) with identical bodies which can yield identical signatures; update the test so the two filter callables are distinguishable—either make $noopA and $noopB have different behavior or metadata (e.g., different parameter handling or returned values) or pass explicit distinct signature strings into createDatabase for the 'myFilter' encode/decode entries—then keep asserting that getHashKey($dbA) !== getHashKey($dbB).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/CacheKeyTest.php`:
- Around line 86-97: The instance filters for $dbA and $dbB (created via
createDatabase) are missing the required "signature" field for the "myFilter"
entries; update the filter definitions used in createDatabase so each "myFilter"
includes a signature property (or adjust/createDatabase to
auto-generate/validate signatures) to match the other tests that expect an
explicit signature for encode/decode filters.
- Around line 13-21: Add a proper PHPDoc type for the $instanceFilters parameter
in the createDatabase method to satisfy PHPStan: document it as an array of
associative arrays with keys 'encode', 'decode', and 'signature' (e.g.
array<int, array{encode:callable, decode:callable, signature:string}>), so tools
know the shape expected by Database::getCacheKeys(); update the PHPDoc above
createDatabase to use that type and keep the method signature unchanged.
---
Outside diff comments:
In `@src/Database/Database.php`:
- Around line 1144-1148: The instance filter signatures can collide because
computeCallableSignature reduces closures to file:startLine; update the instance
filter registration to accept an optional explicit signature and use it when
storing into the instanceFilters map (reference: instanceFilters property and
getInstanceFilters()), and update getCacheKeys() to prefer that provided
signature over computeCallableSignature(); if no explicit signature is passed,
fall back to computeCallableSignature() as before (computeCallableSignature()
and getCacheKeys() are the functions to change). Add a unit test that registers
two same-line closures with different captures but different explicit signatures
and assert their cache keys differ to prevent collisions.
---
Nitpick comments:
In `@tests/unit/CacheKeyTest.php`:
- Around line 77-103: The test
testDifferentInstanceFilterCallablesProduceDifferentCacheKeys uses two closures
($noopA and $noopB) with identical bodies which can yield identical signatures;
update the test so the two filter callables are distinguishable—either make
$noopA and $noopB have different behavior or metadata (e.g., different parameter
handling or returned values) or pass explicit distinct signature strings into
createDatabase for the 'myFilter' encode/decode entries—then keep asserting that
getHashKey($dbA) !== getHashKey($dbB).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 141684cf-8d25-492c-b67a-5d860a3b16ac
📒 Files selected for processing (2)
src/Database/Database.phptests/unit/CacheKeyTest.php
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary by CodeRabbit
Tests
Chores