tmp

symfony · Dec 8, 2019 · 528f9f3 · 528f9f3
1 parent 07b498c
commit 528f9f3
Show file tree

Hide file tree

Showing 3 changed files with 75 additions and 13 deletions.
diff --git a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php b/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php
@@ -60,12 +60,26 @@ protected function loadChoices(): iterable
         ;
     }
 
+    /**
+     * This method is here for legacy.
+     *
+     * To remove in 6.0
+     */
     protected function doLoadValuesForChoices(array $choices): array
     {
+        // Optimize performance for single-field identifiers. We already
+        // know that the IDs are used as values
+        // Attention: This optimization does not check choices for existence
         if ($this->idReader) {
-            return array_filter(array_map(function ($choice): ?string {
-                return $choice instanceof $this->class ? $this->idReader->getIdValue($choice) : null;
-            }, $choices));
+            @trigger_error(sprintf('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 "%s" or define the "choice_value" option instead.', __CLASS__), E_USER_DEPRECATED);
+            // Maintain order and indices of the given objects
+            foreach ($choices as $i => $object) {
+                if ($object instanceof $this->class) {
+                    $values[$i] = $this->idReader->getIdValue($object);
+                }
+            }
+
+            return $values ?? [];
         }
 
         return parent::doLoadValuesForChoices($choices);
@@ -76,13 +90,16 @@ protected function doLoadValuesForChoices(array $choices): array
      */
     protected function doLoadChoicesForValues(array $values, ?callable $value): array
     {
+        $legacy = $this->idReader && null === $value;
+
+        if ($legacy) {
+            @trigger_error(sprintf('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 "%s" or define the choice_value instead.', __CLASS__), E_USER_DEPRECATED);
+        }
+
         // 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->objectLoader) {
+        if (($legacy || \is_array($value) && $this->idReader === $value[0]) && $this->objectLoader) {
             $objectsById = [];
-            $objects = [];
 
             // Maintain order and indices from the given $values
             // An alternative approach to the following loop is to add the
@@ -98,7 +115,7 @@ protected function doLoadChoicesForValues(array $values, ?callable $value): arra
                 }
             }
 
-            return $objects;
+            return $objects ?? [];
         }
 
         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 string|null The ID value
+     * @return string The ID value
      */
     public function getIdValue(object $object = null)
     {
         if (!$object) {
-            return null;
+            return '';
         }
 
         if (!$this->om->contains($object)) {

diff --git a/src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php b/src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php
@@ -190,6 +190,11 @@ public function testLoadValuesForChoicesDoesNotLoadIfEmptyChoices()
         $this->assertSame([], $loader->loadValuesForChoices([]));
     }
 
+    /**
+     * @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" option instead.
+     */
     public function testLoadValuesForChoicesDoesNotLoadIfSingleIntId()
     {
         $loader = new DoctrineChoiceLoader(
@@ -258,8 +263,7 @@ public function testLoadChoicesForValues()
     {
         $loader = new DoctrineChoiceLoader(
             $this->om,
-            $this->class,
-            $this->idReader
+            $this->class
         );
 
         $choices = [$this->obj1, $this->obj2, $this->obj3];
@@ -289,7 +293,12 @@ public function testLoadChoicesForValuesDoesNotLoadIfEmptyValues()
         $this->assertSame([], $loader->loadChoicesForValues([]));
     }
 
-    public function testLoadChoicesForValuesLoadsOnlyChoicesIfValueUseIdReader()
+    /**
+     * @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,
@@ -325,6 +334,42 @@ public function testLoadChoicesForValuesLoadsOnlyChoicesIfValueUseIdReader()
         ));
     }
 
+    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(