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.6] Allow eager-loading mixed relationships of polymorphic collection #23626

Merged
merged 1 commit into from
Mar 26, 2018

Conversation

derekmd
Copy link
Contributor

@derekmd derekmd commented Mar 20, 2018

I've had this macro kicking around for awhile since the framework doesn't offer a way to avoid N+1 queries from morphTo() nested relationships. This adds Illuminate\Database\Eloquent\Collection@loadMorph() to allow nested eager loading by naming the unique relationship(s) for each class name.

Issue reported:

External Laravel community:

Example Use Case

class ActivityFeed
{
    public function parentable()
    {
        // e.g., hydrate a Collection of Event, Photo, or Post instances.
        return $this->morphTo();
    }
}
$activities = ActivityFeed::with('parentable')
    ->get()
    ->loadMorph('parentable', [ // could instead be named loadMixed()? loadMorphed()?
        Event::class => 'calendar',
        Photo::class => 'tags',
        Post::class => ['author', 'commentsCount'],
    ]);

In an ideal world...

...this would be supported:

ActivityFeed::with([
    'parentable' => [
        Event::class => 'calendar',
        Photo::class => 'tags',
        Post::class => ['author', 'commentsCount'],
    ],
    'user:id,name',
    'zonda' => function ($query) {
        $query->ofSomeScope();
    },
])->get();

I really wanted to work this into the existing eager-load infrastructure:

  • Illuminate\Database\Eloquent\Builder@with()
  • Illuminate\Database\Eloquent\Collection@load()
  • Illuminate\Database\Eloquent\Model@with (array object property)

However this is going to require a significant rewrite of the Eloquent internals that store an associative array of query constraint Closures.

Pagination

For better developer experience in controllers, an explicit AbstractPaginator@loadMorph() proxy is added to return the paginator. This allows it to be passed into the view or returned by an API response.

Then controllers don't have to awkwardly do:

$tags = Tag::with('taggable')->latest()->paginate();

$tags->getCollection()->loadMorph('taggable', [
    Post::class => 'category', 'comments',
    Video::class => 'likes',
]);

return view('tags.index')->with('tags', $tags);

but:

$tags = Tag::with('taggable')->latest()->paginate()->loadMorph('taggable', [
    Post::class => ['category', 'comments'],
    Video::class => 'likes',
]);

return view('tags.index')->with('tags', $tags);

@taylorotwell
Copy link
Member

taylorotwell commented Mar 20, 2018

We don't support eager loading morphTo relationships already? I see the code in the framework to support it?

@derekmd
Copy link
Contributor Author

derekmd commented Mar 20, 2018

This is allowing eager-loading from mixed classes that have relationships uncommon between each other. From the above example:

$activities = ActivityFeed::with([
    'parentable.calendar',      // Event@calendar() hasOne
    'parentable.tags',          // Photo@tags() hasMany, relation doesn't exist in Event class
    'parentable.author',        // Post@author() hasOne
    'parentable.commentsCount', // Post@commentsCount hasOne
])
->get();

Illuminate\Database\Eloquent\RelationNotFoundException: Call to undefined relationship [tags] on model [App\Models\Event].

I didn't see anything on https://laravel.com/docs/5.6/eloquent-relationships#eager-loading or in the source code that supports such eager loading?

@derekmd derekmd changed the title [5.6] Allow eager-loading relationships of polymorphic collection [5.6] Allow eager-loading mixed relationships of polymorphic collection Mar 20, 2018
@taylorotwell
Copy link
Member

Gotcha.

@taylorotwell
Copy link
Member

I'm fine if you want to add the paginator helper to this PR so I can review it all at once.

@derekmd derekmd force-pushed the eloquent-collection-load-morph branch 5 times, most recently from 046e004 to 3fbd5e7 Compare March 24, 2018 01:31
Allow nested relations to be eager loaded for
morphTo() relationships of mixed classes.

e.g.,

class ActivityFeed
{
    function parentable()
    {
        return $this->morphTo();
    }
}

$collection = ActivityFeed::with('parentable')
    ->get()
    ->loadMorph('parentable', [
        Event::class => 'calendar',
        Photo::class => 'tags',
        Post::class => ['author', 'commentsCount'],
    ]);

or

$paginator = ActivityFeed::with('parentable')
    ->paginate()
    ->loadMorph('parentable', [
        // etc.
    ]);
@derekmd derekmd force-pushed the eloquent-collection-load-morph branch from 3fbd5e7 to 4367675 Compare March 24, 2018 01:32
@taylorotwell taylorotwell merged commit 4367675 into laravel:5.6 Mar 26, 2018
@derekmd derekmd deleted the eloquent-collection-load-morph branch March 26, 2018 17:01
@sylouuu
Copy link

sylouuu commented Apr 15, 2019

@derekmd is there any plan to work on your "ideal world" (which is amazing btw)?

This problem does not encourage developers to use polymorphic relation. :(

I have to process hundreds of polymorphic records and play with nested relations and the overload of queries is a nightmare.

Best

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.

3 participants