Skip to content

Commit 806d08a

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 03783f8 commit 806d08a

4 files changed

Lines changed: 164 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
@@ -1275,14 +1275,22 @@ public function addCriteria(Criteria $criteria): static
12751275
throw new Query\QueryException('No aliases are set before invoking addCriteria().');
12761276
}
12771277

1278-
$visitor = new QueryExpressionVisitor($this->getAllAliases());
1278+
$visitor = new QueryExpressionVisitor(
1279+
$this->getAllAliases(),
1280+
function (Parameter $parameter): string {
1281+
if ($this->getParameter($parameter->getName()) === null) {
1282+
$this->parameters->add($parameter);
1283+
1284+
return ':' . $parameter->getName();
1285+
}
1286+
1287+
return $this->createNamedParameter($parameter->getValue(), $parameter->getType());
1288+
},
1289+
);
12791290

12801291
$whereExpression = $criteria->getWhereExpression();
12811292
if ($whereExpression) {
12821293
$this->andWhere($visitor->dispatch($whereExpression));
1283-
foreach ($visitor->getParameters() as $parameter) {
1284-
$this->parameters->add($parameter);
1285-
}
12861294
}
12871295

12881296
$orderings = $this->getOrderings($criteria);

‎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: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -594,6 +594,64 @@ public function testAddMultipleSameCriteriaWhere(): void
594594
self::assertNotNull($qb->getParameter('field_1'));
595595
}
596596

597+
#[Group('GH8702')]
598+
public function testAddMultipleCriteriaWhereWithSameField(): void
599+
{
600+
$qb = $this->entityManager->createQueryBuilder();
601+
$qb->select('alias1')->from(CmsUser::class, 'alias1');
602+
603+
$firstCriteria = defined(Criteria::class . '::ASC') ? Criteria::create(true) : Criteria::create();
604+
$firstCriteria->where($firstCriteria->expr()->gte('field', 'value1'));
605+
606+
$secondCriteria = defined(Criteria::class . '::ASC') ? Criteria::create(true) : Criteria::create();
607+
$secondCriteria->where($secondCriteria->expr()->lte('field', 'value2'));
608+
609+
$qb->addCriteria($firstCriteria);
610+
$qb->addCriteria($secondCriteria);
611+
612+
self::assertEquals('alias1.field >= :field AND alias1.field <= :dcValue1', (string) $qb->getDQLPart('where'));
613+
self::assertSame('value1', $qb->getParameter('field')->getValue());
614+
self::assertSame('value2', $qb->getParameter('dcValue1')->getValue());
615+
}
616+
617+
#[Group('GH8702')]
618+
public function testAddCriteriaDoesNotReplaceExistingParameterWithSameName(): void
619+
{
620+
$qb = $this->entityManager->createQueryBuilder();
621+
$qb->select('alias1')
622+
->from(CmsUser::class, 'alias1')
623+
->where('alias1.id = :field')
624+
->setParameter('field', 42);
625+
626+
$criteria = defined(Criteria::class . '::ASC') ? Criteria::create(true) : Criteria::create();
627+
$criteria->where($criteria->expr()->eq('field', 'value'));
628+
629+
$qb->addCriteria($criteria);
630+
631+
self::assertEquals('alias1.id = :field AND alias1.field = :dcValue1', (string) $qb->getDQLPart('where'));
632+
self::assertSame(42, $qb->getParameter('field')->getValue());
633+
self::assertSame('value', $qb->getParameter('dcValue1')->getValue());
634+
}
635+
636+
#[Group('GH8702')]
637+
public function testAddMultipleNullCriteriaWhereWithSameFieldDoesNotAddParameters(): void
638+
{
639+
$qb = $this->entityManager->createQueryBuilder();
640+
$qb->select('alias1')->from(CmsUser::class, 'alias1');
641+
642+
$firstCriteria = defined(Criteria::class . '::ASC') ? Criteria::create(true) : Criteria::create();
643+
$firstCriteria->where($firstCriteria->expr()->eq('field', null));
644+
645+
$secondCriteria = defined(Criteria::class . '::ASC') ? Criteria::create(true) : Criteria::create();
646+
$secondCriteria->where($secondCriteria->expr()->neq('field', null));
647+
648+
$qb->addCriteria($firstCriteria);
649+
$qb->addCriteria($secondCriteria);
650+
651+
self::assertEquals('alias1.field IS NULL AND alias1.field IS NOT NULL', (string) $qb->getDQLPart('where'));
652+
self::assertCount(0, $qb->getParameters());
653+
}
654+
597655
#[Group('DDC-2844')]
598656
public function testAddCriteriaWhereWithMultipleParametersWithSameField(): void
599657
{

0 commit comments

Comments
 (0)