Improve identity map merging, error handling, and type safety

Replace identity map entity swapping with per-property merging:
mutable entities are updated in-place, readonly entities produce a
new merged instance only when properties actually differ. Normalize
PK types (numeric strings cast to int) for consistent identity map
lookups. Add CollectionNotBound exception, cycle-safe persist return
values, and class-string annotations removing phpstan-ignore.

Author: alganet

src/AbstractMapper.php

@@ -10,8 +10,8 @@
 
 use function array_flip;
 use function array_intersect_key;
-use function assert;
 use function count;
+use function ctype_digit;
 use function is_int;
 use function is_scalar;
 use function is_string;
@@ -96,8 +96,9 @@ public function persist(object $object, Collection $onCollection): object
             return $object;
         }
 
-        if ($onCollection->name !== null && $this->tryReplaceFromIdentityMap($object, $onCollection)) {
-            return $object;
+        $merged = $this->tryMergeWithIdentityMap($object, $onCollection);
+        if ($merged !== null) {
+            return $merged;
         }
 
         $this->pending[$object] = 'insert';
@@ -202,53 +203,84 @@ protected function findInIdentityMap(Collection $collection): object|null
             return null;
         }
 
-        $condition = $collection->condition;
-        if (!is_int($condition) && !is_string($condition)) {
+        $condition = $this->normalizeIdValue($collection->condition);
+        if ($condition === null) {
             return null;
         }
 
         return $this->identityMap[$collection->name][$condition] ?? null;
     }
 
-    private function tryReplaceFromIdentityMap(object $entity, Collection $coll): bool
+    private function tryMergeWithIdentityMap(object $entity, Collection $coll): object|null
     {
-        assert($coll->name !== null);
-        $entityId = $this->entityIdValue($entity, $coll->name);
-        $idValue = $entityId;
-
-        if ($idValue === null && is_scalar($coll->condition)) {
-            $idValue = $coll->condition;
+        if ($coll->name === null) {
+            return null;
         }
 
-        if ($idValue === null || (!is_int($idValue) && !is_string($idValue))) {
-            return false;
+        $entityId = $this->entityIdValue($entity, $coll->name);
+        $idValue = $entityId ?? $this->normalizeIdValue($coll->condition);
+
+        if ($idValue === null) {
+            return null;
         }
 
         $existing = $this->identityMap[$coll->name][$idValue] ?? null;
         if ($existing === null || $existing === $entity) {
-            return false;
+            return null;
         }
 
         if ($entityId === null) {
             $idName = $this->style->identifier($coll->name);
             $this->entityFactory->set($entity, $idName, $idValue);
         }
 
-        $this->tracked->offsetUnset($existing);
-        $this->pending->offsetUnset($existing);
-        $this->evictFromIdentityMap($existing, $coll);
-        $this->markTracked($entity, $coll);
-        $this->registerInIdentityMap($entity, $coll);
-        $this->pending[$entity] = 'update';
+        if ($this->entityFactory->isReadOnly($existing)) {
+            $merged = $this->entityFactory->mergeEntities($existing, $entity);
 
-        return true;
+            if ($merged !== $existing) {
+                $this->tracked->offsetUnset($existing);
+                $this->pending->offsetUnset($existing);
+                $this->evictFromIdentityMap($existing, $coll);
+                $this->markTracked($merged, $coll);
+                $this->registerInIdentityMap($merged, $coll);
+            }
+
+            $this->pending[$merged] = 'update';
+
+            return $merged;
+        }
+
+        foreach ($this->entityFactory->extractProperties($entity) as $prop => $value) {
+            $this->entityFactory->set($existing, $prop, $value);
+        }
+
+        if (!$this->isTracked($existing)) {
+            $this->markTracked($existing, $coll);
+        }
+
+        $this->pending[$existing] = 'update';
+
+        return $existing;
     }
 
     private function entityIdValue(object $entity, string $collName): int|string|null
     {
-        $idValue = $this->entityFactory->get($entity, $this->style->identifier($collName));
+        return $this->normalizeIdValue(
+            $this->entityFactory->get($entity, $this->style->identifier($collName)),
+        );
+    }
+
+    private function normalizeIdValue(mixed $value): int|string|null
+    {
+        if (is_int($value)) {
+            return $value;
+        }
+
+        if (is_string($value)) {
+            return ctype_digit($value) ? (int) $value : $value;
+        }
 
-        return is_int($idValue) || is_string($idValue) ? $idValue : null;
+        return null;
     }
 
     public function __get(string $name): Collection

src/CollectionNotBound.php

@@ -0,0 +1,18 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Respect\Data;
+
+use RuntimeException;
+
+final class CollectionNotBound extends RuntimeException
+{
+    public function __construct(string|null $collectionName)
+    {
+        parent::__construct(
+            'Collection \'' . ($collectionName ?? '(unnamed)')
+            . '\' must be attached to a mapper before fetching or persisting',
+        );
+    }
+}

src/Collections/Collection.php

@@ -6,8 +6,8 @@
 
 use ArrayAccess;
 use Respect\Data\AbstractMapper;
+use Respect\Data\CollectionNotBound;
 use Respect\Data\Hydrator;
-use RuntimeException;
 
 /** @implements ArrayAccess<string, Collection> */
 class Collection implements ArrayAccess
@@ -65,9 +65,7 @@ public function persist(object $object, mixed ...$changes): object
             }
         }
 
-        $mapper->persist($object, $this);
-
-        return $object;
+        return $mapper->persist($object, $this);
     }
 
     public function remove(object $object): bool
@@ -150,7 +148,7 @@ private function findMapper(): AbstractMapper|null
 
     private function resolveMapper(): AbstractMapper
     {
-        return $this->findMapper() ?? throw new RuntimeException();
+        return $this->findMapper() ?? throw new CollectionNotBound($this->name);
     }
 
     private function setNext(Collection $collection): void

src/EntityFactory.php

@@ -26,10 +26,10 @@
 /** Creates and manipulates entity objects using Style-based naming conventions */
 class EntityFactory
 {
-    /** @var array<string, ReflectionClass<object>> */
+    /** @var array<class-string, ReflectionClass<object>> */
     private array $classCache = [];
 
-    /** @var array<string, array<string, ReflectionProperty>> */
+    /** @var array<class-string, array<string, ReflectionProperty>> */
     private array $propertyCache = [];
 
     /** @var array<string, class-string> */
@@ -156,6 +156,46 @@ public function withChanges(object $entity, mixed ...$changes): object
         return $clone;
     }
 
+    public function mergeEntities(object $base, object $overlay): object
+    {
+        if ($base::class !== $overlay::class) {
+            throw new DomainException(
+                'Cannot merge entities of different classes: ' . $base::class . ' and ' . $overlay::class,
+            );
+        }
+
+        $overlayProps = $this->extractProperties($overlay);
+        $hasDifference = false;
+
+        foreach ($overlayProps as $name => $value) {
+            $mirror = $this->reflectProperties($base::class)[$name] ?? null;
+            if ($mirror === null) {
+                continue;
+            }
+
+            if (!$mirror->isInitialized($base) || $mirror->getValue($base) !== $value) {
+                $hasDifference = true;
+                break;
+            }
+        }
+
+        if (!$hasDifference) {
+            return $base;
+        }
+
+        $clone = $this->reflectClass($base::class)->newInstanceWithoutConstructor();
+
+        foreach ($this->reflectProperties($base::class) as $name => $prop) {
+            if (array_key_exists($name, $overlayProps)) {
+                $prop->setValue($clone, $overlayProps[$name]);
+            } elseif ($prop->isInitialized($base)) {
+                $prop->setValue($clone, $prop->getValue($base));
+            }
+        }
+
+        return $clone;
+    }
+
     /**
      * Extract persistable columns, resolving entity objects to their reference representations.
      *
@@ -199,7 +239,11 @@ public function extractProperties(object $entity): array
         return $props;
     }
 
-    /** @return array<string, true> */
+    /**
+     * @param class-string $class
+     *
+     * @return array<string, true>
+     */
     private function detectRelationProperties(string $class): array
     {
         if (isset($this->relationCache[$class])) {
@@ -222,13 +266,21 @@ private function detectRelationProperties(string $class): array
         return $this->relationCache[$class] = $relations;
     }
 
-    /** @return ReflectionClass<object> */
+    /**
+     * @param class-string $class
+     *
+     * @return ReflectionClass<object>
+     */
     private function reflectClass(string $class): ReflectionClass
     {
-        return $this->classCache[$class] ??= new ReflectionClass($class); // @phpstan-ignore argument.type
+        return $this->classCache[$class] ??= new ReflectionClass($class);
     }
 
-    /** @return array<string, ReflectionProperty> */
+    /**
+     * @param class-string $class
+     *
+     * @return array<string, ReflectionProperty>
+     */
     private function reflectProperties(string $class): array
     {
         if (!isset($this->propertyCache[$class])) {

src/Hydrators/Flat.php

@@ -50,7 +50,7 @@ public function hydrate(
             );
 
             $entityFactory->set(
-                /** @phpstan-ignore argument.type */
+                /** @phpstan-ignore argument.type (array_pop returns object|null but SplObjectStorage guarantees object key) */
                 $entityInstance,
                 $columnName,
                 $value,

tests/AbstractMapperTest.php

@@ -758,9 +758,10 @@ public function persistUntrackedEntityWithMatchingPkUpdates(): void
         /** @var SplObjectStorage<object, string> $pending */
         $pending = $pendingProp->getValue($mapper);
 
-        $this->assertSame('update', $pending[$replacement]);
-        $this->assertFalse($mapper->isTracked($fetched));
-        $this->assertTrue($mapper->isTracked($replacement));
+        $this->assertSame('update', $pending[$fetched]);
+        $this->assertTrue($mapper->isTracked($fetched));
+        $this->assertFalse($mapper->isTracked($replacement));
+        $this->assertSame('Updated', $fetched->title);
     }
 
     #[Test]
@@ -791,20 +792,23 @@ public function persistReadOnlyViaCollectionPkUpdates(): void
 
         // Create new readonly entity (no PK) and persist via collection[pk]
         $updated = $mapper->entityFactory->create(Stubs\ReadOnlyAuthor::class, name: 'Updated', bio: 'new bio');
-        $mapper->read_only_author[1]->persist($updated);
+        $merged = $mapper->read_only_author[1]->persist($updated);
 
-        // PK should have been set from collection condition
-        $this->assertSame(1, $updated->id);
+        // Merged entity should combine both: PK from fetched, changes from updated
+        $this->assertSame(1, $merged->id);
+        $this->assertSame('Updated', $merged->name);
+        $this->assertSame('new bio', $merged->bio);
 
-        // Old entity should be evicted
+        // Merged entity should be tracked, old fetched evicted
         $this->assertFalse($mapper->isTracked($fetched));
-        $this->assertTrue($mapper->isTracked($updated));
+        $this->assertFalse($mapper->isTracked($updated));
+        $this->assertTrue($mapper->isTracked($merged));
 
         $ref = new ReflectionObject($mapper);
         $pendingProp = $ref->getProperty('pending');
         /** @var SplObjectStorage<object, string> $pending */
         $pending = $pendingProp->getValue($mapper);
-        $this->assertSame('update', $pending[$updated]);
+        $this->assertSame('update', $pending[$merged]);
     }
 
     #[Test]
@@ -1109,12 +1113,13 @@ public function readOnlyMultipleEntitiesFetchAllTracksAll(): void
 
         // Replace one by identity map lookup
         $updated = $mapper->entityFactory->create(Stubs\Immutable\Author::class, name: 'Alice Updated');
-        $mapper->author[1]->persist($updated);
+        $merged = $mapper->author[1]->persist($updated);
 
-        // Original Alice should be evicted, updated Alice takes its place
+        // Original Alice should be evicted, merged entity takes its place
         $this->assertSame(3, $mapper->trackedCount());
-        $this->assertTrue($mapper->isTracked($updated));
+        $this->assertTrue($mapper->isTracked($merged));
         $this->assertFalse($mapper->isTracked($authors[0]));
+        $this->assertSame('Alice Updated', $merged->name);
     }
 
     #[Test]
@@ -1129,11 +1134,12 @@ public function identityMapReplaceSkipsSetWhenPkAlreadyInitialized(): void
 
         $updated = new Stubs\Immutable\Author(id: 1, name: 'Bob');
 
-        // persist via collection[1] — PK already set, should NOT try set() again
-        $mapper->author[1]->persist($updated);
+        // persist via collection[1] — PK already set, merge produces new entity
+        $merged = $mapper->author[1]->persist($updated);
 
-        $this->assertSame(1, $updated->id);
-        $this->assertTrue($mapper->isTracked($updated));
+        $this->assertSame(1, $merged->id);
+        $this->assertSame('Bob', $merged->name);
+        $this->assertTrue($mapper->isTracked($merged));
     }
 
     #[Test]