Skip to content

Commit bc63e1e

Browse files
alganetalganet
committed
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.
1 parent d1664a8 commit bc63e1e

8 files changed

Lines changed: 348 additions & 57 deletions

File tree

‎src/AbstractMapper.php‎

Lines changed: 56 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@
1010

1111
use function array_flip;
1212
use function array_intersect_key;
13-
use function assert;
1413
use function count;
14+
use function ctype_digit;
1515
use function is_int;
1616
use function is_scalar;
1717
use function is_string;
@@ -96,8 +96,9 @@ public function persist(object $object, Collection $onCollection): object
9696
return $object;
9797
}
9898

99-
if ($onCollection->name !== null && $this->tryReplaceFromIdentityMap($object, $onCollection)) {
100-
return $object;
99+
$merged = $this->tryMergeWithIdentityMap($object, $onCollection);
100+
if ($merged !== null) {
101+
return $merged;
101102
}
102103

103104
$this->pending[$object] = 'insert';
@@ -202,53 +203,84 @@ protected function findInIdentityMap(Collection $collection): object|null
202203
return null;
203204
}
204205

205-
$condition = $collection->condition;
206-
if (!is_int($condition) && !is_string($condition)) {
206+
$condition = $this->normalizeIdValue($collection->condition);
207+
if ($condition === null) {
207208
return null;
208209
}
209210

210211
return $this->identityMap[$collection->name][$condition] ?? null;
211212
}
212213

213-
private function tryReplaceFromIdentityMap(object $entity, Collection $coll): bool
214+
private function tryMergeWithIdentityMap(object $entity, Collection $coll): object|null
214215
{
215-
assert($coll->name !== null);
216-
$entityId = $this->entityIdValue($entity, $coll->name);
217-
$idValue = $entityId;
218-
219-
if ($idValue === null && is_scalar($coll->condition)) {
220-
$idValue = $coll->condition;
216+
if ($coll->name === null) {
217+
return null;
221218
}
222219

223-
if ($idValue === null || (!is_int($idValue) && !is_string($idValue))) {
224-
return false;
220+
$entityId = $this->entityIdValue($entity, $coll->name);
221+
$idValue = $entityId ?? $this->normalizeIdValue($coll->condition);
222+
223+
if ($idValue === null) {
224+
return null;
225225
}
226226

227227
$existing = $this->identityMap[$coll->name][$idValue] ?? null;
228228
if ($existing === null || $existing === $entity) {
229-
return false;
229+
return null;
230230
}
231231

232232
if ($entityId === null) {
233233
$idName = $this->style->identifier($coll->name);
234234
$this->entityFactory->set($entity, $idName, $idValue);
235235
}
236236

237-
$this->tracked->offsetUnset($existing);
238-
$this->pending->offsetUnset($existing);
239-
$this->evictFromIdentityMap($existing, $coll);
240-
$this->markTracked($entity, $coll);
241-
$this->registerInIdentityMap($entity, $coll);
242-
$this->pending[$entity] = 'update';
237+
if ($this->entityFactory->isReadOnly($existing)) {
238+
$merged = $this->entityFactory->mergeEntities($existing, $entity);
243239

244-
return true;
240+
if ($merged !== $existing) {
241+
$this->tracked->offsetUnset($existing);
242+
$this->pending->offsetUnset($existing);
243+
$this->evictFromIdentityMap($existing, $coll);
244+
$this->markTracked($merged, $coll);
245+
$this->registerInIdentityMap($merged, $coll);
246+
}
247+
248+
$this->pending[$merged] = 'update';
249+
250+
return $merged;
251+
}
252+
253+
foreach ($this->entityFactory->extractProperties($entity) as $prop => $value) {
254+
$this->entityFactory->set($existing, $prop, $value);
255+
}
256+
257+
if (!$this->isTracked($existing)) {
258+
$this->markTracked($existing, $coll);
259+
}
260+
261+
$this->pending[$existing] = 'update';
262+
263+
return $existing;
245264
}
246265

247266
private function entityIdValue(object $entity, string $collName): int|string|null
248267
{
249-
$idValue = $this->entityFactory->get($entity, $this->style->identifier($collName));
268+
return $this->normalizeIdValue(
269+
$this->entityFactory->get($entity, $this->style->identifier($collName)),
270+
);
271+
}
272+
273+
private function normalizeIdValue(mixed $value): int|string|null
274+
{
275+
if (is_int($value)) {
276+
return $value;
277+
}
278+
279+
if (is_string($value)) {
280+
return ctype_digit($value) ? (int) $value : $value;
281+
}
250282

251-
return is_int($idValue) || is_string($idValue) ? $idValue : null;
283+
return null;
252284
}
253285

254286
public function __get(string $name): Collection

‎src/CollectionNotBound.php‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Respect\Data;
6+
7+
use RuntimeException;
8+
9+
final class CollectionNotBound extends RuntimeException
10+
{
11+
public function __construct(string|null $collectionName)
12+
{
13+
parent::__construct(
14+
'Collection \'' . ($collectionName ?? '(unnamed)')
15+
. '\' must be attached to a mapper before fetching or persisting',
16+
);
17+
}
18+
}

‎src/Collections/Collection.php‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,8 @@
66

77
use ArrayAccess;
88
use Respect\Data\AbstractMapper;
9+
use Respect\Data\CollectionNotBound;
910
use Respect\Data\Hydrator;
10-
use RuntimeException;
1111

1212
/** @implements ArrayAccess<string, Collection> */
1313
class Collection implements ArrayAccess
@@ -65,9 +65,7 @@ public function persist(object $object, mixed ...$changes): object
6565
}
6666
}
6767

68-
$mapper->persist($object, $this);
69-
70-
return $object;
68+
return $mapper->persist($object, $this);
7169
}
7270

7371
public function remove(object $object): bool
@@ -150,7 +148,7 @@ private function findMapper(): AbstractMapper|null
150148

151149
private function resolveMapper(): AbstractMapper
152150
{
153-
return $this->findMapper() ?? throw new RuntimeException();
151+
return $this->findMapper() ?? throw new CollectionNotBound($this->name);
154152
}
155153

156154
private function setNext(Collection $collection): void

‎src/EntityFactory.php‎

Lines changed: 55 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,10 @@
2626
/** Creates and manipulates entity objects using Style-based naming conventions */
2727
class EntityFactory
2828
{
29-
/** @var array<string, ReflectionClass<object>> */
29+
/** @var array<class-string, ReflectionClass<object>> */
3030
private array $classCache = [];
3131

32-
/** @var array<string, array<string, ReflectionProperty>> */
32+
/** @var array<class-string, array<string, ReflectionProperty>> */
3333
private array $propertyCache = [];
3434

3535
/** @var array<string, class-string> */
@@ -156,6 +156,43 @@ public function withChanges(object $entity, mixed ...$changes): object
156156
return $clone;
157157
}
158158

159+
public function mergeEntities(object $base, object $overlay): object
160+
{
161+
if ($base::class !== $overlay::class) {
162+
throw new DomainException(
163+
'Cannot merge entities of different classes: ' . $base::class . ' and ' . $overlay::class,
164+
);
165+
}
166+
167+
$overlayProps = $this->extractProperties($overlay);
168+
$hasDifference = false;
169+
170+
foreach ($overlayProps as $name => $value) {
171+
$mirror = $this->reflectProperties($base::class)[$name];
172+
173+
if (!$mirror->isInitialized($base) || $mirror->getValue($base) !== $value) {
174+
$hasDifference = true;
175+
break;
176+
}
177+
}
178+
179+
if (!$hasDifference) {
180+
return $base;
181+
}
182+
183+
$clone = $this->reflectClass($base::class)->newInstanceWithoutConstructor();
184+
185+
foreach ($this->reflectProperties($base::class) as $name => $prop) {
186+
if (array_key_exists($name, $overlayProps)) {
187+
$prop->setValue($clone, $overlayProps[$name]);
188+
} elseif ($prop->isInitialized($base)) {
189+
$prop->setValue($clone, $prop->getValue($base));
190+
}
191+
}
192+
193+
return $clone;
194+
}
195+
159196
/**
160197
* Extract persistable columns, resolving entity objects to their reference representations.
161198
*
@@ -199,7 +236,11 @@ public function extractProperties(object $entity): array
199236
return $props;
200237
}
201238

202-
/** @return array<string, true> */
239+
/**
240+
* @param class-string $class
241+
*
242+
* @return array<string, true>
243+
*/
203244
private function detectRelationProperties(string $class): array
204245
{
205246
if (isset($this->relationCache[$class])) {
@@ -222,13 +263,21 @@ private function detectRelationProperties(string $class): array
222263
return $this->relationCache[$class] = $relations;
223264
}
224265

225-
/** @return ReflectionClass<object> */
266+
/**
267+
* @param class-string $class
268+
*
269+
* @return ReflectionClass<object>
270+
*/
226271
private function reflectClass(string $class): ReflectionClass
227272
{
228-
return $this->classCache[$class] ??= new ReflectionClass($class); // @phpstan-ignore argument.type
273+
return $this->classCache[$class] ??= new ReflectionClass($class);
229274
}
230275

231-
/** @return array<string, ReflectionProperty> */
276+
/**
277+
* @param class-string $class
278+
*
279+
* @return array<string, ReflectionProperty>
280+
*/
232281
private function reflectProperties(string $class): array
233282
{
234283
if (!isset($this->propertyCache[$class])) {

‎src/Hydrators/Flat.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ public function hydrate(
5050
);
5151

5252
$entityFactory->set(
53-
/** @phpstan-ignore argument.type */
53+
/** @phpstan-ignore argument.type (array_pop returns object|null but SplObjectStorage guarantees object key) */
5454
$entityInstance,
5555
$columnName,
5656
$value,

0 commit comments

Comments
 (0)