[5.2] Impliment nested wheres in has queries #13794
Conversation
wleona3
changed the title
| * @return bool | ||
| */ | ||
| protected function shouldNestWheres(QueryBuilder $query, $originalWhereCount) | ||
| { |
There was a problem hiding this comment.
@wleona3 I don't think we really need to alias shouldNestWheresForScope. Just use the existing method.
|
@wleona3 All in all, I was thinking we could maybe extract the shared logic between I'd suggest something like this: acasar@658663d |
|
Agreed and updated. |
| @@ -1239,7 +1254,7 @@ protected function nestWheresForScope(QueryBuilder $query, $whereCounts) | |||
| // collection (0 and total where count) while also flattening the array | |||
| // and extracting unique values, ensuring that all wheres are sliced. | |||
| $whereOffsets = collect([0, $whereCounts, count($allWheres)]) | |||
| ->flatten()->unique(); | |||
| ->flatten()->unique(); | |||
There was a problem hiding this comment.
Please revert this line to original formatting.
There was a problem hiding this comment.
Yes, please revert this change.
|
Looking better now :) Can you apply the formatting changes and squash the commits? |
|
Changes made and squashed |
|
One change remaining to be reverted please. |
|
Whoops. Corrected. |
|
Thanks. :) |
|
@taylorotwell this is now ready for review. Basically, Model::whereHas('relation', function ($query) {
$query->where('foo', 'bar')->orWhere('foo', 'baz');
}); |
| * @param \Illuminate\Database\Query\Builder $query | ||
| * @return mixed | ||
| */ | ||
| protected function applyCallback(callable $callback, $parameters = [], $query = null) |
There was a problem hiding this comment.
What is the purpose of this method? Could it have a better more descriptive name than applyCallback which sounds very vague and general?
There was a problem hiding this comment.
It applies the callback in a way that doesn't cause any logical issues with boolean order.
To put it simply - "or where" conditions are nested. And since this is now needed in two different places, it was extracted in its own method.
There was a problem hiding this comment.
How about something like applyCallbackAndNestWheres?
|
Can this be pulled into 5.1? |
Sorry, no. Our LTS branch gets big fixes only. |
|
You're saying that this -- #13785 (comment) -- isn't considered a bug? |
|
No. It's a feature. |
|
orWhere() not nesting properly inside of a whereHas() is a feature? How so? EDIT: Maybe I'm asking for the wrong commit to be pulled. |
|
|
The ability to do that is not required, it's a feature. |
|
No idea what Graham is talking about in regards to "being a feature", so apologize for that... but can you not do this? ->where(function ($q) { |
|
@taylorotwell The point is you can achieve the same thing without nesting. The ability to nest these is a feature. |
|
@taylorotwell, I understand what @GrahamCampbell is saying. I see both sides of this discussion. Intuitively, it is weird to have to force the nesting with an extra ->where() closure, but I also understand why auto-nesting it causes other problems in the query builder. All said, the extra ->where() closure isn't the worst solution. Thanks for the explanations and suggestions. |
Implemented based on #13785. Existing scope methods worked, so they've been renamed and aliased.