fix(autorefresh): don't use clone to get the id values (#980)

Author: nikophil

src/Persistence/PersistenceManager.php

@@ -22,6 +22,7 @@
 use Zenstruck\Foundry\Persistence\Exception\ObjectHasUnsavedChanges;
 use Zenstruck\Foundry\Persistence\Exception\ObjectNoLongerExist;
 use Zenstruck\Foundry\Persistence\Exception\RefreshObjectFailed;
+use Zenstruck\Foundry\Persistence\Proxy\PersistedObjectsTracker;
 use Zenstruck\Foundry\Persistence\Relationship\RelationshipMetadata;
 use Zenstruck\Foundry\Persistence\ResetDatabase\ResetDatabaseManager;
 
@@ -85,6 +86,8 @@ public function save(object $object): object
             $this->flush($om);
         }
 
+        PersistedObjectsTracker::updateIds();
+
         return $object;
     }
 
@@ -148,18 +151,16 @@ public function flush(ObjectManager $om): void
      *
      * @param T $object
      */
-    public function autorefresh(object $object, object $clone): void
+    public function autorefresh(object $object, mixed $id, object $clone): void
     {
         $strategy = $this->strategyFor($object::class);
         $om = $strategy->objectManagerFor($object::class);
 
-        $id = $strategy->getIdentifierValues($clone);
-
         if ($id) {
             try {
                 $om->refresh($object);
 
-                if ($strategy->getIdentifierValues($object)) {
+                if ($this->getIdentifierValues($object)) {
                     // no identifier values means the object no longer exists
                     return;
                 }
@@ -398,6 +399,11 @@ public function resetDatabaseManager(): ResetDatabaseManager
         return $this->resetDatabaseManager;
     }
 
+    public function getIdentifierValues(object $object): mixed
+    {
+        return $this->strategyFor($object::class)->getIdentifierValues($object);
+    }
+
     public static function isOrmOnly(): bool
     {
         static $isOrmOnly = null;

src/Persistence/Proxy/PersistedObjectsTracker.php

@@ -23,7 +23,17 @@ final class PersistedObjectsTracker
      *
      * @var list<\WeakReference<object>>
      */
-    private static $buffer = [];
+    private static array $buffer = [];
+
+    /**
+     * @var \WeakMap<object, mixed>
+     */
+    private static \WeakMap $ids;
+
+    public function __construct()
+    {
+        self::$ids ??= new \WeakMap();
+    }
 
     public function refresh(): void
     {
@@ -34,12 +44,33 @@ public function add(object ...$objects): void
     {
         foreach ($objects as $object) {
             self::$buffer[] = \WeakReference::create($object);
+
+            $id = Configuration::instance()->persistence()->getIdentifierValues($object);
+            if ($id) {
+                self::$ids[$object] = $id;
+            }
+        }
+    }
+
+    public static function updateIds(): void
+    {
+        foreach (self::$buffer as $reference) {
+            if (null === $object = $reference->get()) {
+                continue;
+            }
+
+            if (self::$ids->offsetExists($object)) {
+                continue;
+            }
+
+            self::$ids[$object] = Configuration::instance()->persistence()->getIdentifierValues($object);
         }
     }
 
     public static function reset(): void
     {
         self::$buffer = [];
+        self::$ids = new \WeakMap();
     }
 
     public static function countObjects(): int
@@ -51,6 +82,10 @@ public static function countObjects(): int
 
     private static function proxifyObjects(): void
     {
+        if (!Configuration::isBooted()) {
+            return;
+        }
+
         self::$buffer = \array_values(
             \array_map(
                 static function(\WeakReference $weakRef) {
@@ -64,7 +99,9 @@ static function(\WeakReference $weakRef) {
 
                     $clone = clone $object;
                     $reflector->resetAsLazyGhost($object, function($object) use ($clone) {
-                        Configuration::instance()->persistence()->autorefresh($object, $clone);
+                        $id = self::$ids[$object] ?? throw new \LogicException('Canot find the id for object');
+
+                        Configuration::instance()->persistence()->autorefresh($object, $id, $clone);
                     });
 
                     return \WeakReference::create($object);

tests/Fixture/Entity/EdgeCases/EntityWithCloneMethod.php

@@ -0,0 +1,23 @@
+<?php
+
+namespace Zenstruck\Foundry\Tests\Fixture\Entity\EdgeCases;
+
+use Doctrine\ORM\Mapping as ORM;
+
+#[ORM\Entity]
+#[ORM\Table('entity_with_clone_method')]
+class EntityWithCloneMethod
+{
+    #[ORM\Id]
+    #[ORM\GeneratedValue]
+    #[ORM\Column(type: 'integer')]
+    public int|null $id = null;
+
+    #[ORM\Column(nullable: true)]
+    public string|null $prop = null;
+
+    public function __clone()
+    {
+        $this->id = null;
+    }
+}

tests/Integration/GetWebTestClientIsNotBrokenTest.php

@@ -0,0 +1,43 @@
+<?php
+
+namespace Zenstruck\Foundry\Tests\Integration;
+
+use PHPUnit\Framework\Attributes\Depends;
+use PHPUnit\Framework\Attributes\Test;
+use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
+use Zenstruck\Foundry\Test\Factories;
+use Zenstruck\Foundry\Tests\Fixture\Factories\Entity\GenericEntityFactory;
+
+final class GetWebTestClientIsNotBrokenTest extends WebTestCase
+{
+    use Factories, RequiresORM;
+
+    /**
+     * @test
+     */
+    #[Test]
+    public function boots_kernel_and_get_client(): void
+    {
+        $client = self::createClient();
+
+        $object = GenericEntityFactory::createOne();
+        $client->request('GET', "/orm/update/{$object->id}");
+        self::assertResponseIsSuccessful();
+    }
+
+    /**
+     * @test
+     * @depends boots_kernel_and_get_client
+     */
+    #[Test]
+    #[Depends('boots_kernel_and_get_client')]
+    public function assert_test_starts_with_a_non_booted_kernel(): void
+    {
+        self::assertFalse(self::$booted);
+
+        // ensure we can get a client without the error:
+        // Booting the kernel before calling "WebTestCase::createClient()" is not supported
+        self::createClient();
+    }
+
+}

tests/Integration/ORM/AutoRefreshTest.php

@@ -15,6 +15,7 @@
 
 use Doctrine\ORM\EntityManagerInterface;
 use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\Attributes\Depends;
 use PHPUnit\Framework\Attributes\IgnorePhpunitWarnings;
 use PHPUnit\Framework\Attributes\RequiresEnvironmentVariable;
 use PHPUnit\Framework\Attributes\RequiresPhp;
@@ -26,13 +27,15 @@
 use Zenstruck\Foundry\Tests\Fixture\DoctrineCascadeRelationship\ChangesEntityRelationshipCascadePersist;
 use Zenstruck\Foundry\Tests\Fixture\DoctrineCascadeRelationship\UsingRelationships;
 use Zenstruck\Foundry\Tests\Fixture\Entity\Contact;
+use Zenstruck\Foundry\Tests\Fixture\Entity\EdgeCases\EntityWithCloneMethod;
 use Zenstruck\Foundry\Tests\Fixture\Factories\Entity\Address\AddressFactory;
 use Zenstruck\Foundry\Tests\Fixture\Factories\Entity\Category\CategoryFactory;
 use Zenstruck\Foundry\Tests\Fixture\Factories\Entity\Contact\ContactFactory;
 use Zenstruck\Foundry\Tests\Fixture\Factories\Entity\GenericEntityFactory;
 use Zenstruck\Foundry\Tests\Integration\Persistence\AutoRefreshTestCase;
 use Zenstruck\Foundry\Tests\Integration\RequiresORM;
 
+use function Zenstruck\Foundry\Persistence\persistent_factory;
 use function Zenstruck\Foundry\Persistence\refresh_all;
 
 /**
@@ -122,8 +125,26 @@ public function it_can_refresh_objects_with_relationships(): void
         self::assertSame('city', $contact->getAddress()->getCity());
     }
 
+    /**
+     * The previous test creates entities with circular dependencies,
+     * so the PersistedObjectsTracker still have references to them.
+     *
+     * Let's ensure we don't leave the test in an invalide state when the container is reset
+     */
+    #[Test]
+    #[Depends('it_can_refresh_objects_with_relationships')]
+    public function assert_test_starts_with_a_non_booted_kernel(): void
+    {
+        self::assertSame(0, PersistedObjectsTracker::countObjects());
+
+        self::assertFalse(self::$booted);
+
+        // ensure we can get a client without the error:
+        // Booting the kernel before calling "WebTestCase::createClient()" is not supported
+        self::createClient();
+    }
+
     #[Test]
-    #[IgnorePhpunitWarnings(EdgeCasesRelationshipTest::DATA_PROVIDER_WARNING_REGEX)]
     public function it_can_refresh_with_doctrine_proxies(): void
     {
         $contact = ContactFactory::createOne();
@@ -151,6 +172,22 @@ public function it_can_refresh_with_doctrine_proxies(): void
         self::assertSame('foo', $category->getName());
     }
 
+    #[Test]
+    public function it_can_refresh_entity_which_removes_its_id_in_clone(): void
+    {
+        $entity = persistent_factory(EntityWithCloneMethod::class)->create();
+        $entityId = $entity->id;
+
+        $this->objectManager()->getConnection()->executeQuery('UPDATE entity_with_clone_method SET prop = \'foo\' WHERE id = ?', [$entity->id]);
+
+        self::ensureKernelShutdown();
+
+        self::assertTrue((new \ReflectionClass($entity))->isUninitializedLazyObject($entity));
+
+        self::assertSame('foo', $entity->prop);
+        self::assertSame($entityId, $entity->id);
+    }
+
     protected static function factory(): PersistentObjectFactory
     {
         return GenericEntityFactory::new();

tests/Integration/Persistence/AutoRefreshTestCase.php

@@ -62,6 +62,7 @@ public function it_can_refresh_after_kernel_shutdown(): void
 
         // service reset did clear the EM, thus the object is not managed anymore
         self::assertFalse($this->objectManager()->contains($object));
+        self::assertSame($objectId, $object->id);
     }
 
     #[Test]
@@ -80,6 +81,9 @@ public function it_can_refresh_many_objects(): void
 
         self::assertSame('foo', $object1->getProp1());
         self::assertSame('foo', $object2->getProp1());
+
+        self::assertSame($objectId1, $object1->id);
+        self::assertSame($objectId2, $object2->id);
     }
 
     #[Test]
@@ -88,6 +92,8 @@ public function it_can_refresh_after_update_with_browser(): void
         $client = self::createClient();
 
         $object = $this->factory()->create();
+        $objectId = $object->id;
+
         self::assertSame('default1', $object->getProp1());
         self::assertFalse((new \ReflectionClass($object))->isUninitializedLazyObject($object));
 
@@ -100,6 +106,8 @@ public function it_can_refresh_after_update_with_browser(): void
         assert_persisted($object);
         self::assertSame('foo', $object->getProp1());
         self::assertFalse((new \ReflectionClass($object))->isUninitializedLazyObject($object));
+
+        self::assertSame($objectId, $object->id);
     }
 
     #[Test]