-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed issue when paginator orders by a subselect expression #1463
Fixed issue when paginator orders by a subselect expression #1463
Conversation
…rms supporting ROW_NUMBER OVER() function.
Hello, thank you for creating this pull request. I have automatically opened an issue http://www.doctrine-project.org/jira/browse/DDC-3831 We use Jira to track the state of pull requests and the versions they got |
I think the coverage drop is down to the change not running under SQLite. Running the tests under postgres, this change is definitely covered. |
@Ehimen that's alright, we should rather merge the coverage in future. Thanks! |
@@ -1093,6 +1093,10 @@ public function walkOrderByItem($orderByItem) | |||
|
|||
$this->orderedColumnsMap[$sql] = $type; | |||
|
|||
if ($expr instanceof Query\AST\Subselect) { | |||
$sql = "($sql)"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use explicit string concatenation here, please. Eventually simply return early
…ression fix; syntax fixes.
Thanks @Ocramius! I've updated with some changes. Are these what you had in mind? |
Fixed issue when paginator orders by a subselect expression
Fixed issue when paginator orders by a subselect expression Conflicts: tests/Doctrine/Tests/ORM/Tools/Pagination/LimitSubqueryOutputWalkerTest.php
On platforms that support the ROW_NUMBER() OVER() function, we've found issues when ordering by a subselect expression. Have included a test demonstrating the problem that was failing with a syntax error under Postgres: