From 918d2910d9e2b85d1e93e94e05ed405e647185d5 Mon Sep 17 00:00:00 2001 From: Kiel Goodman Date: Sat, 6 Dec 2014 14:40:03 +0000 Subject: [PATCH 1/4] [DDC-3108] Fix regression introduced in DDC-2764 where join aliases were no longer accessible in Criteria expressions --- .../ORM/Query/QueryExpressionVisitor.php | 46 +++++++--- lib/Doctrine/ORM/QueryBuilder.php | 41 ++++++++- .../ORM/Query/QueryExpressionVisitorTest.php | 6 +- tests/Doctrine/Tests/ORM/QueryBuilderTest.php | 90 +++++++++++++++++++ 4 files changed, 165 insertions(+), 18 deletions(-) diff --git a/lib/Doctrine/ORM/Query/QueryExpressionVisitor.php b/lib/Doctrine/ORM/Query/QueryExpressionVisitor.php index da4577132..c99c5ed52 100644 --- a/lib/Doctrine/ORM/Query/QueryExpressionVisitor.php +++ b/lib/Doctrine/ORM/Query/QueryExpressionVisitor.php @@ -45,9 +45,9 @@ class QueryExpressionVisitor extends ExpressionVisitor ); /** - * @var string + * @var array */ - private $rootAlias; + private $queryAliases; /** * @var Expr @@ -62,11 +62,11 @@ class QueryExpressionVisitor extends ExpressionVisitor /** * Constructor * - * @param string $rootAlias + * @param string $queryAliases */ - public function __construct($rootAlias) + public function __construct($queryAliases) { - $this->rootAlias = $rootAlias; + $this->queryAliases = $queryAliases; $this->expr = new Expr(); } @@ -131,7 +131,25 @@ class QueryExpressionVisitor extends ExpressionVisitor */ public function walkComparison(Comparison $comparison) { - $parameterName = str_replace('.', '_', $comparison->getField()); + + if ( ! isset($this->queryAliases[0])) { + throw new \RuntimeException('No aliases are set before invoking walkComparison().'); + } + $field = $comparison->getField(); + + $hasValidAlias = false; + foreach($this->queryAliases as $alias) { + if(strpos($field . '.', $alias . '.') === 0) { + $hasValidAlias = true; + break; + } + } + + $parameterName = str_replace('.', '_', $field); + + if(!$hasValidAlias) { + $field = $this->queryAliases[0] . '.' . $field; + } foreach($this->parameters as $parameter) { if($parameter->getName() === $parameterName) { @@ -146,38 +164,38 @@ class QueryExpressionVisitor extends ExpressionVisitor switch ($comparison->getOperator()) { case Comparison::IN: $this->parameters[] = $parameter; - return $this->expr->in($this->rootAlias . '.' . $comparison->getField(), $placeholder); + return $this->expr->in($field, $placeholder); case Comparison::NIN: $this->parameters[] = $parameter; - return $this->expr->notIn($this->rootAlias . '.' . $comparison->getField(), $placeholder); + return $this->expr->notIn($field, $placeholder); case Comparison::EQ: case Comparison::IS: if ($this->walkValue($comparison->getValue()) === null) { - return $this->expr->isNull($this->rootAlias . '.' . $comparison->getField()); + return $this->expr->isNull($field); } $this->parameters[] = $parameter; - return $this->expr->eq($this->rootAlias . '.' . $comparison->getField(), $placeholder); + return $this->expr->eq($field, $placeholder); case Comparison::NEQ: if ($this->walkValue($comparison->getValue()) === null) { - return $this->expr->isNotNull($this->rootAlias . '.' . $comparison->getField()); + return $this->expr->isNotNull($field); } $this->parameters[] = $parameter; - return $this->expr->neq($this->rootAlias . '.' . $comparison->getField(), $placeholder); + return $this->expr->neq($field, $placeholder); case Comparison::CONTAINS: $parameter->setValue('%' . $parameter->getValue() . '%', $parameter->getType()); $this->parameters[] = $parameter; - return $this->expr->like($this->rootAlias . '.' . $comparison->getField(), $placeholder); + return $this->expr->like($field, $placeholder); default: $operator = self::convertComparisonOperator($comparison->getOperator()); if ($operator) { $this->parameters[] = $parameter; return new Expr\Comparison( - $this->rootAlias . '.' . $comparison->getField(), + $field, $operator, $placeholder ); diff --git a/lib/Doctrine/ORM/QueryBuilder.php b/lib/Doctrine/ORM/QueryBuilder.php index 0dda54c7a..5ac67ebe2 100644 --- a/lib/Doctrine/ORM/QueryBuilder.php +++ b/lib/Doctrine/ORM/QueryBuilder.php @@ -459,6 +459,24 @@ class QueryBuilder return $aliases; } + /** + * Gets all the aliases that have been used in the query. + * Including all select root aliases and join aliases + * + * + * $qb = $em->createQueryBuilder() + * ->select('u') + * ->from('User', 'u') + * ->join('u.articles','a'; + * + * $qb->getAllAliases(); // array('u','a') + * + * @return array + */ + public function getAllAliases() { + return array_merge($this->getRootAliases(),array_keys($this->joinRootAliases)); + } + /** * Gets the root entities of the query. This is the entity aliases involved * in the construction of the query. @@ -1221,8 +1239,12 @@ class QueryBuilder */ public function addCriteria(Criteria $criteria) { - $rootAlias = $this->getRootAlias(); - $visitor = new QueryExpressionVisitor($rootAlias); + $allAliases = $this->getAllAliases(); + if ( ! isset($allAliases[0])) { + throw new \RuntimeException('No aliases are set before invoking addCriteria().'); + } + + $visitor = new QueryExpressionVisitor($this->getAllAliases()); if ($whereExpression = $criteria->getWhereExpression()) { $this->andWhere($visitor->dispatch($whereExpression)); @@ -1233,7 +1255,20 @@ class QueryBuilder if ($criteria->getOrderings()) { foreach ($criteria->getOrderings() as $sort => $order) { - $this->addOrderBy($rootAlias . '.' . $sort, $order); + + $hasValidAlias = false; + foreach($allAliases as $alias) { + if(strpos($sort . '.', $alias . '.') === 0) { + $hasValidAlias = true; + break; + } + } + + if(!$hasValidAlias) { + $sort = $allAliases[0] . '.' . $sort; + } + + $this->addOrderBy($sort, $order); } } diff --git a/tests/Doctrine/Tests/ORM/Query/QueryExpressionVisitorTest.php b/tests/Doctrine/Tests/ORM/Query/QueryExpressionVisitorTest.php index 1761da043..06a23199e 100644 --- a/tests/Doctrine/Tests/ORM/Query/QueryExpressionVisitorTest.php +++ b/tests/Doctrine/Tests/ORM/Query/QueryExpressionVisitorTest.php @@ -47,7 +47,7 @@ class QueryExpressionVisitorTest extends \PHPUnit_Framework_TestCase */ protected function setUp() { - $this->visitor = new QueryExpressionVisitor('o'); + $this->visitor = new QueryExpressionVisitor(['o','p']); } /** @@ -89,6 +89,10 @@ class QueryExpressionVisitorTest extends \PHPUnit_Framework_TestCase // Test parameter conversion array($cb->eq('object.field', 'value'), $qb->eq('o.object.field', ':object_field'), new Parameter('object_field', 'value')), + + // Test alternative rootAlias + array($cb->eq('p.field', 'value'), $qb->eq('p.field', ':p_field'), new Parameter('p_field', 'value')), + array($cb->eq('p.object.field', 'value'), $qb->eq('p.object.field', ':p_object_field'), new Parameter('p_object_field', 'value')), ); } diff --git a/tests/Doctrine/Tests/ORM/QueryBuilderTest.php b/tests/Doctrine/Tests/ORM/QueryBuilderTest.php index fe545927d..ef240e379 100644 --- a/tests/Doctrine/Tests/ORM/QueryBuilderTest.php +++ b/tests/Doctrine/Tests/ORM/QueryBuilderTest.php @@ -522,6 +522,22 @@ class QueryBuilderTest extends \Doctrine\Tests\OrmTestCase $this->assertEquals('u.field DESC', (string) $orderBy[0]); } + public function testAddCriteriaOrderOnJoinAlias() + { + $qb = $this->_em->createQueryBuilder(); + $qb->select('u') + ->from('Doctrine\Tests\Models\CMS\CmsUser', 'u') + ->join('u.article','a'); + + $criteria = new Criteria(); + $criteria->orderBy(array('a.field' => Criteria::DESC)); + + $qb->addCriteria($criteria); + + $this->assertCount(1, $orderBy = $qb->getDQLPart('orderBy')); + $this->assertEquals('a.field DESC', (string) $orderBy[0]); + } + public function testAddCriteriaLimit() { $qb = $this->_em->createQueryBuilder(); @@ -847,6 +863,69 @@ class QueryBuilderTest extends \Doctrine\Tests\OrmTestCase $this->assertEquals(2, $expr->count(), "Modifying the second query should affect the first one."); } + /** + * @group DDC-3108 + */ + public function testAddCriteriaWhereWithJoinAlias() + { + $qb = $this->_em->createQueryBuilder(); + $qb->select('alias1')->from('Doctrine\Tests\Models\CMS\CmsUser', 'alias1'); + $qb->join('alias1.articles','alias2'); + + $criteria = new Criteria(); + $criteria->where($criteria->expr()->eq('field', 'value1')); + $criteria->andWhere($criteria->expr()->gt('alias2.field', 'value2')); + + $qb->addCriteria($criteria); + + $this->assertEquals('alias1.field = :field AND alias2.field > :alias2_field', (string) $qb->getDQLPart('where')); + $this->assertSame('value1', $qb->getParameter('field')->getValue()); + $this->assertSame('value2', $qb->getParameter('alias2_field')->getValue()); + } + + /** + * @group DDC-3108 + */ + public function testAddCriteriaWhereWithDefaultAndJoinAlias() + { + $qb = $this->_em->createQueryBuilder(); + $qb->select('alias1')->from('Doctrine\Tests\Models\CMS\CmsUser', 'alias1'); + $qb->join('alias1.articles','alias2'); + + $criteria = new Criteria(); + $criteria->where($criteria->expr()->eq('alias1.field', 'value1')); + $criteria->andWhere($criteria->expr()->gt('alias2.field', 'value2')); + + $qb->addCriteria($criteria); + + $this->assertEquals('alias1.field = :alias1_field AND alias2.field > :alias2_field', (string) $qb->getDQLPart('where')); + $this->assertSame('value1', $qb->getParameter('alias1_field')->getValue()); + $this->assertSame('value2', $qb->getParameter('alias2_field')->getValue()); + } + + /** + * @group DDC-3108 + */ + public function testAddCriteriaWhereOnJoinAliasWithDuplicateFields() + { + $qb = $this->_em->createQueryBuilder(); + $qb->select('alias1')->from('Doctrine\Tests\Models\CMS\CmsUser', 'alias1'); + $qb->join('alias1.articles','alias2'); + + $criteria = new Criteria(); + $criteria->where($criteria->expr()->eq('alias1.field', 'value1')); + $criteria->andWhere($criteria->expr()->gt('alias2.field', 'value2')); + $criteria->andWhere($criteria->expr()->lt('alias2.field', 'value3')); + + $qb->addCriteria($criteria); + + $this->assertEquals('(alias1.field = :alias1_field AND alias2.field > :alias2_field) AND alias2.field < :alias2_field_2', (string) $qb->getDQLPart('where')); + $this->assertSame('value1', $qb->getParameter('alias1_field')->getValue()); + $this->assertSame('value2', $qb->getParameter('alias2_field')->getValue()); + $this->assertSame('value3', $qb->getParameter('alias2_field_2')->getValue()); + } + + /** * @group DDC-1933 */ @@ -902,6 +981,17 @@ class QueryBuilderTest extends \Doctrine\Tests\OrmTestCase $this->assertEquals('u', $qb->getRootAlias()); } + public function testGetAliasesAddedWithJoins() + { + $qb = $this->_em->createQueryBuilder() + ->select('u') + ->from('Doctrine\Tests\Models\CMS\CmsUser', 'u') + ->join('u.articles','a'); + + $this->assertEquals(array('u', 'a'), $qb->getAllAliases()); + $this->assertEquals('u', $qb->getRootAlias()); + } + public function testBCAddJoinWithoutRootAlias() { $qb = $this->_em->createQueryBuilder() From 349966832f1532c35be36c68ab3e1c94668f1bfb Mon Sep 17 00:00:00 2001 From: Kiel Goodman Date: Sat, 6 Dec 2014 14:59:31 +0000 Subject: [PATCH 2/4] [DDC-3436] Convert short array syntax to legacy style --- tests/Doctrine/Tests/ORM/Query/QueryExpressionVisitorTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Doctrine/Tests/ORM/Query/QueryExpressionVisitorTest.php b/tests/Doctrine/Tests/ORM/Query/QueryExpressionVisitorTest.php index 06a23199e..2b9fc2304 100644 --- a/tests/Doctrine/Tests/ORM/Query/QueryExpressionVisitorTest.php +++ b/tests/Doctrine/Tests/ORM/Query/QueryExpressionVisitorTest.php @@ -47,7 +47,7 @@ class QueryExpressionVisitorTest extends \PHPUnit_Framework_TestCase */ protected function setUp() { - $this->visitor = new QueryExpressionVisitor(['o','p']); + $this->visitor = new QueryExpressionVisitor(array('o','p')); } /** From dc98da585d22332d90b37299c92c474b57ee0009 Mon Sep 17 00:00:00 2001 From: Kiel Goodman Date: Mon, 12 Jan 2015 14:28:55 +0000 Subject: [PATCH 3/4] [DDC-3436] Fix changes requested 1/2 --- lib/Doctrine/ORM/Query/QueryExpressionVisitor.php | 4 ++-- lib/Doctrine/ORM/QueryBuilder.php | 6 +++--- tests/Doctrine/Tests/ORM/QueryBuilderTest.php | 14 +++----------- 3 files changed, 8 insertions(+), 16 deletions(-) diff --git a/lib/Doctrine/ORM/Query/QueryExpressionVisitor.php b/lib/Doctrine/ORM/Query/QueryExpressionVisitor.php index c99c5ed52..7cea4e0bc 100644 --- a/lib/Doctrine/ORM/Query/QueryExpressionVisitor.php +++ b/lib/Doctrine/ORM/Query/QueryExpressionVisitor.php @@ -62,7 +62,7 @@ class QueryExpressionVisitor extends ExpressionVisitor /** * Constructor * - * @param string $queryAliases + * @param array $queryAliases */ public function __construct($queryAliases) { @@ -133,7 +133,7 @@ class QueryExpressionVisitor extends ExpressionVisitor { if ( ! isset($this->queryAliases[0])) { - throw new \RuntimeException('No aliases are set before invoking walkComparison().'); + throw new QueryException('No aliases are set before invoking walkComparison().'); } $field = $comparison->getField(); diff --git a/lib/Doctrine/ORM/QueryBuilder.php b/lib/Doctrine/ORM/QueryBuilder.php index 5ac67ebe2..f3cad510d 100644 --- a/lib/Doctrine/ORM/QueryBuilder.php +++ b/lib/Doctrine/ORM/QueryBuilder.php @@ -473,7 +473,7 @@ class QueryBuilder * * @return array */ - public function getAllAliases() { + private function getAllAliases() { return array_merge($this->getRootAliases(),array_keys($this->joinRootAliases)); } @@ -1234,14 +1234,14 @@ class QueryBuilder * Overrides firstResult and maxResults if they're set. * * @param Criteria $criteria - * * @return QueryBuilder + * @throws Query\QueryException */ public function addCriteria(Criteria $criteria) { $allAliases = $this->getAllAliases(); if ( ! isset($allAliases[0])) { - throw new \RuntimeException('No aliases are set before invoking addCriteria().'); + throw new Query\QueryException('No aliases are set before invoking addCriteria().'); } $visitor = new QueryExpressionVisitor($this->getAllAliases()); diff --git a/tests/Doctrine/Tests/ORM/QueryBuilderTest.php b/tests/Doctrine/Tests/ORM/QueryBuilderTest.php index ef240e379..c10177942 100644 --- a/tests/Doctrine/Tests/ORM/QueryBuilderTest.php +++ b/tests/Doctrine/Tests/ORM/QueryBuilderTest.php @@ -522,6 +522,9 @@ class QueryBuilderTest extends \Doctrine\Tests\OrmTestCase $this->assertEquals('u.field DESC', (string) $orderBy[0]); } + /** + * @group DDC-3108 + */ public function testAddCriteriaOrderOnJoinAlias() { $qb = $this->_em->createQueryBuilder(); @@ -981,17 +984,6 @@ class QueryBuilderTest extends \Doctrine\Tests\OrmTestCase $this->assertEquals('u', $qb->getRootAlias()); } - public function testGetAliasesAddedWithJoins() - { - $qb = $this->_em->createQueryBuilder() - ->select('u') - ->from('Doctrine\Tests\Models\CMS\CmsUser', 'u') - ->join('u.articles','a'); - - $this->assertEquals(array('u', 'a'), $qb->getAllAliases()); - $this->assertEquals('u', $qb->getRootAlias()); - } - public function testBCAddJoinWithoutRootAlias() { $qb = $this->_em->createQueryBuilder() From 0c5ea34fd49b83c9252a95cffdf3b6345327fea2 Mon Sep 17 00:00:00 2001 From: Kiel Goodman Date: Mon, 12 Jan 2015 14:44:46 +0000 Subject: [PATCH 4/4] [DDC-3436] Fix changes requested 2/2 --- lib/Doctrine/ORM/Query/QueryExpressionVisitor.php | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/lib/Doctrine/ORM/Query/QueryExpressionVisitor.php b/lib/Doctrine/ORM/Query/QueryExpressionVisitor.php index 7cea4e0bc..fe547081f 100644 --- a/lib/Doctrine/ORM/Query/QueryExpressionVisitor.php +++ b/lib/Doctrine/ORM/Query/QueryExpressionVisitor.php @@ -135,21 +135,17 @@ class QueryExpressionVisitor extends ExpressionVisitor if ( ! isset($this->queryAliases[0])) { throw new QueryException('No aliases are set before invoking walkComparison().'); } - $field = $comparison->getField(); - $hasValidAlias = false; + $field = $this->queryAliases[0] . '.' . $comparison->getField(); + foreach($this->queryAliases as $alias) { - if(strpos($field . '.', $alias . '.') === 0) { - $hasValidAlias = true; + if(strpos($comparison->getField() . '.', $alias . '.') === 0) { + $field = $comparison->getField(); break; } } - $parameterName = str_replace('.', '_', $field); - - if(!$hasValidAlias) { - $field = $this->queryAliases[0] . '.' . $field; - } + $parameterName = str_replace('.', '_', $comparison->getField()); foreach($this->parameters as $parameter) { if($parameter->getName() === $parameterName) {