fix: only call om::persist() once all objects are created

nikophil · nikophil · commit 0ee9fb860d63 · 2025-03-09T17:27:48.000+01:00
diff --git a/src/Persistence/PersistenceManager.php b/src/Persistence/PersistenceManager.php
@@ -31,6 +31,9 @@ final class PersistenceManager
     private bool $flush = true;
     private bool $persist = true;
 
+    /** @var list<object> */
+    private array $objectsToPersist = [];
+
     /** @var list<callable():void> */
     private array $afterPersistCallbacks = [];
 
@@ -78,6 +81,28 @@ public function save(object $object): object
         return $object;
     }
 
+    public function saveAll(): void
+    {
+        $objectManagers = [];
+
+        foreach ($this->objectsToPersist as $object) {
+            $om = $this->strategyFor($object::class)->objectManagerFor($object::class);
+            $om->persist($object);
+
+            if (!in_array($om, $objectManagers, true)) {
+                $objectManagers[] = $om;
+            }
+        }
+
+        $this->objectsToPersist = [];
+
+        foreach ($objectManagers as $om) {
+            $this->flush($om);
+        }
+
+        $this->callPostPersistCallbacks();
+    }
+
     /**
      * @template T of object
      *
@@ -92,25 +117,13 @@ public function scheduleForInsert(object $object, array $afterPersistCallbacks =
             $object = unproxy($object);
         }
 
-        $om = $this->strategyFor($object::class)->objectManagerFor($object::class);
-        $om->persist($object);
+        $this->objectsToPersist[] = $object;
 
         $this->afterPersistCallbacks = [...$this->afterPersistCallbacks, ...$afterPersistCallbacks];
 
         return $object;
     }
 
-    public function forget(object $object): void
-    {
-        if ($this->isPersisted($object)) {
-            throw new \LogicException('Cannot forget an object already persisted.');
-        }
-
-        $om = $this->strategyFor($object::class)->objectManagerFor($object::class);
-
-        $om->detach($object);
-    }
-
     /**
      * @template T
      *
@@ -126,31 +139,43 @@ public function flushAfter(callable $callback): mixed
 
         $this->flush = true;
 
+        $this->flushAllStrategies();
+
+        $this->callPostPersistCallbacks();
+
+        return $result;
+    }
+
+    private function flushAllStrategies(): void
+    {
         foreach ($this->strategies as $strategy) {
             foreach ($strategy->objectManagers() as $om) {
                 $this->flush($om);
             }
         }
-
-        return $result;
     }
 
     public function flush(ObjectManager $om): void
     {
         if ($this->flush) {
             $om->flush();
+        }
+    }
 
-            if ($this->afterPersistCallbacks) {
-                $afterPersistCallbacks = $this->afterPersistCallbacks;
-                $this->afterPersistCallbacks = [];
+    private function callPostPersistCallbacks(): void
+    {
+        if (!$this->flush || $this->afterPersistCallbacks === []) {
+            return;
+        }
 
-                foreach ($afterPersistCallbacks as $afterPersistCallback) {
-                    $afterPersistCallback();
-                }
+        $afterPersistCallbacks = $this->afterPersistCallbacks;
+        $this->afterPersistCallbacks = [];
 
-                $this->flush($om);
-            }
+        foreach ($afterPersistCallbacks as $afterPersistCallback) {
+            $afterPersistCallback();
         }
+
+        $this->flushAllStrategies();
     }
 
     /**
diff --git a/src/Persistence/PersistentObjectFactory.php b/src/Persistence/PersistentObjectFactory.php
@@ -218,7 +218,7 @@ public function create(callable|array $attributes = []): object
             throw new \LogicException('Persistence cannot be used in unit tests.');
         }
 
-        $configuration->persistence()->save($object);
+        $configuration->persistence()->saveAll();
 
         return $object;
     }
@@ -298,25 +298,23 @@ protected function normalizeParameter(string $field, mixed $value): mixed
             if ($inversedRelationshipMetadata && !$inversedRelationshipMetadata->isCollection) {
                 $inverseField = $inversedRelationshipMetadata->inverseField;
 
-                $inversedObject = $value->withPersistMode(
-                    $this->isPersisting() ? PersistMode::NO_PERSIST_BUT_SCHEDULE_FOR_INSERT : PersistMode::WITHOUT_PERSISTING
-                )
-                    ->notRootFactory()
+                // we need to handle the circular dependency involved by inversed one-to-one relationship:
+                // a placeholder object is used, which will be replaced by the real object, after its instantiation
+                $inverseObjectPlaceholder = (new \ReflectionClass($value::class()))->newInstanceWithoutConstructor();
 
-                    // we need to handle the circular dependency involved by inversed one-to-one relationship:
-                    // a placeholder object is used, which will be replaced by the real object, after its instantiation
-                    ->create([
-                        $inverseField => $placeholder = (new \ReflectionClass(static::class()))->newInstanceWithoutConstructor(),
-                    ]);
+                $this->tempAfterInstantiate[] = function(object $object) use ($value, $inverseField, $field) {
+                    $inverseObject = $value->withPersistMode(
+                        $this->isPersisting() ? PersistMode::NO_PERSIST_BUT_SCHEDULE_FOR_INSERT : PersistMode::WITHOUT_PERSISTING
+                    )
+                        ->notRootFactory()
+                        ->create([$inverseField => $object]);
 
-                $inversedObject = unproxy($inversedObject, withAutoRefresh: false);
+                    $inverseObject = unproxy($inverseObject, withAutoRefresh: false);
 
-                $this->tempAfterInstantiate[] = static function(object $object) use ($inversedObject, $inverseField, $pm, $placeholder) {
-                    $pm->forget($placeholder);
-                    set($inversedObject, $inverseField, $object);
+                    set($object, $field, $inverseObject);
                 };
 
-                return $inversedObject;
+                return $inverseObjectPlaceholder;
             } else {
                 $value = $value->notRootFactory();
             }
@@ -389,6 +387,8 @@ protected function normalizeObject(object $object): object
 
         if (!$persistenceManager->isPersisted($object)) {
             $persistenceManager->scheduleForInsert($object);
+
+            return $object;
         }
 
         try {
diff --git a/tests/Fixture/Entity/EdgeCases/InversedOneToOneWithoutAutoGeneratedId/InverseSide.php b/tests/Fixture/Entity/EdgeCases/InversedOneToOneWithoutAutoGeneratedId/InverseSide.php
@@ -0,0 +1,36 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the zenstruck/foundry package.
+ *
+ * (c) Kevin Bond <kevinbond@gmail.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Zenstruck\Foundry\Tests\Fixture\Entity\EdgeCases\InversedOneToOneWithoutAutoGeneratedId;
+
+use Doctrine\ORM\Mapping as ORM;
+use Symfony\Component\Uid\Uuid;
+
+/**
+ * @author Nicolas PHILIPPE <nikophil@gmail.com>
+ */
+#[ORM\Entity]
+#[ORM\Table('inversed_one_to_one_with_custom_id_inverse')]
+class InverseSide
+{
+    #[ORM\Id]
+    #[ORM\Column(type: 'uuid')]
+    public Uuid $id;
+
+    public function __construct(
+        #[ORM\OneToOne(mappedBy: 'inverseSide', cascade: ['persist'])] // @phpstan-ignore doctrine.associationType
+        public OwningSide $owningSide,
+    ) {
+        $this->id = Uuid::v7();
+    }
+}
diff --git a/tests/Fixture/Entity/EdgeCases/InversedOneToOneWithoutAutoGeneratedId/OwningSide.php b/tests/Fixture/Entity/EdgeCases/InversedOneToOneWithoutAutoGeneratedId/OwningSide.php
@@ -0,0 +1,37 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the zenstruck/foundry package.
+ *
+ * (c) Kevin Bond <kevinbond@gmail.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Zenstruck\Foundry\Tests\Fixture\Entity\EdgeCases\InversedOneToOneWithoutAutoGeneratedId;
+
+use Doctrine\ORM\Mapping as ORM;
+use Symfony\Component\Uid\Uuid;
+
+/**
+ * @author Nicolas PHILIPPE <nikophil@gmail.com>
+ */
+#[ORM\Entity]
+#[ORM\Table('inversed_one_to_one_with_custom_id_owning')]
+class OwningSide
+{
+    #[ORM\Id]
+    #[ORM\Column(type: 'uuid')]
+    public Uuid $id;
+
+    #[ORM\OneToOne(inversedBy: 'owningSide', cascade: ['persist'])]
+    public ?InverseSide $inverseSide = null;
+
+    public function __construct()
+    {
+        $this->id = Uuid::v7();
+    }
+}
diff --git a/tests/Integration/ORM/EdgeCasesRelationshipTest.php b/tests/Integration/ORM/EdgeCasesRelationshipTest.php
@@ -24,6 +24,7 @@
 use Zenstruck\Foundry\Tests\Fixture\Entity\EdgeCases\IndexedOneToMany;
 use Zenstruck\Foundry\Tests\Fixture\Entity\EdgeCases\InversedOneToOneWithManyToOne;
 use Zenstruck\Foundry\Tests\Fixture\Entity\EdgeCases\InversedOneToOneWithNonNullableOwning;
+use Zenstruck\Foundry\Tests\Fixture\Entity\EdgeCases\InversedOneToOneWithoutAutoGeneratedId;
 use Zenstruck\Foundry\Tests\Fixture\Entity\EdgeCases\InversedOneToOneWithOneToMany;
 use Zenstruck\Foundry\Tests\Fixture\Entity\EdgeCases\InversedOneToOneWithSetter;
 use Zenstruck\Foundry\Tests\Fixture\Entity\EdgeCases\ManyToOneToSelfReferencing;
@@ -100,7 +101,7 @@ public function inverse_one_to_one_with_both_nullable(): void
     #[DataProvider('provideCascadeRelationshipsCombinations')]
     #[UsingRelationships(InversedOneToOneWithOneToMany\OwningSide::class, ['inverseSide'])]
     #[UsingRelationships(InversedOneToOneWithOneToMany\Item::class, ['owningSide'])]
-    #[RequiresPhpunit('^11.4')]
+    #[RequiresPhpunit('>=11.4')]
     public function inverse_one_to_one_with_one_to_many(): void
     {
         $inverseSideFactory = persistent_factory(InversedOneToOneWithOneToMany\InverseSide::class);
@@ -141,7 +142,7 @@ public function many_to_many_to_self_referencing_inverse_side(): void
     #[Test]
     #[DataProvider('provideCascadeRelationshipsCombinations')]
     #[UsingRelationships(IndexedOneToMany\ParentEntity::class, ['items'])]
-    #[RequiresPhpunit('^11.4')]
+    #[RequiresPhpunit('>=11.4')]
     public function indexed_one_to_many(): void
     {
         $parentFactory = persistent_factory(IndexedOneToMany\ParentEntity::class);
@@ -163,7 +164,7 @@ public function indexed_one_to_many(): void
     #[Test]
     #[DataProvider('provideCascadeRelationshipsCombinations')]
     #[UsingRelationships(InversedOneToOneWithManyToOne\InverseSide::class, ['owningSide', 'item'])]
-    #[RequiresPhpunit('^11.4')]
+    #[RequiresPhpunit('>=11.4')]
     public function inversed_one_to_one_can_be_used_after_other_relationship(): void
     {
         $inverseSideFactory = persistent_factory(InversedOneToOneWithManyToOne\InverseSide::class);
@@ -187,6 +188,24 @@ public function inversed_one_to_one_can_be_used_after_other_relationship(): void
         self::assertNotNull($inverseSide->item);
     }
 
+    /** @test */
+    #[Test]
+    #[DataProvider('provideCascadeRelationshipsCombinations')]
+    #[UsingRelationships(InversedOneToOneWithoutAutoGeneratedId\OwningSide::class, ['inverseSide'])]
+    #[RequiresPhpunit('>=11.4')]
+    public function inverse_one_to_one_with_custom_id(): void
+    {
+        $owningSideFactory = persistent_factory(InversedOneToOneWithoutAutoGeneratedId\OwningSide::class);
+        $inverseSideFactory = persistent_factory(InversedOneToOneWithoutAutoGeneratedId\InverseSide::class);
+
+        $inverseSide = $inverseSideFactory->create(['owningSide' => $owningSideFactory]);
+
+        $owningSideFactory::assert()->count(1);
+        $inverseSideFactory::assert()->count(1);
+
+        self::assertSame($inverseSide, $inverseSide->owningSide->inverseSide);
+    }
+
     /**
      * @test
      */
