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.

[timacdonald]

Awesome 👏

[staudenmeir]

I did some refactoring and added tests: staudenmeir/framework@5.8...where-has-polymorphic

What do you think of '*' as the wildcard? Would null be better?

We could hint null in the PHPDoc, but people could also use it "accidentally" without knowing about the additional query. With '*', you would need to read the docs (or look at the code). Theoretically, '*' could also be an alias.

[timacdonald]

I think that '*' is the best idea - using null as a flag of significance kinda feels wrong to me.

Although it could be a morph map - I think this functionality just gets introduced in the next version to allow Taylor to point out that '*' is no longer a valid morph map value in the upgrade guide.

They are my thoughts on it anyway.

[staudenmeir]

I also prefer '*', but it's a bit unfortunate that this is the only reason we can't target 5.8.

[timacdonald]

For sure, however the next release is only a few weeks out, as per: laravel/framework#28740 (comment)

[phroggyy]

I don't think this technically prevents anything. This is new functionality, not breaking. This doesn't prevent anyone from using * in their morph map, it just means that anyone who's using * cannot use whereHasPolymorphic

[staudenmeir]

@phroggyy You are absolutely right.

[ilirien]

@timacdonald As I understood, we can only call one scope for all types in whereHasPolymorphic, but we can do something like that:

hasPolymorphic('eventable', [Post::class => function($query) {}, Video::class => function($query) {}]);

What the benefit of this, you have different scopes for different types.

After adding in that way we even don't need whereHasPolymorphic I think.

[ilirien]

And just my opinion, better to call it hasMorph, everywhere in laravel it calls like that. So I think it will be good to follow.

[timacdonald]

I'm 100% on board with (and even keen for) whereHasMorph and hasMorph, even if they are an alias. What do you think @staudenmeir @phroggyy ?

It would line up with the naming of the new morphWith method and a bunch of other morph related methods.

@ilirien I'll reply with some examples when I can with more info about the chaining of scopes.

[staudenmeir]

@ilirien I like your idea, I renamed the methods.

As the typical use case, I would except the same constraint for all types:

Event::whereHasMorph('eventable', [Post::class, Video::class], function ($query) {
    $query->where('title', 'like', '%'.$search.'%');
});

If you want to use different constraints:

Event::whereHasMorph('eventable', Post::class, function ($query) {
    //
})->orWhereHasMorph('eventable', Video::class, function ($query) {
    //
});

[ilirien]

@staudenmeir about different constraints. Yes your example will work, but you will need to wrap it into one group almost always. It's just suggestion how to improve and decrease code lenght.

I know what I'm saying, because I have similar implementation(it little bit hacky). And other devs using it. And we had implementation like yours previously and sometimes devs forgots to wrap and query won't work. You can left scope for all types and add as array value for specific type. And then apply them both for query. It will be maximum flexible and suitable.

[staudenmeir]

@ilirien What's the method signature of your implementation?

[ilirien]

@staudenmeir

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

[staudenmeir]

But this doesn't allow you to specify the same constraint for multiple types (without repeating it or using a variable), right?

[ilirien]

@staudenmeir

In my variant I don't have third parameter as constrain for all types, but it will be great idea. :)

You can pass it and just apply this scope, it's easy to implement

[staudenmeir]

I don't think a variable number of parameters is a good idea.

[timacdonald]

Personally I'm a fan of the current implementation over the [class => function] approach.

[staudenmeir]

A possible compromise: We use the current implementation and pass the type as the second parameter:

Event::whereHasMorph('eventable', [Post::class, Video::class], function ($query, $type) {
    if ($type === Post::class) {
        // 
    }

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

Event::whereHasMorph('eventable', [Post::class, Video::class], function ($query, $type) {
    switch ($type) {
        case Post::class:
            //
            break;
        case Video::class:
            //
            break;
    }
});

[timacdonald]

I think this is a sensible compromise - and could even be handy in other situations we haven't considered as well.

[ilirien]

@staudenmeir I like it, good idea

[staudenmeir]

I added the type parameter: staudenmeir/framework@0a0ed56

Are we ready for a PR?

[timacdonald]

Let me PR my tests to ya repo. I’ll get it done this weekend for ya.

[staudenmeir]

👍 I created a new branch and squashed the commits: staudenmeir/framework@5.8...where-has-morph

[timacdonald]

Just sent through the only test in my suite that I think would add any value - you seem to have covered everything else. Dunno about you, but I say it is time for a good old fashioned PR! 🎉 🤞

[staudenmeir]

Here we go: laravel/framework#28928

[staudenmeir]

We can close this ;-)