From 65cbf18ea863740cd92dce772e02e074035cd653 Mon Sep 17 00:00:00 2001 From: Jules Pietri Date: Mon, 18 Apr 2022 23:46:24 +0200 Subject: [PATCH] [Form] Fix same choice loader with different choice values --- .../Form/ChoiceList/DoctrineChoiceLoader.php | 21 ++++++-------- .../ChoiceList/DoctrineChoiceLoaderTest.php | 3 +- .../Tests/Form/Type/EntityTypeTest.php | 28 ++++++++++++++++++ src/Symfony/Bridge/Doctrine/composer.json | 2 +- .../Loader/CallbackChoiceLoader.php | 12 ++++---- .../Tests/ChoiceList/LazyChoiceListTest.php | 10 +++---- .../Loader/CallbackChoiceLoaderTest.php | 13 +++++++-- .../Loader/IntlCallbackChoiceLoaderTest.php | 14 +++++++-- .../Extension/Core/Type/ChoiceTypeTest.php | 29 +++++++++++++++++++ 9 files changed, 100 insertions(+), 32 deletions(-) diff --git a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php b/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php index c5ba42d471c35..0a5ea326eff0a 100644 --- a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php +++ b/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php @@ -13,7 +13,6 @@ use Doctrine\Persistence\ObjectManager; use Symfony\Component\Form\ChoiceList\ArrayChoiceList; -use Symfony\Component\Form\ChoiceList\ChoiceListInterface; use Symfony\Component\Form\ChoiceList\Loader\ChoiceLoaderInterface; /** @@ -29,9 +28,9 @@ class DoctrineChoiceLoader implements ChoiceLoaderInterface private $objectLoader; /** - * @var ChoiceListInterface + * @var array|null */ - private $choiceList; + private $choices; /** * Creates a new choice loader. @@ -74,15 +73,13 @@ public function __construct(ObjectManager $manager, string $class, IdReader $idR */ public function loadChoiceList($value = null) { - if ($this->choiceList) { - return $this->choiceList; + if (null === $this->choices) { + $this->choices = $this->objectLoader + ? $this->objectLoader->getEntities() + : $this->manager->getRepository($this->class)->findAll(); } - $objects = $this->objectLoader - ? $this->objectLoader->getEntities() - : $this->manager->getRepository($this->class)->findAll(); - - return $this->choiceList = new ArrayChoiceList($objects, $value); + return new ArrayChoiceList($this->choices, $value); } /** @@ -100,7 +97,7 @@ public function loadValuesForChoices(array $choices, $value = null) $optimize = $this->idReader && (null === $value || \is_array($value) && $value[0] === $this->idReader); // Attention: This optimization does not check choices for existence - if ($optimize && !$this->choiceList && $this->idReader->isSingleId()) { + if ($optimize && !$this->choices && $this->idReader->isSingleId()) { $values = []; // Maintain order and indices of the given objects @@ -136,7 +133,7 @@ public function loadChoicesForValues(array $values, $value = null) // a single-field identifier $optimize = $this->idReader && (null === $value || \is_array($value) && $this->idReader === $value[0]); - if ($optimize && !$this->choiceList && $this->objectLoader && $this->idReader->isSingleId()) { + if ($optimize && !$this->choices && $this->objectLoader && $this->idReader->isSingleId()) { $unorderedObjects = $this->objectLoader->getEntitiesByIds($this->idReader->getIdField(), $values); $objectsById = []; $objects = []; diff --git a/src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php b/src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php index 004eec8201336..29da316d20feb 100644 --- a/src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php +++ b/src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php @@ -146,8 +146,7 @@ public function testLoadChoiceListUsesObjectLoaderIfAvailable() $this->assertEquals($choiceList, $loaded = $loader->loadChoiceList()); // no further loads on subsequent calls - - $this->assertSame($loaded, $loader->loadChoiceList()); + $this->assertEquals($loaded, $loader->loadChoiceList()); } public function testLoadValuesForChoices() diff --git a/src/Symfony/Bridge/Doctrine/Tests/Form/Type/EntityTypeTest.php b/src/Symfony/Bridge/Doctrine/Tests/Form/Type/EntityTypeTest.php index cb7eeed32bbf2..f593618f9c8ff 100644 --- a/src/Symfony/Bridge/Doctrine/Tests/Form/Type/EntityTypeTest.php +++ b/src/Symfony/Bridge/Doctrine/Tests/Form/Type/EntityTypeTest.php @@ -1779,4 +1779,32 @@ public function testSubmitNullMultipleUsesDefaultEmptyData() $this->assertEquals($collection, $form->getNormData()); $this->assertEquals($collection, $form->getData()); } + + public function testWithSameLoaderAndDifferentChoiceValueCallbacks() + { + $entity1 = new SingleIntIdEntity(1, 'Foo'); + $entity2 = new SingleIntIdEntity(2, 'Bar'); + $this->persist([$entity1, $entity2]); + + $view = $this->factory->create(FormTypeTest::TESTED_TYPE) + ->add('entity_one', self::TESTED_TYPE, [ + 'em' => 'default', + 'class' => self::SINGLE_IDENT_CLASS, + ]) + ->add('entity_two', self::TESTED_TYPE, [ + 'em' => 'default', + 'class' => self::SINGLE_IDENT_CLASS, + 'choice_value' => function ($choice) { + return $choice ? $choice->name : ''; + }, + ]) + ->createView() + ; + + $this->assertSame('1', $view['entity_one']->vars['choices'][1]->value); + $this->assertSame('2', $view['entity_one']->vars['choices'][2]->value); + + $this->assertSame('Foo', $view['entity_two']->vars['choices']['Foo']->value); + $this->assertSame('Bar', $view['entity_two']->vars['choices']['Bar']->value); + } } diff --git a/src/Symfony/Bridge/Doctrine/composer.json b/src/Symfony/Bridge/Doctrine/composer.json index f14aa99bc8ad4..b4ebc03d319fb 100644 --- a/src/Symfony/Bridge/Doctrine/composer.json +++ b/src/Symfony/Bridge/Doctrine/composer.json @@ -29,7 +29,7 @@ "symfony/stopwatch": "^3.4|^4.0|^5.0", "symfony/config": "^4.2|^5.0", "symfony/dependency-injection": "^3.4|^4.0|^5.0", - "symfony/form": "^4.4.11|^5.0.11", + "symfony/form": "^4.4.41|^5.0.11", "symfony/http-kernel": "^4.3.7", "symfony/messenger": "^4.4|^5.0", "symfony/property-access": "^3.4|^4.0|^5.0", diff --git a/src/Symfony/Component/Form/ChoiceList/Loader/CallbackChoiceLoader.php b/src/Symfony/Component/Form/ChoiceList/Loader/CallbackChoiceLoader.php index 2d6782f558aea..944d6a48826af 100644 --- a/src/Symfony/Component/Form/ChoiceList/Loader/CallbackChoiceLoader.php +++ b/src/Symfony/Component/Form/ChoiceList/Loader/CallbackChoiceLoader.php @@ -23,11 +23,11 @@ class CallbackChoiceLoader implements ChoiceLoaderInterface private $callback; /** - * The loaded choice list. + * The loaded choices. * - * @var ArrayChoiceList + * @var array|null */ - private $choiceList; + private $choices; /** * @param callable $callback The callable returning an array of choices @@ -42,11 +42,11 @@ public function __construct(callable $callback) */ public function loadChoiceList($value = null) { - if (null !== $this->choiceList) { - return $this->choiceList; + if (null === $this->choices) { + $this->choices = ($this->callback)(); } - return $this->choiceList = new ArrayChoiceList(($this->callback)(), $value); + return new ArrayChoiceList($this->choices, $value); } /** diff --git a/src/Symfony/Component/Form/Tests/ChoiceList/LazyChoiceListTest.php b/src/Symfony/Component/Form/Tests/ChoiceList/LazyChoiceListTest.php index 6f30d0896e1ba..94d41cf9e740f 100644 --- a/src/Symfony/Component/Form/Tests/ChoiceList/LazyChoiceListTest.php +++ b/src/Symfony/Component/Form/Tests/ChoiceList/LazyChoiceListTest.php @@ -32,7 +32,7 @@ public function testGetChoiceLoadersLoadsLoadedListOnFirstCall() $this->assertSame(['RESULT'], $list->getChoices()); $this->assertSame(['RESULT'], $list->getChoices()); - $this->assertSame(1, $calls); + $this->assertSame(2, $calls); } public function testGetValuesLoadsLoadedListOnFirstCall() @@ -46,7 +46,7 @@ public function testGetValuesLoadsLoadedListOnFirstCall() $this->assertSame(['RESULT'], $list->getValues()); $this->assertSame(['RESULT'], $list->getValues()); - $this->assertSame(1, $calls); + $this->assertSame(2, $calls); } public function testGetStructuredValuesLoadsLoadedListOnFirstCall() @@ -60,7 +60,7 @@ public function testGetStructuredValuesLoadsLoadedListOnFirstCall() $this->assertSame(['RESULT'], $list->getStructuredValues()); $this->assertSame(['RESULT'], $list->getStructuredValues()); - $this->assertSame(1, $calls); + $this->assertSame(2, $calls); } public function testGetOriginalKeysLoadsLoadedListOnFirstCall() @@ -79,7 +79,7 @@ public function testGetOriginalKeysLoadsLoadedListOnFirstCall() $this->assertSame(['foo' => 'a', 'bar' => 'b', 'baz' => 'c'], $list->getOriginalKeys()); $this->assertSame(['foo' => 'a', 'bar' => 'b', 'baz' => 'c'], $list->getOriginalKeys()); - $this->assertSame(3, $calls); + $this->assertSame(6, $calls); } public function testGetChoicesForValuesForwardsCallIfListNotLoaded() @@ -98,7 +98,7 @@ public function testGetChoicesForValuesForwardsCallIfListNotLoaded() $this->assertSame(['foo', 'bar'], $list->getChoicesForValues(['a', 'b'])); $this->assertSame(['foo', 'bar'], $list->getChoicesForValues(['a', 'b'])); - $this->assertSame(3, $calls); + $this->assertSame(6, $calls); } public function testGetChoicesForValuesUsesLoadedList() diff --git a/src/Symfony/Component/Form/Tests/ChoiceList/Loader/CallbackChoiceLoaderTest.php b/src/Symfony/Component/Form/Tests/ChoiceList/Loader/CallbackChoiceLoaderTest.php index 52dd5f8af580f..69eb787a23dfa 100644 --- a/src/Symfony/Component/Form/Tests/ChoiceList/Loader/CallbackChoiceLoaderTest.php +++ b/src/Symfony/Component/Form/Tests/ChoiceList/Loader/CallbackChoiceLoaderTest.php @@ -67,11 +67,18 @@ public function testLoadChoiceList() $this->assertInstanceOf(ChoiceListInterface::class, self::$loader->loadChoiceList(self::$value)); } - public function testLoadChoiceListOnlyOnce() + public function testLoadChoicesOnlyOnce() { - $loadedChoiceList = self::$loader->loadChoiceList(self::$value); + $calls = 0; + $loader = new CallbackChoiceLoader(function () use (&$calls) { + ++$calls; - $this->assertSame($loadedChoiceList, self::$loader->loadChoiceList(self::$value)); + return [1]; + }); + $loadedChoiceList = $loader->loadChoiceList(); + + $this->assertNotSame($loadedChoiceList, $loader->loadChoiceList()); + $this->assertSame(1, $calls); } public function testLoadChoicesForValuesLoadsChoiceListOnFirstCall() diff --git a/src/Symfony/Component/Form/Tests/ChoiceList/Loader/IntlCallbackChoiceLoaderTest.php b/src/Symfony/Component/Form/Tests/ChoiceList/Loader/IntlCallbackChoiceLoaderTest.php index a5fc262dcd3a1..e2827b0d913be 100644 --- a/src/Symfony/Component/Form/Tests/ChoiceList/Loader/IntlCallbackChoiceLoaderTest.php +++ b/src/Symfony/Component/Form/Tests/ChoiceList/Loader/IntlCallbackChoiceLoaderTest.php @@ -68,11 +68,19 @@ public function testLoadChoiceList() $this->assertInstanceOf(ChoiceListInterface::class, self::$loader->loadChoiceList(self::$value)); } - public function testLoadChoiceListOnlyOnce() + public function testLoadChoicesOnlyOnce() { - $loadedChoiceList = self::$loader->loadChoiceList(self::$value); + $calls = 0; + $loader = new IntlCallbackChoiceLoader(function () use (&$calls) { + ++$calls; - $this->assertSame($loadedChoiceList, self::$loader->loadChoiceList(self::$value)); + return self::$choices; + }); + + $loadedChoiceList = $loader->loadChoiceList(self::$value); + + $this->assertNotSame($loadedChoiceList, $loader->loadChoiceList(self::$value)); + $this->assertSame(1, $calls); } public function testLoadChoicesForValuesLoadsChoiceListOnFirstCall() diff --git a/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php b/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php index 1bf30575275f9..0da56921d93ed 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php @@ -11,6 +11,7 @@ namespace Symfony\Component\Form\Tests\Extension\Core\Type; +use Symfony\Component\Form\ChoiceList\Loader\CallbackChoiceLoader; use Symfony\Component\Form\ChoiceList\View\ChoiceGroupView; use Symfony\Component\Form\ChoiceList\View\ChoiceView; use Symfony\Component\Form\Extension\Validator\ValidatorExtension; @@ -2165,4 +2166,32 @@ public function expandedIsEmptyWhenNoRealChoiceIsSelectedProvider() 'Placeholder submitted / single / not required / with a placeholder -> should not be empty' => [false, '', false, false, 'ccc'], // The placeholder is a selected value ]; } + + public function testWithSameLoaderAndDifferentChoiceValueCallbacks() + { + $choiceLoader = new CallbackChoiceLoader(function () { + return [1, 2, 3]; + }); + + $view = $this->factory->create(FormTypeTest::TESTED_TYPE) + ->add('choice_one', self::TESTED_TYPE, [ + 'choice_loader' => $choiceLoader, + ]) + ->add('choice_two', self::TESTED_TYPE, [ + 'choice_loader' => $choiceLoader, + 'choice_value' => function ($choice) { + return $choice ? (string) $choice * 10 : ''; + }, + ]) + ->createView() + ; + + $this->assertSame('1', $view['choice_one']->vars['choices'][0]->value); + $this->assertSame('2', $view['choice_one']->vars['choices'][1]->value); + $this->assertSame('3', $view['choice_one']->vars['choices'][2]->value); + + $this->assertSame('10', $view['choice_two']->vars['choices'][0]->value); + $this->assertSame('20', $view['choice_two']->vars['choices'][1]->value); + $this->assertSame('30', $view['choice_two']->vars['choices'][2]->value); + } }