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 whereHasMorph() #28928

Merged
merged 7 commits into from
Jun 27, 2019
Merged

[5.8] Add whereHasMorph() #28928

merged 7 commits into from
Jun 27, 2019

Conversation

staudenmeir
Copy link
Contributor

@staudenmeir staudenmeir commented Jun 23, 2019

Due to the nature of polymorphism, whereHas() doesn't work with MorphTo relationships (#5429, #18523).

As a solution, @timacdonald and I are proposing whereHasMorph() and the corresponding methods:

Comment::whereHasMorph('commentable', [Post::class, Video::class], function ($query) {
    $query->where('title', 'foo');
})->get();
select * from "comments"
where (
  (
    "commentable_type" = 'App\Post' and exists (
      select * from "posts" where "comments"."commentable_id" = "posts"."id" and "title" = 'foo'
    )
  ) or (
    "commentable_type" = 'App\Video' and exists (
      select * from "videos" where "comments"."commentable_id" = "videos"."id" and "title" = 'foo'
    )
  )
)

By creating a temporary BelongsTo relationship for each type and allowing relationship instances as the first argument of has(), we can reuse most of the existing code.

If the user has registered custom aliases in the morphMap, they need to provide them instead of class names:

To simplify queries with different constraints, we pass the type as the second parameter:

Comment::whereHasMorph('commentable', [Post::class, Video::class], function ($query, $type) {
    if ($type === Post::class) {
        // $query->
    }

    if ($type === Video::class) {
        // $query->
    }
});

You can also provide a wildcard and let Laravel get the possible types from the database:

Comment::whereHasMorph('commentable', '*', function ($query) {
    $query->where('title', 'foo');
})->get();

The downside here is the additional query that should be mentioned in the docs.

Theoretically, you could use * as an alias, but we think it's a better choice than something like null .

@phroggyy
Copy link
Contributor

phroggyy commented Jun 23, 2019

Is there a reason we can't do a morphMap lookup to allow always passing classes? Seems a bit flawed to have to change things in multiple places, unless I'm missing something. Should just be a matter of Relation::getMorphedModel($fqcn)

@timacdonald
Copy link
Member

timacdonald commented Jun 23, 2019

I might be miss understanding what you mean, but as it stands you can pass:

  • Only class types i.e. fqcn
  • Only morph map values
  • A mixture of morph and class types

i.e. all of the following work...

Relation::morphMap(['posts' => Post::class, 'videos' => Video::class]);

// only classes
Comment::whereHasMorph('commentable', [Post::class, Video::class], function ($query) {
    //
})->etc(...);

// only morph values
Comment::whereHasMorph('commentable', ['posts', 'videos'], function ($query) {
    //
})->etc(...);

// a mixture of both
Comment::whereHasMorph('commentable', ['posts', Video::class], function ($query) {
    //
})->etc(...);

We have to create an on-the-fly relationship - hence passing the $belongsTo relation instance to the has method.

Here is where the morph map lookup is happening:

protected function getBelongsToRelation(MorphTo $relation, $type)
{
$belongsTo = Relation::noConstraints(function () use ($relation, $type) {
return $this->model->belongsTo(
Relation::getMorphedModel($type) ?? $type,
$relation->getForeignKeyName(),
$relation->getOwnerKeyName()
);
});
$belongsTo->getQuery()->mergeConstraintsFrom($relation->getQuery());
return $belongsTo;
}

@phroggyy let us know if that addresses your concern or if I'm way off base with what you mean 🙂


Another nice thing about this is that you have access to the scopes on the models as well. So if Video and Post all share a wherePublished() scope you can access it...

Comment::whereHasMorph('commentable', [Post::class, Video::class], function ($query) {
    $query->wherePublished();
})->etc(...);

@phroggyy
Copy link
Contributor

Ah, gotcha. In that case, the phrasing of the PR is wrong:

If the user has registered custom aliases in the morphMap, they need to provide them instead of class names:

As per your explanation, this is not true. The user may pass a class name or morph value, but whether the morph map is used doesn't matter.

@timacdonald
Copy link
Member

timacdonald commented Jun 23, 2019

🤦‍♂️ Sorry, I actually misspoke. What @staudenmeir said is how this implementation currently works.

@timacdonald
Copy link
Member

timacdonald commented Jun 24, 2019

I've just created a PR to @staudenmeir’s branch that does the reverse lookup and allows classes even when a morph value has been specified - which matches what I said in my earlier comment.

But we might just need to consider if there are any implications to this before merging. Thoughts?

@staudenmeir
Copy link
Contributor Author

staudenmeir commented Jun 24, 2019

I don't think it's a good idea to allow both class names and aliases.

What if users only provide class names and Laravel handles the aliases? This way, users wouldn't have to adjust the queries when they add a morphMap.

@timacdonald
Copy link
Member

timacdonald commented Jun 24, 2019

I don't personally use a morph map, so not sure how valuable my opinion in on this specific part, but that sounds like a good way to go about it.

Either way, I've just created a PR for this behaviour as well.

@staudenmeir
Copy link
Contributor Author

whereHasMorph() now only allows class names and handles aliases itself:

Relation::morphMap(['posts' => Post::class]);

Comment::whereHasMorph('commentable', [Post::class, Video::class], function ($query) {
    $query->where('title', 'foo');
})->get();

@taylorotwell taylorotwell merged commit c676fc0 into laravel:5.8 Jun 27, 2019
@taylorotwell
Copy link
Member

Nice one!

@denitsa-md
Copy link

denitsa-md commented Jul 11, 2019

I just opened this issue #29138 related to this PR. Not sure if I'm doing something wrong but can't seem to get the doesntHaveMorph to work :)

@mikkopursuittechnology
Copy link

mikkopursuittechnology commented Aug 16, 2019

With this improvement can you do nested relationship on morphed classes?

Comment::whereHasMorph('commentable', [Post::class, Video::class], function ($query) {
    $query->has('author');
})->get();

In the example above. I'm trying to look for Comments from Posts and Videos that has an author.

@pelmered
Copy link
Contributor

This is a great addition, but I can't get it to work with a morphToMany-relation. It does not query *_id and *_type in the intermediate table.
I guess we need a whereHasMorphMany as well?

@staudenmeir
Copy link
Contributor Author

@mikkopursuittechnology Yes, you can.

@pelmered whereHasMorph() is only for MorphTo relationships. Use whereHas() for all other relationships.

@pelmered
Copy link
Contributor

@staudenmeir Ok, but whereHas() doesn't work with MorphToMany(). When I use whereHas() I get a RuntimeException with message: Please use whereHasMorph() for MorphTo relationships.

And with whereHasMorph() it queries the wrong table.

Conclusion: we need a whereHasMorphMany() as well for morphToMany-relations.

@staudenmeir
Copy link
Contributor Author

@pelmered How is your relationship defined? This exception is only thrown for MorphTo relationships.

@pelmered
Copy link
Contributor

@staudenmeir Yes, you where right. Where was an error in the inverse of the relationship. Sorry about that. whereHas() works fine.

@usamamuneerchaudhary
Copy link

and how to implement the inverse of the relationship?

@hazemwehbi
Copy link

Hello all,
Can I find in anyone of you any piece of code which apply or use whereHasMorph() ?
Thx in advance

@staudenmeir
Copy link
Contributor Author

@hazemwehbi
Copy link

Thx a lot, I already saw the laravel documentation and followed it but I don't know why it's not working to me.
It show me this error.
"Maximum function nesting level of '250' reached, aborting!"

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.

9 participants