Make Paginator use simpler queries when the paginated query allows it.#12518
Open
d-ph wants to merge 1 commit into
Open
Make Paginator use simpler queries when the paginated query allows it.#12518d-ph wants to merge 1 commit into
d-ph wants to merge 1 commit into
Conversation
Contributor
Author
|
Looks like some tests are failing because some of the test DQL queries are not supported in some databases.
Can I do the following please?
Thanks. |
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.
This PR is a follow-up from the following closed/abandoned PR: #11595. It now targets the
3.7.xgit branch, and addresses all the comments from that past PR (apart from "please update the docs", which is explained further below).To recap: this PR attempts to "action" the following ticket: #8278.
In particular: it implements all the use cases that stof mentioned.
I would like to get a "code sign-off" before I go and update the docs about the Paginator. In particular, I would like to get a "looks good" from stof, and greg0ire (or any other ORM maintainer).
Things that might need further discussion:
The "auto-detection" code spans 3 static functions in the
PaginatorQuerytrait, and takes over 600 lines. It's about 70% of thePaginatorQuerysource file. If preferred, I can extract that code to another class.In
PaginatorQuery::searchEntityAliasesUsageInASTNode(), php reflection is used to "walk" AST expressions, in search for specific "entity aliases usage" (namely: whether ToMany JOIN aliases are used anywhere within those expressions). The function recursively lists all public properties ofAST\Nodeclasses, and seeks for instances ofQuery\AST\PathExpression. I believe that the only "contestable" fact here is that php reflection is used. I would however say that the code makes 0 assumptions about the reflected classes, making it quite "safe" to run in this context.I'd like to note that the "auto-detection" treats the result/count "output walkers" (i.e. the current default "code execution path") as the "fallback" if anything during the auto-detection procedure "looks strange". In particular, the auto-detection does NOT attempt to ascertain whether the output walkers can be run. I personally consider this alright.
Stof, what follows from the point above, is that the code does NOT check whether the
LimitSubqueryOutputWalker.phpcan be run (despite your listing (in one of your past comments) the conditions under which that output walker cannot be run). Again: I consider this acceptable but let me know if you have any further comments here.A new query hint was added:
CountWalker::HINT_DROP_GROUP_BY_CLAUSE. It's needed to handle a few "GROUP BY" cases in the CountWalker. I "docblock'd" that hint as@internalbecause I believe that there are already too many query hints in the Paginator. But if it should be "opened to the public", then this can be discussed further.The auto-detection does not allow to run queries with
GROUP BYby the CountWalker (beside a few exceptions).Less obvious fact: the
CountWalker.phpsupports root entities with composite primary keys, but only if theCOUNT(*)branch in its code is executed (as opposed to theCOUNT(DISTINCT id)code branch).Less obvious fact: Paginator currently supports root entities with composite primary keys, only if the "fast result query" is run.
I created the following directory:
src/Tools/Pagination/ValueObject. Note that there already exists the following dir:tests/Tests/Models/ValueObjects(i.e. in plural). I generally don't use plurals in namespaces (or class names). If however naming consistency is desired, I can rename that directory to:src/Tools/Pagination/ValueObjects.Stof, just a quick note that I ensured that all the cases that you listed in the following past comment, are implemented / checked for:
The code is heavily inline-docblocked. This is how I usually write code but I recognise that there are "other schools of thought" in this matter. I'm fine to drop some of the comments if needed, but please tell me which.
In
PaginatorAutoDetectionTest.php, there are test cases that assert an exception being thrown. There are also a few cases which produce a result that I personally did not expect (but those results are what the already released code already does). Please take note of those uses cases and let me know if they are "not acceptable".Thank you.
cc @stof
Please note that phpstan is failing (at least on my local machine) for reasons that are not related to the code in this PR.