8000 [Form] Added an AbstractChoiceLoader to simplify implementations and … · symfony/symfony@d10b2d2 · GitHub
[go: up one dir, main page]
Skip to content

Commit d10b2d2

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

File tree

8 files changed

+180
-118
lines changed

8 files changed

+180
-118
lines changed

‎src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php

Lines changed: 25 additions & 50 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 <bschussek@gmail.com>
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,63 @@ 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()
70-
: $this->manager->getRepository($this->class)->findAll();
71-
72-
return $this->choiceList = new ArrayChoiceList($objects, $value);
59+
: $this->manager->getRepository($this->class)->findAll()
60+
;
7361
}
7462

7563
/**
76-
* {@inheritdoc}
64+
* This method is here for legacy.
65+
*
66+
* To remove in 6.0
7767
*/
78-
public function loadValuesForChoices(array $choices, callable $value = null)
68+
protected function doLoadValuesForChoices(array $choices): array
7969
{
80-
// Performance optimization
81-
if (empty($choices)) {
82-
return [];
83-
}
84-
8570
// Optimize performance for single-field identifiers. We already
8671
// know that the IDs are used as values
87-
$optimize = $this->idReader && (null === $value || \is_array($value) && $value[0] === $this->idReader);
88-
8972
// Attention: This optimization does not check choices for existence
90-
if ($optimize && !$this->choiceList) {
91-
$values = [];
92-
10000 73+
if ($this->idReader) {
74+
@trigger_error(sprintf('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 "%s" or define the "choice_value" option instead.', __CLASS__), E_USER_DEPRECATED);
9375
// Maintain order and indices of the given objects
76+
$values = [];
9477
foreach ($choices as $i => $object) {
9578
if ($object instanceof $this->class) {
96-
// Make sure to convert to the right format
97-
$values[$i] = (string) $this->idReader->getIdValue($object);
79+
$values[$i] = $this->idReader->getIdValue($object);
9880
}
9981
}
10082

10183
return $values;
10284
}
10385

104-
return $this->loadChoiceList($value)->getValuesForChoices($choices);
86+
return parent::doLoadValuesForChoices($choices);
10587
}
10688

10789
/**
10890
* {@inheritdoc}
10991
*/
110-
public function loadChoicesForValues(array $values, callable $value = null)
92+
protected function doLoadChoicesForValues(array $values, ?callable $value): array
11193
{
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 [];
94+
$legacy = $this->idReader && null === $value;
95+
96+
if ($legacy) {
97+
@trigger_error(sprintf('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 "%s" or define the choice_value instead.', __CLASS__), E_USER_DEPRECATED);
12098
}
12199

122100
// Optimize performance in case we have an object loader and
123101
// 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 = [];
102+
if (($legacy || \is_array($value) && $this->idReader === $value[0]) && $this->objectLoader) {
129103
$objects = [];
104+
$objectsById = [];
130105

131106
// Maintain order and indices from the given $values
132107
// An alternative approach to the following loop is to add the
133108
// "INDEX BY" clause to the Doctrine query in the loader,
134109
// 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;
110+
foreach ($this->objectLoader->getEntitiesByIds($this->idReader->getIdField(), $values) as $object) {
111+
$objectsById[$this->idReader->getIdValue($object)] = $object;
137112
}
138113

139114
foreach ($values as $i => $id) {
@@ -145,6 +120,6 @@ public function loadChoicesForValues(array $values, callable $value = null)
145120
return $objects;
146121
}
147122

148-
return $this->loadChoiceList($value)->getChoicesForValues($values);
123+
return parent::doLoadChoicesForValues($values, $value);
149124
}
150125
}

‎src/Symfony/Bridge/Doctrine/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
/**

‎src/Symfony/Bridge/Doctrine/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 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" 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(

‎src/Symfony/Bridge/Doctrine/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",

‎src/Symfony/Component/Form/CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ CHANGELOG
44
5.1.0
55
-----
66

7+
* Added an `AbstractChoiceLoader` to simplify implementations and handle global optimizations
78
* The `view_timezone` option defaults to the `model_timezone` if no `reference_date` is configured.
89
* Added default `inputmode` attribute to Search, Email and Tel form types.
910
* Implementing the `FormConfigInterface` without implementing the `getIsEmptyCallback()` method

‎src/Symfony/Component/Form/ChoiceList/Loader/AbstractChoiceLoader.php

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Component\Form\ChoiceList\Loader;
13+
14+
use Symfony\Component\Form\ChoiceList\ArrayChoiceList;
15+
16+
/**
17+
* @author Jules Pietri <jules@heahprod.com>
18+
*/
19+
abstract class AbstractChoiceLoader implements ChoiceLoaderInterface
20+
{
21+
/**
22+
* The loaded choice list.
23+
*
24+
* @var ArrayChoiceList
25+
*/
26+
private $choiceList;
27+
28+
/**
29+
* @final
30+
*
31+
* {@inheritdoc}
32+
*/
33+
public function loadChoiceList(callable $value = null)
34+
{
35+
return $this->choiceList ?? ($this->choiceList = new ArrayChoiceList($this->loadChoices(), $value));
36+
}
37+
38+
/**
39+
* {@inheritdoc}
40+
*/
41+
public function loadChoicesForValues(array $values, callable $value = null)
42+
{
43+
// Optimize
44+
if (!$values) {
45+
return [];
46+
}
47+
48+
< 17AE span class=pl-k>if ($this->choiceList) {
49+
return $this->choiceList->getChoicesForValues($values);
50+
}
51+
52+
return $this->doLoadChoicesForValues($values, $value);
53+
}
54+
55+
/**
56+
* {@inheritdoc}
57+
*/
58+
public function loadValuesForChoices(array $choices, callable $value = null)
59+
{
60+
// Optimize
61+
if (!$choices) {
62+
return [];
63+
}
64+
65+
if ($value) {
66+
// if a value callback exists, use it
67+
return array_map($value, $choices);
68+
}
69+
70+
if ($this->choiceList) {
71+
return $this->choiceList->getValuesForChoices($choices);
72+
}
73+
74+
return $this->doLoadValuesForChoices($choices);
75+
}
76+
77+
abstract protected function loadChoices(): iterable;
78+
79+
protected function doLoadChoicesForValues(array $values, ?callable $value): array
80+
{
81+
return $this->loadChoiceList($value)->getChoicesForValues($values);
82+
}
83+
84+
protected function doLoadValuesForChoices(array $choices): array
85+
{
86+
return $this->loadChoiceList()->getValuesForChoices($choices);
< 415B /code>
87+
}
88+
}

0 commit comments

Comments
 (0)
0