[Form] Added an AbstractChoiceLoader to simplify implementations and …

…handle global optimizations

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 <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,44 @@ 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);
+            : $this->manager->getRepository($this->class)->findAll()
+        ;
     }
 
-    /**
-     * {@inheritdoc}
-     */
-    public function loadValuesForChoices(array $choices, callable $value = null)
+    protected function doLoadValuesForChoices(array $choices): array
     {
-        // Performance optimization
-        if (empty($choices)) {
-            return [];
+        if ($this->idReader) {
+            return array_filter(array_map(function ($choice): ?string {
+                return $choice instanceof $this->class ? $this->idReader->getIdValue($choice) : null;
+            }, $choices));
         }
 
-        // 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 = [];
-
-            // 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);
+        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 [];
-        }
-
         // 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]);
+        $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);
+        if ($optimize && $this->objectLoader) {
             $objectsById = [];
             $objects = [];
 
             // 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 +101,6 @@ public function loadChoicesForValues(array $values, callable $value = null)
             return $objects;
         }
 
-        return $this->loadChoiceList($value)->getChoicesForValues($values);
+        return parent::doLoadChoicesForValues($values, $value);
     }
 }

src/Symfony/Bridge/Doctrine/Form/ChoiceList/IdReader.php

@@ -84,7 +84,7 @@ public function isIntId(): bool
      *
      * This method assumes that the object has a single-column ID.
      *
-     * @return mixed The ID value
+     * @return string|null The ID value
      */
     public function getIdValue(object $object = null)
     {
@@ -104,7 +104,7 @@ public function getIdValue(object $object = null)
             $idValue = $this->associationIdReader->getIdValue($idValue);
         }
 
-        return $idValue;
+        return (string) $idValue;
     }
 
     /**

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()
@@ -205,7 +209,7 @@ public function testLoadValuesForChoicesDoesNotLoadIfSingleIntId()
         $this->assertSame(['2'], $loader->loadValuesForChoices([$this->obj2]));
     }
 
-    public function testLoadValuesForChoicesLoadsIfSingleIntIdAndValueGiven()
+    public function testLoadValuesForChoicesDoesNotLoadIfSingleIntIdAndValueGiven()
     {
         $loader = new DoctrineChoiceLoader(
             $this->om,
@@ -216,7 +220,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 +289,7 @@ public function testLoadChoicesForValuesDoesNotLoadIfEmptyValues()
         $this->assertSame([], $loader->loadChoicesForValues([]));
     }
 
-    public function testLoadChoicesForValuesLoadsOnlyChoicesIfSingleIntId()
+    public function testLoadChoicesForValuesLoadsOnlyChoicesIfValueUseIdReader()
     {
         $loader = new DoctrineChoiceLoader(
             $this->om,

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",

src/Symfony/Component/Form/CHANGELOG.md

@@ -1,6 +1,11 @@
 CHANGELOG
 =========
 
+5.1.0
+-----
+
+ * added an `AbstractChoiceLoader` to simplify implementations and handle global optimizations
+
 5.0.0
 -----
 

src/Symfony/Component/Form/ChoiceList/Loader/AbstractChoiceLoader.php

@@ -0,0 +1,88 @@
+<?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)
+    {
+        // Optimize
+        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)
+    {
+        // Optimize
+        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);
+    }
+}

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 <jules@heahprod.com>
  */
-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)();
     }
 }