symfony · fabpot · Feb 12, 2020 · Apr 7, 2019
diff --git 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 <bschussek@gmail.com>
 */
-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 @@ public function __construct(ObjectManager $manager, string $class, IdReader $idR
    /**
     * {@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 @@ public function loadChoicesForValues(array $values, callable $value = null)
            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
@@ -84,12 +84,12 @@ public function isIntId(): bool
     *
     * 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 @@ public function getIdValue(object $object = null)
            $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
@@ -100,6 +100,10 @@ protected function setUp(): void
            ->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 @@ public function testLoadValuesForChoicesDoesNotLoadIfEmptyChoices()
        $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 @@ public function testLoadValuesForChoicesDoesNotLoadIfSingleIntId()
        $this->assertSame(['2'], $loader->loadValuesForChoices([$this->obj2]));
    }

-    public function testLoadValuesForChoicesLoadsIfSingleIntIdAndValueGiven()
+    public function testLoadValuesForChoicesDoesNotLoadIfSingleIntIdAndValueGiven()
    {
        $loader = new DoctrineChoiceLoader(
            $this->om,
@@ -216,7 +225,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);

@@ -254,8 +263,7 @@ public function testLoadChoicesForValues()
    {
        $loader = new DoctrineChoiceLoader(
            $this->om,
-            $this->class,
-            $this->idReader
+            $this->class
        );

        $choices = [$this->obj1, $this->obj2, $this->obj3];
@@ -285,7 +293,12 @@ public function testLoadChoicesForValuesDoesNotLoadIfEmptyValues()
        $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 @@ public function testLoadChoicesForValuesLoadsOnlyChoicesIfSingleIntId()
        ));
    }

+    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
@@ -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
@@ -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
@@ -0,0 +1,86 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * 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 <jules@heahprod.com>
+ */
+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);
+    }
+}