-
-
Notifications
You must be signed in to change notification settings - Fork 37
Fix: many #163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: many #163
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughUpdated RoadRunnerStore::many to return an associative array keyed by the original (unprefixed) keys when a prefix is configured; added a CHANGELOG entry noting this fix. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant S as RoadRunnerStore
participant B as Backend Storage
Note over C,S: many(keys) -> ensure returned keys are original (unprefixed)
C->>S: many([k1, k2, ...])
alt prefix configured
S->>B: getMultiple([prefix+k1, prefix+k2, ...])
B-->>S: {prefix+k1: v1, prefix+k2: v2, ...}
Note over S: Convert to array of values\nand array_combine([k1,k2,...], [v1,v2,...])
else no prefix
S->>B: getMultiple([k1, k2, ...])
B-->>S: {k1: v1, k2: v2, ...}
end
S-->>C: {k1: v1, k2: v2, ...}
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.28)Path /fixes does not exist 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
23-23: Clarify the changelog entry wording.The current wording "returns values with
prefix" is ambiguous—it could describe either the old buggy behavior or the new fixed behavior. Consider rephrasing to make it clear this describes the problem that was fixed.Suggested rewording:
-- When using `prefix`, the `many` method returns values with `prefix` +- When using `prefix`, the `many` method now returns values keyed by their original (unprefixed) keysor:
-- When using `prefix`, the `many` method returns values with `prefix` +- The `many` method incorrectly returned prefixed keys when a prefix was configured
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)src/Cache/RoadRunnerStore.php(1 hunks)
🔇 Additional comments (1)
src/Cache/RoadRunnerStore.php (1)
39-39: VerifygetMultiplepreserves order and returns entries for all requested keys
array_combinewill throw if the returned values array length doesn’t match the input keys or if ordering differs. Manually confirm thatSpiral\RoadRunner\KeyValue\StorageInterface::getMultiplealways yields the same count and order (including nulls for missing keys), or else replace with a safe loop mapping each key to its value (defaulting to null).
Description
When using
prefix, themanymethod returns values withprefixbefore
after
Checklist
CHANGELOG.mdfileSummary by CodeRabbit
Bug Fixes
Documentation