Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit d10b2d2

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

File tree

8 files changed

+180
-118
lines changed
Filter options

8 files changed

+180
-118
lines changed

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

Copy file name to clipboardExpand all lines: src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php
+25-50Lines 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-
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

Copy file name to clipboardExpand all lines: src/Symfony/Bridge/Doctrine/Form/ChoiceList/IdReader.php
+3-3Lines 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

Copy file name to clipboardExpand all lines: src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php
+54-5Lines 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

Copy file name to clipboardExpand all lines: src/Symfony/Bridge/Doctrine/composer.json
+2-2Lines 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

Copy file name to clipboardExpand all lines: src/Symfony/Component/Form/CHANGELOG.md
+1Lines 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

Copy file name to clipboard
+88Lines 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+
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);
87+
}
88+
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.