From 1394df2deaec9975bbafca5d7b767140872470d6 Mon Sep 17 00:00:00 2001 From: Jules Pietri Date: Sun, 7 Apr 2019 15:11:26 +0200 Subject: [PATCH] [Form] Added an AbstractChoiceLoader to simplify implementations and handle global optimizations --- .../Form/ChoiceList/DoctrineChoiceLoader.php | 73 +++++----------- .../Doctrine/Form/ChoiceList/IdReader.php | 6 +- .../ChoiceList/DoctrineChoiceLoaderTest.php | 59 +++++++++++-- src/Symfony/Bridge/Doctrine/composer.json | 4 +- src/Symfony/Component/Form/CHANGELOG.md | 1 + .../Loader/AbstractChoiceLoader.php | 86 +++++++++++++++++++ .../Loader/CallbackChoiceLoader.php | 52 ++--------- .../Loader/IntlCallbackChoiceLoader.php | 14 +-- 8 files changed, 174 insertions(+), 121 deletions(-) create mode 100644 src/Symfony/Component/Form/ChoiceList/Loader/AbstractChoiceLoader.php diff --git a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php b/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php index cb09a3d2c1..99be884f34 100644 --- a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php +++ b/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php @@ -12,27 +12,20 @@ namespace Symfony\Bridge\Doctrine\Form\ChoiceList; use Doctrine\Persistence\ObjectManager; -use Symfony\Component\Form\ChoiceList\ArrayChoiceList; -use Symfony\Component\Form\ChoiceList\ChoiceListInterface; -use Symfony\Component\Form\ChoiceList\Loader\ChoiceLoaderInterface; +use Symfony\Component\Form\ChoiceList\Loader\AbstractChoiceLoader; /** * Loads choices using a Doctrine object manager. * * @author Bernhard Schussek */ -class DoctrineChoiceLoader implements ChoiceLoaderInterface +class DoctrineChoiceLoader extends AbstractChoiceLoader { private $manager; private $class; private $idReader; private $objectLoader; - /** - * @var ChoiceListInterface - */ - private $choiceList; - /** * Creates a new choice loader. * @@ -59,81 +52,57 @@ class DoctrineChoiceLoader implements ChoiceLoaderInterface /** * {@inheritdoc} */ - public function loadChoiceList(callable $value = null) + protected function loadChoices(): iterable { - if ($this->choiceList) { - return $this->choiceList; - } - - $objects = $this->objectLoader + return $this->objectLoader ? $this->objectLoader->getEntities() : $this->manager->getRepository($this->class)->findAll(); - - return $this->choiceList = new ArrayChoiceList($objects, $value); } /** - * {@inheritdoc} + * @internal to be remove in Symfony 6 */ - public function loadValuesForChoices(array $choices, callable $value = null) + protected function doLoadValuesForChoices(array $choices): array { - // Performance optimization - if (empty($choices)) { - return []; - } - // Optimize performance for single-field identifiers. We already // know that the IDs are used as values - $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) { - $values = []; - + if ($this->idReader) { + 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__); // Maintain order and indices of the given objects + $values = []; foreach ($choices as $i => $object) { if ($object instanceof $this->class) { - // Make sure to convert to the right format - $values[$i] = (string) $this->idReader->getIdValue($object); + $values[$i] = $this->idReader->getIdValue($object); } } return $values; } - return $this->loadChoiceList($value)->getValuesForChoices($choices); + return parent::doLoadValuesForChoices($choices); } - /** - * {@inheritdoc} - */ - public function loadChoicesForValues(array $values, callable $value = null) + protected function doLoadChoicesForValues(array $values, ?callable $value): array { - // Performance optimization - // Also prevents the generation of "WHERE id IN ()" queries through the - // object loader. At least with MySQL and on the development machine - // this was tested on, no exception was thrown for such invalid - // statements, consequently no test fails when this code is removed. - // https://github.com/symfony/symfony/pull/8981#issuecomment-24230557 - if (empty($values)) { - return []; + $legacy = $this->idReader && null === $value; + + if ($legacy) { + 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__); } // Optimize performance in case we have an object loader and // a single-field identifier - $optimize = $this->idReader && (null === $value || \is_array($value) && $this->idReader === $value[0]); - - if ($optimize && !$this->choiceList && $this->objectLoader) { - $unorderedObjects = $this->objectLoader->getEntitiesByIds($this->idReader->getIdField(), $values); - $objectsById = []; + if (($legacy || \is_array($value) && $this->idReader === $value[0]) && $this->objectLoader) { $objects = []; + $objectsById = []; // Maintain order and indices from the given $values // An alternative approach to the following loop is to add the // "INDEX BY" clause to the Doctrine query in the loader, // but I'm not sure whether that's doable in a generic fashion. - foreach ($unorderedObjects as $object) { - $objectsById[(string) $this->idReader->getIdValue($object)] = $object; + foreach ($this->objectLoader->getEntitiesByIds($this->idReader->getIdField(), $values) as $object) { + $objectsById[$this->idReader->getIdValue($object)] = $object; } foreach ($values as $i => $id) { @@ -145,6 +114,6 @@ class DoctrineChoiceLoader implements ChoiceLoaderInterface return $objects; } - return $this->loadChoiceList($value)->getChoicesForValues($values); + return parent::doLoadChoicesForValues($values, $value); } } diff --git a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/IdReader.php b/src/Symfony/Bridge/Doctrine/Form/ChoiceList/IdReader.php index 195e7ce80f..980c0ce89f 100644 --- a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/IdReader.php +++ b/src/Symfony/Bridge/Doctrine/Form/ChoiceList/IdReader.php @@ -84,12 +84,12 @@ class IdReader * * This method assumes that the object has a single-column ID. * - * @return mixed The ID value + * @return string The ID value */ public function getIdValue(object $object = null) { if (!$object) { - return null; + return ''; } if (!$this->om->contains($object)) { @@ -104,7 +104,7 @@ class IdReader $idValue = $this->associationIdReader->getIdValue($idValue); } - return $idValue; + return (string) $idValue; } /** diff --git a/src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php b/src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php index 182f2703e0..9ed8dcd400 100644 --- a/src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php +++ b/src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php @@ -100,6 +100,10 @@ class DoctrineChoiceLoaderTest extends TestCase ->method('getClassMetadata') ->with($this->class) ->willReturn(new ClassMetadata($this->class)); + $this->repository->expects($this->any()) + ->method('findAll') + ->willReturn([$this->obj1, $this->obj2, $this->obj3]) + ; } public function testLoadChoiceList() @@ -186,6 +190,11 @@ class DoctrineChoiceLoaderTest extends TestCase $this->assertSame([], $loader->loadValuesForChoices([])); } + /** + * @group legacy + * + * @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. + */ public function testLoadValuesForChoicesDoesNotLoadIfSingleIntId() { $loader = new DoctrineChoiceLoader( @@ -205,7 +214,7 @@ class DoctrineChoiceLoaderTest extends TestCase $this->assertSame(['2'], $loader->loadValuesForChoices([$this->obj2])); } - public function testLoadValuesForChoicesLoadsIfSingleIntIdAndValueGiven() + public function testLoadValuesForChoicesDoesNotLoadIfSingleIntIdAndValueGiven() { $loader = new DoctrineChoiceLoader( $this->om, @@ -216,7 +225,7 @@ class DoctrineChoiceLoaderTest extends TestCase $choices = [$this->obj1, $this->obj2, $this->obj3]; $value = function (\stdClass $object) { return $object->name; }; - $this->repository->expects($this->once()) + $this->repository->expects($this->never()) ->method('findAll') ->willReturn($choices); @@ -254,8 +263,7 @@ class DoctrineChoiceLoaderTest extends TestCase { $loader = new DoctrineChoiceLoader( $this->om, - $this->class, - $this->idReader + $this->class ); $choices = [$this->obj1, $this->obj2, $this->obj3]; @@ -285,7 +293,12 @@ class DoctrineChoiceLoaderTest extends TestCase $this->assertSame([], $loader->loadChoicesForValues([])); } - public function testLoadChoicesForValuesLoadsOnlyChoicesIfSingleIntId() + /** + * @group legacy + * + * @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. + */ + public function legacyTestLoadChoicesForValuesLoadsOnlyChoicesIfValueUseIdReader() { $loader = new DoctrineChoiceLoader( $this->om, @@ -321,6 +334,42 @@ class DoctrineChoiceLoaderTest extends TestCase )); } + public function testLoadChoicesForValuesLoadsOnlyChoicesIfValueUseIdReader() + { + $loader = new DoctrineChoiceLoader( + $this->om, + $this->class, + $this->idReader, + $this->objectLoader + ); + + $choices = [$this->obj2, $this->obj3]; + + $this->idReader->expects($this->any()) + ->method('getIdField') + ->willReturn('idField'); + + $this->repository->expects($this->never()) + ->method('findAll'); + + $this->objectLoader->expects($this->once()) + ->method('getEntitiesByIds') + ->with('idField', [4 => '3', 7 => '2']) + ->willReturn($choices); + + $this->idReader->expects($this->any()) + ->method('getIdValue') + ->willReturnMap([ + [$this->obj2, '2'], + [$this->obj3, '3'], + ]); + + $this->assertSame( + [4 => $this->obj3, 7 => $this->obj2], + $loader->loadChoicesForValues([4 => '3', 7 => '2'], [$this->idReader, 'getIdValue'] + )); + } + public function testLoadChoicesForValuesLoadsAllIfSingleIntIdAndValueGiven() { $loader = new DoctrineChoiceLoader( diff --git a/src/Symfony/Bridge/Doctrine/composer.json b/src/Symfony/Bridge/Doctrine/composer.json index 9513bbe58f..26eeb9eb49 100644 --- a/src/Symfony/Bridge/Doctrine/composer.json +++ b/src/Symfony/Bridge/Doctrine/composer.json @@ -27,7 +27,7 @@ "symfony/stopwatch": "^4.4|^5.0", "symfony/config": "^4.4|^5.0", "symfony/dependency-injection": "^4.4|^5.0", - "symfony/form": "^5.0", + "symfony/form": "^5.1", "symfony/http-kernel": "^5.0", "symfony/messenger": "^4.4|^5.0", "symfony/property-access": "^4.4|^5.0", @@ -49,7 +49,7 @@ "conflict": { "phpunit/phpunit": "<5.4.3", "symfony/dependency-injection": "<4.4", - "symfony/form": "<5", + "symfony/form": "<5.1", "symfony/http-kernel": "<5", "symfony/messenger": "<4.4", "symfony/property-info": "<5", diff --git a/src/Symfony/Component/Form/CHANGELOG.md b/src/Symfony/Component/Form/CHANGELOG.md index 97ed3b791d..24935f0449 100644 --- a/src/Symfony/Component/Form/CHANGELOG.md +++ b/src/Symfony/Component/Form/CHANGELOG.md @@ -4,6 +4,7 @@ CHANGELOG 5.1.0 ----- + * Added an `AbstractChoiceLoader` to simplify implementations and handle global optimizations * The `view_timezone` option defaults to the `model_timezone` if no `reference_date` is configured. * Added default `inputmode` attribute to Search, Email and Tel form types. * Implementing the `FormConfigInterface` without implementing the `getIsEmptyCallback()` method diff --git a/src/Symfony/Component/Form/ChoiceList/Loader/AbstractChoiceLoader.php b/src/Symfony/Component/Form/ChoiceList/Loader/AbstractChoiceLoader.php new file mode 100644 index 0000000000..ea736a52c6 --- /dev/null +++ b/src/Symfony/Component/Form/ChoiceList/Loader/AbstractChoiceLoader.php @@ -0,0 +1,86 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Form\ChoiceList\Loader; + +use Symfony\Component\Form\ChoiceList\ArrayChoiceList; + +/** + * @author Jules Pietri + */ +abstract class AbstractChoiceLoader implements ChoiceLoaderInterface +{ + /** + * The loaded choice list. + * + * @var ArrayChoiceList + */ + private $choiceList; + + /** + * @final + * + * {@inheritdoc} + */ + public function loadChoiceList(callable $value = null) + { + return $this->choiceList ?? ($this->choiceList = new ArrayChoiceList($this->loadChoices(), $value)); + } + + /** + * {@inheritdoc} + */ + public function loadChoicesForValues(array $values, callable $value = null) + { + if (!$values) { + return []; + } + + if ($this->choiceList) { + return $this->choiceList->getChoicesForValues($values); + } + + return $this->doLoadChoicesForValues($values, $value); + } + + /** + * {@inheritdoc} + */ + public function loadValuesForChoices(array $choices, callable $value = null) + { + if (!$choices) { + return []; + } + + if ($value) { + // if a value callback exists, use it + return array_map($value, $choices); + } + + if ($this->choiceList) { + return $this->choiceList->getValuesForChoices($choices); + } + + return $this->doLoadValuesForChoices($choices); + } + + abstract protected function loadChoices(): iterable; + + protected function doLoadChoicesForValues(array $values, ?callable $value): array + { + return $this->loadChoiceList($value)->getChoicesForValues($values); + } + + protected function doLoadValuesForChoices(array $choices): array + { + return $this->loadChoiceList()->getValuesForChoices($choices); + } +} diff --git a/src/Symfony/Component/Form/ChoiceList/Loader/CallbackChoiceLoader.php b/src/Symfony/Component/Form/ChoiceList/Loader/CallbackChoiceLoader.php index 5b4abf7a89..1811d434e8 100644 --- a/src/Symfony/Component/Form/ChoiceList/Loader/CallbackChoiceLoader.php +++ b/src/Symfony/Component/Form/ChoiceList/Loader/CallbackChoiceLoader.php @@ -11,67 +11,25 @@ namespace Symfony\Component\Form\ChoiceList\Loader; -use Symfony\Component\Form\ChoiceList\ArrayChoiceList; - /** - * Loads an {@link ArrayChoiceList} instance from a callable returning an array of choices. + * Loads an {@link ArrayChoiceList} instance from a callable returning iterable choices. * * @author Jules Pietri */ -class CallbackChoiceLoader implements ChoiceLoaderInterface +class CallbackChoiceLoader extends AbstractChoiceLoader { private $callback; /** - * The loaded choice list. - * - * @var ArrayChoiceList - */ - private $choiceList; - - /** - * @param callable $callback The callable returning an array of choices + * @param callable $callback The callable returning iterable choices */ public function __construct(callable $callback) { $this->callback = $callback; } - /** - * {@inheritdoc} - */ - public function loadChoiceList(callable $value = null) + protected function loadChoices(): iterable { - if (null !== $this->choiceList) { - return $this->choiceList; - } - - return $this->choiceList = new ArrayChoiceList(($this->callback)(), $value); - } - - /** - * {@inheritdoc} - */ - public function loadChoicesForValues(array $values, callable $value = null) - { - // Optimize - if (empty($values)) { - return []; - } - - return $this->loadChoiceList($value)->getChoicesForValues($values); - } - - /** - * {@inheritdoc} - */ - public function loadValuesForChoices(array $choices, callable $value = null) - { - // Optimize - if (empty($choices)) { - return []; - } - - return $this->loadChoiceList($value)->getValuesForChoices($choices); + return ($this->callback)(); } } diff --git a/src/Symfony/Component/Form/ChoiceList/Loader/IntlCallbackChoiceLoader.php b/src/Symfony/Component/Form/ChoiceList/Loader/IntlCallbackChoiceLoader.php index 279a7254ff..546937b900 100644 --- a/src/Symfony/Component/Form/ChoiceList/Loader/IntlCallbackChoiceLoader.php +++ b/src/Symfony/Component/Form/ChoiceList/Loader/IntlCallbackChoiceLoader.php @@ -24,13 +24,7 @@ class IntlCallbackChoiceLoader extends CallbackChoiceLoader */ public function loadChoicesForValues(array $values, callable $value = null) { - // Optimize - $values = array_filter($values); - if (empty($values)) { - return []; - } - - return $this->loadChoiceList($value)->getChoicesForValues($values); + return parent::loadChoicesForValues(array_filter($values), $value); } /** @@ -38,17 +32,13 @@ class IntlCallbackChoiceLoader extends CallbackChoiceLoader */ public function loadValuesForChoices(array $choices, callable $value = null) { - // Optimize $choices = array_filter($choices); - if (empty($choices)) { - return []; - } // If no callable is set, choices are the same as values if (null === $value) { return $choices; } - return $this->loadChoiceList($value)->getValuesForChoices($choices); + return parent::loadValuesForChoices($choices, $value); } }