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

[12.x] Add support for nested array notation within loadMissing #54554

Draft
wants to merge 4 commits into
base: 12.x
Choose a base branch
from

Conversation

lioneaglesolutions
Copy link
Contributor

@lioneaglesolutions lioneaglesolutions commented Feb 11, 2025

Currently loadMissing on collections and models does not work using the nested array syntax that is supported by with and load.

$users = User::with(['posts'])->get();

$users->loadMissing(['posts' => ['comments']]);

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 and loadMissing methods to all support the same nested array syntax.

@lioneaglesolutions lioneaglesolutions changed the title [11.x] Add supported for nested array notation within loadMissing [11.x] Add support for nested array notation within loadMissing Feb 11, 2025
@Junveloper
Copy link

I like how this PR introduces the needed consistency between with and load.

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 loadMissing does not throw an exception, I did not suspect the above to cause n+1 issue. So I am hoping that this PR gets approved, or at least get loadMissing to throw an exception when nested array is provided.

@taylorotwell
Copy link
Member

Could we not re-use the logic we use for this elsewhere? It feels odd to have to recreate this logic here.

@taylorotwell taylorotwell marked this pull request as draft February 14, 2025 15:52
@shaedrich
Copy link
Contributor

shaedrich commented Feb 14, 2025

Would be convenient if this was part of the Arr helper 🤔

@lioneaglesolutions
Copy link
Contributor Author

Could we not re-use the logic we use for this elsewhere? It feels odd to have to recreate this logic here.

@taylorotwell re-use which part exactly? The loading of the relations or the preparation of the relations to load?

I initially looked at parseWithRelations

protected function parseWithRelations(array $relations)
{
if ($relations === []) {
return [];
}
$results = [];
foreach ($this->prepareNestedWithRelationships($relations) as $name => $constraints) {
// We need to separate out any nested includes, which allows the developers
// to load deep relationships using "dots" without stating each level of
// the relationship with its own key in the array of eager-load names.
$results = $this->addNestedWiths($name, $results);
$results[$name] = $constraints;
}
return $results;
}
but this seemed very specific to a new query.

I then looked at the load method within Collection and this already seemed very different to loadMissing, hence I figured it would be easiest to implement a basic flattening method to prepare the loadMissing relations as it already seemed pretty separate to load and with.

I'm more than happy to approach it differently if you have any pointers.

@lioneaglesolutions lioneaglesolutions changed the base branch from 11.x to 12.x March 4, 2025 10:15
@lioneaglesolutions lioneaglesolutions changed the title [11.x] Add support for nested array notation within loadMissing [12.x] Add support for nested array notation within loadMissing Mar 4, 2025
@lioneaglesolutions
Copy link
Contributor Author

@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.

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