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

Conversation

Projects
None yet
5 participants
@staudenmeir
Copy link
Contributor

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 .

staudenmeir and others added some commits Jun 21, 2019

Merge pull request #2 from timacdonald/access_scope_test
Add test to check model scopes are accessible
@phroggyy

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2019

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

@timacdonald

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

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 added some commits Jun 25, 2019

@staudenmeir

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

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

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@taylorotwell

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

Nice one!

@denitsa-cm

This comment has been minimized.

Copy link

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.