Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Prevent DataTransferObjectCollection from iterating over array copy #166

Merged
merged 1 commit into from
Feb 11, 2021

Conversation

rtuin
Copy link
Contributor

@rtuin rtuin commented Feb 9, 2021

With the introduction of 2.8.0 (88ffe87) the DataTransferObjectCollection iterates over copy of the internal $collection array instead of over the current value of array itself.

This breaks backwards compatibility as demonstrated by the testcase below. Before 2.8.0, this test would pass and on 2.8.0 it fails.

    public function test_it_iterates_over_internal_array(): void
    {
        $list = new class() extends DataTransferObjectCollection {
        };

        $list[] = new TestDataTransferObject(['testProperty' => 1]);
        $list[] = new TestDataTransferObject(['testProperty' => 2]);
        $list[] = new TestDataTransferObject(['testProperty' => 3]);

        $traversed = [];

        foreach ($list as $id => $item) {
            $traversed[$id] = $item->testProperty;
        }

        $this->assertEquals([0 => 1, 1 => 2, 2 => 3,], $traversed);
    }

// Actual yield: Array ()
// Expected yield: Array (0 => 1, 1 => 2, 2 => 3)

This PR is the simplest solution to make the above test pass, but also breaks backwards compatibility as it changes the type of the protected $collection field.

I think it is a design decision on which type of backwards compatibility to break, so I'm wondering about your opinion on this.
From one perspective it also makes some sense to have the collections Immutable (implicit), while on the other hand the current API is explicit about immutability for DTO's (non collections) already.

There are multiple ways to re-introduce the behavior described above, other that the proposed code:

  1. Revert the associative collection support and go back to the previous way of iterating (not really moving forward)
  2. Keep the protected array $collection = []; and create a new ArrayIterator every time the offsetSet method is called (prone to side-effects, resetting the iterator position)
  3. Moving associative array support to a specialized concrete DataTransferObjectAssociativeCollection, removing the ArrayIterator from DataTransferObjectCollection, that class would probably also need # 2.

@brendt
Copy link
Contributor

brendt commented Feb 11, 2021

@rubenvanassche can you check this one?

@rubenvanassche
Copy link
Member

Looks great, thanks!

@rubenvanassche rubenvanassche merged commit 769ade9 into spatie:master Feb 11, 2021
@rtuin rtuin deleted the not-iterating-over-array-copy branch February 11, 2021 09:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants