[5.2] Merge wheres to hasQuery for withCount() #13612
Conversation
Fixes missing wheres defined on the relation, see #13414 (comment)
|
Added tests for this behavior, seems to be good now, if @taylorotwell can verify this is how the |
|
@acasar would be a good one to check this over. |
| @@ -1030,6 +1030,7 @@ public function withCount($relations) | |||
| $relation->getRelated()->newQuery(), $this | |||
| ); | |||
|
|
|||
| $this->mergeModelDefinedRelationWheresToHasQuery($query, $relation); | |||
| call_user_func($constraints, $query); | |||
There was a problem hiding this comment.
@barryvdh I think that call_user_func should come before mergeModelDefinedRelationWheresToHasQuery.
That's because you can potentially remove global scopes from the $query in the callback and mergeModelDefinedRelationWheresToHasQuery takes care of removing them from the $relation too.
There was a problem hiding this comment.
Okay.
Should I also call toBase() instead of query, to apply the scopes back?
There was a problem hiding this comment.
Ah, yeah you should. Nice catch!
So, line 1036 instead of $query->getQuery() it should be $query->toBase()
There was a problem hiding this comment.
Okay so I changed both, although I'm not really sure how to write an actual test for that specific case..
There was a problem hiding this comment.
I think it should be very similar to this: https://github.com/laravel/framework/blob/5.2/tests/Database/DatabaseEloquentSoftDeletesIntegrationTest.php#L410:L427
There was a problem hiding this comment.
So something like this? 4abb185
GrahamCampbell
changed the title
GrahamCampbell
changed the title
|
👍 Looks good! |
|
Good work @barryvdh Can confirm this has fixed my issue |
|
👍 Good work. ;) |
Fixes missing wheres defined on the relation when creating the subquery for a relation count, see #13414 (comment)