minor #30966 [DoctrineBridge] Deprecated using IdReader when optimiza…

…tion is not possible (HeahDude)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[DoctrineBridge] Deprecated using IdReader when optimization is not possible

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | yes <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | ~
| License       | MIT
| Doc PR        | ~

Follow up of #30962. (Review only the second commit until #30962 is merged).

Commits
-------

a234c89 [DoctrineBridge] Deprecated using IdReader when optimization is not possible

UPGRADE-4.3.md

@@ -38,6 +38,12 @@ DependencyInjection
           env(NAME): '1.5'
    ```
 
+Doctrine Bridge
+---------------
+
+ * Passing an `IdReader` to the `DoctrineChoiceLoader` when the query cannot be optimized with single id field has been deprecated, pass `null` instead
+ * Not passing an `IdReader` to the `DoctrineChoiceLoader` when the query can be optimized with single id field has been deprecated
+
 EventDispatcher
 ---------------
 

UPGRADE-5.0.md

@@ -60,6 +60,9 @@ DoctrineBridge
 
  * Deprecated injecting `ClassMetadataFactory` in `DoctrineExtractor`, an instance of `EntityManagerInterface` should be
    injected instead
+ * Passing an `IdReader` to the `DoctrineChoiceLoader` when the query cannot be optimized with single id field will throw an exception, pass `null` instead
+ * Not passing an `IdReader` to the `DoctrineChoiceLoader` when the query can be optimized with single id field will throw an exception
+
 
 DomCrawler
 ----------

src/Symfony/Bridge/Doctrine/CHANGELOG.md

@@ -6,6 +6,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
 
 4.2.0
 -----

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

@@ -42,18 +42,25 @@ class DoctrineChoiceLoader implements ChoiceLoaderInterface
      *
      * @param ObjectManager              $manager      The object manager
      * @param string                     $class        The class name of the loaded objects
-     * @param IdReader                   $idReader     The reader for the object IDs
+     * @param IdReader|null              $idReader     The reader for the object IDs
      * @param EntityLoaderInterface|null $objectLoader The objects loader
      */
     public function __construct(ObjectManager $manager, string $class, IdReader $idReader = null, EntityLoaderInterface $objectLoader = null)
     {
         $classMetadata = $manager->getClassMetadata($class);
 
+        if ($idReader && !$idReader->isSingleId()) {
+            @trigger_error(sprintf('Passing an instance of "%s" to "%s" with an entity class "%s" that has a composite id is deprecated since Symfony 4.3 and will throw an exception in 5.0.', IdReader::class, __CLASS__, $class), E_USER_DEPRECATED);
+
+            // In Symfony 5.0
+            // throw new \InvalidArgumentException(sprintf('The second argument `$idReader` of "%s" must be null when the query cannot be optimized because of composite id fields.', __METHOD__));
+        }
+
         if ((5 > \func_num_args() || false !== func_get_arg(4)) && null === $idReader) {
             $idReader = new IdReader($manager, $classMetadata);
 
             if ($idReader->isSingleId()) {
-                @trigger_error(sprintf('Not explicitly passing an instance of "%s" when it can optimize single id entity "%s" has been deprecated in 4.3 and will not apply any optimization in 5.0.', IdReader::class, $class), E_USER_DEPRECATED);
+                @trigger_error(sprintf('Not explicitly passing an instance of "%s" to "%s" when it can optimize single id entity "%s" has been deprecated in 4.3 and will not apply any optimization in 5.0.', IdReader::class, __CLASS__, $class), E_USER_DEPRECATED);
             } else {
                 $idReader = null;
             }
@@ -93,7 +100,7 @@ public function loadValuesForChoices(array $choices, $value = null)
 
         // Optimize performance for single-field identifiers. We already
         // know that the IDs are used as values
-        $optimize = null === $value || \is_array($value) && $value[0] === $this->idReader;
+        $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()) {

src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php

@@ -163,13 +163,10 @@ public function configureOptions(OptionsResolver $resolver)
         };
 
         $choiceName = function (Options $options) {
-            /** @var IdReader $idReader */
-            $idReader = $options['id_reader'];
-
             // If the object has a single-column, numeric ID, use that ID as
             // field name. We can only use numeric IDs as names, as we cannot
             // guarantee that a non-numeric ID contains a valid form name
-            if ($idReader->isIntId()) {
+            if ($options['id_reader'] instanceof IdReader && $options['id_reader']->isIntId()) {
                 return [__CLASS__, 'createChoiceName'];
             }
 
@@ -181,12 +178,9 @@ public function configureOptions(OptionsResolver $resolver)
         // are indexed by an incrementing integer.
         // Use the ID/incrementing integer as choice value.
         $choiceValue = function (Options $options) {
-            /** @var IdReader $idReader */
-            $idReader = $options['id_reader'];
-
             // If the entity has a single-column ID, use that ID as value
-            if ($idReader->isSingleId()) {
-                return [$idReader, 'getIdValue'];
+            if ($options['id_reader'] instanceof IdReader && $options['id_reader']->isSingleId()) {
+                return [$options['id_reader'], 'getIdValue'];
             }
 
             // Otherwise, an incrementing integer is used as value automatically
@@ -240,7 +234,11 @@ public function configureOptions(OptionsResolver $resolver)
                 $this->idReaders[$hash] = new IdReader($options['em'], $classMetadata);
             }
 
-            return $this->idReaders[$hash];
+            if ($this->idReaders[$hash]->isSingleId()) {
+                return $this->idReaders[$hash];
+            }
+
+            return null;
         };
 
         $resolver->setDefaults([

src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php

@@ -81,6 +81,11 @@ protected function setUp()
         $this->idReader = $this->getMockBuilder('Symfony\Bridge\Doctrine\Form\ChoiceList\IdReader')
             ->disableOriginalConstructor()
             ->getMock();
+        $this->idReader->expects($this->any())
+            ->method('isSingleId')
+            ->willReturn(true)
+        ;
+
         $this->objectLoader = $this->getMockBuilder('Symfony\Bridge\Doctrine\Form\ChoiceList\EntityLoaderInterface')->getMock();
         $this->obj1 = (object) ['name' => 'A'];
         $this->obj2 = (object) ['name' => 'B'];
@@ -151,7 +156,7 @@ public function testLoadValuesForChoices()
         $loader = new DoctrineChoiceLoader(
             $this->om,
             $this->class,
-            $this->idReader
+            null
         );
 
         $choices = [$this->obj1, $this->obj2, $this->obj3];
@@ -189,10 +194,6 @@ public function testLoadValuesForChoicesDoesNotLoadIfSingleIntId()
             $this->idReader
         );
 
-        $this->idReader->expects($this->any())
-            ->method('isSingleId')
-            ->willReturn(true);
-
         $this->repository->expects($this->never())
             ->method('findAll');
 
@@ -215,10 +216,6 @@ public function testLoadValuesForChoicesLoadsIfSingleIntIdAndValueGiven()
         $choices = [$this->obj1, $this->obj2, $this->obj3];
         $value = function (\stdClass $object) { return $object->name; };
 
-        $this->idReader->expects($this->any())
-            ->method('isSingleId')
-            ->willReturn(true);
-
         $this->repository->expects($this->once())
             ->method('findAll')
             ->willReturn($choices);
@@ -239,10 +236,6 @@ public function testLoadValuesForChoicesDoesNotLoadIfValueIsIdReader()
 
         $value = [$this->idReader, 'getIdValue'];
 
-        $this->idReader->expects($this->any())
-            ->method('isSingleId')
-            ->willReturn(true);
-
         $this->repository->expects($this->never())
             ->method('findAll');
 
@@ -303,10 +296,6 @@ public function testLoadChoicesForValuesLoadsOnlyChoicesIfSingleIntId()
 
         $choices = [$this->obj2, $this->obj3];
 
-        $this->idReader->expects($this->any())
-            ->method('isSingleId')
-            ->willReturn(true);
-
         $this->idReader->expects($this->any())
             ->method('getIdField')
             ->willReturn('idField');
@@ -343,10 +332,6 @@ public function testLoadChoicesForValuesLoadsAllIfSingleIntIdAndValueGiven()
         $choices = [$this->obj1, $this->obj2, $this->obj3];
         $value = function (\stdClass $object) { return $object->name; };
 
-        $this->idReader->expects($this->any())
-            ->method('isSingleId')
-            ->willReturn(true);
-
         $this->repository->expects($this->once())
             ->method('findAll')
             ->willReturn($choices);
@@ -369,10 +354,6 @@ public function testLoadChoicesForValuesLoadsOnlyChoicesIfValueIsIdReader()
         $choices = [$this->obj2, $this->obj3];
         $value = [$this->idReader, 'getIdValue'];
 
-        $this->idReader->expects($this->any())
-            ->method('isSingleId')
-            ->willReturn(true);
-
         $this->idReader->expects($this->any())
             ->method('getIdField')
             ->willReturn('idField');
@@ -398,7 +379,7 @@ public function testLoadChoicesForValuesLoadsOnlyChoicesIfValueIsIdReader()
     /**
      * @group legacy
      *
-     * @expectedDeprecation Not explicitly passing an instance of "Symfony\Bridge\Doctrine\Form\ChoiceList\IdReader" when it can optimize single id entity "%s" has been deprecated in 4.3 and will not apply any optimization in 5.0.
+     * @expectedDeprecation Not explicitly passing an instance of "Symfony\Bridge\Doctrine\Form\ChoiceList\IdReader" to "Symfony\Bridge\Doctrine\Form\ChoiceList\DoctrineChoiceLoader" when it can optimize single id entity "%s" has been deprecated in 4.3 and will not apply any optimization in 5.0.
      */
     public function testLoaderWithoutIdReaderCanBeOptimized()
     {
@@ -445,14 +426,6 @@ public function testLoaderWithoutIdReaderCanBeOptimized()
 
         $choices = [$obj1, $obj2];
 
-        $this->idReader->expects($this->any())
-            ->method('isSingleId')
-            ->willReturn(true);
-
-        $this->idReader->expects($this->any())
-            ->method('getIdField')
-            ->willReturn('idField');
-
         $this->repository->expects($this->never())
             ->method('findAll');
 
@@ -461,13 +434,29 @@ public function testLoaderWithoutIdReaderCanBeOptimized()
             ->with('idField', ['1'])
             ->willReturn($choices);
 
-        $this->idReader->expects($this->any())
-            ->method('getIdValue')
-            ->willReturnMap([
-                [$obj1, '1'],
-                [$obj2, '2'],
-            ]);
-
         $this->assertSame([$obj1], $loader->loadChoicesForValues(['1']));
     }
+
+    /**
+     * @group legacy
+     *
+     * @deprecationMessage Passing an instance of "Symfony\Bridge\Doctrine\Form\ChoiceList\IdReader" to "Symfony\Bridge\Doctrine\Form\ChoiceList\DoctrineChoiceLoader" with an entity class "stdClass" that has a composite id is deprecated since Symfony 4.3 and will throw an exception in 5.0.
+     */
+    public function testPassingIdReaderWithoutSingleIdEntity()
+    {
+        $idReader = $this->createMock(IdReader::class);
+        $idReader->expects($this->once())
+            ->method('isSingleId')
+            ->willReturn(false)
+        ;
+
+        $loader = new DoctrineChoiceLoader(
+            $this->om,
+            $this->class,
+            $idReader,
+            $this->objectLoader
+        );
+
+        $this->assertInstanceOf(DoctrineChoiceLoader::class, $loader);
+    }
 }