[Fix] embed relations serialization - #3410
florianJacques wants to merge 8 commits into
Conversation
|
Hello, |
|
In order to review the PR properly, could you add some tests that are fixed by the change?
GitHub Action fails to commit the fixes that are found by phpcs. |
|
Yes of course, I will write the tests in the next few days |
You can run |
|
Thanks, Done! I'll write the tests when I have some time... |
|
Adding test ok |
GromNaN
left a comment
There was a problem hiding this comment.
It seems to me that this is more of a documentation issue that something we should fix in the code.
As for any relationship, if you want to load the data from an embedded relationship you have to use the Model::$with property or call Builder::with().
| $user->addresses()->saveMany([new Address(['city' => 'London'])]); | ||
|
|
||
| //Reload document | ||
| $user = UserWithEmbeds::where('name', 'John Doe')->first(); |
There was a problem hiding this comment.
In order to get the address property correctly casted, you must call with('address'):
| $user = UserWithEmbeds::where('name', 'John Doe')->first(); | |
| $user = UserWithEmbeds::where('name', 'John Doe')->with('addresses')->first(); |
|
|
||
| class UserWithEmbeds extends User | ||
| { | ||
| protected $withEmbeds = ['addresses']; |
There was a problem hiding this comment.
Having this new configuration to "fix" the bugged behavior of the embed relationship seems wrong.
The default $with property can be leveraged:
| protected $withEmbeds = ['addresses']; | |
| protected $with = ['addresses']; |
| * | ||
| * @return bool | ||
| */ | ||
| public function isEmbeddedRelation($key) |
There was a problem hiding this comment.
This new method is not used and not tested. It doesn't seems to be part of the fix.
|
Thank you for the contribution and for adding tests. I verified that using the standard Closing in favour of documenting the correct use of |
Fixed the serialization of embed relations.
These relationships were not necessarily loaded, and could not use their attribute mutators.
Checklist