-
Notifications
You must be signed in to change notification settings - Fork 11.3k
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
[12.x] Add support for nested array notation within loadMissing
#54554
base: 12.x
Are you sure you want to change the base?
[12.x] Add support for nested array notation within loadMissing
#54554
Conversation
loadMissing
loadMissing
I like how this PR introduces the needed consistency between Additional issue I encountered with the loadMissing method, aside from its lack of support for nested array syntax, is that it still accepts a nested array and loads the top-level relationship, giving the impression that it is working correctly. For example: public function exampleTest()
{
$posts = Post::get();
DB::enableQueryLog();
$posts->loadMissing([
'comments' => [
'parent'
]
]);
$this->assertCount(1, DB::getQueryLog());
$this->assertTrue($posts[0]->relationLoaded('comments')); // Passes - which potential misleads developers to think that the given syntax is working
$this->assertTrue($posts[0]->comments[0]->relationLoaded('parent')); // Fails - but it doesn't
} Because providing a nested array to |
Could we not re-use the logic we use for this elsewhere? It feels odd to have to recreate this logic here. |
Would be convenient if this was part of the |
@taylorotwell re-use which part exactly? The loading of the relations or the preparation of the relations to load? I initially looked at framework/src/Illuminate/Database/Eloquent/Builder.php Lines 1654 to 1672 in a254dbd
I then looked at the I'm more than happy to approach it differently if you have any pointers. |
loadMissing
loadMissing
@taylorotwell I was hoping to get some further input on how you think I should best proceed with this PR please? Keen to get this merged but would like to do it with minimal changes and re-use where possible. |
Currently
loadMissing
on collections and models does not work using the nested array syntax that is supported bywith
andload
.The above code wouldn't load anything additional.
This pull request attempts to solve this by flattening the nested arrays into dot notation while still supporting attribute selecting.
Whilst it's not the prettiest solution but open to suggestions on how it could be improved, it is inline with how the
with
method prepares relations.This PR will bring some welcomed consistency across the
with
,load
andloadMissing
methods to all support the same nested array syntax.