From 76fda9562c8726d229ad90f24f159fee38309750 Mon Sep 17 00:00:00 2001 From: flip111 Date: Thu, 26 Sep 2013 14:11:56 +0200 Subject: [PATCH 1/3] Update SqlWalker.php fixed wrong GROUP BY clause on SQL Server platform Without this patch a query would like like: ``` SELECT c0_.Country AS sclr0 FROM Continent c0_ WITH (NOLOCK) WHERE c0_.Country = 38 GROUP BY sclr0 ``` Using the column alias in the GROUP BY clause. However this is not allowed on SQL Server. References: 1. http://stackoverflow.com/a/3841804 2. http://technet.microsoft.com/en-us/library/ms189499.aspx (Logical Processing Order of the SELECT statement) The correct query should be: ``` SELECT c0_.Country AS sclr0 FROM Continent c0_ WITH (NOLOCK) WHERE c0_.Country = 38 GROUP BY c0_.Country ``` --- lib/Doctrine/ORM/Query/SqlWalker.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php index 082f95f50..c3fb467de 100644 --- a/lib/Doctrine/ORM/Query/SqlWalker.php +++ b/lib/Doctrine/ORM/Query/SqlWalker.php @@ -21,6 +21,7 @@ namespace Doctrine\ORM\Query; use Doctrine\DBAL\LockMode; use Doctrine\DBAL\Types\Type; +use Doctrine\DBAL\Platforms\SQLServerPlatform; use Doctrine\ORM\Mapping\ClassMetadata; use Doctrine\ORM\Query; use Doctrine\ORM\Query\QueryException; @@ -1624,7 +1625,11 @@ class SqlWalker implements TreeWalker // ResultVariable if (isset($this->queryComponents[$groupByItem]['resultVariable'])) { - return $this->walkResultVariable($groupByItem); + if ($this->platform instanceof SQLServerPlatform) { + return $this->walkPathExpression($this->queryComponents[$groupByItem]['resultVariable']->pathExpression); + } else { + return $this->walkResultVariable($groupByItem); + } } // IdentificationVariable From 72ae7f5497adf0afdcf532c82fc8fc94e495226f Mon Sep 17 00:00:00 2001 From: flip111 Date: Mon, 30 Sep 2013 11:08:42 +0200 Subject: [PATCH 2/3] Changed GroupBy alias to real column name for all platforms and adjusted failing test accordingly. Has fallback in cases where real column name is not possible (example: Doctrine\Tests\ORM\Query\SelectSqlGenerationTest::testGroupBySupportsIdentificationVariable) --- lib/Doctrine/ORM/Query/SqlWalker.php | 5 +++-- tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php | 3 +-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php index c3fb467de..946579b8a 100644 --- a/lib/Doctrine/ORM/Query/SqlWalker.php +++ b/lib/Doctrine/ORM/Query/SqlWalker.php @@ -21,7 +21,6 @@ namespace Doctrine\ORM\Query; use Doctrine\DBAL\LockMode; use Doctrine\DBAL\Types\Type; -use Doctrine\DBAL\Platforms\SQLServerPlatform; use Doctrine\ORM\Mapping\ClassMetadata; use Doctrine\ORM\Query; use Doctrine\ORM\Query\QueryException; @@ -1625,7 +1624,9 @@ class SqlWalker implements TreeWalker // ResultVariable if (isset($this->queryComponents[$groupByItem]['resultVariable'])) { - if ($this->platform instanceof SQLServerPlatform) { + if ($this->queryComponents[$groupByItem]['resultVariable'] instanceof AST\PathExpression) { + return $this->walkPathExpression($this->queryComponents[$groupByItem]['resultVariable']); + } elseif (isset($this->queryComponents[$groupByItem]['resultVariable']->pathExpression)) { return $this->walkPathExpression($this->queryComponents[$groupByItem]['resultVariable']->pathExpression); } else { return $this->walkResultVariable($groupByItem); diff --git a/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php b/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php index 2587905ed..208b57b6b 100644 --- a/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php +++ b/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php @@ -1573,7 +1573,7 @@ class SelectSqlGenerationTest extends \Doctrine\Tests\OrmTestCase { $this->assertSqlGeneration( 'SELECT u, u.status AS st FROM Doctrine\Tests\Models\CMS\CmsUser u GROUP BY st', - 'SELECT c0_.id AS id0, c0_.status AS status1, c0_.username AS username2, c0_.name AS name3, c0_.status AS status4 FROM cms_users c0_ GROUP BY status4' + 'SELECT c0_.id AS id0, c0_.status AS status1, c0_.username AS username2, c0_.name AS name3, c0_.status AS status4 FROM cms_users c0_ GROUP BY c0_.status' ); } @@ -2257,4 +2257,3 @@ class DDC1474Entity } } - From 228a501014fa0e0de30019154bf755faf5786fc5 Mon Sep 17 00:00:00 2001 From: flip111 Date: Tue, 1 Oct 2013 16:53:53 +0200 Subject: [PATCH 3/3] Made the code prettier :) --- lib/Doctrine/ORM/Query/SqlWalker.php | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php index 946579b8a..fb6177919 100644 --- a/lib/Doctrine/ORM/Query/SqlWalker.php +++ b/lib/Doctrine/ORM/Query/SqlWalker.php @@ -1624,13 +1624,17 @@ class SqlWalker implements TreeWalker // ResultVariable if (isset($this->queryComponents[$groupByItem]['resultVariable'])) { - if ($this->queryComponents[$groupByItem]['resultVariable'] instanceof AST\PathExpression) { - return $this->walkPathExpression($this->queryComponents[$groupByItem]['resultVariable']); - } elseif (isset($this->queryComponents[$groupByItem]['resultVariable']->pathExpression)) { - return $this->walkPathExpression($this->queryComponents[$groupByItem]['resultVariable']->pathExpression); - } else { - return $this->walkResultVariable($groupByItem); + $resultVariable = $this->queryComponents[$groupByItem]['resultVariable']; + + if ($resultVariable instanceof AST\PathExpression) { + return $this->walkPathExpression($resultVariable); } + + if (isset($resultVariable->pathExpression)) { + return $this->walkPathExpression($resultVariable->pathExpression); + } + + return $this->walkResultVariable($groupByItem); } // IdentificationVariable