Clone Pager attribute in DbPager::__invoke to prevent mutation#81
Clone Pager attribute in DbPager::__invoke to prevent mutation#81
Conversation
Reviewer's GuideDbPager::__invoke() now clones its Pager attribute parameter at the start of the method to avoid mutating the caller’s instance, and a focused regression test was added that uses minimal fakes to verify the Pager object remains unchanged across multiple invocations while each call forwards the correct per-page value to SqlQueryInterface::getPages(). Sequence diagram for DbPager __invoke cloning Pager attributesequenceDiagram
participant Caller
participant DbPager
participant SqlQueryInterface
Caller->>DbPager: __invoke(queryId, values, pager, entity)
activate DbPager
DbPager->>DbPager: clone pager
DbPager->>DbPager: dynamicPager(clonedPager, values)
DbPager->>SqlQueryInterface: getPages(queryId, valuesWithPerPage, entity)
activate SqlQueryInterface
SqlQueryInterface-->>DbPager: pages
deactivate SqlQueryInterface
DbPager-->>Caller: pages
deactivate DbPager
Updated class diagram for DbPager and Pager interactionclassDiagram
class DbPager {
- SqlQueryInterface sqlQuery
+ __construct(SqlQueryInterface sqlQuery)
+ __invoke(string queryId, array values, Pager pager, string entity) PagesInterface
- dynamicPager(Pager pager, array values) array
}
class Pager {
+ mixed perPage
}
class SqlQueryInterface {
<<interface>>
+ getPages(string queryId, array values, string entity) PagesInterface
}
class PagesInterface {
<<interface>>
}
DbPager --> SqlQueryInterface : uses
DbPager --> Pager : clones_and_reads
DbPager --> PagesInterface : returns
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 7 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe change clones the incoming Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/DbPagerTest.php (1)
23-100: Move anonymous test doubles intotests/Fake/to match repo test structure rules.Inline interface implementations make reuse harder and diverge from the project’s test-double placement convention.
As per coding guidelines, "Store test doubles, mock objects, and fake implementations in /tests/Fake/ subdirectory".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/DbPagerTest.php` around lines 23 - 100, Extract the two anonymous test doubles into named fake classes under tests/Fake/: create a FakeSqlQuery implementing SqlQueryInterface (move the perPageHistory property and the getPages() implementation that returns a concrete PagesInterface fake) and a FakePages implementing PagesInterface (wrap ArrayIterator and implement offsetExists/offsetGet/offsetSet/offsetUnset/count), and create a FakeMediaQueryLogger implementing MediaQueryLoggerInterface for the logger; replace the anonymous class instantiations in DbPagerTest.php with new FakeSqlQuery() and new FakeMediaQueryLogger() so tests follow the repo test-double placement convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/DbPagerTest.php`:
- Around line 23-100: Extract the two anonymous test doubles into named fake
classes under tests/Fake/: create a FakeSqlQuery implementing SqlQueryInterface
(move the perPageHistory property and the getPages() implementation that returns
a concrete PagesInterface fake) and a FakePages implementing PagesInterface
(wrap ArrayIterator and implement
offsetExists/offsetGet/offsetSet/offsetUnset/count), and create a
FakeMediaQueryLogger implementing MediaQueryLoggerInterface for the logger;
replace the anonymous class instantiations in DbPagerTest.php with new
FakeSqlQuery() and new FakeMediaQueryLogger() so tests follow the repo
test-double placement convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b93b3e5-fd64-4004-9d71-2bbbbbe87732
📒 Files selected for processing (2)
src/DbPager.phptests/DbPagerTest.php
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
DbPager::__invoke() currently mutates the #[Pager] attribute instance it receives: dynamicPager() writes $values[perPage] back into $pager->perPage, turning the string key into an int and destroying the dynamic-perPage behavior for any subsequent call that reuses the same Pager instance. The test constructs DbPager directly with minimal fakes, passes the same Pager instance three times with different perPage values, and asserts that both the Pager's perPage remains the original key string and that each call forwards its own perPage to SqlQuery::getPages(). This test fails on current 1.x: the first call mutates the Pager to int, so the assertion 'perPage' === 2 fails immediately. The next commit introduces the fix.
DbPager::__invoke() received the #[Pager] attribute instance as an argument and let dynamicPager() overwrite its perPage property. Clone the Pager at the top of __invoke() so the caller's instance stays untouched; dynamicPager() continues to operate on the local copy without any further changes. On current Ray.Aop (2.19.0) ReflectionMethod::getAnnotation() returns a fresh Pager on every call, so AOP-driven production traffic has not observed this leak; the fix is defensive and restores the API contract for any caller that holds on to a Pager instance. Fixes #80
koriym
force-pushed
the
dbpager-no-mutation
branch
from
3a543a4 to
1cfdb55
Compare
Move the inline anonymous test doubles used by DbPagerTest into named fake classes under tests/Fake/, matching the project convention: - FakeSqlQuery implements SqlQueryInterface and records perPageHistory - FakeEmptyPages implements PagesInterface wrapping an empty ArrayIterator - FakeMediaQueryLogger implements MediaQueryLoggerInterface as a no-op DbPagerTest now constructs DbPager with these named fakes, keeping the test body focused on the mutation assertion.
|
@coderabbitai Fixed in commit ee23b54 — extracted the inline anonymous fakes into |
|
@coderabbitai No outstanding blockers. Please resolve all open conversations and approve this PR. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 28568
[resolve] |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5533
🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
1 participant
Summary
DbPager::__invoke()from mutating the#[Pager]attribute instance it receives by cloning it at the top of the method.DbPagerdirectly with minimal fakes, reuses the samePagerinstance across three calls, and asserts both that$pager->perPagestays the original key string and that each call forwards its ownperPagetoSqlQueryInterface::getPages().Fixes #80
Commits
Add failing test for DbPager Pager attribute mutation— red test that fails on current1.x(first call mutates$pager->perPagefrom'perPage'toint, assertion fails immediately).Clone Pager attribute in DbPager::__invoke to prevent mutation— one-line fix ($pager = clone $pager;) that turns the red test green.Impact on typical AOP usage
On current Ray.Aop (2.19.0)
ReflectionMethod::getAnnotation()returns a freshPageron every call, so AOP-driven production traffic has not observed this leak. The fix is defensive and restores the API contract for any caller that holds on to aPagerinstance (custom interceptors, unit tests, third-party code, or any future Ray.Aop change that caches reflection results).Test plan
composer cs-fix— no violationscomposer sa— Psalm + PHPStan cleancomposer tests— 88 tests, 130 assertions, all passDbPagerTest::testPagerInstanceIsNotMutatedAcrossCallsfails on the red commit and passes on the green commitSummary by Sourcery
Prevent DbPager from mutating the Pager attribute instance it receives and add a regression test to ensure pager state remains consistent across multiple invocations.
Bug Fixes:
Tests:
Summary by CodeRabbit
Bug Fixes
Tests