Skip to content

Commit c785206

Browse files
Marukome0743Marukome0743
committed
Fix criteria parameter name collisions
Let QueryBuilder bind parameters generated from Criteria so that it can resolve collisions across separate addCriteria() calls. Keep parameter naming local to QueryExpressionVisitor when no binder is provided. This follows the design suggested on the earlier attempt instead of passing all existing query parameters into the visitor. Fixes #8702
1 parent 2bae808 commit c785206

4 files changed

Lines changed: 165 additions & 29 deletions

File tree

‎src/Query/QueryExpressionVisitor.php‎

Lines changed: 30 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
namespace Doctrine\ORM\Query;
66

7+
use Closure;
78
use Doctrine\Common\Collections\ArrayCollection;
89
use Doctrine\Common\Collections\Expr\Comparison;
910
use Doctrine\Common\Collections\Expr\CompositeExpression;
@@ -32,11 +33,19 @@ class QueryExpressionVisitor extends ExpressionVisitor
3233
/** @var list<mixed> */
3334
private array $parameters = [];
3435

35-
/** @param mixed[] $queryAliases */
36+
/** @var Closure(Parameter): non-empty-string|null */
37+
private readonly Closure|null $parameterBinder;
38+
39+
/**
40+
* @param mixed[] $queryAliases
41+
* @param (callable(Parameter): non-empty-string)|null $parameterBinder
42+
*/
3643
public function __construct(
3744
private readonly array $queryAliases,
45+
callable|null $parameterBinder = null,
3846
) {
39-
$this->expr = new Expr();
47+
$this->expr = new Expr();
48+
$this->parameterBinder = $parameterBinder === null ? null : Closure::fromCallable($parameterBinder);
4049
}
4150

4251
/**
@@ -103,76 +112,72 @@ public function walkComparison(Comparison $comparison): mixed
103112
}
104113
}
105114

106-
$parameter = new Parameter($parameterName, $this->walkValue($comparison->getValue()));
107-
$placeholder = ':' . $parameterName;
115+
$parameter = new Parameter($parameterName, $this->walkValue($comparison->getValue()));
108116

109117
switch ($comparison->getOperator()) {
110118
case Comparison::IN:
111-
$this->parameters[] = $parameter;
112-
113-
return $this->expr->in($field, $placeholder);
119+
return $this->expr->in($field, $this->bindParameter($parameter));
114120

115121
case Comparison::NIN:
116-
$this->parameters[] = $parameter;
117-
118-
return $this->expr->notIn($field, $placeholder);
122+
return $this->expr->notIn($field, $this->bindParameter($parameter));
119123

120124
case Comparison::EQ:
121125
case Comparison::IS:
122126
if ($this->walkValue($comparison->getValue()) === null) {
123127
return $this->expr->isNull($field);
124128
}
125129

126-
$this->parameters[] = $parameter;
127-
128-
return $this->expr->eq($field, $placeholder);
130+
return $this->expr->eq($field, $this->bindParameter($parameter));
129131

130132
case Comparison::NEQ:
131133
if ($this->walkValue($comparison->getValue()) === null) {
132134
return $this->expr->isNotNull($field);
133135
}
134136

135-
$this->parameters[] = $parameter;
136-
137-
return $this->expr->neq($field, $placeholder);
137+
return $this->expr->neq($field, $this->bindParameter($parameter));
138138

139139
case Comparison::CONTAINS:
140140
$parameter->setValue('%' . $parameter->getValue() . '%', $parameter->getType());
141-
$this->parameters[] = $parameter;
142141

143-
return $this->expr->like($field, $placeholder);
142+
return $this->expr->like($field, $this->bindParameter($parameter));
144143

145144
case Comparison::MEMBER_OF:
146145
return $this->expr->isMemberOf($comparison->getField(), $comparison->getValue()->getValue());
147146

148147
case Comparison::STARTS_WITH:
149148
$parameter->setValue($parameter->getValue() . '%', $parameter->getType());
150-
$this->parameters[] = $parameter;
151149

152-
return $this->expr->like($field, $placeholder);
150+
return $this->expr->like($field, $this->bindParameter($parameter));
153151

154152
case Comparison::ENDS_WITH:
155153
$parameter->setValue('%' . $parameter->getValue(), $parameter->getType());
156-
$this->parameters[] = $parameter;
157154

158-
return $this->expr->like($field, $placeholder);
155+
return $this->expr->like($field, $this->bindParameter($parameter));
159156

160157
default:
161158
$operator = self::convertComparisonOperator($comparison->getOperator());
162159
if ($operator) {
163-
$this->parameters[] = $parameter;
164-
165160
return new Expr\Comparison(
166161
$field,
167162
$operator,
168-
$placeholder,
163+
$this->bindParameter($parameter),
169164
);
170165
}
171166

172167
throw new RuntimeException('Unknown comparison operator: ' . $comparison->getOperator());
173168
}
174169
}
175170

171+
/** @return non-empty-string */
172+
private function bindParameter(Parameter $parameter): string
173+
{
174+
$this->parameters[] = $parameter;
175+
176+
return $this->parameterBinder === null
177+
? ':' . $parameter->getName()
178+
: ($this->parameterBinder)($parameter);
179+
}
180+
176181
public function walkValue(Value $value): mixed
177182
{
178183
return $value->getValue();

‎src/QueryBuilder.php‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1255,14 +1255,22 @@ public function addCriteria(Criteria $criteria): static
12551255
throw new Query\QueryException('No aliases are set before invoking addCriteria().');
12561256
}
12571257

1258-
$visitor = new QueryExpressionVisitor($this->getAllAliases());
1258+
$visitor = new QueryExpressionVisitor(
1259+
$this->getAllAliases(),
1260+
function (Parameter $parameter): string {
1261+
if ($this->getParameter($parameter->getName()) === null) {
1262+
$this->parameters->add($parameter);
1263+
1264+
return ':' . $parameter->getName();
1265+
}
1266+
1267+
return $this->createNamedParameter($parameter->getValue(), $parameter->getType());
1268+
},
1269+
);
12591270

12601271
$whereExpression = $criteria->getWhereExpression();
12611272
if ($whereExpression) {
12621273
$this->andWhere($visitor->dispatch($whereExpression));
1263-
foreach ($visitor->getParameters() as $parameter) {
1264-
$this->parameters->add($parameter);
1265-
}
12661274
}
12671275

12681276
foreach ($criteria->orderings() as $sort => $order) {

‎tests/Tests/ORM/Functional/Ticket/GH8702Test.php‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Doctrine\Tests\ORM\Functional\Ticket;
6+
7+
use Doctrine\Common\Collections\Criteria;
8+
use Doctrine\ORM\Mapping\Column;
9+
use Doctrine\ORM\Mapping\Entity;
10+
use Doctrine\ORM\Mapping\Id;
11+
use Doctrine\Tests\OrmFunctionalTestCase;
12+
use PHPUnit\Framework\Attributes\Group;
13+
14+
use function defined;
15+
16+
#[Group('GH8702')]
17+
class GH8702Test extends OrmFunctionalTestCase
18+
{
19+
protected function setUp(): void
20+
{
21+
parent::setUp();
22+
23+
$this->createSchemaForModels(GH8702Item::class);
24+
25+
$this->_em->persist(new GH8702Item(1, 1));
26+
$this->_em->persist(new GH8702Item(2, 2));
27+
$this->_em->persist(new GH8702Item(3, 3));
28+
$this->_em->flush();
29+
$this->_em->clear();
30+
}
31+
32+
public function testAddingMultipleCriteriaOnTheSameField(): void
33+
{
34+
$from = defined(Criteria::class . '::ASC') ? Criteria::create(true) : Criteria::create();
35+
$from->where($from->expr()->gte('value', 2));
36+
37+
$to = defined(Criteria::class . '::ASC') ? Criteria::create(true) : Criteria::create();
38+
$to->where($to->expr()->lte('value', 2));
39+
40+
$items = $this->_em->createQueryBuilder()
41+
->select('item')
42+
->from(GH8702Item::class, 'item')
43+
->addCriteria($from)
44+
->addCriteria($to)
45+
->getQuery()
46+
->getResult();
47+
48+
self::assertCount(1, $items);
49+
self::assertSame(2, $items[0]->id);
50+
}
51+
}
52+
53+
#[Entity]
54+
class GH8702Item
55+
{
56+
public function __construct(
57+
#[Id]
58+
#[Column(type: 'integer')]
59+
public int $id,
60+
#[Column(type: 'integer')]
61+
public int $value,
62+
) {
63+
}
64+
}

‎tests/Tests/ORM/QueryBuilderTest.php‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
use RuntimeException;
3232

3333
use function array_filter;
34+
use function defined;
3435

3536
/**
3637
* Test case for the QueryBuilder class used to build DQL query string in a
@@ -540,6 +541,64 @@ public function testAddMultipleSameCriteriaWhere(): void
540541
self::assertNotNull($qb->getParameter('field_1'));
541542
}
542543

544+
#[Group('GH8702')]
545+
public function testAddMultipleCriteriaWhereWithSameField(): void
546+
{
547+
$qb = $this->entityManager->createQueryBuilder();
548+
$qb->select('alias1')->from(CmsUser::class, 'alias1');
549+
550+
$firstCriteria = defined(Criteria::class . '::ASC') ? Criteria::create(true) : Criteria::create();
551+
$firstCriteria->where($firstCriteria->expr()->gte('field', 'value1'));
552+
553+
$secondCriteria = defined(Criteria::class . '::ASC') ? Criteria::create(true) : Criteria::create();
554+
$secondCriteria->where($secondCriteria->expr()->lte('field', 'value2'));
555+
556+
$qb->addCriteria($firstCriteria);
557+
$qb->addCriteria($secondCriteria);
558+
559+
self::assertEquals('alias1.field >= :field AND alias1.field <= :dcValue1', (string) $qb->getDQLPart('where'));
560+
self::assertSame('value1', $qb->getParameter('field')->getValue());
561+
self::assertSame('value2', $qb->getParameter('dcValue1')->getValue());
562+
}
563+
564+
#[Group('GH8702')]
565+
public function testAddCriteriaDoesNotReplaceExistingParameterWithSameName(): void
566+
{
567+
$qb = $this->entityManager->createQueryBuilder();
568+
$qb->select('alias1')
569+
->from(CmsUser::class, 'alias1')
570+
->where('alias1.id = :field')
571+
->setParameter('field', 42);
572+
573+
$criteria = defined(Criteria::class . '::ASC') ? Criteria::create(true) : Criteria::create();
574+
$criteria->where($criteria->expr()->eq('field', 'value'));
575+
576+
$qb->addCriteria($criteria);
577+
578+
self::assertEquals('alias1.id = :field AND alias1.field = :dcValue1', (string) $qb->getDQLPart('where'));
579+
self::assertSame(42, $qb->getParameter('field')->getValue());
580+
self::assertSame('value', $qb->getParameter('dcValue1')->getValue());
581+
}
582+
583+
#[Group('GH8702')]
584+
public function testAddMultipleNullCriteriaWhereWithSameFieldDoesNotAddParameters(): void
585+
{
586+
$qb = $this->entityManager->createQueryBuilder();
587+
$qb->select('alias1')->from(CmsUser::class, 'alias1');
588+
589+
$firstCriteria = defined(Criteria::class . '::ASC') ? Criteria::create(true) : Criteria::create();
590+
$firstCriteria->where($firstCriteria->expr()->eq('field', null));
591+
592+
$secondCriteria = defined(Criteria::class . '::ASC') ? Criteria::create(true) : Criteria::create();
593+
$secondCriteria->where($secondCriteria->expr()->neq('field', null));
594+
595+
$qb->addCriteria($firstCriteria);
596+
$qb->addCriteria($secondCriteria);
597+
598+
self::assertEquals('alias1.field IS NULL AND alias1.field IS NOT NULL', (string) $qb->getDQLPart('where'));
599+
self::assertCount(0, $qb->getParameters());
600+
}
601+
543602
#[Group('DDC-2844')]
544603
public function testAddCriteriaWhereWithMultipleParametersWithSameField(): void
545604
{

0 commit comments

Comments
 (0)