Fix Pagination correctness and current Laravel parity - #479
Conversation
Validate decoded cursor envelopes before strict construction so malformed input returns null instead of raising type errors. Preserve mixed database order values and make cursor JSON encoding fail through JsonException without adding recursive validation or another serialization pass. Add counterfactual coverage for non-string inputs, invalid encodings, incomplete and incorrectly typed envelopes, boolean and floating-point cursor values, and JSON failure and substitution behavior.
Read each request-owned pagination value directly from RequestContext once per resolver invocation while keeping the view factory lazily resolved from the worker container. Correct the lifecycle documentation so the static resolver ownership is explicit and no request object is retained. Cover context-free defaults, request precedence, container request rebinding, lazy view rebinding, exact context-read counts, and forced concurrent coroutine isolation.
Express the readable onEachSide requirement as a local contract-and-structural intersection at the only consumer. This removes the broad PHPStan suppression without narrowing the Laravel-compatible public paginator contract or adding a runtime guard. Retain focused window behavior coverage while the source analysis gate pins both halves of the local type requirement.
Align paginator contracts and implementations with current Laravel value and fragment types, preserve integer query keys, distinguish omitted pages from explicit zero, and make JSON failures explicit in one encoding pass. Restore zero-per-page safety, unconditional cursor item indexing, precise iterator and generic results, complete static reset defaults, and truthful render boundaries. Expand runtime and max-level type coverage for supported append values, exact page resolution, keyed cursor pages in both directions, invalid UTF-8 and caller flags, static reset slots, fragments, iterators, rendering, and current zero-per-page behavior.
Add the required native void return types to the remaining pagination resource and morph-loading tests. This keeps the package test surface consistent with repository typing rules without changing assertions or runtime behavior.
Return the concrete Eloquent cursor paginator from the shared cursor builder, restore model generics across all Eloquent paginator results, and make Query Builder cursor per-page nullability match the integer required by its callee. Preserve explicit zero pages instead of consulting ambient resolver state. Add runtime regressions for Query and Eloquent page-zero behavior, reflect the non-null cursor per-page boundary, and pin the concrete and generic Query and Eloquent result surfaces with max-level type fixtures.
Widen both relation families to forward Cursor objects already supported by their underlying builders, restore their precise paginator result types, and replace adjacent loose wildcard-column comparisons with strict comparisons. Exercise real SQLite pagination with floating-point order values and Cursor objects through belongs-to-many and has-many-through relations, and pin every relation paginator result at max analysis level.
Clamp all four public Scout pagination producers to page one before invoking any first-party or custom engine. This prevents explicit zero or negative values from reaching engine-specific indexing rules while preserving omitted-page resolver behavior. Use raw-engine regressions for zero, negative, and resolver-derived pages so database-side clamping cannot hide the shared-boundary defect.
Encode collection JSON with JSON_THROW_ON_ERROR in the existing single pass and document the propagated exception on both direct and pretty JSON boundaries. Caller flags such as invalid UTF-8 substitution remain authoritative. Cover eager and lazy collections for direct failure, delegated pretty-output failure, and successful substitution.
Use one throwing json_encode call at the Fluent and MessageBag boundaries and document the same failure through their pretty-output delegates. Preserve caller-supplied recovery flags without introducing a shared helper or wrapper exception. Add focused regressions for malformed UTF-8, delegated failure, successful substitution, and ordinary output.
Make NewAccessToken JSON serialization throw JsonException from its existing single encoding pass while preserving caller options. Cover malformed UTF-8 failure, recovery through substitution flags, and normal token output.
Make ApiResource JSON serialization fail through JsonException rather than returning false through a declared string boundary. Retain one encoding pass and caller flag precedence, with regressions for malformed UTF-8, substitution, and successful resources.
Port the current conditional choose result so one draw remains mixed and repeated draws are inferred as a list. Add a focused max-level fixture for both branches without changing runtime behavior.
Document the existing Route domain getter and setter as a conditional result while preserving the current native signature and runtime path. Add the complete current Routing type fixture so domain, middleware, and metadata forms remain covered together.
Add PaginationServiceProvider to root package discovery alongside the already-correct split metadata and default-provider registration. Pin both manifests with executable metadata coverage so standalone and monorepo installation cannot drift.
Record the public Tailwind-only view and current_page_url differences, point developers to the supported custom view APIs, and correct the Pagination guide wording. Bring the Database README into the repository documentation and provenance order without adding duplicate user guidance.
Record the final evidence-backed design for cursor validation, query and page semantics, JSON failures, resolver ownership, database and search contracts, current parity, tests, performance, and rejected machinery. The plan includes the core anti-overengineering rules and the exact post-compaction implementation context.
Add the signed-off Pagination ledger entry, revalidate carried and cross-package findings at their owning records, update the dependency and routing indexes, and mark Pagination complete in the package checklist. Retain durable Foundation, Translation, and View routes, record the deliberate Query cursor per-page narrowing, and preserve exact checklist parity across all first-level packages.
…ctness-parity # Conflicts: # docs/plans/2026-07-12-0900-framework-coroutine-state-lifecycle-audit.md # docs/plans/2026-07-12-0915-framework-coroutine-state-lifecycle-audit-ledger.md # src/database/README.md
📝 WalkthroughWalkthroughPagination behavior, cursor validation, resolver lifecycle, JSON serialization, database contracts, Scout page handling, package metadata, and related tests were updated for current framework parity. ChangesPagination correctness and parity
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RequestContext
participant PaginationState
participant Paginator
participant ViewFactory
RequestContext->>PaginationState: provide page, query, path, and cursor values
PaginationState->>Paginator: return request-scoped resolver results
Paginator->>ViewFactory: resolve the view factory lazily
ViewFactory-->>Paginator: return the configured view
Paginator-->>Paginator: serialize output or render the paginator
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Greptile SummaryThis PR corrects pagination behavior and updates Laravel-compatible contracts while preserving coroutine-local request ownership.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported structured-cursor issue is prevented at every cursor construction path before query bindings are produced.
|
| Filename | Overview |
|---|---|
| src/pagination/src/Cursor.php | Constructor-level validation now rejects array-valued cursor parameters across decoded, direct, and generated cursor paths, fully addressing the previous structured-value finding. |
| src/pagination/src/AbstractCursorPaginator.php | Updates cursor parameter and collection behavior while routing generated cursors through the new constructor invariant. |
| src/pagination/src/PaginationState.php | Resolves request pagination state directly from coroutine-owned request context without retaining request instances. |
| src/database/src/Concerns/BuildsQueries.php | Updates cursor and page resolution contracts used by Query and Eloquent builders. |
| src/scout/src/Builder.php | Clamps resolved pages before dispatching pagination requests to search engines. |
| tests/Pagination/CursorTest.php | Covers flat, nested, and empty structured cursor values for both encoded and direct construction paths. |
| tests/Integration/Database/EloquentCursorPaginateTest.php | Adds cursor-pagination integration coverage using isolated fixture models and hardcoded test data. |
Reviews (2): Last reviewed commit: "Reject structured cursor parameters" | Re-trigger Greptile
Reject array-valued cursor parameters at Cursor construction before Query Builder can silently flatten them into a different binding. Keep malformed decoded cursors on the existing null path while direct and generated cursors fail fast with the offending parameter name. Cover decoded flat, nested, and empty arrays, direct construction, generated cursor links, and valid bool, float, and backed-enum values. Record the owning boundary, Laravel-facing behavior, and bounded construction cost in the Pagination plan and audit ledger.
|
@greptile-apps Addressed the structured cursor finding in d21db9b. The finding was valid. The guard is intentionally limited to arrays. Backed enums and dates are valid bindings, and other unsupported objects already fail instead of changing value silently. Regression coverage includes flat, nested, and empty decoded arrays, direct construction, generated cursor links, and valid mixed values. |
|
@greptile-apps Follow-up summary:
The complete |
|
The fix is well-structured. The guard in the constructor is clear and correctly placed — catching arrays before The approach is sound. Rejecting at construction is the right boundary: it covers all four code paths (decoded, direct, generated, user-created) without duplicating the check in each callsite. One minor consideration: The The scope restriction to arrays only is appropriate. Backed enums serialize via Overall the fix looks correct and the test coverage (flat, nested, empty arrays + valid mixed values) is appropriately thorough. No issues with the implementation. |
|
The fix looks correct and well-scoped. A few observations: What's solid:
One minor inconsistency worth noting:
Test coverage: The finding is fully addressed. LGTM. |
|
The implementation looks correct. The changes are well-structured:
One minor note: the Everything else looks clean. The shallow scan in the constructor loop is proportionate to the map size and adds no I/O or retained state as noted. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/Scout/Unit/BuilderTest.php (1)
720-726: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the callback's builder parameter or add a targeted suppression.
PHPMD reports
$_as unused at Line 721. Rename it to$builderand assert that the callback receives aBuilderinstance. If the parameter is intentionally positional, add a targeted PHPMD suppression instead.Proposed cleanup
- function (Builder $_, int $perPage, int $page) use (&$pages, $rawResults): array { + function (Builder $builder, int $perPage, int $page) use (&$pages, $rawResults): array { + $this->assertInstanceOf(Builder::class, $builder); $this->assertSame(15, $perPage);🤖 Prompt for 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. In `@tests/Scout/Unit/BuilderTest.php` around lines 720 - 726, Update the paginate callback in the test to rename the unused Builder parameter from $_ to $builder and assert that $builder is an instance of Builder, preserving the existing pagination assertions and return behavior.Source: Linters/SAST tools
tests/Integration/Database/EloquentCursorPaginateTest.php (1)
27-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType each schema callback parameter.
Lines 27, 34, and 39 use untyped
$tableparameters. UseBlueprint $table, as line 19 already does.As per coding guidelines, use native types for parameters wherever permitted.
Proposed fix
-Schema::create('test_users', function ($table) { +Schema::create('test_users', function (Blueprint $table) { @@ -Schema::create('test_owners', function ($table) { +Schema::create('test_owners', function (Blueprint $table) { @@ -Schema::create('test_post_user', function ($table) { +Schema::create('test_post_user', function (Blueprint $table) {🤖 Prompt for 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. In `@tests/Integration/Database/EloquentCursorPaginateTest.php` around lines 27 - 42, Update the schema callbacks in the test setup to type each $table parameter as Blueprint, matching the existing typed callback nearby. Apply this to the callbacks creating test_users, test_owners, and test_post_user, reusing the file’s existing Blueprint import.Source: Coding guidelines
🤖 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/pagination/README.md`:
- Line 8: Update the “Differences From Laravel” heading in the README to use
setext level-two Markdown syntax instead of ATX syntax, preserving the heading
text and level.
---
Nitpick comments:
In `@tests/Integration/Database/EloquentCursorPaginateTest.php`:
- Around line 27-42: Update the schema callbacks in the test setup to type each
$table parameter as Blueprint, matching the existing typed callback nearby.
Apply this to the callbacks creating test_users, test_owners, and
test_post_user, reusing the file’s existing Blueprint import.
In `@tests/Scout/Unit/BuilderTest.php`:
- Around line 720-726: Update the paginate callback in the test to rename the
unused Builder parameter from $_ to $builder and assert that $builder is an
instance of Builder, preserving the existing pagination assertions and return
behavior.
🪄 Autofix
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 Plus
Run ID: f65b56f3-5e0d-45d1-9d15-751a8e485d63
📒 Files selected for processing (59)
composer.jsondocs/plans/2026-07-12-0900-framework-coroutine-state-lifecycle-audit.mddocs/plans/2026-07-12-0915-framework-coroutine-state-lifecycle-audit-ledger.mddocs/plans/2026-08-06-0928-pagination-correctness-current-parity-and-query-contracts.mdsrc/api-client/src/ApiResource.phpsrc/boost/docs/pagination.mdsrc/collections/src/Enumerable.phpsrc/collections/src/Traits/EnumeratesValues.phpsrc/contracts/src/Pagination/CursorPaginator.phpsrc/contracts/src/Pagination/Paginator.phpsrc/database/README.mdsrc/database/src/Concerns/BuildsQueries.phpsrc/database/src/Eloquent/Builder.phpsrc/database/src/Eloquent/Relations/BelongsToMany.phpsrc/database/src/Eloquent/Relations/HasOneOrManyThrough.phpsrc/database/src/Query/Builder.phpsrc/pagination/README.mdsrc/pagination/src/AbstractCursorPaginator.phpsrc/pagination/src/AbstractPaginator.phpsrc/pagination/src/Cursor.phpsrc/pagination/src/CursorPaginator.phpsrc/pagination/src/LengthAwarePaginator.phpsrc/pagination/src/PaginationState.phpsrc/pagination/src/Paginator.phpsrc/pagination/src/UrlWindow.phpsrc/routing/src/Route.phpsrc/sanctum/src/NewAccessToken.phpsrc/scout/src/Builder.phpsrc/support/src/Fluent.phpsrc/support/src/Lottery.phpsrc/support/src/MessageBag.phptests/ApiClient/ApiResourceTest.phptests/Database/DatabaseEloquentIntegrationTest.phptests/Database/DatabaseQueryBuilderTest.phptests/Integration/Database/EloquentCursorPaginateTest.phptests/Pagination/CursorPaginatorLoadMorphCountTest.phptests/Pagination/CursorPaginatorLoadMorphTest.phptests/Pagination/CursorPaginatorTest.phptests/Pagination/CursorResourceTest.phptests/Pagination/CursorTest.phptests/Pagination/LengthAwarePaginatorTest.phptests/Pagination/PackageMetadataTest.phptests/Pagination/PaginationResolverTest.phptests/Pagination/PaginatorLoadMorphCountTest.phptests/Pagination/PaginatorLoadMorphTest.phptests/Pagination/PaginatorResourceTest.phptests/Pagination/PaginatorTest.phptests/Pagination/UrlWindowTest.phptests/Sanctum/NewAccessTokenTest.phptests/Scout/Unit/BuilderTest.phptests/Support/SupportCollectionTest.phptests/Support/SupportFluentTest.phptests/Support/SupportMessageBagTest.phptypes/Database/Eloquent/Builder.phptypes/Database/Eloquent/Relations.phptypes/Database/Query/Builder.phptypes/Pagination/Paginator.phptypes/Routing/Route.phptypes/Support/Lottery.php
|
|
||
| Documentation: https://hypervel.org/docs/pagination | ||
|
|
||
| ## Differences From Laravel |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the configured setext heading style.
Markdownlint reports MD003 for this heading. Replace the ATX heading with a setext level-two heading.
Proposed fix
-## Differences From Laravel
+Differences From Laravel
+-----------------------📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## Differences From Laravel | |
| Differences From Laravel | |
| ----------------------- |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 8-8: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
🤖 Prompt for 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.
In `@src/pagination/README.md` at line 8, Update the “Differences From Laravel”
heading in the README to use setext level-two Markdown syntax instead of ATX
syntax, preserving the heading text and level.
Source: Linters/SAST tools
Summary
This PR fixes Pagination correctness issues across cursor decoding, query construction, page resolution, JSON serialization, database builders, relations, and Scout. It also brings the package up to date with current Laravel paginator behavior and type contracts while preserving Hypervel's coroutine-safe request ownership.
The changes keep paginator instances operation-local, retain boot-owned static configuration, and remove duplicate request resolution from the paginator hot path. Public Laravel APIs remain compatible except where an existing invalid input already failed deeper in the call stack or produced incorrect behavior.
For more details, see: docs/plans/2026-08-06-0928-pagination-correctness-current-parity-and-query-contracts.md
What changed
RequestContextwithout capturing request instances or resolving the request through the container.UrlWindowrequirement explicit.JsonExceptionwithout adding extra serialization passes.Performance
The request resolvers now perform less work by reading coroutine request state once and avoiding container resolution. The only recurring added work is cursor result reindexing required by the public collection contract and a single integer clamp before Scout dispatch. No locks, retries, additional queries, network calls, unbounded caches, or retained request state are introduced.
Validation
composer fixSummary by CodeRabbit
New Features
Bug Fixes
Documentation