From 7ae847371585311406c3c1b3306e748ac8e7b93d Mon Sep 17 00:00:00 2001 From: Javier Spagnoletti Date: Sun, 23 Aug 2020 21:55:21 -0300 Subject: [PATCH] Leverage `ModelManagerInterface::findBy()` --- .../ModelToIdPropertyTransformer.php | 16 ++-- .../ModelsToArrayTransformer.php | 30 +++---- src/Util/TraversableToCollection.php | 55 ++++++++++++ .../ModelToIdPropertyTransformerTest.php | 59 +++++++------ .../ModelsToArrayTransformerTest.php | 87 +++++++++++++++++++ tests/Util/TraversableToCollectionTest.php | 71 +++++++++++++++ 6 files changed, 268 insertions(+), 50 deletions(-) create mode 100644 src/Util/TraversableToCollection.php create mode 100644 tests/Util/TraversableToCollectionTest.php diff --git a/src/Form/DataTransformer/ModelToIdPropertyTransformer.php b/src/Form/DataTransformer/ModelToIdPropertyTransformer.php index 2b945b8192f..093ced2dbed 100644 --- a/src/Form/DataTransformer/ModelToIdPropertyTransformer.php +++ b/src/Form/DataTransformer/ModelToIdPropertyTransformer.php @@ -17,6 +17,7 @@ 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; /** @@ -89,12 +90,9 @@ public function __construct( */ public function reverseTransform($value) { - /** @phpstan-var ArrayCollection $collection */ - $collection = new ArrayCollection(); - if (empty($value)) { if ($this->multiple) { - return $collection; + return new ArrayCollection(); } return null; @@ -110,13 +108,19 @@ public function reverseTransform($value) foreach ($value as $key => $id) { if ('_labels' === $key) { + unset($value[$key]); + continue; } - $collection->add($this->modelManager->find($this->className, $id)); + $value[$key] = (string) $id; } - return $collection; + $query = $this->modelManager->createQuery($this->className); + $this->modelManager->addIdentifiersToQuery($this->className, $query, $value); + $result = $this->modelManager->executeQuery($query); + + return TraversableToCollection::transform($result); } /** diff --git a/src/Form/DataTransformer/ModelsToArrayTransformer.php b/src/Form/DataTransformer/ModelsToArrayTransformer.php index c8e87bbb47d..86c17df438f 100644 --- a/src/Form/DataTransformer/ModelsToArrayTransformer.php +++ b/src/Form/DataTransformer/ModelsToArrayTransformer.php @@ -18,6 +18,7 @@ use Doctrine\Common\Util\ClassUtils; use Sonata\AdminBundle\Form\ChoiceList\ModelChoiceLoader; use Sonata\AdminBundle\Model\ModelManagerInterface; +use Sonata\AdminBundle\Util\TraversableToCollection; use Sonata\Doctrine\Adapter\AdapterInterface; use Symfony\Component\Form\ChoiceList\LazyChoiceList; use Symfony\Component\Form\DataTransformerInterface; @@ -55,8 +56,6 @@ class ModelsToArrayTransformer implements DataTransformerInterface protected $choiceList; /** - * ModelsToArrayTransformer constructor. - * * @param LazyChoiceList|ModelChoiceLoader $choiceList * @param ModelManagerInterface $modelManager * @param string $class @@ -168,23 +167,22 @@ public function reverseTransform($value) throw new UnexpectedTypeException($value, 'array'); } + $value = array_map('strval', $value); + + $query = $this->modelManager->createQuery($this->class); + $this->modelManager->addIdentifiersToQuery($this->class, $query, $value); + $result = $this->modelManager->executeQuery($query); + /** @phpstan-var ArrayCollection $collection */ - $collection = new ArrayCollection(); - $notFound = []; - - // optimize this into a SELECT WHERE IN query - foreach ($value as $key) { - if ($model = $this->modelManager->find($this->class, $key)) { - $collection->add($model); - } else { - $notFound[] = $key; - } - } + $collection = TraversableToCollection::transform($result); + + $diffCount = \count($value) - $collection->count(); - if (\count($notFound) > 0) { + if (0 !== $diffCount) { throw new TransformationFailedException(sprintf( - 'The entities with keys "%s" could not be found', - implode('", "', $notFound) + '%u keys could not be found in the provided values: "%s".', + $diffCount, + implode('", "', $value) )); } diff --git a/src/Util/TraversableToCollection.php b/src/Util/TraversableToCollection.php new file mode 100644 index 00000000000..716f53aeffe --- /dev/null +++ b/src/Util/TraversableToCollection.php @@ -0,0 +1,55 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Sonata\AdminBundle\Util; + +use Doctrine\Common\Collections\ArrayCollection; +use Doctrine\Common\Collections\Collection; + +/** + * @author Javier Spagnoletti + */ +final class TraversableToCollection +{ + /** + * @param mixed $value + * + * @throws \TypeError + * + * @return Collection + * + * @phpstan-return Collection + */ + public static function transform($value): Collection + { + if ($value instanceof Collection) { + return $value; + } + + if ($value instanceof \Traversable) { + return new ArrayCollection(iterator_to_array($value)); + } + + if (\is_array($value)) { + return new ArrayCollection($value); + } + + throw new \TypeError(sprintf( + 'Argument 1 passed to "%s()" must be of type "%s" or "%s", %s given.', + __METHOD__, + \Traversable::class, + 'array', + \is_object($value) ? 'instance of "'.\get_class($value).'"' : '"'.\gettype($value).'"' + )); + } +} diff --git a/tests/Form/DataTransformer/ModelToIdPropertyTransformerTest.php b/tests/Form/DataTransformer/ModelToIdPropertyTransformerTest.php index 455569c43ee..ec0bef3dc8a 100644 --- a/tests/Form/DataTransformer/ModelToIdPropertyTransformerTest.php +++ b/tests/Form/DataTransformer/ModelToIdPropertyTransformerTest.php @@ -14,7 +14,9 @@ namespace Sonata\AdminBundle\Tests\Form\DataTransformer; use Doctrine\Common\Collections\ArrayCollection; +use Doctrine\Common\Collections\Collection; use PHPUnit\Framework\TestCase; +use Sonata\AdminBundle\Datagrid\ProxyQueryInterface; use Sonata\AdminBundle\Form\DataTransformer\ModelToIdPropertyTransformer; use Sonata\AdminBundle\Model\ModelManagerInterface; use Sonata\AdminBundle\Tests\Fixtures\Entity\Foo; @@ -58,40 +60,43 @@ public function testReverseTransform(): void */ public function testReverseTransformMultiple(array $expected, $params, Foo $entity1, Foo $entity2, Foo $entity3): void { - $transformer = new ModelToIdPropertyTransformer($this->modelManager, Foo::class, 'bar', true); - - $this->modelManager - ->method('find') - ->willReturnCallback(static function (string $className, int $value) use ($entity1, $entity2, $entity3) { - if (Foo::class !== $className) { - return; + $modelManager = $this->createMock(ModelManagerInterface::class); + $transformer = new ModelToIdPropertyTransformer($modelManager, Foo::class, 'bar', true); + $proxyQuery = $this->createMock(ProxyQueryInterface::class); + $modelManager + ->expects($this->exactly($params ? 1 : 0)) + ->method('createQuery') + ->with($this->equalTo(Foo::class)) + ->willReturn($proxyQuery); + $modelManager + ->expects($this->exactly($params ? 1 : 0)) + ->method('executeQuery') + ->with($this->equalTo($proxyQuery)) + ->willReturnCallback(static function (ProxyQueryInterface $query) use ($params, $entity1, $entity2, $entity3): array { + $collection = []; + + if (\in_array(123, $params, true)) { + $collection[] = $entity1; } - if (123 === $value) { - return $entity1; + if (\in_array(456, $params, true)) { + $collection[] = $entity2; } - if (456 === $value) { - return $entity2; + if (\in_array(789, $params, true)) { + $collection[] = $entity3; } - if (789 === $value) { - return $entity3; - } + return $collection; }); - $collection = new ArrayCollection(); - $this->modelManager - ->method('getModelCollectionInstance') - ->with($this->equalTo(Foo::class)) - ->willReturn($collection); - $result = $transformer->reverseTransform($params); - $this->assertInstanceOf(ArrayCollection::class, $result); + $this->assertInstanceOf(Collection::class, $result); + $this->assertCount(\count($expected), $result); $this->assertSame($expected, $result->getValues()); } - public function getReverseTransformMultipleTests() + public function getReverseTransformMultipleTests(): iterable { $entity1 = new Foo(); $entity1->setBaz(123); @@ -105,12 +110,10 @@ public function getReverseTransformMultipleTests() $entity3->setBaz(789); $entity3->setBar('example3'); - return [ - [[], null, $entity1, $entity2, $entity3], - [[], false, $entity1, $entity2, $entity3], - [[$entity1], [123, '_labels' => ['example']], $entity1, $entity2, $entity3], - [[$entity1, $entity2, $entity3], [123, 456, 789, '_labels' => ['example', 'example2', 'example3']], $entity1, $entity2, $entity3], - ]; + yield [[], null, $entity1, $entity2, $entity3]; + yield [[], false, $entity1, $entity2, $entity3]; + yield [[$entity1], [123, '_labels' => ['example']], $entity1, $entity2, $entity3]; + yield [[$entity1, $entity2, $entity3], [123, 456, 789, '_labels' => ['example', 'example2', 'example3']], $entity1, $entity2, $entity3]; } /** diff --git a/tests/Form/DataTransformer/ModelsToArrayTransformerTest.php b/tests/Form/DataTransformer/ModelsToArrayTransformerTest.php index 770002e78d5..83463f607bc 100644 --- a/tests/Form/DataTransformer/ModelsToArrayTransformerTest.php +++ b/tests/Form/DataTransformer/ModelsToArrayTransformerTest.php @@ -13,11 +13,15 @@ namespace Sonata\AdminBundle\Tests\Form\DataTransformer; +use Doctrine\Common\Collections\Collection; use PHPUnit\Framework\TestCase; +use Sonata\AdminBundle\Datagrid\ProxyQueryInterface; use Sonata\AdminBundle\Form\ChoiceList\ModelChoiceLoader; use Sonata\AdminBundle\Form\DataTransformer\ModelsToArrayTransformer; use Sonata\AdminBundle\Model\ModelManagerInterface; use Sonata\AdminBundle\Tests\Fixtures\Entity\Foo; +use Symfony\Component\Form\Exception\TransformationFailedException; +use Symfony\Component\Form\Exception\UnexpectedTypeException; class ModelsToArrayTransformerTest extends TestCase { @@ -44,4 +48,87 @@ public function testLegacyConstructor(): void $this->assertInstanceOf(ModelsToArrayTransformer::class, $transformer); } + + /** + * @dataProvider reverseTransformProvider + */ + public function testReverseTransform(?array $value): void + { + $modelManager = $this->createStub(ModelManagerInterface::class); + + if (null !== $value) { + $proxyQuery = $this->createStub(ProxyQueryInterface::class); + $modelManager + ->method('createQuery') + ->with($this->equalTo(Foo::class)) + ->willReturn($proxyQuery); + $modelManager + ->method('executeQuery') + ->with($this->equalTo($proxyQuery)) + ->willReturn($value); + } + + $transformer = new ModelsToArrayTransformer( + $modelManager, + Foo::class + ); + + $result = $transformer->reverseTransform($value); + + if (null === $value) { + $this->assertNull($result); + } else { + $this->assertInstanceOf(Collection::class, $result); + $this->assertCount(\count($value), $result); + } + } + + public function reverseTransformProvider(): iterable + { + yield [['a']]; + yield [['a', 'b', 3]]; + yield [null]; + } + + public function testReverseTransformUnexpectedType(): void + { + $value = 'unexpected'; + $modelManager = $this->createStub(ModelManagerInterface::class); + + $transformer = new ModelsToArrayTransformer( + $modelManager, + Foo::class + ); + + $this->expectException(UnexpectedTypeException::class); + $this->expectExceptionMessage('Expected argument of type "array", "string" given'); + + $transformer->reverseTransform($value); + } + + public function testReverseTransformFailed(): void + { + $value = ['a', 'b']; + $reverseTransformCollection = ['a']; + $modelManager = $this->createStub(ModelManagerInterface::class); + $proxyQuery = $this->createStub(ProxyQueryInterface::class); + $modelManager + ->method('createQuery') + ->with($this->equalTo(Foo::class)) + ->willReturn($proxyQuery); + $modelManager + ->method('executeQuery') + ->with($this->equalTo($proxyQuery)) + ->willReturn($reverseTransformCollection); + + $transformer = new ModelsToArrayTransformer( + $modelManager, + Foo::class + ); + + $this->expectException(TransformationFailedException::class); + $this->expectExceptionMessage('1 keys could not be found in the provided values: "a", "b".'); + + $transformer->reverseTransform($value); + } } diff --git a/tests/Util/TraversableToCollectionTest.php b/tests/Util/TraversableToCollectionTest.php new file mode 100644 index 00000000000..27fb7d2a93f --- /dev/null +++ b/tests/Util/TraversableToCollectionTest.php @@ -0,0 +1,71 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Sonata\AdminBundle\Tests\Util; + +use Doctrine\Common\Collections\ArrayCollection; +use Doctrine\Common\Collections\Collection; +use PHPUnit\Framework\TestCase; +use Sonata\AdminBundle\Util\TraversableToCollection; + + +/** + * @author Javier Spagnoletti + */ +class TraversableToCollectionTest extends TestCase +{ + /** + * @dataProvider provideTraversableValues + */ + public function testTransform(int $expectedCount, $value): void + { + $collection = TraversableToCollection::transform($value); + + $this->assertInstanceOf(Collection::class, $collection); + $this->assertCount($expectedCount, $collection); + } + + public function provideTraversableValues(): iterable + { + yield [0, []]; + yield [1, [null]]; + yield [1, [0]]; + yield [1, ['a']]; + yield [3, ['a', 'b', 'other_offset' => 'c']]; + yield [2, (static function () { yield from ['d', 'e']; })()]; + yield [4, new ArrayCollection(['f', 'g', 'h', 'i'])]; + } + + /** + * @dataProvider provideInvalidValues + */ + public function testFailedTransform(string $invalidType, $value): void + { + $this->expectException(\TypeError::class); + $this->expectExceptionMessage(sprintf( + 'Argument 1 passed to "Sonata\AdminBundle\Util\TraversableToCollection::transform()" must be of type "Traversable" or "array", %s given.', + $invalidType + )); + + TraversableToCollection::transform($value); + } + + public function provideInvalidValues(): iterable + { + yield ['"NULL"', null]; + yield ['"integer"', 0]; + yield ['"integer"', 1]; + yield ['"string"', 'a']; + yield ['instance of "stdClass"', new \stdClass()]; + } +}