Paginator OrderBy fix take 2 #1337
Conversation
…query-output-only-once'"" This reverts commit 6a17559.
|
Hello, thank you for creating this pull request. I have automatically opened an issue http://www.doctrine-project.org/jira/browse/DDC-3623 We use Jira to track the state of pull requests and the versions they got |
| $selects = []; | ||
| if (preg_match_all('/PathExpression\([^\)]+"identificationVariable": \'([^\']*)\'[^\)]+"field": \'([^\']*)\'[^\)]+\)/i', $orderByDump, $matches, PREG_SET_ORDER)) { | ||
| foreach($matches as $match) { | ||
| $idVar = $match[1]; |
There was a problem hiding this comment.
Please use named subpatterns if possible
| */ | ||
| public function preserveSqlOrdering(SelectStatement $AST, array $sqlIdentifier, $innerSql, $sql) | ||
| public function preserveSqlOrdering(array $sqlIdentifier, $innerSql, $sql, $orderByClause) |
There was a problem hiding this comment.
Hmmm this is public API. Can we really change method signature and return type here?
There was a problem hiding this comment.
This should probably be private.
…ificantly less shameful hack
| // Remove order by clause from the inner query | ||
| // It will be re-appended in the outer select generated by this method | ||
| $orderByClause = $AST->orderByClause; | ||
| $AST->orderByClause = null; |
There was a problem hiding this comment.
I'm not quite sure if this does any harm to non SQL Server platforms which actually support ORDER BY clauses in subqueries. This might change results, no? So an option could be to throw an exception for SQL Server or just ignore the ORDER BY as done here. Please correct me if I'm wrong here...
There was a problem hiding this comment.
essentially an order by clause in a subquery without a limit applied is meaningless - there may be scenarios where ordering by in a subquery can be implemented by a DBA to help with joins on subordinate keys, but the ORM doesn't do that.
|
Is this ticket related to this one? #1151 |
|
@guilhermeblanco fixed capitalization |
|
@raziel057 I believe this should fix #1151 |
|
@zeroedin-bill Ok thanks. I can check if the bug is fixed as soon as the PR is merged. |
| * | ||
| * @return string | ||
| * | ||
| * @throws \RuntimeException | ||
| */ | ||
| public function walkSelectStatement(SelectStatement $AST) | ||
| public function walkSelectStatement(SelectStatement $AST, $addMissingItemsFromOrderByToSelect = true) |
There was a problem hiding this comment.
why are you adding an extra argument compared to the walker API ? this looks wrong to me (and it can cause issues later).
The logic needing to be shared between 2 cases should be extracted to a separate method and the case of addMissingItemsFromOrderByToSelect should use another method than walkSelectStatement for the case it needs a separate logic
There was a problem hiding this comment.
The reason for this is because the walker clones itself and then calls walkSelectStatement on the clone. The method that does the cloning and that nonsense is called from walkSelectStatement. Not sure how better to structure that call chain so as to avoid recursion and still allow the functionality.
I'm open to suggestions...
There was a problem hiding this comment.
@stof would it make sense to move the body of walkSelectStatement to a separate method, say doWalkSelectStatement(SelectStatement $AST, $addMissingItemsFromOrderByToSelect) and then have the main walkSelectStatement method just call that? Not sure that obfuscation would help anything but it does resolve the function signature weirdness.
…sues' into hotfix/#1342-paginator-functional-test-integration
…sues' into hotfix/#1342-paginator-functional-test-integration
|
This broke the same the thing we reported in #1267. Getting pretty much the same error:
|
|
@mrkrstphr please make a failing functional test. |
|
I'll work on putting one together later today. |
This PR fixes issues created in #1220, reported in #1267. The original PR was reverted in #1283.
The issue was that in queries ordered by joined association columns, the joined column aliases did not exist in the outer query created in LimitSubqueryOutputWalker.
This PR adds functionality to LimitSubqueryOutputWalker which finds any such columns referenced in the OrderBy clause, and adds them to the SelectStatement prior to SQL generation.