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

[5.7] Modify ELoquent Api Resource merge() methods to accept JsonResource object #27068

Merged

Conversation

amaelftah
Copy link
Contributor

@amaelftah amaelftah commented Jan 4, 2019

this PR , allows both merge & mergeWhen to accept JsonResource object , to remove duplicate of referencing same attributes in different resources .

simplified example of use case , we want to retrieve a user with his profile , but the case is the UserResource mustn't change cause the class has been used in different places in a messy project and could cause api compatibility issues if changed :

class UserResource extends JsonResource
{
     public function toArray($request)
     {
        return ['name' => $this->name , 'age' => $this->age];
     }

}
class UserWithAvatarResource extends JsonResource
{
     public function toArray($request)
     {
       //old way
       return ['name' => $this->name , 'age' => $this->age , 'avatar' => $this->avatar];

        //another way
       return array_merge(
             (new UserResource($this))->jsonSerialize() ,
             ['avatar' => $this->avatar] 
        );

       //when PR accepted
       return [
            $this->merge(new UserResource($this)),
            'avatar' => $this->avatar, 
        ];
     }

}

@bonzai
Copy link
Contributor

bonzai commented Jan 5, 2019

You can just do something like this if you want to merge two resources:

class UserWithAvatarResource extends UserResource
{
     public function toArray($request)
     {
        return array_merge(parent::toArray($request), [
            'avatar' => $this->avatar,
        ]);
     }
}

I'm not sure if adding support for JsonSerializable is a good idea (at least in version 5.7). If this would be accepted, I would also add support for Arrayable contract.

@amaelftah
Copy link
Contributor Author

@bonzai nice idea 👍 , but in some cases we will need to merge multiple other resources .

i think this isn't a breaking change for 5.7 as it doesn't alter any old behavior

@taylorotwell taylorotwell merged commit e193458 into laravel:5.7 Jan 5, 2019
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.

None yet

3 participants