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.8] Add support for typed eager loads #28647

Closed
wants to merge 8 commits into from

Conversation

Projects
None yet
5 participants
@brendt
Copy link
Contributor

commented May 29, 2019

This PR adds support for eagerly loading relations of morphed models, based on their type.

An example:

  • Comment has a morph relation to commentable to Video or Post
  • Post has a relation to User

With this PR, you can load all these specific relation via queries, instead of using loadMorph on collections (#23626). Here's what the above example would look like:

Comment::query()
    ->with(['commentable' => function (MorphTo $morphTo) {
        $morphTo->withMorph(Post::class, ['user']);
    }])
    ->get();

The advantage of using this approach compared to loadMorph, is that nested relations are supported when using with on the query builder.

This PR is in draft, because I've got two questions still:

  • I chose the name withMorph to be similar to loadMorph, though maybe someone has a better idea?
  • Can someone point me into the right direction on how to test this. I'm getting these errors: Call to a member function connection() when running DatabaseEloquentMorphToTest on its own, so I'm unable to try things out myself right now. Edit: figured out the right place to test this.

brendt added some commits May 29, 2019

@brendt brendt force-pushed the brendt:improved-morphed-eager-loads branch from d4dd81c to 749515b May 29, 2019

@brendt brendt force-pushed the brendt:improved-morphed-eager-loads branch from 749515b to b80a1c8 May 29, 2019

brendt added some commits May 29, 2019

@brendt brendt force-pushed the brendt:improved-morphed-eager-loads branch from e2a461c to f661771 May 29, 2019

@driesvints driesvints changed the title Add support for typed eager loads [5.8] Add support for typed eager loads May 29, 2019

@taylorotwell

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Can you explain the code example? I can't understand what it is doing from just reading it. Also show the alternative syntax to accomplish this in current Laravel 5.8 without this PR.

@brendt

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

@taylorotwell the test should clarify things further:

    public function test_with_morph_loading()
    {
        $comments = Comment::query()
            ->with(['commentable' => function (MorphTo $morphTo) {
                $morphTo->withMorph(Post::class, ['user']);
            }])
            ->get();

        $this->assertTrue($comments[0]->relationLoaded('commentable'));
        $this->assertTrue($comments[0]->commentable->relationLoaded('user'));
        $this->assertTrue($comments[1]->relationLoaded('commentable'));
    }

This example says "load all polymorphic commentable relations, and IF the commentable is a Post, also load its user relation"

Right now, you can use loadMorph on EloquentCollection, but it cannot handle nested relations. Take the previous example, but with the Comment model also belonging to a Feed.

$feeds = Feed::query()
    ->whereUser($user)
    ->with('comments.commentable')
    ->get();

$feeds->loadMorph('comments.commentable', [ // Nested relations don't work here
    Post::class => 'user'
]);

You cannot use loadMorph to load the post's user relation, without looping over each feed item individually. This causes n+1 issues with large data sets.

That's the issue this PR solves, as you can load the above example like so:

$feeds = Feed::query()
    ->whereUser($user)
    ->with('comments.commentable', function (MorphTo $morphTo) {
        $morphTo->withMorph(Post::class, ['user']);
    })
    ->get();
@staudenmeir

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

Is it desirable to also/only allow the syntax of loadMorph()?

Comment::query()
    ->with(['commentable' => function (MorphTo $morphTo) {
        $morphTo->withMorph([Post::class => ['user']]);
    }])
    ->get();
@brendt

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

@staudenmeir Sounds like a good idea! I changed it accordingly

@staudenmeir

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

Please remove $modelClass from the PHPDoc.

We could rename the $with parameter to $relations to be consistent with loadMorph().

I think we should also support single relations like loadMorph() does:

Comment::query()
    ->with(['commentable' => function (MorphTo $morphTo) {
        $morphTo->withMorph([Post::class => 'user']);
    }])                                     ^^^^^^
    ->get();
@brendt

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

@staudenmeir I added support for single relations: a6700e5#diff-4c052acf0022213a4cc23f53ba42720eR123

$morphTo->withMorph([Post::class => 'user']);
Update src/Illuminate/Database/Eloquent/Relations/MorphTo.php
Co-Authored-By: Dries Vints <dries.vints@gmail.com>

@taylorotwell taylorotwell marked this pull request as ready for review Jun 8, 2019

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

This has been merged but looks like GitHub didn't mark it as merged because of draft status. Thanks for the contribution! Could you send a PR to the docs when you get a chance?

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

PS I renamed the method morphWith.

@brendt

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

Thanks! I'll send a PR to the docs later this week

@brendt brendt deleted the brendt:improved-morphed-eager-loads branch Jun 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.