Skip to content

Commit c6a2331

Browse files
xabbuhxabbuh
committed
bug #46098 [Form] Fix same choice loader with different choice values (HeahDude)
This PR was squashed before being merged into the 4.4 branch. Discussion ---------- [Form] Fix same choice loader with different choice values | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | #44655 | License | MIT | Doc PR | ~ It appears that deprecating the caching in the `LazyChoiceList` (cf #18359) was a mistake. The bug went under the radar because in practice every choice field has its own loader instance. However, optimizations made in #30994 then revealed the flaw (cf #42206) as the loaders were actually shared across many fields. While working on a fix I ended up implementing something similar to what's proposed in #44655. I'll send a PR for 5.4 as well. Commits ------- 65cbf18 [Form] Fix same choice loader with different choice values
2 parents 1a7fa5d + 65cbf18 commit c6a2331

File tree

9 files changed

+100
-32
lines changed

9 files changed

+100
-32
lines changed

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

+9-12
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313

1414
use Doctrine\Persistence\ObjectManager;
1515
use Symfony\Component\Form\ChoiceList\ArrayChoiceList;
16-
use Symfony\Component\Form\ChoiceList\ChoiceListInterface;
1716
use Symfony\Component\Form\ChoiceList\Loader\ChoiceLoaderInterface;
1817

1918
/**
@@ -29,9 +28,9 @@ class DoctrineChoiceLoader implements ChoiceLoaderInterface
2928
private $objectLoader;
3029

3130
/**
32-
* @var ChoiceListInterface
31+
* @var array|null
3332
*/
34-
private $choiceList;
33+
private $choices;
3534

3635
/**
3736
* Creates a new choice loader.
@@ -74,15 +73,13 @@ public function __construct(ObjectManager $manager, string $class, IdReader $idR
7473
*/
7574
public function loadChoiceList($value = null)
7675
{
77-
if ($this->choiceList) {
78-
return $this->choiceList;
76+
if (null === $this->choices) {
77+
$this->choices = $this->objectLoader
78+
? $this->objectLoader->getEntities()
79+
: $this->manager->getRepository($this->class)->findAll();
7980
}
8081

81-
$objects = $this->objectLoader
82-
? $this->objectLoader->getEntities()
83-
: $this->manager->getRepository($this->class)->findAll();
84-
85-
return $this->choiceList = new ArrayChoiceList($objects, $value);
82+
return new ArrayChoiceList($this->choices, $value);
8683
}
8784

8885
/**
@@ -100,7 +97,7 @@ public function loadValuesForChoices(array $choices, $value = null)
10097
$optimize = $this->idReader && (null === $value || \is_array($value) && $value[0] === $this->idReader);
10198

10299
// Attention: This optimization does not check choices for existence
103-
if ($optimize && !$this->choiceList && $this->idReader->isSingleId()) {
100+
if ($optimize && !$this->choices && $this->idReader->isSingleId()) {
104101
$values = [];
105102

106103
// Maintain order and indices of the given objects
@@ -136,7 +133,7 @@ public function loadChoicesForValues(array $values, $value = null)
136133
// a single-field identifier
137134
$optimize = $this->idReader && (null === $value || \is_array($value) && $this->idReader === $value[0]);
138135

139-
if ($optimize && !$this->choiceList && $this->objectLoader && $this->idReader->isSingleId()) {
136+
if ($optimize && !$this->choices && $this->objectLoader && $this->idReader->isSingleId()) {
140137
$unorderedObjects = $this->objectLoader->getEntitiesByIds($this->idReader->getIdField(), $values);
141138
$objectsById = [];
142139
$objects = [];

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

+1-2
Original file line numberDiff line numberDiff line change
@@ -146,8 +146,7 @@ public function testLoadChoiceListUsesObjectLoaderIfAvailable()
146146
$this->assertEquals($choiceList, $loaded = $loader->loadChoiceList());
147147

148148
// no further loads on subsequent calls
149-
150-
$this->assertSame($loaded, $loader->loadChoiceList());
149+
$this->assertEquals($loaded, $loader->loadChoiceList());
151150
}
152151

153152
public function testLoadValuesForChoices()

‎src/Symfony/Bridge/Doctrine/Tests/Form/Type/EntityTypeTest.php

+28
Original file line numberDiff line numberDiff line change
@@ -1779,4 +1779,32 @@ public function testSubmitNullMultipleUsesDefaultEmptyData()
17791779
$this->assertEquals($collection, $form->getNormData());
17801780
$this->assertEquals($collection, $form->getData());
17811781
}
1782+
1783+
public function testWithSameLoaderAndDifferentChoiceValueCallbacks()
1784+
{
1785+
$entity1 = new SingleIntIdEntity(1, 'Foo');
1786+
$entity2 = new SingleIntIdEntity(2, 'Bar');
1787+
$this->persist([$entity1, $entity2]);
1788+
1789+
$view = $this->factory->create(FormTypeTest::TESTED_TYPE)
1790+
->add('entity_one', self::TESTED_TYPE, [
1791+
'em' => 'default',
1792+
'class' => self::SINGLE_IDENT_CLASS,
1793+
])
1794+
->add('entity_two', self::TESTED_TYPE, [
1795+
'em' => 'default',
1796+
'class' => self::SINGLE_IDENT_CLASS,
1797+
'choice_value' => function ($choice) {
1798+
return $choice ? $choice->name : '';
1799+
},
1800+
])
1801+
->createView()
1802+
;
1803+
1804+
$this->assertSame('1', $view['entity_one']->vars['choices'][1]->value);
1805+
$this->assertSame('2', $view['entity_one']->vars['choices'][2]->value);
1806+
1807+
$this->assertSame('Foo', $view['entity_two']->vars['choices']['Foo']->value);
1808+
$this->assertSame('Bar', $view['entity_two']->vars['choices']['Bar']->value);
1809+
}
17821810
}

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
"symfony/stopwatch": "^3.4|^4.0|^5.0",
3030
"symfony/config": "^4.2|^5.0",
3131
"symfony/dependency-injection": "^3.4|^4.0|^5.0",
32-
"symfony/form": "^4.4.11|^5.0.11",
32+
"symfony/form": "^4.4.41|^5.0.11",
3333
"symfony/http-kernel": "^4.3.7",
3434
"symfony/messenger": "^4.4|^5.0",
3535
"symfony/property-access": "^3.4|^4.0|^5.0",

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

+6-6
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,11 @@ class CallbackChoiceLoader implements ChoiceLoaderInterface
2323
private $callback;
2424

2525
/**
26-
* The loaded choice list.
26+
* The loaded choices.
2727
*
28-
* @var ArrayChoiceList
28+
* @var array|null
2929
*/
30-
private $choiceList;
30+
private $choices;
3131

3232
/**
3333
* @param callable $callback The callable returning an array of choices
@@ -42,11 +42,11 @@ public function __construct(callable $callback)
4242
*/
4343
public function loadChoiceList($value = null)
4444
{
45-
if (null !== $this->choiceList) {
46-
return $this->choiceList;
45+
if (null === $this->choices) {
46+
$this->choices = ($this->callback)();
4747
}
4848

49-
return $this->choiceList = new ArrayChoiceList(($this->callback)(), $value);
49+
return new ArrayChoiceList($this->choices, $value);
5050
}
5151

5252
/**

‎src/Symfony/Component/Form/Tests/ChoiceList/LazyChoiceListTest.php

+5-5
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ public function testGetChoiceLoadersLoadsLoadedListOnFirstCall()
3232

3333
$this->assertSame(['RESULT'], $list->getChoices());
3434
$this->assertSame(['RESULT'], $list->getChoices());
35-
$this->assertSame(1, $calls);
35+
$this->assertSame(2, $calls);
3636
}
3737

3838
public function testGetValuesLoadsLoadedListOnFirstCall()
@@ -46,7 +46,7 @@ public function testGetValuesLoadsLoadedListOnFirstCall()
4646

4747
$this->assertSame(['RESULT'], $list->getValues());
4848
$this->assertSame(['RESULT'], $list->getValues());
49-
$this->assertSame(1, $calls);
49+
$this->assertSame(2, $calls);
5050
}
5151

5252
public function testGetStructuredValuesLoadsLoadedListOnFirstCall()
@@ -60,7 +60,7 @@ public function testGetStructuredValuesLoadsLoadedListOnFirstCall()
6060

6161
$this->assertSame(['RESULT'], $list->getStructuredValues());
6262
$this->assertSame(['RESULT'], $list->getStructuredValues());
63-
$this->assertSame(1, $calls);
63+
$this->assertSame(2, $calls);
6464
}
6565

6666
public function testGetOriginalKeysLoadsLoadedListOnFirstCall()
@@ -79,7 +79,7 @@ public function testGetOriginalKeysLoadsLoadedListOnFirstCall()
7979

8080
$this->assertSame(['foo' => 'a', 'bar' => 'b', 'baz' => 'c'], $list->getOriginalKeys());
8181
$this->assertSame(['foo' => 'a', 'bar' => 'b', 'baz' => 'c'], $list->getOriginalKeys());
82-
$this->assertSame(3, $calls);
82+
$this->assertSame(6, $calls);
8383
}
8484

8585
public function testGetChoicesForValuesForwardsCallIfListNotLoaded()
@@ -98,7 +98,7 @@ public function testGetChoicesForValuesForwardsCallIfListNotLoaded()
9898

9999
$this->assertSame(['foo', 'bar'], $list->getChoicesForValues(['a', 'b']));
100100
$this->assertSame(['foo', 'bar'], $list->getChoicesForValues(['a', 'b']));
101-
$this->assertSame(3, $calls);
101+
$this->assertSame(6, $calls);
102102
}
103103

104104
public function testGetChoicesForValuesUsesLoadedList()

‎src/Symfony/Component/Form/Tests/ChoiceList/Loader/CallbackChoiceLoaderTest.php

+10-3
Original file line numberDiff line numberDiff line change
@@ -67,11 +67,18 @@ public function testLoadChoiceList()
6767
$this->assertInstanceOf(ChoiceListInterface::class, self::$loader->loadChoiceList(self::$value));
6868
}
6969

70-
public function testLoadChoiceListOnlyOnce()
70+
public function testLoadChoicesOnlyOnce()
7171
{
72-
$loadedChoiceList = self::$loader->loadChoiceList(self::$value);
72+
$calls = 0;
73+
$loader = new CallbackChoiceLoader(function () use (&$calls) {
74+
++$calls;
7375

74-
$this->assertSame($loadedChoiceList, self::$loader->loadChoiceList(self::$value));
76+
return [1];
77+
});
78+
$loadedChoiceList = $loader->loadChoiceList();
79+
80+
$this->assertNotSame($loadedChoiceList, $loader->loadChoiceList());
81+
$this->assertSame(1, $calls);
7582
}
7683

7784
public function testLoadChoicesForValuesLoadsChoiceListOnFirstCall()

‎src/Symfony/Component/Form/Tests/ChoiceList/Loader/IntlCallbackChoiceLoaderTest.php

+11-3
Original file line numberDiff line numberDiff line change
@@ -68,11 +68,19 @@ public function testLoadChoiceList()
6868
$this->assertInstanceOf(ChoiceListInterface::class, self::$loader->loadChoiceList(self::$value));
6969
}
7070

71-
public function testLoadChoiceListOnlyOnce()
71+
public function testLoadChoicesOnlyOnce()
7272
{
73-
$loadedChoiceList = self::$loader->loadChoiceList(self::$value);
73+
$calls = 0;
74+
$loader = new IntlCallbackChoiceLoader(function () use (&$calls) {
75+
++$calls;
7476

75-
$this->assertSame($loadedChoiceList, self::$loader->loadChoiceList(self::$value));
77+
return self::$choices;
78+
});
79+
80+
$loadedChoiceList = $loader->loadChoiceList(self::$value);
81+
82+
$this->assertNotSame($loadedChoiceList, $loader->loadChoiceList(self::$value));
83+
$this->assertSame(1, $calls);
7684
}
7785

7886
public function testLoadChoicesForValuesLoadsChoiceListOnFirstCall()

‎src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php

+29
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
namespace Symfony\Component\Form\Tests\Extension\Core\Type;
1313

14+
use Symfony\Component\Form\ChoiceList\Loader\CallbackChoiceLoader;
1415
use Symfony\Component\Form\ChoiceList\View\ChoiceGroupView;
1516
use Symfony\Component\Form\ChoiceList\View\ChoiceView;
1617
use Symfony\Component\Form\Extension\Validator\ValidatorExtension;
@@ -2165,4 +2166,32 @@ public function expandedIsEmptyWhenNoRealChoiceIsSelectedProvider()
21652166
'Placeholder submitted / single / not required / with a placeholder -> should not be empty' => [false, '', false, false, 'ccc'], // The placeholder is a selected value
21662167
];
21672168
}
2169+
2170+
public function testWithSameLoaderAndDifferentChoiceValueCallbacks()
2171+
{
2172+
$choiceLoader = new CallbackChoiceLoader(function () {
2173+
return [1, 2, 3];
2174+
});
2175+
2176+
$view = $this->factory->create(FormTypeTest::TESTED_TYPE)
2177+
->add('choice_one', self::TESTED_TYPE, [
2178+
'choice_loader' => $choiceLoader,
2179+
])
2180+
->add('choice_two', self::TESTED_TYPE, [
2181+
'choice_loader' => $choiceLoader,
2182+
'choice_value' => function ($choice) {
2183+
return $choice ? (string) $choice * 10 : '';
2184+
},
2185+
])
2186+
->createView()
2187+
;
2188+
2189+
$this->assertSame('1', $view['choice_one']->vars['choices'][0]->value);
2190+
$this->assertSame('2', $view['choice_one']->vars['choices'][1]->value);
2191+
$this->assertSame('3', $view['choice_one']->vars['choices'][2]->value);
2192+
2193+
$this->assertSame('10', $view['choice_two']->vars['choices'][0]->value);
2194+
$this->assertSame('20', $view['choice_two']->vars['choices'][1]->value);
2195+
$this->assertSame('30', $view['choice_two']->vars['choices'][2]->value);
2196+
}
21682197
}

0 commit comments

Comments
 (0)