This repository was archived by the owner on Apr 17, 2019. It is now read-only.
Repository navigation
This repository was archived by the owner on Apr 17, 2019. It is now read-only.
Datatable::query with a query that has groupBy gives incorrect total row count. #253
Description
$query = DB::table('orders')
->orderBy('user_id');
->groupBy('user_id');The row count will always be the total amount of orders for the first row returned.
The query that is built is:
select count(*) as aggregate from `orders` group by `user_id`
-- this returns a row for each user_idHowever, because of the group by, the query that is built should be:
select count(DISTINCT user_id) as aggregate from `orders`
-- the following returns only one row. the total amount of users that have ordersI am trying to look into an easy way to fix this issue, but so far it looks like there might not be one. It looks like extending Query\Builder and exposing the protected $groups array might be the way to go.
Activity
timgws commentedon Mar 8, 2015
Pull requests have been merged \o/
bytestream commentedon Sep 26, 2015
Can we get the same merged into the develop branch for Laravel 5?
I noticed there's already a
setNoGroupByOnCount()function in the QueryEngine (in the develop branch) - see #136. That being said, this does not work if you useIlluminate\Database\Eloquent\Builder(seems to be built directly forIlluminate\Database\Query\Builder)timgws commentedon Sep 27, 2015
@bytestream good catch, I will work on this a little later today.
bytestream commentedon Sep 29, 2015
@timgws any news on this? I'm happy to submit a pull request resolving the issue with the Eloquent Builder in the develop branch
timgws commentedon Sep 30, 2015
@bytestream feel free to submit a pull request. I am currently focusing my efforts at the moment on a new branch, refactoring some of the code for a new version5 branch.
If you don't do it today, I will try to get around to "soon" (I want to say after I finish work, but that might not be the case).
Fixes Chumper#253
timgws commentedon Oct 6, 2015
This is almost on point, @bytestream, but I would recommend splitting out all of that code to a separate function. At the moment there is code in two places that does exactly the same thing.
bytestream commentedon Oct 6, 2015
Good point - I did this in a rush.
I'll update the pull request before I leave in a few hours
On 6 October 2015 at 14:52, Tim Groeneveld notifications@github.com wrote:
Fixes Chumper#253
Merge pull request #340 from bytestream/develop
timgws commentedon Oct 7, 2015
Thank you very much for this, @bytestream