Repository navigation
Add PagesInterface::getNbPages() - #108
Merged
Merged
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.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 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 docstrings
🧪 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.
koriym
marked this pull request as ready for review
July 8, 2026 17:29
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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.
Contributor
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
llms-full.txt labelled $page->total as the page count; it is the item count. The tutorial notes said the page count was unavailable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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, with configurable items per page.