Naive implementation of WhereHas with Polymorphic relations

[timacdonald]

I have wanted to be able to query polymorphic relations with the whereHas method, as I'm sure anyone using them has also wanted to.

I came across this article that shows a way to implement this in your app: https://zaengle.com/blog/using-wherehas-in-laravel-polymorphic-relations

That article (which is awesome) shows an implementation that requires you to add a new method for each new type of relation, so not something the framework can really handle for you.

I hacked on this for some time and came up with the following implementation that allows you to specify a whereHas and provide the class type you want to filter against. Because this is a bit more of a dynamic approach that could be applied to all apps, I thought I'd at least float the idea here, after all...this is laravel/ideas

This is working well for me in my applications, but I'm aware I've only used it, and tested it, against its happy path. I would love someone with better knowledge of the query builder, and database queries in general, to roast this thing and pull it apart before I try and PR it to the core.

How it could look on a model...

$events = Event::whereEventable(Post::class, function ($query) {
    $query->wherePublished();
})->etc(...);

---

You do have to be specific about the class you are checking against, but multiple checks can be chained...

$events = Event::whereEventable(Post::class, function ($query) {
    $query->wherePublished();
})->orWhere->whereEventable(Comment::class, function ($query) {
    $query->wherePublished();
})->etc(...);

With this 👆 it should be possible to allow you to pass in an array of classes, and handle the check against multiple classes instead of having to repeat the closure logic, but I'll implement that if this looks okay to people.

So although I have not yet implemented it, it should be possible to do this...

$classes = [Post::class, Comment::class];

$events = Event::whereEventable($classes, function ($query) {
    $query->wherePublished();
})->etc(...);

---

It also handles morph map values...

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

$events = Event::whereEventable('posties', function ($query) {
    $query->wherePublished();
})->etc(...);

---

I threw it all in this gist, so jump in and check it out, and please point out how this is not ever going to work.

Gist: https://gist.github.com/timacdonald/128e7d150f7214b51a030193d3d2b937

I know the naming isn't great. If this was PR'd I assume it would have a more generic name like whereHasPolymorphic(...) rather than my generic filterPolymorphicRelation(...). It'll also be on the eloquent query builder, not on the model.

@staudenmeir no doubt you are busy, but if you have a few minutes to look at this it would be great! Just ping'ing you as I know you have a really strong knowledge of the query builder. (P.S. thanks for all the work you do on Laravel)

I guess it'd also be handy to know if others think this would even be useful. No point having it in core if only a handful of people are gonna use it.

I'll also work out a way to specify the full key and type columns. But don't wanna do that work if this is just a non-starter.

This approach still requires the developer to add a wrapping method whereEventable In this example, but the underlying helper on the query builder I think would useful.

This is a previous thread that is probably useful to reference as well: laravel/framework#18523

No doubt performance is going to be a concern

[staudenmeir]

I guess it'd also be handy to know if others think this would even be useful. No point having it in core if only a handful of people are gonna use it.

Judging from the participation in laravel/framework#5429 and laravel/framework#18523, there's definitely a demand for this ;-)

I think we should aim for a "complete" solution that also supports counting:

Event::has('eventable', '>', 3)

Event::whereHas('eventable', function($query) {}, '=', 5)

I've experimented with this a while ago. We can use the BelongsTo workaround without actually defining the relationships in the model:

$relation = $model->belongsTo(Post::class, 'eventable_id', 'id')

I'm imagining method signatures like this:

public function hasPolymorphic($relation, $types, $operator = '>=', $count = 1, $boolean = 'and', Closure $callback = null)

public function whereHasPolymorphic($relation, $types, Closure $callback = null, $operator = '>=', $count = 1)

Event::hasPolymorphic('eventable', [Post::class, Video::class])

Event::whereHasPolymorphic('eventable', [Post::class, Video::class], function($query) {})

With an additional query (and a warning in the docs), we could even support a wildcard:

Event::hasPolymorphic('eventable', ['*'])

[timacdonald]

This is great. Thanks for taking the time to look at this - I really appreciate it.

I'm gonna hack on an update to my original version. I've got a little test suite setup to help validate a few things that I'll refine and continue to add to.

My current feature lists for this to be ready for a PR...

Must haves:

• Consistent naming with existing methods
• Access to model specific scopes
• Complete has and whereHas like functionality
• Pass array of classes and run query through all

Nice to haves:

• Proxy to polymorphic methods from the standard has and whereHas methods.
• Wildcard support

Question:

I'm wondering if we should allow the array of classes now. Looking at it again I'm wondering if it is ambiguous.

Event::hasPolymorphic('eventable', [Post::class, Video::class]);

A developer might look at this an not be sure if it is an and or or type query. Does it have to have both a Post and a Video class? Or does it only have to have on or the other.

Because of this, I'm thinking about dropping this off the list of features I'm aiming for - and that can be debated, probably with Taylor's input (if we get to that stage).

Alternatively we could also introduce these, but I'm less keen on this...

// and
Event::hasAllPolymorphic('eventable', [Post::class, Video::class]);

// or
Event::hasAnyPolymorphic('eventable', [Post::class, Video::class]);

Thoughts on this?

[staudenmeir]

IMO, the array of classes/types is a crucial feature. Otherwise, using [where]HasPolymorphic() with multiple types would require quite a lot of code.

A developer might look at this an not be sure if it is an and or or type query.

I think this would have to be clarified in the documentation and the PHPDoc.

orHasPolymorphic() and orWhereHasPolymorphic() would handle the or case.

[timacdonald]

Sounds good. I'm gonna take your guidance on that then and we'll make it happen.

Also, thinking about the wildcard, I'm wondering if the class(es) are excluded that it almost denotes a wildcard. It is less explicit, but could also be an API option.

// these are both wildcard queries...

Event::hasPolymorphic('eventable')

Event::whereHasPolymorphic('eventable', function ($query) {})

[staudenmeir]

I thought about that, but I think the wildcard should be an explicit choice.

The additional query it executes every time the relationship is used shouldn't come as a surprise to the user.

[staudenmeir]

A developer might look at this an not be sure if it is an and or or type query.

I think this would have to be clarified in the documentation and the PHPDoc.

orHasPolymorphic() and orWhereHasPolymorphic() would handle the or case.

Sorry, that was nonsense. hasPolymorphic() can only be an or query.

A single row can't have a post and a video...

[phroggyy]

@timacdonald did you make any progress on this? I'd be happy to lend a hand with this if desired, as it's something I've wanted for a while now, without any really satisfactory solution that doesn't result in major caveats.

One of the important ones for me is to execute it within a single query, the same way whereHas results in an exists query, to avoid massive memory use of filtering the results after the fact.

For the sake of streamlining, I'd be interested in seeing if we could tie this in with the existing whereHas, but leave a disclaimer in the docs that you'll need to manually add constraints (hopefully in a sensible manner, like providing a PolymorphicExistsQuery that people can chain onto), not entirely sure how that'd be done yet though.

[timacdonald]

I haven't pushed forward any further yet, but am keen to keep moving it along when I can. Would love any input / help / guidance if you are keen.

I'm no expert in this area, so I'm gonna say stuff and anyone can (please) feel free to correct me.

The current implementation does a sub query to check against the ID's, but all of the work is done in the database, i.e. we are not pulling the records out and then checking the IDs after the fact (if that is what you meant).

Because it is going to require a different API (as far as I can tell at this point) I think it makes sense to make explicit methods as @staudenmeir and I have discussed above.

Event::hasPolymorphic('eventable', Post::class, '=', 2);

Event::whereHasPolymorphic('eventable', Post::class, function ($query) {
    //
});

Although that GIST is a working version, I agree with @staudenmeir that it would be nice to have a full featured version matching the existing has and whereHas methods.

Will get a repo up this weekend and perhaps we can move discussion over to it.

Please send me your thoughts and ideas.

[timacdonald]

I can see benefit in adding to the existing whereHas but I think it is going to have to be different enough that a new API is probably a good idea - but I could definitely have my mind changed on this after seeing some examples of how it could work.

[staudenmeir]

I've created a first draft: staudenmeir/framework@ad46e29

You can provide a list of types or explicitely use the wildcard:

Event::hasPolymorphic('eventable', [Post::class, Video::class]);

Event::hasPolymorphic('eventable', ['*']);

Event::whereHasPolymorphic('eventable', ['*'], function ($query) {
    //
});

Regarding the list of types: What would you say is the expected behavior when using a $morphMap? Does the user provide aliases or classes? For the draft, I chose aliases.

[timacdonald]

This is flippin sweet @staudenmeir thanks for putting this together. Lets make this (if you are cool with it) the canonical place for us to hack on this feature.

I've just sent a PR with some tweaks I think could be good: https://github.com/staudenmeir/framework/pull/1

[timacdonald]

@jesseschutt just pinging you here because this came out of your original blog post and thought you may have some wisdom to share. Hopefully we can get this to a point and cover all bases to get it into core 🤞

[timacdonald]

I think the alias / class thing you've got looks right to me and was how my first version worked as well I believe.

So the morph aliases takes precedence but if there is no morph map value it just uses what you passed, which allows for the class to go through.

[timacdonald]

@staudenmeir I'll PR the functional tests I've already written to your repo when I get a chance as well.

[staudenmeir]

👍 I've also started writing integration tests.