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

Breaking change in ResourceCollection #32513

Closed
Shhu opened this issue Apr 23, 2020 · 14 comments
Closed

Breaking change in ResourceCollection #32513

Shhu opened this issue Apr 23, 2020 · 14 comments

Comments

@Shhu
Copy link
Contributor

Shhu commented Apr 23, 2020

  • Laravel Version: 7.7.1
  • PHP Version: 7.4.4
  • Database Driver & Version: mysql 8

Description:

cf #32296 (comment)
$this->collection->transform() throw an error Trying to get property 'resource' of non-object'

Works in 7.5.2, break in 7.6.0 and above

Have to replace
$this->collection->transform( by $this->collectResource($this->collection)->transform(

Steps To Reproduce:

Usage of $this->collection->transform based on https://medium.com/@dinotedesco/laravel-api-resources-what-if-you-want-to-manipulate-your-models-before-transformation-8982846ad22c


use Illuminate\Http\Resources\Json\ResourceCollection;
use App\User;

class IndexCollection extends ResourceCollection
{
    public function toArray($request): array
    {
        return [
            'data' => $this->collection->transform(static function (User $user) {
                return [
                    'id'       => $user->id,
                    'email'    => $user->email,
                ];
            }),
        ];
    }
} ```
@driesvints
Copy link
Member

Now that I see it like this, I'm not so sure this is a bug. We don't document transforming the dataset like this anywhere.

@GrahamCampbell
Copy link
Member

Maybe we should add extra guards in the code to help with developer experience, giving good exceptions?

@preeteshjain
Copy link

@driesvints We know it's not the standard procedure, but sometimes people have to customize the payload a bit. Where a Resource::collection and ResourceCollection have to be different. In such cases, $this->collection->transform is a great help. But #32296 breaks that.

@driesvints
Copy link
Member

What are the values of the collection? How are you instantiating them?

@preeteshjain
Copy link

I will explain with an example.

Say you have a Post model and you fetch a post using a PostResource whenever a GET request comes to /api/posts/<id>. Now you have added a lot of values and relationships to that, that are not really needed in the PostCollection. All your PostCollection needs, is an id and a title. In that case, it is necessary to customize the PostCollection a bit. Which can be done using $this->collection->transform.

But now that can't be done. #32296 will break a lot of these kind of applications.

@driesvints
Copy link
Member

driesvints commented Apr 23, 2020

What's preventing you from doing it the way that it's documented?

<?php

namespace App\Http\Resources;

use Illuminate\Http\Resources\Json\JsonResource;

class Post extends JsonResource
{
    /**
     * Transform the resource into an array.
     *
     * @param  \Illuminate\Http\Request  $request
     * @return array
     */
    public function toArray($request)
    {
        return [
            'id' => $this->id,
            'title' => $this->title,
        ];
    }
}
use App\Http\Resources\ Post as PostResource;
use App\Post;

Route::get('/posts', function () {
    return PostResource::collection(Post::all());
});

@Shhu
Copy link
Contributor Author

Shhu commented Apr 23, 2020

I use it like that :

    public function __invoke(Request $request): IndexCollection
    {
        // some logic

        return new IndexCollection($pages);
    }

I prefere return a IndexCollection than an AnonymousResourceCollection and i can get all my select options metas in the Collection instead of adding it in controller with ->additional([])
I dont mind the change, it doesnt block me, just the error gave me a headache to find the cause.

@driesvints
Copy link
Member

Well, typically the collection uses the singular resource for the collection:

Typically, the $this->collection property of a resource collection is automatically populated with the result of mapping each item of the collection to its singular resource class. The singular resource class is assumed to be the collection's class name without the trailing Collection string.

I see your use case but I don't think we ever really supported that.

@preeteshjain
Copy link

preeteshjain commented Apr 23, 2020

What's preventing you from doing it the way that it's documented?

Because some people (like me) keep PostResource reserved for single post and PostCollection for well, collection of posts, with some differences in the payload between the both.

I get that it's not the ideal way, but I was just explaining why people might want to use this kind of technique. It's pretty obvious. I agree with @GrahamCampbell, that there should be some kind of exception, if something is not supported.

Like @Shhu, it took me 2 whole days to figure out why that particular code was not working, only to find out later that a pull request was behind the errors.

@mholmes-hs
Copy link

mholmes-hs commented Apr 23, 2020

This one took me a little while to figure out as well. I can't say for sure where I saw an example for the array style transform:

$queryResult
            ->getCollection()
            ->transform(function($pet) {
                return [
                    ...
                ];

But it did offer some benefits over the way transform is shown in the official documentation. Mainly, changing model attributes directly caused issues with some of my model mutators (eg reformatting dates to be strings, adding '$' to currency fields, etc). I wound up re-coding the transformers but I had to add extra attributes to my models that don't exist in the schema (which felt a little hacky).

@taylorotwell
Copy link
Member

Fixed in 7.8.0.

@glauberm
Copy link

Was getting this error on Laravel v6.18. I guess I was doing the transformation based on this StackOverflow answer. I don't have enough reputation to comment a warning about it, but if anyone can, please do.

@jasonvarga
Copy link
Contributor

From the OP:

Steps To Reproduce:

Usage of $this->collection->transform based on https://medium.com/@dinotedesco/laravel-api-resources-what-if-you-want-to-manipulate-your-models-before-transformation-8982846ad22c

This article only ever transforms into resource objects. Not into plain arrays.

They even say that in the article:

Ok, if the resource collection doesn’t automagically convert the model into the corresponding resource what do we do?

We transform the collection of models into a collection of resources.

@airgrim
Copy link

airgrim commented May 25, 2020

Fixed in 7.8.0.

@taylorotwell can you do the same for 6.x branch please?

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

No branches or pull requests

9 participants