Skip to content

Conversation

@staudenmeir
Copy link
Contributor

MorphTo::associate() doesn't consider a custom owner key and always uses the primary key:

class Comment extends Model
{
    public function commentable()
    {
        return $this->morphTo(null, null, null, 'slug');
    }
}

$post = Post::create(['slug' => 'foo']);

$comment = (new Comment)->commentable()->associate($post);

dump($comment->commentable_id);

// expected: "foo"
// actual:   1

@taylorotwell taylorotwell merged commit f7a16ce into laravel:5.8 Aug 27, 2019
@staudenmeir staudenmeir deleted the morph-to-associate branch August 27, 2019 20:01
@lindyhopchris
Copy link

@staudenmeir did you test this within a real app? I've got failing tests now.

The scenario is Comment can morph to Post or Video. Post and Video have different key names.

If you have a Comment that is currently associated to a Video, and you use associate on the relationship to now associate it to a Post, the owner key is incorrect.

This is because this line...
https://github.com/laravel/framework/blob/5.8/src/Illuminate/Database/Eloquent/Concerns/HasRelationships.php#L236-L238

...passes through the owner key based on the model it is currently associated to. Before your change this didn't make any difference because the key name when associating the relationship was always taken from the model that is being passed in to associate().

So I believe your change is breaking without any changes to the HasRelationships method.

@staudenmeir
Copy link
Contributor Author

Thanks for reporting. Reverted in #29802.

There's apparently no other way to fix the bug.

@lindyhopchris
Copy link

Ah great, thanks for reverting. It's a shame there's no other way to fix it, as your change made sense... as long as both morph types had the same custom owner key.

taylorotwell pushed a commit that referenced this pull request Aug 31, 2019
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