Respect order of values provider in ModelsToArrayTransformer

phpstan-baseline.neon

@@ -13,7 +13,7 @@ parameters:
             message: '#^Method Sonata\\AdminBundle\\Tests\\Fixtures\\Entity\\FooToStringNull\:\:__toString\(\) should return string but returns null\.$#'
             path: tests/Fixtures/Entity/FooToStringNull.php
         - # The phpstan-param is less precise than the psalm-param https://github.com/phpstan/phpstan/issues/4703
-            message: '#^Parameter \#3 \$idx of method Sonata\\AdminBundle\\Model\\ModelManagerInterface\<T of object\>\:\:addIdentifiersToQuery\(\) expects non-empty-array\<int\|string\>, non-empty-array\<array\<string\>\|int\|string\> given\.$#'
+            message: '#^Parameter \#1 \$value of method Sonata\\AdminBundle\\Form\\DataTransformer\\ModelsToArrayTransformer\<T of object\>\:\:reverseTransform\(\) expects array\<int\|string\>\|null, array\<array\<string\>\|int\|string\> given\.$#'
             path: src/Form/DataTransformer/ModelToIdPropertyTransformer.php
         - # `treatPhpdocAsCertain: false` should not report this https://github.com/phpstan/phpstan/issues/5333
             message: '#^Instanceof between Symfony\\Component\\Routing\\Route and Symfony\\Component\\Routing\\Route will always evaluate to true\.$#'

src/Form/DataTransformer/ModelToIdPropertyTransformer.php

@@ -17,7 +17,6 @@
 use Doctrine\Common\Collections\Collection;
 use Doctrine\Common\Util\ClassUtils;
 use Sonata\AdminBundle\Model\ModelManagerInterface;
-use Sonata\AdminBundle\Util\TraversableToCollection;
 use Symfony\Component\Form\DataTransformerInterface;
 
 /**
@@ -118,15 +117,7 @@ public function reverseTransform($value)
 
         unset($value['_labels']);
 
-        if ([] === $value) {
-            $result = $value;
-        } else {
-            $query = $this->modelManager->createQuery($this->className);
-            $this->modelManager->addIdentifiersToQuery($this->className, $query, $value);
-            $result = $this->modelManager->executeQuery($query);
-        }
-
-        return TraversableToCollection::transform($result);
+        return (new ModelsToArrayTransformer($this->modelManager, $this->className))->reverseTransform($value);
     }
 
     /**

src/Form/DataTransformer/ModelsToArrayTransformer.php

@@ -13,11 +13,10 @@
 
 namespace Sonata\AdminBundle\Form\DataTransformer;
 
+use Doctrine\Common\Collections\ArrayCollection;
 use Doctrine\Common\Collections\Collection;
 use Doctrine\Common\Util\ClassUtils;
 use Sonata\AdminBundle\Model\ModelManagerInterface;
-use Sonata\AdminBundle\Util\TraversableToCollection;
-use Sonata\Doctrine\Adapter\AdapterInterface;
 use Symfony\Component\Form\DataTransformerInterface;
 use Symfony\Component\Form\Exception\TransformationFailedException;
 use Symfony\Component\Form\Exception\UnexpectedTypeException;
@@ -68,7 +67,15 @@ public function transform($value): array
 
         $array = [];
         foreach ($value as $model) {
-            $array[] = implode(AdapterInterface::ID_SEPARATOR, $this->getIdentifierValues($model));
+            $identifier = $this->modelManager->getNormalizedIdentifier($model);
+            if (null === $identifier) {
+                throw new TransformationFailedException(sprintf(
+                    'No identifier was found for the model "%s".',
+                    ClassUtils::getClass($model)
+                ));
+            }
+
+            $array[] = $identifier;
         }
 
         return $array;
@@ -94,42 +101,38 @@ public function reverseTransform($value)
         }
 
         if ([] === $value) {
-            $result = $value;
-        } else {
-            $query = $this->modelManager->createQuery($this->class);
-            $this->modelManager->addIdentifiersToQuery($this->class, $query, $value);
-            $result = $this->modelManager->executeQuery($query);
+            return new ArrayCollection();
         }
 
-        $collection = TraversableToCollection::transform($result);
-
-        $diffCount = \count($value) - $collection->count();
-
-        if (0 !== $diffCount) {
-            throw new TransformationFailedException(sprintf(
-                '%u keys could not be found in the provided values: "%s".',
-                $diffCount,
-                implode('", "', $value)
-            ));
+        $query = $this->modelManager->createQuery($this->class);
+        $this->modelManager->addIdentifiersToQuery($this->class, $query, $value);
+        $queryResult = $this->modelManager->executeQuery($query);
+
+        $modelsById = [];
+        foreach ($queryResult as $model) {
+            $identifier = $this->modelManager->getNormalizedIdentifier($model);
+            if (null === $identifier) {
+                throw new TransformationFailedException(sprintf(
+                    'No identifier was found for the model "%s".',
+                    ClassUtils::getClass($model)
+                ));
+            }
+
+            $modelsById[$identifier] = $model;
         }
 
-        return $collection;
-    }
+        $result = [];
+        foreach ($value as $identifier) {
+            if (!isset($modelsById[$identifier])) {
+                throw new TransformationFailedException(sprintf(
+                    'No model was found for the identifier "%s".',
+                    $identifier,
+                ));
+            }
 
-    /**
-     * @return array<int|string>
-     *
-     * @phpstan-param T $model
-     */
-    private function getIdentifierValues(object $model): array
-    {
-        try {
-            return $this->modelManager->getIdentifierValues($model);
-        } catch (\Exception $e) {
-            throw new \InvalidArgumentException(sprintf(
-                'Unable to retrieve the identifier values for entity %s',
-                ClassUtils::getClass($model)
-            ), 0, $e);
+            $result[] = $modelsById[$identifier];
         }
+
+        return new ArrayCollection($result);
     }
 }

tests/Form/DataTransformer/ModelToIdPropertyTransformerTest.php

@@ -96,6 +96,13 @@ public function testReverseTransformMultiple(array $expected, $params, Foo $enti
 
                 return $collection;
             });
+        $modelManager
+            ->method('getNormalizedIdentifier')
+            ->willReturnMap([
+                [$entity1, '123'],
+                [$entity2, '456'],
+                [$entity3, '789'],
+            ]);
 
         $result = $transformer->reverseTransform($params);
         static::assertInstanceOf(Collection::class, $result);

tests/Form/DataTransformer/ModelsToArrayTransformerTest.php

@@ -18,6 +18,7 @@
 use Sonata\AdminBundle\Datagrid\ProxyQueryInterface;
 use Sonata\AdminBundle\Form\DataTransformer\ModelsToArrayTransformer;
 use Sonata\AdminBundle\Model\ModelManagerInterface;
+use Sonata\AdminBundle\Tests\Fixtures\Entity\Entity;
 use Sonata\AdminBundle\Tests\Fixtures\Entity\Foo;
 use Symfony\Component\Form\Exception\TransformationFailedException;
 use Symfony\Component\Form\Exception\UnexpectedTypeException;
@@ -34,50 +35,16 @@ public function testConstructor(): void
         static::assertInstanceOf(ModelsToArrayTransformer::class, $transformer);
     }
 
-    /**
-     * @param array<int|string>|null $value
-     *
-     * @dataProvider reverseTransformProvider
-     */
-    public function testReverseTransform(?array $value): void
+    public function testReverseTransformWithNull(): void
     {
         $modelManager = $this->createMock(ModelManagerInterface::class);
 
-        if (null !== $value) {
-            $proxyQuery = $this->createStub(ProxyQueryInterface::class);
-            $modelManager
-                ->method('createQuery')
-                ->with(static::equalTo(Foo::class))
-                ->willReturn($proxyQuery);
-            $modelManager
-                ->method('executeQuery')
-                ->with(static::equalTo($proxyQuery))
-                ->willReturn($value);
-        }
-
         $transformer = new ModelsToArrayTransformer(
             $modelManager,
             Foo::class
         );
 
-        $result = $transformer->reverseTransform($value);
-
-        if (null === $value) {
-            static::assertNull($result);
-        } else {
-            static::assertInstanceOf(Collection::class, $result);
-            static::assertCount(\count($value), $result);
-        }
-    }
-
-    /**
-     * @phpstan-return iterable<array-key, array{array<int|string>|null}>
-     */
-    public function reverseTransformProvider(): iterable
-    {
-        yield [['a']];
-        yield [['a', 'b', 3]];
-        yield [null];
+        static::assertNull($transformer->reverseTransform(null));
     }
 
     public function testReverseTransformWithEmptyArray(): void
@@ -104,6 +71,39 @@ public function testReverseTransformWithEmptyArray(): void
         static::assertCount(0, $result);
     }
 
+    public function testReverseTransformRespectOrder(): void
+    {
+        $object1 = new Entity(1);
+        $object2 = new Entity(2);
+        $object3 = new Entity(3);
+
+        $proxyQuery = $this->createStub(ProxyQueryInterface::class);
+        $modelManager = $this->createMock(ModelManagerInterface::class);
+        $modelManager
+            ->method('createQuery')
+            ->willReturn($proxyQuery);
+        $modelManager
+            ->method('executeQuery')
+            ->willReturn([$object1, $object2, $object3]);
+        $modelManager
+            ->method('getNormalizedIdentifier')
+            ->willReturnMap([
+                [$object1, '1'],
+                [$object2, '2'],
+                [$object3, '3'],
+            ]);
+
+        $transformer = new ModelsToArrayTransformer(
+            $modelManager,
+            Entity::class
+        );
+
+        $result = $transformer->reverseTransform([1, 3, 2]);
+
+        static::assertInstanceOf(Collection::class, $result);
+        static::assertSame([$object1, $object3, $object2], $result->toArray());
+    }
+
     /**
      * @psalm-suppress InvalidArgument
      */
@@ -126,8 +126,9 @@ public function testReverseTransformUnexpectedType(): void
 
     public function testReverseTransformFailed(): void
     {
-        $value = ['a', 'b'];
-        $reverseTransformCollection = ['a'];
+        $object1 = new Entity(1);
+        $object2 = new Entity(2);
+
         $modelManager = $this->createMock(ModelManagerInterface::class);
         $proxyQuery = $this->createStub(ProxyQueryInterface::class);
         $modelManager
@@ -137,16 +138,22 @@ public function testReverseTransformFailed(): void
         $modelManager
             ->method('executeQuery')
             ->with(static::equalTo($proxyQuery))
-            ->willReturn($reverseTransformCollection);
+            ->willReturn([$object1]);
+        $modelManager
+            ->method('getNormalizedIdentifier')
+            ->willReturnMap([
+                [$object1, '1'],
+                [$object2, '2'],
+            ]);
 
         $transformer = new ModelsToArrayTransformer(
             $modelManager,
             Foo::class
         );
 
         $this->expectException(TransformationFailedException::class);
-        $this->expectExceptionMessage('1 keys could not be found in the provided values: "a", "b".');
+        $this->expectExceptionMessage('No model was found for the identifier "2".');
 
-        $transformer->reverseTransform($value);
+        $transformer->reverseTransform([1, 2]);
     }
 }