From 82e1931807c72780e620242b343e6f2b30c37b65 Mon Sep 17 00:00:00 2001 From: Jules Pietri Date: Sun, 7 Apr 2019 15:11:26 +0200 Subject: [PATCH] [Form] Added an to simplify implementations and handle global optimizations --- src/Symfony/Bridge/Doctrine/CHANGELOG.md | 1 + .../Form/ChoiceList/DoctrineChoiceLoader.php | 67 ++------------ .../Doctrine/Form/ChoiceList/IdReader.php | 4 +- .../ChoiceList/DoctrineChoiceLoaderTest.php | 30 ++++-- .../Loader/AbstractChoiceLoader.php | 91 +++++++++++++++++++ .../Loader/CallbackChoiceLoader.php | 48 +--------- .../Loader/IntlCallbackChoiceLoader.php | 13 +-- 7 files changed, 126 insertions(+), 128 deletions(-) create mode 100644 src/Symfony/Component/Form/ChoiceList/Loader/AbstractChoiceLoader.php diff --git a/src/Symfony/Bridge/Doctrine/CHANGELOG.md b/src/Symfony/Bridge/Doctrine/CHANGELOG.md index 3bcf9a77dfe56..c7cfff6f71aa2 100644 --- a/src/Symfony/Bridge/Doctrine/CHANGELOG.md +++ b/src/Symfony/Bridge/Doctrine/CHANGELOG.md @@ -7,6 +7,7 @@ CHANGELOG * changed guessing of DECIMAL to set the `input` option of `NumberType` to string * deprecated not passing an `IdReader` to the `DoctrineChoiceLoader` when query can be optimized with a single id field * deprecated passing an `IdReader` to the `DoctrineChoiceLoader` when entities have a composite id + * added an `AbstractChoiceLoader` to simplify implementations and handle global optimizations 4.2.0 ----- diff --git a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php b/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php index cd040d12a9b03..16994be4aaf9a 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\Common\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. * @@ -75,71 +68,23 @@ public function __construct(ObjectManager $manager, string $class, IdReader $idR /** * {@inheritdoc} */ - public function loadChoiceList($value = null) + public function loadChoices(): array { - 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} */ - public function loadValuesForChoices(array $choices, $value = null) + public function doLoadChoicesForValues(array $values, ?callable $value = null): 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 && $this->idReader->isSingleId()) { - $values = []; - - // Maintain order and indices of the given objects - 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); - } - } - - return $values; - } - - return $this->loadChoiceList($value)->getValuesForChoices($choices); - } - - /** - * {@inheritdoc} - */ - public function loadChoicesForValues(array $values, $value = null) - { - // 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 []; - } - // 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 && $this->idReader->isSingleId()) { + if ($optimize && $this->objectLoader && $this->idReader->isSingleId()) { $unorderedObjects = $this->objectLoader->getEntitiesByIds($this->idReader->getIdField(), $values); $objectsById = []; $objects = []; diff --git a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/IdReader.php b/src/Symfony/Bridge/Doctrine/Form/ChoiceList/IdReader.php index 3509d9b03b329..bde44260d5204 100644 --- a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/IdReader.php +++ b/src/Symfony/Bridge/Doctrine/Form/ChoiceList/IdReader.php @@ -91,7 +91,7 @@ public function isIntId(): bool public function getIdValue($object) { if (!$object) { - return; + return ''; } if (!$this->om->contains($object)) { @@ -106,7 +106,7 @@ public function getIdValue($object) $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 5a5fba5afaf57..b5160ff69182a 100644 --- a/src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php +++ b/src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php @@ -94,12 +94,18 @@ protected function setUp() $this->om->expects($this->any()) ->method('getRepository') ->with($this->class) - ->willReturn($this->repository); + ->willReturn($this->repository) + ; $this->om->expects($this->any()) ->method('getClassMetadata') ->with($this->class) - ->willReturn(new ClassMetadata($this->class)); + ->willReturn(new ClassMetadata($this->class)) + ; + $this->repository->expects($this->any()) + ->method('findAll') + ->willReturn([$this->obj1, $this->obj2, $this->obj3]) + ; } public function testLoadChoiceList() @@ -116,7 +122,8 @@ public function testLoadChoiceList() $this->repository->expects($this->once()) ->method('findAll') - ->willReturn($choices); + ->willReturn($choices) + ; $this->assertEquals($choiceList, $loader->loadChoiceList($value)); @@ -202,7 +209,7 @@ public function testLoadValuesForChoicesDoesNotLoadIfSingleIntId() ->with($this->obj2) ->willReturn('2'); - $this->assertSame(['2'], $loader->loadValuesForChoices([$this->obj2])); + $this->assertSame(['2'], $loader->loadValuesForChoices([$this->obj2], [$this->idReader, 'getIdValue'])); } public function testLoadValuesForChoicesLoadsIfSingleIntIdAndValueGiven() @@ -216,7 +223,7 @@ public function testLoadValuesForChoicesLoadsIfSingleIntIdAndValueGiven() $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); @@ -285,7 +292,7 @@ public function testLoadChoicesForValuesDoesNotLoadIfEmptyValues() $this->assertSame([], $loader->loadChoicesForValues([])); } - public function testLoadChoicesForValuesLoadsOnlyChoicesIfSingleIntId() + public function testLoadChoicesForValuesLoadsOnlyChoicesIfValueUseIdReader() { $loader = new DoctrineChoiceLoader( $this->om, @@ -317,8 +324,8 @@ public function testLoadChoicesForValuesLoadsOnlyChoicesIfSingleIntId() $this->assertSame( [4 => $this->obj3, 7 => $this->obj2], - $loader->loadChoicesForValues([4 => '3', 7 => '2'] - )); + $loader->loadChoicesForValues([4 => '3', 7 => '2'], [$this->idReader, 'getIdValue']) + ); } public function testLoadChoicesForValuesLoadsAllIfSingleIntIdAndValueGiven() @@ -426,8 +433,13 @@ public function testLoaderWithoutIdReaderCanBeOptimized() $choices = [$obj1, $obj2]; + $this->objectLoader->expects($this->never()) + ->method('getEntities') + ; + $this->repository->expects($this->never()) - ->method('findAll'); + ->method('findAll') + ; $this->objectLoader->expects($this->once()) ->method('getEntitiesByIds') 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 0000000000000..922bcc2a18591 --- /dev/null +++ b/src/Symfony/Component/Form/ChoiceList/Loader/AbstractChoiceLoader.php @@ -0,0 +1,91 @@ + + * + * 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; +use Symfony\Component\Form\ChoiceList\ChoiceListInterface; + +/** + * @author Jules Pietri + */ +abstract class AbstractChoiceLoader implements ChoiceLoaderInterface +{ + /** + * The loaded choice list. + * + * @var ChoiceListInterface + */ + protected $choiceList; + + /** + * {@inheritdoc} + */ + public function loadChoiceList($value = null) + { + if (null !== $this->choiceList) { + return $this->choiceList; + } + + return $this->choiceList = new ArrayChoiceList($this->loadChoices(), $value); + } + + /** + * {@inheritdoc} + */ + public function loadChoicesForValues(array $values, $value = null) + { + // Optimize + if (empty($values)) { + return []; + } + + if ($this->choiceList) { + return $this->choiceList->getChoicesForValues($values, $value); + } + + return $this->doLoadChoicesForValues($values, $value); + } + + /** + * {@inheritdoc} + */ + public function loadValuesForChoices(array $choices, $value = null) + { + // Optimize + if (empty($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(): array; + + 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 2d6782f558aea..6ad01c4b4bb49 100644 --- a/src/Symfony/Component/Form/ChoiceList/Loader/CallbackChoiceLoader.php +++ b/src/Symfony/Component/Form/ChoiceList/Loader/CallbackChoiceLoader.php @@ -11,24 +11,15 @@ 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. * * @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 */ @@ -37,41 +28,8 @@ public function __construct(callable $callback) $this->callback = $callback; } - /** - * {@inheritdoc} - */ - public function loadChoiceList($value = null) + protected function loadChoices(): array { - if (null !== $this->choiceList) { - return $this->choiceList; - } - - return $this->choiceList = new ArrayChoiceList(($this->callback)(), $value); - } - - /** - * {@inheritdoc} - */ - public function loadChoicesForValues(array $values, $value = null) - { - // Optimize - if (empty($values)) { - return []; - } - - return $this->loadChoiceList($value)->getChoicesForValues($values); - } - - /** - * {@inheritdoc} - */ - public function loadValuesForChoices(array $choices, $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 1a119bb96cdcf..b8fb579f2f4fd 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, $value = null) { - // Optimize - $values = array_filter($values); - if (empty($values)) { - return []; - } - - return $this->loadChoiceList($value)->getChoicesForValues($values); + return parent::loadChoicesForValues(array_filter($values), $value); } /** @@ -40,15 +34,12 @@ public function loadValuesForChoices(array $choices, $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); } }