Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[9.x] Traversable should have priority over JsonSerializable in EnumerateValues #44456

Merged
merged 1 commit into from
Oct 6, 2022

Conversation

brunoalod
Copy link
Contributor

Using an unrelated third party package I came across this problem, this exact same issue happened to someone else, as per (laravel/ideas#2556) and was introduced in #14628.

Problem

EnumerateValues::getArrayableItems checks for JsonSerializable before checking for Traversable.

This is a problem because a class implementing both JsonSerializable and Traversable will have its JSON representation used in the Collection instead of its actual iterable representantion, which should have priority. There is no need to transform a Traversable object to a JSON.

This is not an expected behaviour since a Traversable object can be traversed (no pun intended).

Consider the following:

// Person.php

class Person
{
    public string $firstName;
    public string $lastName;

    public function __construct(string $firstName, string $lastName)
    {
        $this->firstName = $firstName;
        $this->lastName = $lastName;
    }

    public function getFullName(): string
    {
        return $this->firstName . ' ' .  $this->lastName;
    }
}
// MyIterator.php

use ArrayIterator;
use IteratorAggregate;
use JsonSerializable;
use Traversable;

class MyIterator implements IteratorAggregate, JsonSerializable
{
    public function __construct(
        public $items,
    ) {
    }

    public function getIterator(): Traversable
    {
        return new ArrayIterator($this->items);
    }

    public function jsonSerialize(): mixed
    {
        return array_map(fn ($item) => (array) $item, $this->items);
    }
}
// Example.php

$iterator = new MyIterator([
    new Person('John', 'Doe'),
    new Person('Jane', 'Smith'),
]);

$collection = collect($iterator);

$collection->first()->getFullName();

EnumerateValues::getArrayableItems will use MyIterator JSON representantion, and now the items in the Collection will no longer be of type Person, meaning that $collection->first()->getFullName() will throw Call to a member function getFullName() on array.

That's assumming that the implementation of jsonSerialize returns an array of the items, but it can be worse if it returns other data such as:

public function jsonSerialize(): mixed
{
    return [
        'otherData' => ['my', 'other', 'data'],
        'data' => array_map(fn ($item) => (array) $item, $this->items),
    ];
}

Although this can be solved in user-land, EnumerateValues should accurately parse the items given the fact that Traversable is a native PHP feature.

@taylorotwell taylorotwell merged commit 51888a8 into laravel:9.x Oct 6, 2022
@georgefromohio
Copy link

This somehow breaks when converting a ResourceCollection because it is both Traversable and JsonSerializable but we don't want it get the iterator_to_array result of a ResourceCollection... T or F?

@Geoffry304
Copy link

Geoffry304 commented Feb 15, 2023

#This is giving problems when you want to collect a resource that extends JsonResource and use the toArray collect function. Only the top level is changed to an Array but the items underneath are staying as a resource.

namespace App\Http\Resources;

use Illuminate\Http\Resources\Json\JsonResource;

class TestResource extends JsonResource
{

    /**
     * Transform the resource into an array.
     *
     * @param Request $request
     * @return array
     */
    public function toArray($request)
    {
        return [
            'id' => $this->id,
            'name' => $this->name
            
        ];
    }

}

This is not giving the correct format:

$collection = collect(TestResource::collection($newArray))->toArray();

@georgefromohio
Copy link

@Geoffry304, correct… I had to override this with my own function because it broke my collections.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants