Add PagesInterface::getNbPages()#108
Conversation
PagesInterface exposed only the result count via Countable::count(), never the number of pages. Every consumer had to recompute `ceil(count / perPage)` by hand, duplicating logic that already lives in the underlying Pagerfanta and forcing callers to keep perPage around just for that division. Add `getNbPages(): int` to PagesInterface and implement it in Pages (`ceil` of `count / perPage`) and MappedPages (delegation). SqlQuery now forwards the perPage it already receives in getPages() to the Pages constructor, so callers can read the page count directly from the PagesInterface without recomputing it. Note: this adds a method to a public interface and a required constructor argument to the final Pages class.
|
Warning Review limit reached
Next review available in: 57 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThis PR adds ChangesgetNbPages Pagination Support
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant SqlQuery
participant Pages
participant MappedPages
SqlQuery->>Pages: construct with perPage
MappedPages->>Pages: getNbPages()
Pages->>Pages: count results and calculate page total
Pages-->>MappedPages: return page count
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
- MappedPages: test that getNbPages() delegates to the wrapped pages. - Pages: add perPage=2 cases (exact division and ceil round-up) so getNbPages() is covered beyond the perPage=1 smoke test. 119 tests, 221 assertions OK; phpstan and psalm clean.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Pages.php`:
- Around line 100-105: The getNbPages method in Pages can divide by zero when
perPage is 0, so add a guard in Pages and its constructor path to prevent
invalid pagination state. Update the Pages constructor to validate or normalize
perPage before it is stored, and make getNbPages return a safe result when
perPage is zero instead of performing the division. Use the existing Pages and
getNbPages symbols to locate the fix, and keep the behavior consistent with how
SqlQuery::getPages() supplies perPage through the pager factory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eed3fa4c-cb2c-40a0-bcba-0a845ee38e86
📒 Files selected for processing (10)
src/MappedPages.phpsrc/Pages.phpsrc/PagesInterface.phpsrc/SqlQuery.phptests/Fake/FakeEmptyPages.phptests/Fake/FakePages.phptests/Fake/MappedPagesFakePages.phptests/MappedPagesTest.phptests/PagesTest.phptests/SqlQueryTest.php
CodeRabbit flagged that Pages::getNbPages() divides by perPage, which throws DivisionByZeroError if perPage is 0. Pages has a public constructor and can be instantiated directly, so validate perPage there instead of relying on callers like SqlQuery::getPages().
- getNbPages() now returns a minimum of 1 for empty result sets, matching Pagerfanta's minimumNbPages() so both layers report the same page count (the stated goal of pairing this with Ray.AuraSqlModule#92). - Memoize nbResults in Pages so count() and getNbPages() share a single COUNT query, as Pagerfanta does. Without this each call re-ran the COUNT - or a full fetch for non-rewritable SQL. - Update FakeEmptyPages, the interface docblock, and add unit and integration coverage for both behaviors.
Countable::count() must return int<0, max>. getNbResults() is declared int<0, max> in Pagerfanta's AdapterInterface, but routing it through the int|null nbResults property widened it to int. Annotate the property with the range type to keep the narrowing.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Pages.php`:
- Line 107: Update the memoization expression in Pages::count() so the
ExtendedPdoAdapter::getNbResults() result is clamped to a non-negative integer
before assigning it to nbResults, for example by applying max(0, ...). Preserve
the existing null-coalescing assignment and Countable return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c9e8500-f074-44f0-babf-1c79509d440f
📒 Files selected for processing (5)
src/Pages.phpsrc/PagesInterface.phptests/Fake/FakeEmptyPages.phptests/PagesTest.phptests/SqlQueryTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- src/PagesInterface.php
- tests/SqlQueryTest.php
- tests/PagesTest.php
2 participants
Summary
PagesInterfaceexposed only the result count viaCountable::count(), never the number of pages. Every consumer had to recomputeceil(count / perPage)by hand, duplicating logic that already lives in the underlyingPagerfantaand forcing callers to keepperPagearound just for that division.Changes
getNbPages(): inttoPagesInterface.Pages(ceilofcount / perPage) andMappedPages(delegation).SqlQuery::getPages()now forwards theperPageit already receives to thePagesconstructor, so callers can read the page count directly from thePagesInterfacewithout recomputing it.FakePages,MappedPagesFakePages,FakeEmptyPages) and addtestPagerNbPages.Breaking changes
PagesInterface::getNbPages).Pagesclass (int $perPage).All in-repo implementers (
Pages,MappedPages, and the three test fakes) are updated in this PR.Verification
No files were checked); the change mirrors the existing style.Related
Companion to ray-di/Ray.AuraSqlModule#92, which adds
nbPagesto thePagevalue object. Together they let consumers read the page count from either layer without recomputingceil(count / perPage).Summary by CodeRabbit
getNbPages, including support for configurable items-per-page.InvalidPerPageException.