Conversation
Member
vjik
commented
Jun 11, 2025
vjik
commented
| Q | A |
|---|---|
| Is bugfix? | ❌ |
| New feature? | ✔️ |
| Breaks BC? | ❌ |
| Fix #212 |
Tigrov
reviewed
Jun 12, 2025
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for using default sort orders in KeysetPaginator when no explicit order is provided, including backwards pagination support, and updates related tests and documentation.
- Introduces
Sort::getDefaultOrder()to retrieve default sorting fields. - Modifies
KeysetPaginatorto apply a single default sort key when the provided sort is empty. - Updates tests to validate default ordering behavior; bumps
rector/rectordev dependency and updatesCHANGELOG.md.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Paginator/KeysetPaginatorTest.php | Replaces “not sorted” exception test with default-order tests; adjusts exception test to use empty sort |
| src/Reader/Sort.php | Adds getDefaultOrder() method to expose configured defaults |
| src/Paginator/KeysetPaginator.php | Refactors constructor and withSort() to use default sort via prepareSortInDataReader(); removes old assertSort() |
| composer.json | Bumps rector/rector from 2.0.15 to 2.0.18 |
| CHANGELOG.md | Logs addition of getDefaultOrder() and enhanced default-order behavior in KeysetPaginator |
Comments suppressed due to low confidence (1)
src/Paginator/KeysetPaginator.php:439
- The docblock above declares a more specific return type (FilterableDataInterface&LimitableDataInterface&ReadableDataInterface&SortableDataInterface) than the declared return type
ReadableDataInterface. Please align the signature and docblock so they match the intended interfaces returned bywithSort().
private function prepareSortInDataReader(ReadableDataInterface $dataReader, ?Sort $sort): ReadableDataInterface
| $this->expectExceptionMessage('Data should be always sorted to work with keyset pagination.'); | ||
|
|
||
| (new KeysetPaginator($dataReader))->withSort(Sort::only(['id', 'name'])); | ||
| (new KeysetPaginator($dataReader))->withSort(Sort::only([])); |
There was a problem hiding this comment.
This test expects an exception when using an empty sort, but KeysetPaginator now applies the default order and won’t throw. Consider disabling default sorting on the Sort instance (e.g., via a dedicated API) or updating the test to reflect that an empty sort yields default ordering rather than an error.
samdark
approved these changes
Jun 12, 2025
Tigrov
approved these changes
Jun 13, 2025
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
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.