[5.5] Add inverse side in has/morph one/many relationships

[sileence]

This is a more complete proposal of #20011

This is an idea to fix #20006

By specifying the inverse side of the has many relationship like this:

    public function comments()
    {
        return $this->hasMany(Comment::class)->inversedBy('post');
    }

Then $post->comments->first()->postwon't generate an extra SQL and won't duplicate the post object, it will be the same! This will help to avoid the N+1 described in the issue.

This should work with has one, morph one, has many and morph many relations (basically the relations that have inverse side).

I tested with eager and lazy loading and with default, etc. I haven't added unit tests for the core but I've been working with these integration tests:

https://github.com/sileence/eloquent_push_tests/blob/master/tests/Unit/EloquentInversedByTest.php

[sisve]

If I have two Comment instances ($first, $second), and they both have referencing the same post, would $first->post and $second->post return the same Post model? If $first->post triggers the loading, will $second->post also get populated?

[thecrypticace]

I would love to see something like this. It'd be especially useful when traversing parent child relationships.

I've written code several times to load and stitch together the parent / child relationships of entities in a table because trying to use Item::with("children.children.children.children.children…") can only get you so far.

[sileence]

@sisve actually they do, but that has nothing to do with this proposal:

        $post = factory(Post::class)->create();

        factory(Comment::class)->times(2)->create([
            'post_id' => $post->id,
        ]);

        $comments = Comment::with('post')->get();

        $this->assertSame($comments->first()->post, $comments->last()->post);

What I'm proposing is that when querying:

$post = Post::with('comments')->first(),

$post->comments->first()->post points to the same $post object (if the inverse side of the relation is specified).

[fernandobandeira]

Since #20055 was merged you can replace this with

Collection::wrap($models)->each->setRelation($this->inverseSide, $parent);

You prob saw it just keeping it here to remember about it 😄

[sileence]

Hehe that was my idea when I sent the first PR 😄

[taylorotwell]

Your PR breaks even simple operations like $user->posts()->count().

[sileence]

@taylorotwell I didn't include tests with count(). Please let me have a look.

[sileence]

@taylorotwell I fixed it by calling the setRelation method only if the result is a collection or a model:

https://github.com/laravel/framework/pull/20053/files#diff-c8675355c360b9bd4ef485c30ff5e025R69

I was too ambitious 😞 I forgot that some Builder methods can also return scalar values.

[Lelectrolux]

First, I'm happy to delete the following or move it to a gist if it doesn't belong here, but I think some real life example/use case might be a good idea. I will answer question if you have any.

---

I got a few places where this feature would be handy. I run an app where we have a bunch of stuff (Hotel/Restaurant/Shop/Activity) classified by Region (Like US State if it speaks more to you) then City.

Here is a stripped down example.

---

Routes

// actually, those are generated from an array ['hotel', 'restaurant', 'activity', ...] in the
// RouteServiceProvider, and the controllers are generated too, but beside the point
Route::get('/hotels/')->name('hotel.show')->uses('ListHotelController');
Route::get('/hotels/{region}/{city}/{hotel}')->name('hotel.list')->uses('ShowHotelController');

Models

class Region extends Model {
    public function cities() {
        $this->hasMany(City::class)->orderBy('name');
    }
}

class City extends Model {
    public function hotels() {
        $this->hasMany(Hotel::class)->orderBy('name');
    }

    public function region() {
        $this->belongsTo(Region::class);
    }
}

interface HasUrl {
    public function url();
}

class Hotel extends Model implement HasUrl{
    public function city() {
        $this->belongsTo(City::class);
    }

    // Then in my views I can use $hotel->url() to get a link easily.
    public function url() {
        return route('hotel.show', [
            $this->city->region,
            $this->city,
            $this,
        ]);
    }
}

Repository

class BaseRepository {
     // ex: 'hotel'
     protected $model;
     
     public function __construct($model) {
         $this->model = $model;
     }
     
     public function list() {
         $regions = Region::with('city.'.$this->model)->get();
         
         $this->setInverseRelations($regions);
         
         return $regions;
     }
     
     // this is the ugly part
     protected function setInverseRelations($regions) {
         $regions->each(function($region, $_) {
            $region->cities->each(function($city, $_) use ($region) {
                $city->setRelation('region', $region);
                
                $city->{str_plural($this->model)}->each(function($model, $_) use ($city) {
                    $model->setRelation('city', $city);
                }
            });
         });
     }
 }

Controllers

abstract class ListModelController {
    protected $repository;
    
    protected $model;

    public function __invoke() {
        return view('model.list')
            ->with(['regions' => $this->repository->list(), 'modelType' => $this->model]);
    }
}

class ListHotelController extends ListModelController {
    public function __construct() {
        $this->model = 'hotel';
        
        $this->repository = new class($this->model) extend BaseRepository {}
    }
}

View

{-- 'model.list' view, obviously hugely simplified --}
<ul>
@foreach($regions as $region)
    <li>
        <p>{{ $region->name }}</p>
        <ul>
        @foreach($region->city as $city)
            <li>
                <p>{{ $city->name }}</p>
                <ul>
                 @foreach($city->{$modelType} as $model)
                     {-- LOOK HERE, $model->url()--}
                     <li><a href="{{ $model->url() }}">{{ $model->name }}</a></li>
                 @endforeach
                </ul>
            </li>
        @endforeach
        </ul>
    </li>
@endforeach
</ul>

The whole setInverseRelations($regions) mess is necessary to prevent the N+1 problem in that view.

Instead of the url() method, we could have done

public function createUrlFrom(Region $region, City $city) {
    return route('hotel.list', [$region, $city, $this]);
}

but it felt strange to pass related model from outside the class, and would be strange when working from the other side :

//In places where we loaded only one hotel
$this->createUrlFrom($this->city->region, $this->city);

This is one of the place I could clean up if this feature is added.

Side notes :

• All Models redefine the getRouteKey method
• All Models implements the HasUrl interface (which is a bit more than 1 method actually)

EDITED: deleted some loosely related comments. Original post here

[patrickcarlohickman]

Does this solution avoid infinite recursion when dumping to JSON? I think that was a big issue when people tried to tackle this before.

[sileence]

@Lelectrolux I've been in the same situation. One of the many tricks I've used to solve that problem was to cache the complete slug / url in a column. But definitely this PR will solve that issue. I think your "side step" comments are very interesting as well but belong to laravel/internals.

@patrickcarlohickman that's something I haven't tried but I will today. I shouldn't be that complicated to prevent that problem.

[Lelectrolux]

@sileence Yeah, one way to do it is cache the complete slug, but in our app, region and city have url with slugs too. So if I change the Region slug I either have all related cities and activities/hotels/restaurant/etc. with bad slugs or regen all in a Model::updated() event.

Changing region or city slug as a HUGE cost in our app, too much for an event for us. That and the fact we would want all modification in the same transaction in that case, and need to adress redirects from old urls.

I'll move the side step part to the internals, and add a ref to an unedited gist here. No time to do it today, thougt.

[garygreen]

@sileence just a thought, but is there a way of making this logic "lazy" ?

I think it would make sense to always have a inversedBy relation so I'm just wondering what kind of performance hit that would be if it's always assocaiting the inversed by relation for every item in the collection, so could it be done "lazily" if an inversed relation is attempted to be accessed?

E.g:

@foreach ($post->comments as $comment)
   // I won't be accessing $comment->post here at all, so don't bother associating the inversed post relation.
@endforeach

@foreach ($post->comments as $comment)
   // I will be accessing $comment->post here so now automatically see that it was loaded before and associate with the comment.
@endforeach

[sileence]

@patrickcarlohickman thanks for pointing that out, I just submitted a fix for that. I had the idea of loading the inverse relations in a separate array, that way they won't be used when exporting a model as json or pushing the model. What do you think?

[taylorotwell]

To me this is just more complexity in Eloquent to maintain that I'm not sure is super necessary. Especially lazy loading them via some other "inverseRelations" property.

[KristofMorva]

@taylorotwell While lazy loading is not a priority (nor this exact solution), I think a solution
for this problem could increase Laravel application performance (and/or get rid of boilerplate code), and 5.5 seems to be the perfect place for this change, so it'd be great if the idea wasn't dropped. We meet this problem in every larger project we have, and it seems totally unnecessary to always do some user-code to solve this.

[linaspasv]

@taylorotwell I do agree with @KristofMorva we need these kind of solutions especially for larger projects. I hope eloquent could be revisited at some point in time regards flexibility and performance.