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

Commit d9379ad

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

File tree

9 files changed

+127
-137
lines changed

9 files changed

+127
-137
lines changed

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

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

1414
use Doctrine\Common\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,33 @@ 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
/**
7664
* {@inheritdoc}
7765
*/
78-
public function loadValuesForChoices(array $choices, callable $value = null)
66+
protected function doLoadChoicesForValues(array $values, ?callable $value): array
7967
{
80-
// Performance optimization
81-
if (empty($choices)) {
82-
return [];
83-
}
84-
85-
// Optimize performance for single-field identifiers. We already
86-
// know that the IDs are used as values
87-
$optimize = $this->idReader && (null === $value || \is_array($value) && $value[0] === $this->idReader);
88-
89-
// Attention: This optimization does not check choices for existence
90-
if ($optimize && !$this->choiceList && $this->idReader->isSingleId()) {
91-
$values = [];
92-
93-
// Maintain order and indices of the given objects
94-
foreach ($choices as $i => $object) {
95-
if ($object instanceof $this->class) {
96-
// Make sure to convert to the right format
97-
$values[$i] = (string) $this->idReader->getIdValue($object);
98-
}
99-
}
100-
101-
return $values;
102-
}
103-
104-
return $this->loadChoiceList($value)->getValuesForChoices($choices);
105-
}
106-
107-
/**
108-
* {@inheritdoc}
109-
*/
110-
public function loadChoicesForValues(array $values, callable $value = null)
111-
{
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 [];
120-
}
121-
12268
// Optimize performance in case we have an object loader and
12369
// a single-field identifier
124-
$optimize = $this->idReader && (null === $value || \is_array($value) && $this->idReader === $value[0]);
70+
$optimize = $this->idReader && (null === $value || (\is_array($value) && $this->idReader === $value[0]));
12571

126-
if ($optimize && !$this->choiceList && $this->objectLoader && $this->idReader->isSingleId()) {
127-
$unorderedObjects = $this->objectLoader->getEntitiesByIds($this->idReader->getIdField(), $values);
72+
if ($optimize && $this->objectLoader) {
12873
$objectsById = [];
12974
$objects = [];
13075

13176
// Maintain order and indices from the given $values
13277
// An alternative approach to the following loop is to add the
13378
// "INDEX BY" clause to the Doctrine query in the loader,
13479
// 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;
80+
foreach ($this->objectLoader->getEntitiesByIds($this->idReader->getIdField(), $values) as $object) {
81+
$objectsById[$this->idReader->getIdValue($object)] = $object;
13782
}
13883

13984
foreach ($values as $i => $id) {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ 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|null The ID value
8888
*/
8989
public function getIdValue(object $object = null)
9090
{
@@ -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/Form/Type/DoctrineType.php

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,6 @@ abstract class DoctrineType extends AbstractType implements ResetInterface
5050
*
5151
* For backwards compatibility, objects are cast to strings by default.
5252
*
53-
*
5453
* @internal This method is public to be usable as callback. It should not
5554
* be used in user code.
5655
*/
@@ -143,8 +142,7 @@ public function configureOptions(OptionsResolver $resolver)
143142
$options['em'],
144143
$options['class'],
145144
$options['id_reader'],
146-
$entityLoader,
147-
false
145+
$entityLoader
148146
);
149147

150148
if (null !== $hash) {

‎src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -94,12 +94,18 @@ protected function setUp(): void
9494
$this->om->expects($this->any())
9595
->method('getRepository')
9696
->with($this->class)
97-
->willReturn($this->repository);
97+
->willReturn($this->repository)
98+
;
9899

99100
$this->om->expects($this->any())
100101
->method('getClassMetadata')
101102
->with($this->class)
102-
->willReturn(new ClassMetadata($this->class));
103+
->willReturn(new ClassMetadata($this->class))
104+
;
105+
$this->repository->expects($this->any())
106+
->method('findAll')
107+
->willReturn([$this->obj1, $this->obj2, $this->obj3])
108+
;
103109
}
104110

105111
public function testLoadChoiceList()
@@ -116,7 +122,8 @@ public function testLoadChoiceList()
116122

117123
$this->repository->expects($this->once())
118124
->method('findAll')
119-
->willReturn($choices);
125+
->willReturn($choices)
126+
;
120127

121128
$this->assertEquals($choiceList, $loader->loadChoiceList($value));
122129

@@ -202,10 +209,10 @@ public function testLoadValuesForChoicesDoesNotLoadIfSingleIntId()
202209
->with($this->obj2)
203210
->willReturn('2');
204211

205-
$this->assertSame(['2'], $loader->loadValuesForChoices([$this->obj2]));
212+
$this->assertSame(['2'], $loader->loadValuesForChoices([$this->obj2], [$this->idReader, 'getIdValue']));
206213
}
207214

208-
public function testLoadValuesForChoicesLoadsIfSingleIntIdAndValueGiven()
215+
public function testLoadValuesForChoicesDoesNotLoadIfSingleIntIdAndValueGiven()
209216
{
210217
$loader = new DoctrineChoiceLoader(
211218
$this->om,
@@ -216,7 +223,7 @@ public function testLoadValuesForChoicesLoadsIfSingleIntIdAndValueGiven()
216223
$choices = [$this->obj1, $this->obj2, $this->obj3];
217224
$value = function (\stdClass $object) { return $object->name; };
218225

219-
$this->repository->expects($this->once())
226+
$this->repository->expects($this->never())
220227
->method('findAll')
221228
->willReturn($choices);
222229

@@ -285,7 +292,7 @@ public function testLoadChoicesForValuesDoesNotLoadIfEmptyValues()
285292
$this->assertSame([], $loader->loadChoicesForValues([]));
286293
}
287294

288-
public function testLoadChoicesForValuesLoadsOnlyChoicesIfSingleIntId()
295+
public function testLoadChoicesForValuesLoadsOnlyChoicesIfValueUseIdReader()
289296
{
290297
$loader = new DoctrineChoiceLoader(
291298
$this->om,
@@ -317,8 +324,8 @@ public function testLoadChoicesForValuesLoadsOnlyChoicesIfSingleIntId()
317324

318325
$this->assertSame(
319326
[4 => $this->obj3, 7 => $this->obj2],
320-
$loader->loadChoicesForValues([4 => '3', 7 => '2']
321-
));
327+
$loader->loadChoicesForValues([4 => '3', 7 => '2'])
328+
);
322329
}
323330

324331
public function testLoadChoicesForValuesLoadsAllIfSingleIntIdAndValueGiven()

‎src/Symfony/Bridge/Doctrine/composer.json

Lines changed: 1 addition & 1 deletion
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",

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,11 @@
11
CHANGELOG
22
=========
33

4+
5.1.0
5+
-----
6+
7+
* added an `AbstractChoiceLoader` to simplify implementations and handle global optimizations
8+
49
5.0.0
510
-----
611

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

Lines 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+
* {@inheritdoc}
30+
*/
31+
public function loadChoiceList(callable $value = null)
32+
{
33+
return $this->choiceList ?? ($this->choiceList = new ArrayChoiceList($this->loadChoices(), $value));
34+
}
35+
36+
/**
37+
* {@inheritdoc}
38+
*/
39+
public function loadChoicesForValues(array $values, callable $value = null)
40+
{
41+
// Optimize
42+
if (!$values) {
43+
return [];
44+
}
45+
46+
if ($this->choiceList) {
47+
return $this->choiceList->getChoicesForValues($values);
48+
}
49+
50+
return $this->doLoadChoicesForValues($values, $value);
51+
}
52+
53+
/**
54+
* {@inheritdoc}
55+
*/
56+
public function loadValuesForChoices(array $choices, callable $value = null)
57+
{
58+
// Optimize
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