Skip to content

Commit 4768a48

Browse files
alganetalganet
committed
Simplify Collection subtype DSL and improve type safety
Replace verbose two-step factory patterns (Filtered::by()->name(), Composite::with()->name(), Typed::by()->name()) with direct static calls (Filtered::name(), Composite::name(), Typed::name()). - Make $name the first, non-null constructor parameter on subtypes - Remove $condition from subtype constructors - Remove Filtered::by(), Composite::with(), Typed::by(), Collection::using() - Extract with() from __call for shared arg processing - Variadic constructor on Collection replaces separate condition param - AbstractMapper::__call clones registered collections with conditions
1 parent b10c37d commit 4768a48

14 files changed

Lines changed: 150 additions & 103 deletions

‎phpstan.neon.dist‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ parameters:
44
- src/
55
- tests/
66
ignoreErrors:
7-
- message: '/Call to an undefined (static )?method Respect\\Data\\(AbstractMapper|Collections\\(Collection|Filtered|Composite|Typed))::\w+\(\)\./'
7+
- message: '/Call to an undefined (static )?method Respect\\Data\\(AbstractMapper|InMemoryMapper|Collections\\(Collection|Filtered|Composite|Typed))::\w+\(\)\./'
88
- message: '/Access to an undefined property Respect\\Data\\(AbstractMapper|InMemoryMapper|Collections\\Collection)::\$\w+\./'
99
- message: '/Unsafe usage of new static\(\)\./'
1010
-

‎src/AbstractMapper.php‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -132,10 +132,17 @@ public function __set(string $alias, Collection $collection): void
132132
$this->registerCollection($alias, $collection);
133133
}
134134

135-
/** @param array<int, mixed> $children */
136-
public function __call(string $name, array $children): Collection
135+
/** @param list<Collection|array<scalar, mixed>|scalar|null> $arguments */
136+
public function __call(string $name, array $arguments): Collection
137137
{
138-
$collection = Collection::__callstatic($name, $children);
138+
if (isset($this->collections[$name])) {
139+
$collection = clone $this->collections[$name];
140+
$collection->mapper = $this;
141+
142+
return $collection->with(...$arguments);
143+
}
144+
145+
$collection = Collection::__callstatic($name, $arguments);
139146
$collection->mapper = $this;
140147

141148
return $collection;

‎src/Collections/Collection.php‎

Lines changed: 52 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,6 @@
1010
use Respect\Data\Hydrator;
1111
use RuntimeException;
1212

13-
use function is_array;
14-
use function is_scalar;
15-
1613
/** @implements ArrayAccess<string, Collection> */
1714
class Collection implements ArrayAccess
1815
{
@@ -35,17 +32,15 @@ class Collection implements ArrayAccess
3532

3633
public bool $more { get => $this->hasChildren || $this->next !== null; }
3734

38-
/** @param array<mixed>|scalar|null $condition */
35+
/** @var array<scalar, mixed>|scalar|null */
36+
public private(set) array|int|float|string|bool|null $condition = [];
37+
38+
/** @param (Collection|array<scalar, mixed>|scalar|null) ...$args */
3939
public function __construct(
4040
public private(set) string|null $name = null,
41-
public private(set) array|int|float|string|bool|null $condition = [],
41+
self|array|int|float|string|bool|null ...$args,
4242
) {
43-
}
44-
45-
/** @param array<mixed>|scalar|null $condition */
46-
public static function using(array|int|float|string|bool|null $condition): static
47-
{
48-
return new static(condition: $condition);
43+
$this->with(...$args);
4944
}
5045

5146
public function addChild(Collection $child): void
@@ -120,6 +115,16 @@ public function stack(Collection $collection): static
120115
return $this;
121116
}
122117

118+
/** @param self|array<scalar, mixed>|scalar|null ...$arguments */
119+
public function with(self|array|int|float|string|bool|null ...$arguments): static
120+
{
121+
foreach ($arguments as $arg) {
122+
$arg instanceof Collection ? $this->addChild($arg) : $this->condition = $arg;
123+
}
124+
125+
return $this;
126+
}
127+
123128
private function findMapper(): AbstractMapper|null
124129
{
125130
$node = $this;
@@ -145,12 +150,10 @@ private function setNext(Collection $collection): void
145150
$this->next = $collection;
146151
}
147152

148-
/** @param array<int, mixed> $children */
149-
public static function __callStatic(string $name, array $children): static
153+
/** @param array<int, mixed> $arguments */
154+
public static function __callStatic(string $name, array $arguments): static
150155
{
151-
$collection = new static();
152-
153-
return $collection->__call($name, $children);
156+
return new static($name, ...$arguments);
154157
}
155158

156159
public function __get(string $name): static
@@ -163,22 +166,46 @@ public function __get(string $name): static
163166
return $this->stack(new self($name));
164167
}
165168

166-
/** @param array<int, mixed> $children */
169+
/** @param list<self|array<scalar, mixed>|scalar|null> $children */
167170
public function __call(string $name, array $children): static
168171
{
169172
if (!isset($this->name)) {
170173
$this->name = $name;
171-
foreach ($children as $child) {
172-
if ($child instanceof Collection) {
173-
$this->addChild($child);
174-
} elseif (is_array($child) || is_scalar($child) || $child === null) {
175-
$this->condition = $child;
176-
}
177-
}
178174

179-
return $this;
175+
return $this->with(...$children);
180176
}
181177

182178
return $this->stack((new Collection())->__call($name, $children));
183179
}
180+
181+
public function __clone(): void
182+
{
183+
if ($this->next !== null) {
184+
$this->next = clone $this->next;
185+
$this->next->parent = $this;
186+
}
187+
188+
$clonedChildren = [];
189+
190+
foreach ($this->children as $child) {
191+
$cloned = clone $child;
192+
$cloned->parent = $this;
193+
$clonedChildren[] = $cloned;
194+
}
195+
196+
$this->children = $clonedChildren;
197+
$this->parent = null;
198+
199+
if ($this->last === null) {
200+
return;
201+
}
202+
203+
$node = $this;
204+
205+
while ($node->next !== null) {
206+
$node = $node->next;
207+
}
208+
209+
$this->last = $node !== $this ? $node : null;
210+
}
184211
}

‎src/Collections/Composite.php‎

Lines changed: 6 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -6,29 +6,17 @@
66

77
final class Composite extends Collection
88
{
9-
/**
10-
* @param array<string, list<string>> $compositions
11-
* @param array<mixed>|scalar|null $condition
12-
*/
9+
/** @param array<string, list<string>> $compositions */
1310
public function __construct(
11+
string $name,
1412
public private(set) readonly array $compositions = [],
15-
string|null $name = null,
16-
array|int|float|string|bool|null $condition = [],
1713
) {
18-
parent::__construct($name, $condition);
19-
}
20-
21-
/** @param array<string, list<string>> $compositions */
22-
public static function with(array $compositions): static
23-
{
24-
return new static(compositions: $compositions);
14+
parent::__construct($name);
2515
}
2616

27-
/** @param array<int, mixed> $children */
28-
public static function __callStatic(string $name, array $children): static
17+
/** @param array<int, array<string, list<string>>> $arguments */
18+
public static function __callStatic(string $name, array $arguments): static
2919
{
30-
$collection = new static();
31-
32-
return $collection->__call($name, $children);
20+
return new static($name, ...$arguments);
3321
}
3422
}

‎src/Collections/Filtered.php‎

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -14,28 +14,17 @@ final class Filtered extends Collection
1414
// phpcs:ignore PSR2.Classes.PropertyDeclaration
1515
public bool $identifierOnly { get => $this->filters === [self::IDENTIFIER_ONLY]; }
1616

17-
/**
18-
* @param list<string> $filters
19-
* @param array<mixed>|scalar|null $condition
20-
*/
17+
/** @param list<string> $filters */
2118
public function __construct(
19+
string $name,
2220
public private(set) readonly array $filters = [],
23-
string|null $name = null,
24-
array|int|float|string|bool|null $condition = [],
2521
) {
26-
parent::__construct($name, $condition);
22+
parent::__construct($name);
2723
}
2824

29-
public static function by(string ...$names): static
25+
/** @param array<scalar, string> $arguments */
26+
public static function __callStatic(string $name, array $arguments): static
3027
{
31-
return new static(filters: array_values($names));
32-
}
33-
34-
/** @param array<int, mixed> $children */
35-
public static function __callStatic(string $name, array $children): static
36-
{
37-
$collection = new static();
38-
39-
return $collection->__call($name, $children);
28+
return new static($name, array_values($arguments));
4029
}
4130
}

‎src/Collections/Typed.php‎

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -10,18 +10,11 @@
1010

1111
final class Typed extends Collection
1212
{
13-
/** @param array<mixed>|scalar|null $condition */
1413
public function __construct(
14+
string $name,
1515
public private(set) readonly string $type = '',
16-
string|null $name = null,
17-
array|int|float|string|bool|null $condition = [],
1816
) {
19-
parent::__construct($name, $condition);
20-
}
21-
22-
public static function by(string $type): static
23-
{
24-
return new static(type: $type);
17+
parent::__construct($name);
2518
}
2619

2720
public function resolveEntityName(EntityFactory $factory, object $row): string
@@ -31,11 +24,9 @@ public function resolveEntityName(EntityFactory $factory, object $row): string
3124
return is_string($name) ? $name : ($this->name ?? '');
3225
}
3326

34-
/** @param array<int, mixed> $children */
35-
public static function __callStatic(string $name, array $children): static
27+
/** @param array<int, string> $arguments */
28+
public static function __callStatic(string $name, array $arguments): static
3629
{
37-
$collection = new static();
38-
39-
return $collection->__call($name, $children);
30+
return new static($name, ...$arguments);
4031
}
4132
}

‎tests/AbstractMapperTest.php‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,59 @@ public function hydrationMatchesIntFkToStringPk(): void
283283
$this->assertEquals('5', $mapper->entityFactory->get($post, 'id'));
284284
}
285285

286+
#[Test]
287+
public function callingRegisteredCollectionClonesAndAppliesCondition(): void
288+
{
289+
$mapper = new InMemoryMapper();
290+
$mapper->seed('post', [
291+
['id' => 1, 'title' => 'Hello'],
292+
['id' => 2, 'title' => 'World'],
293+
]);
294+
295+
$mapper->postTitles = Filtered::posts('title');
296+
297+
$conditioned = $mapper->postTitles(['id' => 2]);
298+
299+
$this->assertInstanceOf(Filtered::class, $conditioned);
300+
$this->assertEquals('posts', $conditioned->name);
301+
$this->assertEquals(['title'], $conditioned->filters);
302+
$this->assertEquals(['id' => 2], $conditioned->condition);
303+
$this->assertEquals([], $mapper->postTitles->condition, 'Original collection should be unchanged');
304+
}
305+
306+
#[Test]
307+
public function callingRegisteredCollectionWithoutConditionReturnsClone(): void
308+
{
309+
$mapper = new InMemoryMapper();
310+
$mapper->postTitles = Filtered::posts('title');
311+
312+
$clone = $mapper->postTitles();
313+
314+
$this->assertInstanceOf(Filtered::class, $clone);
315+
$this->assertNotSame($mapper->postTitles, $clone);
316+
$this->assertEquals('posts', $clone->name);
317+
$this->assertEquals(['title'], $clone->filters);
318+
}
319+
320+
#[Test]
321+
public function callingRegisteredChainedCollectionDoesNotMutateTemplate(): void
322+
{
323+
$mapper = new InMemoryMapper();
324+
$mapper->seed('post', []);
325+
$mapper->seed('comment', []);
326+
327+
$mapper->commentedPosts = Collection::posts()->comment();
328+
329+
$clone = $mapper->commentedPosts();
330+
$clone->author; // stacks 'author' onto the clone's chain
331+
332+
$original = $mapper->commentedPosts;
333+
$this->assertNull(
334+
$original->next?->next,
335+
'Stacking on a clone should not mutate the registered collection',
336+
);
337+
}
338+
286339
#[Test]
287340
public function filteredPersistDelegatesToParentCollection(): void
288341
{

‎tests/CollectionIteratorTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ public function getChildrenConsiderEmpties(): void
7575
#[Test]
7676
public function getChildrenUseCollectionChildren(): void
7777
{
78-
$coll = Collection::foo(Collection::bar(), Collection::baz());
78+
$coll = Collection::foo()->with(Collection::bar(), Collection::baz());
7979
[$fooChild, $barChild] = $coll->children;
8080
$items = iterator_to_array(CollectionIterator::recursive($coll));
8181
$this->assertContains($fooChild, $items);

‎tests/Collections/CollectionTest.php‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -296,14 +296,6 @@ public function fetchAllOnCollectionShouldExceptionIfMapperDontExist(): void
296296
Collection::foo()->fetchAll();
297297
}
298298

299-
#[Test]
300-
public function usingShouldCreateCollectionWithCondition(): void
301-
{
302-
$coll = Collection::using(42);
303-
$this->assertInstanceOf(Collection::class, $coll);
304-
$this->assertEquals(42, $coll->condition);
305-
}
306-
307299
#[Test]
308300
public function getParentShouldReturnNullWhenNoParent(): void
309301
{

‎tests/Collections/CompositeTest.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,8 @@ class CompositeTest extends TestCase
1616
#[Test]
1717
public function collectionCanBeCreatedStaticallyWithChildren(): void
1818
{
19-
$children1 = Composite::with(['foo' => ['bar']])->bar();
20-
$children2 = Composite::with(['bat' => ['bar']])->baz()->bat();
19+
$children1 = Composite::bar(['foo' => ['bar']]);
20+
$children2 = Composite::baz(['bat' => ['bar']])->bat();
2121
$coll = Collection::foo($children1, $children2)->bar();
2222
$this->assertInstanceOf(Collection::class, $coll);
2323
$this->assertInstanceOf(Collection::class, $coll->next);

0 commit comments

Comments
 (0)