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 1394df2

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

File tree

8 files changed

+174
-121
lines changed
Filter options

8 files changed

+174
-121
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
+21-52Lines 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 <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,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
}

‎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 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(

‎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
+86Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
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+
if (!$values) {
44+
return [];
45+
}
46+
47+
if ($this->choiceList) {
48+
return $this->choiceList->getChoicesForValues($values);
49+
}
50+
51+
return $this->doLoadChoicesForValues($values, $value);
52+
}
53+
54+
/**
55+
* {@inheritdoc}
56+
*/
57+
public function loadValuesForChoices(array $choices, callable $value = null)
58+
{
59+
if (!$choices) {
60+
return [];
61+
}
62+
63+
if ($value) {
64+
// if a value callback exists, use it
65+
return array_map($value, $choices);
66+
}
67+
68+
if ($this->choiceList) {
69+
return $this->choiceList->getValuesForChoices($choices);
70+
}
71+
72+
return $this->doLoadValuesForChoices($choices);
73+
}
74+
75+
abstract protected function loadChoices(): iterable;
76+
77+
protected function doLoadChoicesForValues(array $values, ?callable $value): array
78+
{
79+
return $this->loadChoiceList($value)->getChoicesForValues($values);
80+
}
81+
82+
protected function doLoadValuesForChoices(array $choices): array
83+
{
84+
return $this->loadChoiceList()->getValuesForChoices($choices);
85+
}
86+
}

0 commit comments

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