laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.42k stars 10.99k forks source link

[5.4] Custom Intermediate (Pivot) Models not instantiable #17770

Closed JuanDMeGon closed 7 years ago

JuanDMeGon commented 7 years ago

Description:

When trying to use a custom intermediate model (a model that extends from Illuminate\Database\Eloquent\Relations\Pivot) as a regular model (Pivotextends from Model), it fails, because is not able to create an instance.

Too few arguments to function Illuminate\Database\Eloquent\Relations\Pivot::__construct(), 0 passed

I checked the commit but did not found anything would help on this: 4439576c9a9da5d6dcf8d4fd3ab4210576450eb6

Steps To Reproduce:

Create a model that extends from Pivot instead of Model directly:

...
use Illuminate\Database\Eloquent\Relations\Pivot;
...
class CustomIntermediate extends Pivot
{
    ......
}

Then try to use the model for some specific functions like all(), with(), query(), etc.

Route::get('test', function() {
    return CustomIntermediate::all();
});
GrahamCampbell commented 7 years ago

Please post your entire custom class code.

JuanDMeGon commented 7 years ago

Hello @GrahamCampbell, I just checked it again on a fresh Laravel project: composer create-project laravel/laravel testing 5.4

Then, created a Test model: php artisan make:model Test

Modified the Test to extends from pivot:

<?php

namespace App;

use Illuminate\Database\Eloquent\Relations\Pivot;

class Test extends Pivot
{
    //
}

Then on the web.php routes issued this:

Route::get('/', function () {
    return App\Test::all();
});

Then going to my browser obtained this:

Type error: Argument 1 passed to Illuminate\Database\Eloquent\Relations\Pivot::__construct() must be an instance of Illuminate\Database\Eloquent\Model, none given, called in {somePath}\vendor\laravel\framework\src\Illuminate\Database\Eloquent\Model.php on line 335

arjasco commented 7 years ago

Reason for wanting to instantiate a custom pivot model on its own?

My understand is it's for use within a relationship with the ->using() method on your relationship definition. You may as well extend Model?

JuanDMeGon commented 7 years ago

If you check the definition of Illuminate\Database\Eloquent\Relations\Pivot it extends from eloquent Model, so in my understanding it still being a regular model. The custom pivot model acts, as as you said, like a pivot AND is a model too I should be able to use it directly or to resolve the many to many relationship through it. Maybe I am misunderstanding something here but it does not seem logic for me to use a custom pivot model and not be able to use it as a regular model as well. Possibly @pstephan1187 can give us some orientation for this?

arjasco commented 7 years ago

I see your logic, it may extend Model but doesn't mean it completely reflects the same behaviour.

https://github.com/laravel/framework/blob/5.4/src/Illuminate/Database/Eloquent/Relations/Pivot.php#L47

Looks like the Pivot requires a parent model, and this is provided when the relationship builds that pivot here:

https://github.com/laravel/framework/blob/5.4/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php#L378

I might be wrong so i would be interested too. I think the idea is so you can define some further logic on your custom pivot model. Eg when updating a property your might want to use a mutator or maybe define some helper methods on that pivot model which you couldn't do before.

JuanDMeGon commented 7 years ago

Yeah, it is possible that it does not need to reflects the same behaviour but so it should not extends directly from model but from a modified model or similar. Let see if someone can clarify this for us. Thanks for commenting.

dees040 commented 7 years ago

Is there any progress? Getting this issue as well.

JuanDMeGon commented 7 years ago

@dees040 Unfortunately not. It seems like a not important issue yet. I decided to do not use it for now and use the conventional old approach. Hopefully, later this will be treated.

ghost commented 7 years ago

I have the same issue and error. The docs aren't really clear about the purpose of the $relation->using('model') functionality.

For example is it possible to do something like this with that functionality:

users

posts

post_user

post_user_role

$user->load('posts.pivot.roles');

That would be awesome, but can't get it to function.

pstephan1187 commented 7 years ago

@rsdrsd you need to load in post_user_role field on the pivot table:

function posts(){
    return $this->belongsToMany(\App\Post::class)->withPivot('post_user_role');
}
minhchu commented 7 years ago

I think the document should show a practical example using intermediate table.

JuanDMeGon commented 7 years ago

You are probably right. I would like to add an example to the documentation, but it seems like it does not work like I think it does, so may is a matter of time.

cjaoude commented 7 years ago

Guys, agreed, some documentation would be nice, but after toying with it, here's how it works.

Let's say M-M for User and Books. And you're also adding a store_id attribute to the pivot table. How can you get that Store model based on the store_id?

// User.php

public function books()
{
    return $this->belongsToMany(Book::class)
                ->withPivot('store_id')
                ->using(BookUser::class);
}

So now when you hit $user->books->first()->pivot you're getting the BookUser model. Remember, $user->books returns a collection. You want a Book from the collection to make use of BookUser. (...so above i just used ->first())

// BookUser.php
class BookUser extends Pivot {
    public function store()
    {
        return $this->belongsTo(Store::class);
    }
}

Bingo. Now.... $user->books->first()->pivot->store yields the Store of store_id.

So now you might use it for something like this...

@foreach ($user->books as $book)
  Store name: {{ $book->pivot->store->name }}
@endforeach

So that's it. Each row in the pivot table is now coming back to you as a BookUser model. And you can do relations, saves, updates, whatever.

I hope that helps.

JuanDMeGon commented 7 years ago

Yes, it is the expected behavior here. In your example, you should be able to use the BookUser model as the pivot, that is good. In fact, when you use the pivot property it returns an instance of BookUser, but when you want to directly create an instance of BookUser instead of go trough the other models, it just fails.

cjaoude commented 7 years ago

Yes, BookUser can't be instanced directly. [edit...] It extends Pivot class which extends Model but for whatever reason is not quite the behavior as a normal Model.

It's 'intermediary' and we just need to use it that way. It's very useful.

JuanDMeGon commented 7 years ago

@cjaoude Yes, it is a Pivot class, wich extends from Model. Check the definition of here. It extends directly from Model. If you think about that is the reason why you can obtain an instance when using the pivot property.

I clearly understand that Pivot is like a "special model," but it is extending from Model and should provide the same behavior expected from a regular model. In another case, it should extend from another kind of "Model."

That is, in fact, the main reason for the discussion here.

cjaoude commented 7 years ago

I understand what you mean. My posts are to answer how to use intermediary models, which some people have asked about above.

JuanDMeGon commented 7 years ago

Yes, you are right. Surely that will be very helpful for all.

💯 :)

rdpascua commented 7 years ago

Now how can we eagerload the intermediate table?

$foo->with('bar.pivot.baz'); doesn't work since pivot relation doesn't exists.

Dreambox13 commented 7 years ago

I spend at least one week trying to find that question @rdpascua. Anyways, after a long efford I tried a threeway pivot and works just fine. I don't know if it helps you.

rdpascua commented 7 years ago

I've created a concerete class instead of using the pivot :/

Sent from my iPhone

On 27 Apr 2017, at 1:56 PM, DiakosavvasN notifications@github.com<mailto:notifications@github.com> wrote:

I spend at least one week trying to find that question @rdpascuahttps://github.com/rdpascua. Anyways, after a long efford I tried a threeway pivot and works just fine. I don't know if it helps you.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/laravel/framework/issues/17770#issuecomment-297619189, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ACYexmwQCiEz90tyu4ZwAsiENI0RUgV9ks5r0C4YgaJpZM4L3VWH.

dherran commented 7 years ago

@Dreambox13 @rdpascua can you provide an example of how you solved the problem of eager loading the intermediate table?

rdpascua commented 7 years ago

i haven't, I just created a new model name based on the relation

Sent from my iPhone

On 11 Jun 2017, at 6:35 AM, Danny Herran notifications@github.com<mailto:notifications@github.com> wrote:

@Dreambox13https://github.com/dreambox13 @rdpascuahttps://github.com/rdpascua can you provide an example of how you solved the problem of eager loading the intermediate table?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/laravel/framework/issues/17770#issuecomment-307594147, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ACYexvhBV1vaNTZPuaDF2RA3VvFy2zMZks5sCxpJgaJpZM4L3VWH.

decadence commented 7 years ago

I face similar issue but it's expected behavior because Pivot constructor needs some values indeed. Would be nice if we can use it as normal Model.

antonkomarev commented 7 years ago

Faced same issue recently.

dherran commented 7 years ago

This proposal solved it for me @decadence @a-komarev : https://github.com/laravel/internals/issues/175

Read the comments all the way through. It explains the changes required to eager load a pivot model relationships on 5.4.

antonkomarev commented 7 years ago

@dherran Thanks for pointing to the solution.

decadence commented 7 years ago

@dherran thanks. But would be cool to have this out of box.

darkylmnx commented 7 years ago

i'm having the same issue, i can't instantiate a pivot model, here's my structure and the use case

posts (belongsToMany tags)

tags (belongsToMany posts)

post_tag (pivot)

votable (polymorphic)

and as the tables explain by themselves, you can vote for a post_tag just like the likedin tag recommandation feature

the api would be something like : POST /post-tag/{post_tag}/likes And inside this something like

$post_tag = PostTag::findOrFail($id);
$post_tag->votes()->save(new Vote)

But this doesn't work because you can't instantiate, the only thing i can do now is

POST /post/{post}/tag/{tag}/likes

$post = Post::findOrFail($id);
$post_tag = $post->tags()->findOrFail($tag)->pivot;
$post_tag->votes()->save(new Vote);

that's so much cumbersome and so less performant as you have so many queries here while the only info needed is the pivot table record to save the associated vote, and not the tag record nor the post record

brunocascio commented 7 years ago

Take a look at https://stackoverflow.com/a/44932979/2723301

I've implemented in a better way.

darkylmnx commented 7 years ago

@brunocascio the solution in your link doesn't make the pivot model instantiable since the pivots are gotten from the related query Banner

What we want here is BannerRegion to be in instantiable (based on your link)

decadence commented 7 years ago

Merged in 5.5.

JuanDMeGon commented 7 years ago

Awesome! Thank you @BertvanHoekelen

caseydwyer commented 7 years ago

@JuanDMeGon +1 for posting. I've gone hunting for this a couple of times; glad to see it make its way into the 5.5 release. Can anyone confirm if this addition to 5.5 will also allow for eager loading?

eg, @rdpascua's example: $foo->with('bar.pivot.baz');

pnlinh-ivc commented 6 years ago

@cjaoude How can I return data relate with pivot table to json without loop?

mazyvan commented 5 years ago

I don't clearly understand why this issue gets to long in comments. I mean AFAIK following the Liskov substitution principle the behavior this had was is not correct. Now I'm looking a way to go workaround with this in 5.4. I have a MorphPivot class and I need it to relation with another class. But I can't. Any idea? BTW I know this isn't the proper place to ask the question, but I bet some of you guys had facing the same problem