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

nikophil · nikophil · commit a4d38a882b6e · 2025-03-14T15:02:25.000+01:00
diff --git a/src/Persistence/PersistenceManager.php b/src/Persistence/PersistenceManager.php
@@ -31,7 +31,10 @@ final class PersistenceManager
     private bool $flush = true;
     private bool $persist = true;
 
-    /** @var list<callable():void> */
+    /** @var array<int, list<object>> */
+    private array $objectsToPersist = [];
+
+    /** @var array<int, list<callable():void>> */
     private array $afterPersistCallbacks = [];
 
     /**
@@ -78,6 +81,45 @@ public function save(object $object): object
         return $object;
     }
 
+    /**
+     * We're using so-called "transactions" to group multiple persist/flush operations
+     * This prevents such code to persist the whole batch of objects in the normalization phase:
+     * ```php
+     * SomeFactory::createOne(['item' => lazy(fn() => OtherFactory::createOne())]);
+     * ```.
+     */
+    public function startTransaction(): void
+    {
+        $this->objectsToPersist[] = [];
+        $this->afterPersistCallbacks[] = [];
+    }
+
+    public function commit(): void
+    {
+        $objectManagers = [];
+
+        $objectsToPersist = \array_pop($this->objectsToPersist);
+
+        if (null === $objectsToPersist) {
+            return;
+        }
+
+        foreach ($objectsToPersist as $object) {
+            $om = $this->strategyFor($object::class)->objectManagerFor($object::class);
+            $om->persist($object);
+
+            if (!\in_array($om, $objectManagers, true)) {
+                $objectManagers[] = $om;
+            }
+        }
+
+        foreach ($objectManagers as $om) {
+            $this->flush($om);
+        }
+
+        $this->callPostPersistCallbacks();
+    }
+
     /**
      * @template T of object
      *
@@ -92,23 +134,19 @@ public function scheduleForInsert(object $object, array $afterPersistCallbacks =
             $object = unproxy($object);
         }
 
-        $om = $this->strategyFor($object::class)->objectManagerFor($object::class);
-        $om->persist($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.');
+        if (0 === \count($this->objectsToPersist)) {
+            throw new \LogicException('No transaction started yet.');
         }
 
-        $om = $this->strategyFor($object::class)->objectManagerFor($object::class);
+        $transactionCount = \count($this->objectsToPersist) - 1;
+        $this->objectsToPersist[$transactionCount][] = $object;
+
+        $this->afterPersistCallbacks[$transactionCount] = [
+            ...$this->afterPersistCallbacks[$transactionCount],
+            ...$afterPersistCallbacks,
+        ];
 
-        $om->detach($object);
+        return $object;
     }
 
     /**
@@ -126,11 +164,9 @@ public function flushAfter(callable $callback): mixed
 
         $this->flush = true;
 
-        foreach ($this->strategies as $strategy) {
-            foreach ($strategy->objectManagers() as $om) {
-                $this->flush($om);
-            }
-        }
+        $this->flushAllStrategies();
+
+        $this->callPostPersistCallbacks();
 
         return $result;
     }
@@ -139,17 +175,6 @@ public function flush(ObjectManager $om): void
     {
         if ($this->flush) {
             $om->flush();
-
-            if ($this->afterPersistCallbacks) {
-                $afterPersistCallbacks = $this->afterPersistCallbacks;
-                $this->afterPersistCallbacks = [];
-
-                foreach ($afterPersistCallbacks as $afterPersistCallback) {
-                    $afterPersistCallback();
-                }
-
-                $this->flush($om);
-            }
         }
     }
 
@@ -372,6 +397,30 @@ public static function isOrmOnly(): bool
         })();
     }
 
+    private function flushAllStrategies(): void
+    {
+        foreach ($this->strategies as $strategy) {
+            foreach ($strategy->objectManagers() as $om) {
+                $this->flush($om);
+            }
+        }
+    }
+
+    private function callPostPersistCallbacks(): void
+    {
+        if (!$this->flush || [] === $this->afterPersistCallbacks) {
+            return;
+        }
+
+        $afterPersistCallbacks = \array_pop($this->afterPersistCallbacks);
+
+        foreach ($afterPersistCallbacks as $afterPersistCallback) {
+            $afterPersistCallback();
+        }
+
+        $this->flushAllStrategies();
+    }
+
     /**
      * @param class-string $class
      *
diff --git a/src/Persistence/PersistentObjectFactory.php b/src/Persistence/PersistentObjectFactory.php
@@ -198,6 +198,10 @@ final public static function truncate(): void
      */
     public function create(callable|array $attributes = []): object
     {
+        if (PersistMode::PERSIST === $this->persistMode() && $this->isRootFactory) {
+            Configuration::instance()->persistence()->startTransaction();
+        }
+
         $object = parent::create($attributes);
 
         foreach ($this->tempAfterInstantiate as $callback) {
@@ -218,7 +222,7 @@ public function create(callable|array $attributes = []): object
             throw new \LogicException('Persistence cannot be used in unit tests.');
         }
 
-        $configuration->persistence()->save($object);
+        $configuration->persistence()->commit();
 
         return $object;
     }
@@ -298,25 +302,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 +391,8 @@ protected function normalizeObject(object $object): object
 
         if (!$persistenceManager->isPersisted($object)) {
             $persistenceManager->scheduleForInsert($object);
+
+            return $object;
         }
 
         try {
diff --git a/tests/Fixture/Entity/EdgeCases/InversedOneToOneWithManyToOne/InverseSide.php b/tests/Fixture/Entity/EdgeCases/InversedOneToOneWithManyToOne/InverseSide.php
@@ -31,7 +31,7 @@ class InverseSide extends Base
 
     public function __construct(
         #[ORM\Column()]
-        public string $mandatoryField
+        public string $mandatoryField,
     ) {
     }
 }
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
@@ -26,6 +26,7 @@
 use Zenstruck\Foundry\Tests\Fixture\Entity\EdgeCases\InversedOneToOneWithManyToOne;
 use Zenstruck\Foundry\Tests\Fixture\Entity\EdgeCases\InversedOneToOneWithNonNullableOwning;
 use Zenstruck\Foundry\Tests\Fixture\Entity\EdgeCases\InversedOneToOneWithOneToMany;
+use Zenstruck\Foundry\Tests\Fixture\Entity\EdgeCases\InversedOneToOneWithoutAutoGeneratedId;
 use Zenstruck\Foundry\Tests\Fixture\Entity\EdgeCases\InversedOneToOneWithSetter;
 use Zenstruck\Foundry\Tests\Fixture\Entity\EdgeCases\ManyToOneToSelfReferencing;
 use Zenstruck\Foundry\Tests\Fixture\Entity\EdgeCases\OneToManyWithUnionType;
@@ -102,7 +103,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);
@@ -143,7 +144,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);
@@ -189,6 +190,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 */
     #[Test]
     public function object_with_union_type(): void
diff --git a/tests/Integration/ORM/EntityRelationship/EntityFactoryRelationshipTestCase.php b/tests/Integration/ORM/EntityRelationship/EntityFactoryRelationshipTestCase.php
diff --git a/tests/Integration/Persistence/GenericFactoryTestCase.php b/tests/Integration/Persistence/GenericFactoryTestCase.php

Original file line number	Diff line number	Diff line change
`@@ -31,7 +31,7 @@ class InverseSide extends Base`
`31`	`31`
`32`	`32`	`public function __construct(`
`33`	`33`	`#[ORM\Column()]`
`34`		`- public string $mandatoryField`
	`34`	`+ public string $mandatoryField,`
`35`	`35`	`) {`
`36`	`36`	`}`
`37`	`37`	`}`