Repository: spatie/data-transfer-object (php)

Issue:
ArrayAccess limitations
@swaner mentioned an issue with the `ArrayAccess` casting found in the `ArrayCaster` class in #206. I have a fix for that specific issue, but in implementing it I have discovered some oversights in my original implementation of the `ArrayAccess` caster which causes it to not work for very basic `ArrayAccess` implementations that do not also implement `IteratorAggregate`.

My current thinking is to implement some validation in the `ArrayCaster` that ensures the object also implements `IteratorAggregate`. I don't think this would really be a breaking change, as it would simply add in a check for a scenario we couldn't cover. As a bonus, we would also gain support for `ArrayObject` implementations in that caster.

@brendt - Any thoughts here? Happy to create a PR if this sounds like the right course.

Pull request:
Fix ArrayAccess limitations
This PR fixes #216 and the limitations it describes.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c9fba81..71fc571 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,12 @@
 
 All notable changes to `data-transfer-object` will be documented in this file
 
+## Unreleased
+
+- Fix for an empty value being created when casting `ArrayAccess` objects (#216)
+- Add logic exception when attempting to cast `ArrayAccess` objects that are not traversable (#216)
+- Allow the `ArrayCaster` to retain values that are already instances of the `itemType` (#217)
+
 ## 3.3.0 - 2021-06-01
 
 - Expose DTO and validation error array in ValidationException (#213)
diff --git a/README.md b/README.md
index 0f1b688..adaf8ed 100644
--- a/README.md
+++ b/README.md
@@ -176,53 +176,11 @@ abstract class BaseDataTransferObject extends DataTransferObject
 
 ### Using custom caster arguments
 
-Any caster can be passed custom arguments, the built-in `ArrayCaster` implementation is such an example:
+Any caster can be passed custom arguments, the built-in [`ArrayCaster` implementation](https://github.com/spatie/data-transfer-object/blob/master/src/Casters/ArrayCaster.php) is a good example of how this may be used.
 
-```php
-class ArrayCaster implements Caster
-{
-    public function __construct(
-        private string $type,
-        private string $itemType,
-    ) {
-    }
-
-    public function cast(mixed $value): array|ArrayAccess
-    {
-        if ($this->type == 'array') {
-            return $this->castArray($value);
-        }
-
-        if (is_subclass_of($this->type, ArrayAccess::class)) {
-            return $this->castArrayAccess($value);
-        }
-
-        throw new LogicException("Caster [ArrayCaster] may only be used to cast arrays or objects that implement ArrayAccess.");
-    }
-
-    private function castArray(mixed $value): array
-    {
-        return array_map(
-            fn(array $data) => new $this->itemType(...$data),
-            $value
-        );
-    }
-
-    private function castArrayAccess(mixed $value): ArrayAccess
-    {
-        $arrayAccess = new $this->type();
-
-        array_walk(
-            $value,
-            fn(array $data) => $arrayAccess[] = new $this->itemType(...$data)
-        );
-
-        return $arrayAccess;
-    }
-}
-```
+Using named arguments when passing input to your caster will help make your code more clear, but they are not required.
 
-Note that you don't need to use named arguments to pass input as generic caster arguments, though they do make it more clear:
+For example:
 
 ```php
     /** @var \Spatie\DataTransferObject\Tests\Foo[] */
@@ -234,7 +192,7 @@ Note that you don't need to use named arguments to pass input as generic caster
     public array $collectionWithoutNamedArguments;
 ```
 
-Also note that the first argument passed to the caster constructor is always the type of the value being casted.
+Note that the first argument passed to the caster constructor is always the type of the value being casted.
 All other arguments will be the ones passed as extra arguments in the `CastWith` attribute.
 
 ## Validation
diff --git a/src/Casters/ArrayCaster.php b/src/Casters/ArrayCaster.php
index cede4b3..8b00c2a 100644
--- a/src/Casters/ArrayCaster.php
+++ b/src/Casters/ArrayCaster.php
@@ -5,6 +5,7 @@ namespace Spatie\DataTransferObject\Casters;
 use ArrayAccess;
 use LogicException;
 use Spatie\DataTransferObject\Caster;
+use Traversable;
 
 class ArrayCaster implements Caster
 {
@@ -17,33 +18,51 @@ class ArrayCaster implements Caster
     public function cast(mixed $value): array | ArrayAccess
     {
         if ($this->type == 'array') {
-            return $this->castArray($value);
+            return $this->mapInto(
+                destination: [],
+                items: $value
+            );
         }
 
         if (is_subclass_of($this->type, ArrayAccess::class)) {
-            return $this->castArrayAccess($value);
+            return $this->mapInto(
+                destination: new $this->type(),
+                items: $value
+            );
         }
 
-        throw new LogicException("Caster [ArrayCaster] may only be used to cast arrays or objects that implement ArrayAccess.");
+        throw new LogicException(
+            "Caster [ArrayCaster] may only be used to cast arrays or objects that implement ArrayAccess."
+        );
     }
 
-    private function castArray(mixed $value): array
+    private function mapInto(array | ArrayAccess $destination, mixed $items): array | ArrayAccess
     {
-        return array_map(
-            fn (array $data) => new $this->itemType(...$data),
-            $value
-        );
+        if ($destination instanceof ArrayAccess && ! is_subclass_of($destination, Traversable::class)) {
+            throw new LogicException(
+                "Caster [ArrayCaster] may only be used to cast ArrayAccess objects that are traversable."
+            );
+        }
+
+        foreach ($items as $key => $item) {
+            $destination[$key] = $this->castItem($item);
+        }
+
+        return $destination;
     }
 
-    private function castArrayAccess(mixed $value): ArrayAccess
+    private function castItem(mixed $data)
     {
-        $arrayAccess = new $this->type();
+        if ($data instanceof $this->itemType) {
+            return $data;
+        }
 
-        array_walk(
-            $value,
-            fn (array $data) => $arrayAccess[] = new $this->itemType(...$data)
-        );
+        if (is_array($data)) {
+            return new $this->itemType(...$data);
+        }
 
-        return $arrayAccess;
+        throw new LogicException(
+            "Caster [ArrayCaster] each item must be an array or an instance of the specified item type [{$this->itemType}]."
+        );
     }
 }


Test patch:
diff --git a/tests/CustomCasterArgumentsTest.php b/tests/CustomCasterArgumentsTest.php
index 33f06b0..a41970d 100644
--- a/tests/CustomCasterArgumentsTest.php
+++ b/tests/CustomCasterArgumentsTest.php
@@ -2,7 +2,12 @@
 
 namespace Spatie\DataTransferObject\Tests;
 
+use ArrayAccess;
+use ArrayIterator;
+use ArrayObject;
 use Illuminate\Support\Collection;
+use Iterator;
+use IteratorAggregate;
 use LogicException;
 use Spatie\DataTransferObject\Attributes\CastWith;
 use Spatie\DataTransferObject\Casters\ArrayCaster;
@@ -17,12 +22,12 @@ class CustomCasterArgumentsTest extends TestCase
             [
                 'collectionOfFoo' => [
                     ['name' => 'a'],
-                    ['name' => 'b'],
+                    new Foo(name: 'b'),
                     ['name' => 'c'],
                 ],
                 'collectionOfBaz' => [
                     ['name' => 'a'],
-                    ['name' => 'b'],
+                    new Baz(name: 'b'),
                     ['name' => 'c'],
                 ],
             ]
@@ -37,9 +42,50 @@ class CustomCasterArgumentsTest extends TestCase
         $this->assertContainsOnlyInstancesOf(Baz::class, $bar->collectionOfBaz);
     }
 
-    public function test_generic_array_caster_on_array_access_type()
+    public function test_generic_array_caster_on_invalid_type()
     {
-        $bar = new BarJr(
+        $this->expectException(LogicException::class);
+        $this->expectExceptionMessage('Caster [ArrayCaster] may only be used to cast arrays or objects that implement ArrayAccess.');
+
+        new BarIllogical(
+            [
+                'collectionOfFoo' => [
+                    ['name' => 'a'],
+                    new Foo(name: 'b'),
+                    ['name' => 'c'],
+                ],
+                'collectionOfBaz' => [
+                    ['name' => 'a'],
+                    new Baz(name: 'b'),
+                    ['name' => 'c'],
+                ],
+            ]
+        );
+    }
+
+    /**
+     * @see https://github.com/spatie/data-transfer-object/issues/216
+     */
+    public function test_casting_an_empty_array_object_will_not_add_ghost_value()
+    {
+        $object = new BarJr([
+            'collectionOfFoo' => new Collection(),
+            'collectionOfBaz' => new Collection(),
+        ]);
+
+        $this->assertInstanceOf(Collection::class, $object->collectionOfFoo);
+        $this->assertEmpty($object->collectionOfFoo);
+
+        $this->assertInstanceOf(Collection::class, $object->collectionOfBaz);
+        $this->assertEmpty($object->collectionOfBaz);
+    }
+
+    public function test_it_cannot_cast_array_access_without_traversable()
+    {
+        $this->expectException(LogicException::class);
+        $this->expectExceptionMessage('Caster [ArrayCaster] may only be used to cast ArrayAccess objects that are traversable.');
+
+        new DTOWithArrayAccessImplementation(
             [
                 'collectionOfFoo' => [
                     ['name' => 'a'],
@@ -53,35 +99,163 @@ class CustomCasterArgumentsTest extends TestCase
                 ],
             ]
         );
+    }
 
-        $this->assertInstanceOf(Collection::class, $bar->collectionOfFoo);
-        $this->assertCount(3, $bar->collectionOfFoo);
-        $this->assertContainsOnlyInstancesOf(Foo::class, $bar->collectionOfFoo);
+    public function test_it_can_cast_iterator_aggregate()
+    {
+        $object = new DTOWithArrayAccessIteratorAggregate(
+            [
+                'collectionOfFoo' => [
+                    ['name' => 'a'],
+                    ['name' => 'b'],
+                    ['name' => 'c'],
+                ],
+                'collectionOfBaz' => [
+                    ['name' => 'a'],
+                    ['name' => 'b'],
+                    ['name' => 'c'],
+                    ['name' => 'd'],
+                ],
+            ]
+        );
 
-        $this->assertInstanceOf(Collection::class, $bar->collectionOfFoo);
-        $this->assertCount(3, $bar->collectionOfBaz);
-        $this->assertContainsOnlyInstancesOf(Baz::class, $bar->collectionOfBaz);
+        $this->assertInstanceOf(ArrayAccessIteratorAggregate::class, $object->collectionOfFoo);
+        $this->assertContainsOnlyInstancesOf(Foo::class, $object->collectionOfFoo);
+        $this->assertCount(3, $object->collectionOfFoo);
+
+        $this->assertInstanceOf(ArrayAccessIteratorAggregate::class, $object->collectionOfBaz);
+        $this->assertContainsOnlyInstancesOf(Baz::class, $object->collectionOfBaz);
+        $this->assertCount(4, $object->collectionOfBaz);
     }
 
-    public function test_generic_array_caster_on_invalid_type()
+    public function test_it_can_cast_iterator()
+    {
+        $object = new DTOWithArrayAccessIterator(
+            [
+                'collectionOfFoo' => [
+                    ['name' => 'a'],
+                    ['name' => 'b'],
+                    ['name' => 'c'],
+                ],
+                'collectionOfBaz' => [
+                    ['name' => 'a'],
+                    ['name' => 'b'],
+                    ['name' => 'c'],
+                    ['name' => 'd'],
+                ],
+            ]
+        );
+
+        $this->assertInstanceOf(ArrayAccessIterator::class, $object->collectionOfFoo);
+        $this->assertContainsOnlyInstancesOf(Foo::class, $object->collectionOfFoo);
+        $this->assertCount(3, $object->collectionOfFoo);
+
+        $this->assertInstanceOf(ArrayAccessIterator::class, $object->collectionOfBaz);
+        $this->assertContainsOnlyInstancesOf(Baz::class, $object->collectionOfBaz);
+        $this->assertCount(4, $object->collectionOfBaz);
+    }
+
+    public function test_it_can_cast_array_object()
+    {
+        $object = new DTOWithArrayObject(
+            [
+                'collectionOfFoo' => [
+                    ['name' => 'a'],
+                    ['name' => 'b'],
+                    ['name' => 'c'],
+                ],
+                'collectionOfBaz' => [
+                    ['name' => 'a'],
+                    ['name' => 'b'],
+                    ['name' => 'c'],
+                    ['name' => 'd'],
+                ],
+            ]
+        );
+
+        $this->assertInstanceOf(ArrayObject::class, $object->collectionOfFoo);
+        $this->assertContainsOnlyInstancesOf(Foo::class, $object->collectionOfFoo);
+        $this->assertCount(3, $object->collectionOfFoo);
+
+        $this->assertInstanceOf(ArrayObject::class, $object->collectionOfBaz);
+        $this->assertContainsOnlyInstancesOf(Baz::class, $object->collectionOfBaz);
+        $this->assertCount(4, $object->collectionOfBaz);
+    }
+
+    public function test_it_throws_exception_when_casting_non_array()
     {
         $this->expectException(LogicException::class);
-        $this->expectExceptionMessage('Caster [ArrayCaster] may only be used to cast arrays or objects that implement ArrayAccess.');
+        $this->expectExceptionMessage(
+            "Caster [ArrayCaster] each item must be an array or an instance of the specified item type [Spatie\DataTransferObject\Tests\Foo]."
+        );
 
-        new BarIllogical(
+        new DTOWithArrayObject(
+            [
+                'collectionOfFoo' => [
+                    ['name' => 'a'],
+                    1,
+                    ['name' => 'c'],
+                ],
+                'collectionOfBaz' => [
+                    ['name' => 'a'],
+                    ['name' => 'b'],
+                    ['name' => 'c'],
+                    ['name' => 'd'],
+                ],
+            ]
+        );
+    }
+
+    public function test_that_array_keys_get_cast()
+    {
+        $object = new Bar(
             [
                 'collectionOfFoo' => [
+                    'one' => ['name' => 'a'],
+                    'two' => ['name' => 'b'],
+                    'three' => ['name' => 'c'],
+                ],
+                'collectionOfBaz' => [
                     ['name' => 'a'],
                     ['name' => 'b'],
                     ['name' => 'c'],
+                    ['name' => 'd'],
+                ],
+            ]
+        );
+
+        $this->assertArrayHasKey('one', $object->collectionOfFoo);
+        $this->assertArrayHasKey('two', $object->collectionOfFoo);
+        $this->assertArrayHasKey('three', $object->collectionOfFoo);
+        $this->assertInstanceOf(Foo::class, $object->collectionOfFoo['one']);
+        $this->assertInstanceOf(Foo::class, $object->collectionOfFoo['two']);
+        $this->assertInstanceOf(Foo::class, $object->collectionOfFoo['three']);
+    }
+
+    public function test_that_array_object_keys_get_cast()
+    {
+        $object = new DTOWithArrayObject(
+            [
+                'collectionOfFoo' => [
+                    'one' => ['name' => 'a'],
+                    'two' => ['name' => 'b'],
+                    'three' => ['name' => 'c'],
                 ],
                 'collectionOfBaz' => [
                     ['name' => 'a'],
                     ['name' => 'b'],
                     ['name' => 'c'],
+                    ['name' => 'd'],
                 ],
             ]
         );
+
+        $this->assertArrayHasKey('one', $object->collectionOfFoo);
+        $this->assertArrayHasKey('two', $object->collectionOfFoo);
+        $this->assertArrayHasKey('three', $object->collectionOfFoo);
+        $this->assertInstanceOf(Foo::class, $object->collectionOfFoo['one']);
+        $this->assertInstanceOf(Foo::class, $object->collectionOfFoo['two']);
+        $this->assertInstanceOf(Foo::class, $object->collectionOfFoo['three']);
     }
 }
 
@@ -127,3 +301,114 @@ class Baz extends DataTransferObject
 {
     public string $name;
 }
+
+class ArrayAccessImplementation implements ArrayAccess
+{
+    public function __construct(protected array $state = [])
+    {
+    }
+
+    public function offsetExists($offset)
+    {
+        return key_exists($offset, $this->state);
+    }
+
+    public function offsetGet($offset)
+    {
+        return $this->state[$offset];
+    }
+
+    public function offsetSet($offset, $value)
+    {
+        if (! empty($offset)) {
+            $this->state[$offset] = $value;
+        } else {
+            $this->state[] = $value;
+        }
+    }
+
+    public function offsetUnset($offset)
+    {
+        unset($this->state[$offset]);
+    }
+}
+
+class ArrayAccessIteratorAggregate extends ArrayAccessImplementation implements IteratorAggregate
+{
+    public function getIterator()
+    {
+        return new ArrayIterator($this->state);
+    }
+}
+
+class ArrayAccessIterator extends ArrayAccessImplementation implements Iterator
+{
+    public function current()
+    {
+        return current($this->state);
+    }
+
+    public function key()
+    {
+        return key($this->state);
+    }
+
+    public function next()
+    {
+        return next($this->state);
+    }
+
+    public function rewind()
+    {
+        return reset($this->state);
+    }
+
+    public function valid()
+    {
+        return isset($this->state[$this->key()]);
+    }
+}
+
+class DTOWithArrayAccessImplementation extends DataTransferObject
+{
+    /** @var \Spatie\DataTransferObject\Tests\Foo[] */
+    #[CastWith(ArrayCaster::class, itemType: Foo::class)]
+    public ArrayAccessImplementation $collectionOfFoo;
+
+    /** @var \Spatie\DataTransferObject\Tests\Baz[] */
+    #[CastWith(ArrayCaster::class, itemType: Baz::class)]
+    public ArrayAccessImplementation $collectionOfBaz;
+}
+
+class DTOWithArrayAccessIteratorAggregate extends DataTransferObject
+{
+    /** @var \Spatie\DataTransferObject\Tests\Foo[] */
+    #[CastWith(ArrayCaster::class, itemType: Foo::class)]
+    public ArrayAccessIteratorAggregate $collectionOfFoo;
+
+    /** @var \Spatie\DataTransferObject\Tests\Baz[] */
+    #[CastWith(ArrayCaster::class, itemType: Baz::class)]
+    public ArrayAccessIteratorAggregate $collectionOfBaz;
+}
+
+class DTOWithArrayAccessIterator extends DataTransferObject
+{
+    /** @var \Spatie\DataTransferObject\Tests\Foo[] */
+    #[CastWith(ArrayCaster::class, itemType: Foo::class)]
+    public ArrayAccessIterator $collectionOfFoo;
+
+    /** @var \Spatie\DataTransferObject\Tests\Baz[] */
+    #[CastWith(ArrayCaster::class, itemType: Baz::class)]
+    public ArrayAccessIterator $collectionOfBaz;
+}
+
+class DTOWithArrayObject extends DataTransferObject
+{
+    /** @var \Spatie\DataTransferObject\Tests\Foo[] */
+    #[CastWith(ArrayCaster::class, itemType: Foo::class)]
+    public ArrayObject $collectionOfFoo;
+
+    /** @var \Spatie\DataTransferObject\Tests\Baz[] */
+    #[CastWith(ArrayCaster::class, itemType: Baz::class)]
+    public ArrayObject $collectionOfBaz;
+}