[CursorPaginator] Various tweaks and improvements#12404
[CursorPaginator] Various tweaks and improvements#12404seb-jean wants to merge 1 commit intodoctrine:3.7.xfrom
Conversation
|
I suggest using a better name than |
d13637d to
61add19
Compare
fetchJoinCollectionqueryProducesDuplicates
a44d5c9 to
ac7634f
Compare
I renamed |
ac7634f to
1f128e4
Compare
src/Tools/CursorPagination/Exception/RowNumberOverFunctionNotEnabled.php
Outdated
Show resolved
Hide resolved
7470a01 to
dbfe2e4
Compare
queryProducesDuplicates3ca0823 to
2cfb7c2
Compare
2cfb7c2 to
f7ca25b
Compare
703c842 to
3d7894b
Compare
| { | ||
| protected function isDistinctEnabled(AbstractQuery $query): bool | ||
| { | ||
| return ($query->getHints()[CursorPaginator::HINT_ENABLE_DISTINCT] ?? true) === true; |
There was a problem hiding this comment.
couldn't we keep the paginator.distinct.enable hint even when using the cursor paginator ?
There was a problem hiding this comment.
Done. CursorPagination\LimitSubqueryWalker has been removed entirely. CursorPaginator now uses Pagination\LimitSubqueryWalker directly, which already checks Paginator::HINT_ENABLE_DISTINCT. The
CursorPaginator::HINT_ENABLE_DISTINCT constant has been removed as well.
| /** | ||
| * Appends a custom tree walker to the tree walkers hint. | ||
| * | ||
| * @param class-string $walkerClass |
There was a problem hiding this comment.
I suggest using class-string<TreeWalker> here for extra safety.
| } | ||
|
|
||
| if ($query->getHint(self::HINT_QUERY_PRODUCES_DUPLICATES) === false) { | ||
| foreach ($this->getQueryComponents() as $component) { |
There was a problem hiding this comment.
Technically, you could have a filtering clause that ensures that a toMany join does not actually produce duplicates in the result. Should we forbid such case from using the more optimized paginated by doing an explicit choice ?
There was a problem hiding this comment.
Good point. We removed the automatic detection entirely. Since queryProducesDuplicates: false is already an explicit choice by the developer (mirroring fetchJoinCollection in Paginator)
| private readonly AbstractPlatform $platform; | ||
| private readonly ResultSetMapping $rsm; | ||
| private readonly int $firstResult; | ||
| protected int $firstResult; |
There was a problem hiding this comment.
instead of turning this into a mutable protected parameter just to reset it to 0 in the cursor-related LimitSubqueryOutputWalker child class, I would suggest that the CursorPaginator sets the first result to 0 in the Query itself (just like it already sets the max results in it)
There was a problem hiding this comment.
Done. CursorPagination\LimitSubqueryOutputWalker has been removed entirely. CursorPaginator now calls $subQuery->setFirstResult(0) directly alongside setMaxResults($limit + 1), and uses Pagination\LimitSubqueryOutputWalker directly. $firstResult is back to private readonly in the base class.
| return $this->count; | ||
| } | ||
|
|
||
| private function getCountQuery(): Query |
There was a problem hiding this comment.
As the counting implementation is the same in both paginators, I suggest extracting an internal trait used in both classes to avoid duplicating maintenance.
There was a problem hiding this comment.
Done. The shared logic (getCountQuery and unbindUnusedQueryParams) has been extracted into an internal CountQuery trait in src/Tools/Pagination/CountQuery.php, used by both Paginator and CursorPaginator.
The $count property has also been moved into the trait.
| return $this->items->count(); | ||
| } | ||
|
|
||
| public function getTotalCount(): int |
There was a problem hiding this comment.
getTotalCount does not seem to be covered by tests.
There was a problem hiding this comment.
Fixed, two tests have been added.
3d7894b to
796cec7
Compare
| .. note:: | ||
|
|
||
| Passing ``false`` on a query that joins a to-many relation may throw a | ||
| ``LogicException`` at runtime when Doctrine can detect the mistake. |
There was a problem hiding this comment.
this LogicException has now been removed.
| @@ -4,42 +4,60 @@ | |||
|
|
|||
| namespace Doctrine\ORM\Tools\CursorPagination; | |||
There was a problem hiding this comment.
@greg0ire I suggest moving this CursorPaginator to the Pagination namespace instead of having 2 namespaces related to pagination (with CursorPagination reusing big parts of Pagination). What do you think about that ?
This is a decision we need to take before the 3.7.0 release, as moving the class after that would be a BC break.
| use function array_key_exists; | ||
|
|
||
| /** | ||
| * Provides the count query implementation shared by {@see Paginator} and {@see CursorPaginator}. |
There was a problem hiding this comment.
This @see referencing a short class name would need a use statement.
| $countQuery->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, CountOutputWalker::class); | ||
| $countQuery->setResultSetMapping($rsm); | ||
| } else { | ||
| $this->appendTreeWalker($countQuery, CountWalker::class); |
There was a problem hiding this comment.
appendTreeWalker should be moved to the trait as it is used here.
| $countQuery->setHint(CountWalker::HINT_DISTINCT, true); | ||
| } | ||
|
|
||
| if ($this->useOutputWalker($countQuery)) { |
There was a problem hiding this comment.
useOutputWalker should probably be moved to the trait (or the trait should define an abstract private method for it)
| * | ||
| * @internal | ||
| */ | ||
| trait CountQuery |
There was a problem hiding this comment.
maybe this should be a PaginatorTrait instead, copying more private methods (cloneQuery, appendTreeWalker and convertWhereInIdentifiersToDatabaseValues for instance, and maybe everything about the output walker config)
|
|
||
| $this->items = new ArrayCollection($whereInQuery->getResult()); | ||
| } else { | ||
| $this->items = new ArrayCollection($subQuery->getResult()); |
There was a problem hiding this comment.
I would use a local variable storing the array here instead of creating an ArrayCollection that will be dropped just after. the usage of an ArrayCollection is overkill here.
| $this->orderByItems = $subQuery->getHint(CursorWalker::HINT_CURSOR_ORDER_BY_ITEMS) ?: []; | ||
|
|
||
| if ($this->cursor !== null && $this->cursor->isPrevious()) { | ||
| $this->items = new ArrayCollection(array_reverse($this->items->toArray(), true)); |
There was a problem hiding this comment.
if the result is a list, we should not preserve keys when reversing.
Problem
When a DQL query joins a toMany relation, a single entity can span multiple SQL rows. The previous implementation applied
setMaxResults()at the SQL level, causing pages to silently return fewer items than expected and incorrecthasMoredetection. Raised by @stof in Add cursor-based pagination #12364 (review).Countableinconsistency — The previouscount()returned the number of items on the current page, inconsistent withPaginatorwhich returns the total. Raised by @stof in Add cursor-based pagination #12364 (review).paginate()only accepted an encoded string — Passing aCursorinstance directly required an unnecessary round-trip throughencodeToString(). Raised inAllow
Cursorinstance inCursorPaginator::paginate()instead of only encoded string #12405.Changes
queryProducesDuplicatesdefaults totrueCursorPaginator—queryProducesDuplicatesnow defaults totrue:LimitSubqueryOutputWalker/LimitSubqueryWalkerWhereInWalkergetTotalCount() + count()
count()— returns the number of items on the current page (no DB query)getTotalCount()— runs aCOUNTquery (identical implementation to Paginator::count())Countableis not implemented to avoid triggering an implicit COUNT query.paginate()acceptsCursor|string|nullpaginate()now accepts aCursorinstance directly in addition to an encoded string, removing the need for an unnecessary encode/decode round-trip when the cursor is already available as an object: