Merge pull request #1967 from gorghoa/feat/1799-handle-immutable-resource-on-persist

teohhanhui · web-flow · commit dcc973654aff · 2018-05-24T19:16:40.000+02:00
Allow DataPersister to return a new instance instead of mutating
diff --git a/src/Bridge/Doctrine/Common/DataPersister.php b/src/Bridge/Doctrine/Common/DataPersister.php
@@ -48,7 +48,7 @@ public function supports($data): bool
     public function persist($data)
     {
         if (!$manager = $this->getManager($data)) {
-            return;
+            return $data;
         }
 
         if (!$manager->contains($data)) {
@@ -57,6 +57,8 @@ public function persist($data)
 
         $manager->flush();
         $manager->refresh($data);
+
+        return $data;
     }
 
     /**
diff --git a/src/DataPersister/ChainDataPersister.php b/src/DataPersister/ChainDataPersister.php
@@ -51,7 +51,7 @@ public function persist($data)
     {
         foreach ($this->persisters as $persister) {
             if ($persister->supports($data)) {
-                $persister->persist($data);
+                return $persister->persist($data) ?? $data;
             }
         }
     }
diff --git a/src/DataPersister/DataPersisterInterface.php b/src/DataPersister/DataPersisterInterface.php
@@ -33,6 +33,8 @@ public function supports($data): bool;
      * Persists the data.
      *
      * @param mixed $data
+     *
+     * @return object|void Void will not be supported in API Platform 3, an object should always be returned
      */
     public function persist($data);
 
diff --git a/src/EventListener/WriteListener.php b/src/EventListener/WriteListener.php
@@ -50,7 +50,13 @@ public function onKernelView(GetResponseForControllerResultEvent $event)
             case 'PUT':
             case 'PATCH':
             case 'POST':
-                $this->dataPersister->persist($controllerResult);
+                $persistResult = $this->dataPersister->persist($controllerResult);
+
+                if (null === $persistResult) {
+                    @trigger_error(sprintf('Returning void from %s::persist() is deprecated since API Platform 2.3 and will not be supported in API Platform 3, an object should always be returned.', DataPersisterInterface::class), E_USER_DEPRECATED);
+                }
+
+                $event->setControllerResult($persistResult ?? $controllerResult);
                 break;
             case 'DELETE':
                 $this->dataPersister->remove($controllerResult);
diff --git a/src/GraphQl/Resolver/Factory/ItemMutationResolverFactory.php b/src/GraphQl/Resolver/Factory/ItemMutationResolverFactory.php
@@ -93,9 +93,13 @@ public function __invoke(string $resourceClass = null, string $rootClass = null,
                     $context += $resourceMetadata->getGraphqlAttribute($operationName, 'denormalization_context', [], true);
                     $item = $this->normalizer->denormalize($args['input'], $resourceClass, ItemNormalizer::FORMAT, $context);
                     $this->validate($item, $info, $resourceMetadata, $operationName);
-                    $this->dataPersister->persist($item);
+                    $persistResult = $this->dataPersister->persist($item);
 
-                    return $this->normalizer->normalize($item, ItemNormalizer::FORMAT, $normalizationContext) + $data;
+                    if (null === $persistResult) {
+                        @trigger_error(sprintf('Returning void from %s::persist() is deprecated since API Platform 2.3 and will not be supported in API Platform 3, an object should always be returned.', DataPersisterInterface::class), E_USER_DEPRECATED);
+                    }
+
+                    return $this->normalizer->normalize($persistResult ?? $item, ItemNormalizer::FORMAT, $normalizationContext) + $data;
                 case 'delete':
                     if ($item) {
                         $this->dataPersister->remove($item);
diff --git a/tests/Bridge/Doctrine/Common/DataPersisterTest.php b/tests/Bridge/Doctrine/Common/DataPersisterTest.php
@@ -56,7 +56,8 @@ public function testPersist()
         $managerRegistryProphecy = $this->prophesize(ManagerRegistry::class);
         $managerRegistryProphecy->getManagerForClass(Dummy::class)->willReturn($objectManagerProphecy->reveal())->shouldBeCalled();
 
-        (new DataPersister($managerRegistryProphecy->reveal()))->persist($dummy);
+        $result = (new DataPersister($managerRegistryProphecy->reveal()))->persist($dummy);
+        $this->assertSame($dummy, $result);
     }
 
     public function testPersistIfEntityAlreadyManaged()
@@ -72,15 +73,19 @@ public function testPersistIfEntityAlreadyManaged()
         $managerRegistryProphecy = $this->prophesize(ManagerRegistry::class);
         $managerRegistryProphecy->getManagerForClass(Dummy::class)->willReturn($objectManagerProphecy->reveal())->shouldBeCalled();
 
-        (new DataPersister($managerRegistryProphecy->reveal()))->persist($dummy);
+        $result = (new DataPersister($managerRegistryProphecy->reveal()))->persist($dummy);
+        $this->assertSame($dummy, $result);
     }
 
     public function testPersistWithNullManager()
     {
+        $dummy = new Dummy();
+
         $managerRegistryProphecy = $this->prophesize(ManagerRegistry::class);
         $managerRegistryProphecy->getManagerForClass(Dummy::class)->willReturn(null)->shouldBeCalled();
 
-        (new DataPersister($managerRegistryProphecy->reveal()))->persist(new Dummy());
+        $result = (new DataPersister($managerRegistryProphecy->reveal()))->persist($dummy);
+        $this->assertSame($dummy, $result);
     }
 
     public function testRemove()
diff --git a/tests/EventListener/WriteListenerTest.php b/tests/EventListener/WriteListenerTest.php
@@ -31,6 +31,37 @@ public function testOnKernelViewWithControllerResultAndPersist()
         $dummy = new Dummy();
         $dummy->setName('Dummyrino');
 
+        $dataPersisterProphecy = $this->prophesize(DataPersisterInterface::class);
+        $dataPersisterProphecy->supports($dummy)->willReturn(true)->shouldBeCalled();
+        $dataPersisterProphecy->persist($dummy)->willReturn($dummy)->shouldBeCalled();
+
+        $request = new Request();
+        $request->attributes->set('_api_resource_class', Dummy::class);
+
+        $event = new GetResponseForControllerResultEvent(
+            $this->prophesize(HttpKernelInterface::class)->reveal(),
+            $request,
+            HttpKernelInterface::MASTER_REQUEST,
+            $dummy
+        );
+
+        foreach (['PATCH', 'PUT', 'POST'] as $httpMethod) {
+            $request->setMethod($httpMethod);
+
+            (new WriteListener($dataPersisterProphecy->reveal()))->onKernelView($event);
+            $this->assertSame($dummy, $event->getControllerResult());
+        }
+    }
+
+    /**
+     * @group legacy
+     * @expectedDeprecation Returning void from ApiPlatform\Core\DataPersister\DataPersisterInterface::persist() is deprecated since API Platform 2.3 and will not be supported in API Platform 3, an object should always be returned.
+     */
+    public function testOnKernelViewWithControllerResultAndPersistReturningVoid()
+    {
+        $dummy = new Dummy();
+        $dummy->setName('Dummyrino');
+
         $dataPersisterProphecy = $this->prophesize(DataPersisterInterface::class);
         $dataPersisterProphecy->supports($dummy)->willReturn(true)->shouldBeCalled();
         $dataPersisterProphecy->persist($dummy)->shouldBeCalled();
@@ -49,6 +80,46 @@ public function testOnKernelViewWithControllerResultAndPersist()
             $request->setMethod($httpMethod);
 
             (new WriteListener($dataPersisterProphecy->reveal()))->onKernelView($event);
+            $this->assertSame($dummy, $event->getControllerResult());
+        }
+    }
+
+    /**
+     * @see https://github.com/api-platform/core/issues/1799
+     */
+    public function testOnKernelViewWithControllerResultAndPersistWithImmutableResource()
+    {
+        $dummy = new Dummy();
+        $dummy->setName('Dummyrino');
+
+        $dummy2 = new Dummy();
+        $dummy2->setName('Dummyferoce');
+
+        $dataPersisterProphecy = $this->prophesize(DataPersisterInterface::class);
+        $dataPersisterProphecy->supports($dummy)->willReturn(true)->shouldBeCalled();
+
+        $dataPersisterProphecy
+            ->persist($dummy)
+            ->willReturn($dummy2) // Persist is not mutating $dummy, but return a brand new technically unrelated object instead
+            ->shouldBeCalled()
+        ;
+
+        $request = new Request();
+        $request->attributes->set('_api_resource_class', Dummy::class);
+
+        foreach (['PATCH', 'PUT', 'POST'] as $httpMethod) {
+            $event = new GetResponseForControllerResultEvent(
+                $this->prophesize(HttpKernelInterface::class)->reveal(),
+                $request,
+                HttpKernelInterface::MASTER_REQUEST,
+                $dummy
+            );
+
+            $request->setMethod($httpMethod);
+
+            (new WriteListener($dataPersisterProphecy->reveal()))->onKernelView($event);
+
+            $this->assertSame($dummy2, $event->getControllerResult());
         }
     }
 

Original file line number	Diff line number	Diff line change
`@@ -48,7 +48,7 @@ public function supports($data): bool`
`48`	`48`	`public function persist($data)`
`49`	`49`	`{`
`50`	`50`	`if (!$manager = $this->getManager($data)) {`
`51`		`- return;`
	`51`	`+ return $data;`
`52`	`52`	`}`
`53`	`53`
`54`	`54`	`if (!$manager->contains($data)) {`
`@@ -57,6 +57,8 @@ public function persist($data)`
`57`	`57`
`58`	`58`	`$manager->flush();`
`59`	`59`	`$manager->refresh($data);`
	`60`	`+`
	`61`	`+ return $data;`
`60`	`62`	`}`
`61`	`63`
`62`	`64`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -51,7 +51,7 @@ public function persist($data)`
`51`	`51`	`{`
`52`	`52`	`foreach ($this->persisters as $persister) {`
`53`	`53`	`if ($persister->supports($data)) {`
`54`		`- $persister->persist($data);`
	`54`	`+ return $persister->persist($data) ?? $data;`
`55`	`55`	`}`
`56`	`56`	`}`
`57`	`57`	`}`
Original file line number	Diff line number	Diff line change
`@@ -33,6 +33,8 @@ public function supports($data): bool;`
`33`	`33`	`* Persists the data.`
`34`	`34`	`*`
`35`	`35`	`* @param mixed $data`
	`36`	`+ *`
	`37`	`+ * @return object\|void Void will not be supported in API Platform 3, an object should always be returned`
`36`	`38`	`*/`
`37`	`39`	`public function persist($data);`
`38`	`40`