symfony · HeahDude · Apr 7, 2019 · fabpot · Apr 8, 2019 · fabpot
diff --git 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
@@ -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.
      *
@@ -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
@@ -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
@@ -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
@@ -0,0 +1,91 @@
+<?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;
+use Symfony\Component\Form\ChoiceList\ChoiceListInterface;
+
+/**
+ * @author Jules Pietri <jules@heahprod.com>
+ */
+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
@@ -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 <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
      */
@@ -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)();
     }
 }