Skip to content

Commit f1692c4

Browse files
HeahDudeHeahDude
committed
[Form] Added an AbstractChoiceLoader to simplify implementations and handle global optimizations
1 parent d9c124b commit f1692c4

File tree

4 files changed

+80
-62
lines changed

4 files changed

+80
-62
lines changed

‎Form/ChoiceList/DoctrineChoiceLoader.php‎

Lines changed: 21 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -12,27 +12,20 @@
1212
namespace Symfony\Bridge\Doctrine\Form\ChoiceList;
1313

1414
use Doctrine\Persistence\ObjectManager;
15-
use Symfony\Component\Form\ChoiceList\ArrayChoiceList;
16-
use Symfony\Component\Form\ChoiceList\ChoiceListInterface;
17-
use Symfony\Component\Form\ChoiceList\Loader\ChoiceLoaderInterface;
15+
use Symfony\Component\Form\ChoiceList\Loader\AbstractChoiceLoader;
1816

1917
/**
2018
* Loads choices using a Doctrine object manager.
2119
*
2220
* @author Bernhard Schussek <[email protected]>
2321
*/
24-
class DoctrineChoiceLoader implements ChoiceLoaderInterface
22+
class DoctrineChoiceLoader extends AbstractChoiceLoader
2523
{
2624
private $manager;
2725
private $class;
2826
private $idReader;
2927
private $objectLoader;
3028

31-
/**
32-
* @var ChoiceListInterface
33-
*/
34-
private $choiceList;
35-
3629
/**
3730
* Creates a new choice loader.
3831
*
@@ -59,81 +52,57 @@ public function __construct(ObjectManager $manager, string $class, IdReader $idR
5952
/**
6053
* {@inheritdoc}
6154
*/
62-
public function loadChoiceList(callable $value = null)
55+
protected function loadChoices(): iterable
6356
{
64-
if ($this->choiceList) {
65-
return $this->choiceList;
66-
}
67-
68-
$objects = $this->objectLoader
57+
return $this->objectLoader
6958
? $this->objectLoader->getEntities()
7059
: $this->manager->getRepository($this->class)->findAll();
71-
72-
return $this->choiceList = new ArrayChoiceList($objects, $value);
7360
}
7461

7562
/**
76-
* {@inheritdoc}
63+
* @internal to be remove in Symfony 6
7764
*/
78-
public function loadValuesForChoices(array $choices, callable $value = null)
65+
protected function doLoadValuesForChoices(array $choices): array
7966
{
80-
// Performance optimization
81-
if (empty($choices)) {
82-
return [];
83-
}
84-
8567
// Optimize performance for single-field identifiers. We already
8668
// know that the IDs are used as values
87-
$optimize = $this->idReader && (null === $value || \is_array($value) && $value[0] === $this->idReader);
88-
8969
// Attention: This optimization does not check choices for existence
90-
if ($optimize && !$this->choiceList) {
91-
$values = [];
92-
70+
if ($this->idReader) {
71+
trigger_deprecation('symfony/doctrine-bridge', '5.1', 'Not defining explicitly the IdReader as value callback when query can be optimized is deprecated. Don\'t pass the IdReader to "%s" or define the "choice_value" option instead.', __CLASS__);
9372
// Maintain order and indices of the given objects
73+
$values = [];
9474
foreach ($choices as $i => $object) {
9575
if ($object instanceof $this->class) {
96-
// Make sure to convert to the right format
97-
$values[$i] = (string) $this->idReader->getIdValue($object);
76+
$values[$i] = $this->idReader->getIdValue($object);
9877
}
9978
}
10079

10180
return $values;
10281
}
10382

104-
return $this->loadChoiceList($value)->getValuesForChoices($choices);
83+
return parent::doLoadValuesForChoices($choices);
10584
}
10685

107-
/**
108-
* {@inheritdoc}
109-
*/
110-
public function loadChoicesForValues(array $values, callable $value = null)
86+
protected function doLoadChoicesForValues(array $values, ?callable $value): array
11187
{
112-
// Performance optimization
113-
// Also prevents the generation of "WHERE id IN ()" queries through the
114-
// object loader. At least with MySQL and on the development machine
115-
// this was tested on, no exception was thrown for such invalid
116-
// statements, consequently no test fails when this code is removed.
117-
// https://github.com/symfony/symfony/pull/8981#issuecomment-24230557
118-
if (empty($values)) {
119-
return [];
88+
$legacy = $this->idReader && null === $value;
89+
90+
if ($legacy) {
91+
trigger_deprecation('symfony/doctrine-bridge', '5.1', 'Not defining explicitly the IdReader as value callback when query can be optimized is deprecated. Don\'t pass the IdReader to "%s" or define the "choice_value" option instead.', __CLASS__);
12092
}
12193

12294
// Optimize performance in case we have an object loader and
12395
// a single-field identifier
124-
$optimize = $this->idReader && (null === $value || \is_array($value) && $this->idReader === $value[0]);
125-
126-
if ($optimize && !$this->choiceList && $this->objectLoader) {
127-
$unorderedObjects = $this->objectLoader->getEntitiesByIds($this->idReader->getIdField(), $values);
128-
$objectsById = [];
96+
if (($legacy || \is_array($value) && $this->idReader === $value[0]) && $this->objectLoader) {
12997
$objects = [];
98+
$objectsById = [];
13099

131100
// Maintain order and indices from the given $values
132101
// An alternative approach to the following loop is to add the
133102
// "INDEX BY" clause to the Doctrine query in the loader,
134103
// but I'm not sure whether that's doable in a generic fashion.
135-
foreach ($unorderedObjects as $object) {
136-
$objectsById[(string) $this->idReader->getIdValue($object)] = $object;
104+
foreach ($this->objectLoader->getEntitiesByIds($this->idReader->getIdField(), $values) as $object) {
105+
$objectsById[$this->idReader->getIdValue($object)] = $object;
137106
}
138107

139108
foreach ($values as $i => $id) {
@@ -145,6 +114,6 @@ public function loadChoicesForValues(array $values, callable $value = null)
145114
return $objects;
146115
}
147116

148-
return $this->loadChoiceList($value)->getChoicesForValues($values);
117+
return parent::doLoadChoicesForValues($values, $value);
149118
}
150119
}

‎Form/ChoiceList/IdReader.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -84,12 +84,12 @@ public function isIntId(): bool
8484
*
8585
* This method assumes that the object has a single-column ID.
8686
*
87-
* @return mixed The ID value
87+
* @return string The ID value
8888
*/
8989
public function getIdValue(object $object = null)
9090
{
9191
if (!$object) {
92-
return null;
92+
return '';
9393
}
9494

9595
if (!$this->om->contains($object)) {
@@ -104,7 +104,7 @@ public function getIdValue(object $object = null)
104104
$idValue = $this->associationIdReader->getIdValue($idValue);
105105
}
106106

107-
return $idValue;
107+
return (string) $idValue;
108108
}
109109

110110
/**

‎Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php‎

Lines changed: 54 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,10 @@ protected function setUp(): void
100100
->method('getClassMetadata')
101101
->with($this->class)
102102
->willReturn(new ClassMetadata($this->class));
103+
$this->repository->expects($this->any())
104+
->method('findAll')
105+
->willReturn([$this->obj1, $this->obj2, $this->obj3])
106+
;
103107
}
104108

105109
public function testLoadChoiceList()
@@ -186,6 +190,11 @@ public function testLoadValuesForChoicesDoesNotLoadIfEmptyChoices()
186190
$this->assertSame([], $loader->loadValuesForChoices([]));
187191
}
188192

193+
/**
194+
* @group legacy
195+
*
196+
* @expectedDeprecation Since symfony/doctrine-bridge 5.1: Not defining explicitly the IdReader as value callback when query can be optimized is deprecated. Don't pass the IdReader to "Symfony\Bridge\Doctrine\Form\ChoiceList\DoctrineChoiceLoader" or define the "choice_value" option instead.
197+
*/
189198
public function testLoadValuesForChoicesDoesNotLoadIfSingleIntId()
190199
{
191200
$loader = new DoctrineChoiceLoader(
@@ -205,7 +214,7 @@ public function testLoadValuesForChoicesDoesNotLoadIfSingleIntId()
205214
$this->assertSame(['2'], $loader->loadValuesForChoices([$this->obj2]));
206215
}
207216

208-
public function testLoadValuesForChoicesLoadsIfSingleIntIdAndValueGiven()
217+
public function testLoadValuesForChoicesDoesNotLoadIfSingleIntIdAndValueGiven()
209218
{
210219
$loader = new DoctrineChoiceLoader(
211220
$this->om,
@@ -216,7 +225,7 @@ public function testLoadValuesForChoicesLoadsIfSingleIntIdAndValueGiven()
216225
$choices = [$this->obj1, $this->obj2, $this->obj3];
217226
$value = function (\stdClass $object) { return $object->name; };
218227

219-
$this->repository->expects($this->once())
228+
$this->repository->expects($this->never())
220229
->method('findAll')
221230
->willReturn($choices);
222231

@@ -254,8 +263,7 @@ public function testLoadChoicesForValues()
254263
{
255264
$loader = new DoctrineChoiceLoader(
256265
$this->om,
257-
$this->class,
258-
$this->idReader
266+
$this->class
259267
);
260268

261269
$choices = [$this->obj1, $this->obj2, $this->obj3];
@@ -285,7 +293,12 @@ public function testLoadChoicesForValuesDoesNotLoadIfEmptyValues()
285293
$this->assertSame([], $loader->loadChoicesForValues([]));
286294
}
287295

288-
public function testLoadChoicesForValuesLoadsOnlyChoicesIfSingleIntId()
296+
/**
297+
* @group legacy
298+
*
299+
* @expectedDeprecation Not defining explicitly the IdReader as value callback when query can be optimized has been deprecated in 5.1. Don't pass the IdReader to "Symfony\Bridge\Doctrine\Form\ChoiceList\DoctrineChoiceLoader" or define the choice_value instead.
300+
*/
301+
public function legacyTestLoadChoicesForValuesLoadsOnlyChoicesIfValueUseIdReader()
289302
{
290303
$loader = new DoctrineChoiceLoader(
291304
$this->om,
@@ -321,6 +334,42 @@ public function testLoadChoicesForValuesLoadsOnlyChoicesIfSingleIntId()
321334
));
322335
}
323336

337+
public function testLoadChoicesForValuesLoadsOnlyChoicesIfValueUseIdReader()
338+
{
339+
$loader = new DoctrineChoiceLoader(
340+
$this->om,
341+
$this->class,
342+
$this->idReader,
343+
$this->objectLoader
344+
);
345+
346+
$choices = [$this->obj2, $this->obj3];
347+
348+
$this->idReader->expects($this->any())
349+
->method('getIdField')
350+
->willReturn('idField');
351+
352+
$this->repository->expects($this->never())
353+
->method('findAll');
354+
355+
$this->objectLoader->expects($this->once())
356+
->method('getEntitiesByIds')
357+
->with('idField', [4 => '3', 7 => '2'])
358+
->willReturn($choices);
359+
360+
$this->idReader->expects($this->any())
361+
->method('getIdValue')
362+
->willReturnMap([
363+
[$this->obj2, '2'],
364+
[$this->obj3, '3'],
365+
]);
366+
367+
$this->assertSame(
368+
[4 => $this->obj3, 7 => $this->obj2],
369+
$loader->loadChoicesForValues([4 => '3', 7 => '2'], [$this->idReader, 'getIdValue']
370+
));
371+
}
372+
324373
public function testLoadChoicesForValuesLoadsAllIfSingleIntIdAndValueGiven()
325374
{
326375
$loader = new DoctrineChoiceLoader(

‎composer.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
"symfony/stopwatch": "^4.4|^5.0",
2828
"symfony/config": "^4.4|^5.0",
2929
"symfony/dependency-injection": "^4.4|^5.0",
30-
"symfony/form": "^5.0",
30+
"symfony/form": "^5.1",
3131
"symfony/http-kernel": "^5.0",
3232
"symfony/messenger": "^4.4|^5.0",
3333
"symfony/property-access": "^4.4|^5.0",
@@ -49,7 +49,7 @@
4949
"conflict": {
5050
"phpunit/phpunit": "<5.4.3",
5151
"symfony/dependency-injection": "<4.4",
52-
"symfony/form": "<5",
52+
"symfony/form": "<5.1",
5353
"symfony/http-kernel": "<5",
5454
"symfony/messenger": "<4.4",
5555
"symfony/property-info": "<5",

0 commit comments

Comments
 (0)