Fix get all records

[sebacastroh]

All Submissions:

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

1. Does your submission pass tests and did you add any new tests needed for your feature?
2. Did you update all templates (if applicable)?
3. Did you add the relevant documentation (if applicable)?
4. Did you test locally to make sure your feature works as intended?

Changes to Core Features:

• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your core changes, as applicable?
• Have you successfully ran tests with your changes locally?

About the PR

As discussed in #1235 , using count does not always return the proper result when the user wants to see all records of a table. This happens when using GROUP BY or similar functions.

An example

Let's say I have this query builder

public function builder(): Builder
{
    $suffix_roles = '_1';
    $suffix_regions = '_2';
    $query = User::withRoles($suffix_roles)
        ->withRegions($suffix_regions)
        ->groupBy(['users.id'])
        ->select([DB::raw('MIN(CONCAT_WS(";;",`roles' . $suffix_roles . '`.`precedence`, `roles' . $suffix_roles . '`.`readable_name`)) as `precedence`'),
                  'regions' . $suffix_regions . '.label as region_name',]);

    return $query;
}

Additionally I select other columns in the columns function.

In this example, we know that we should get 20 rows. When using count (when $perPage = -1) on this builder, it generates the following query

select
	count(*) as aggregate
from
	`users`
left join (`model_has_roles` as `model_has_roles_1`
inner join `roles` as `roles_1` on
	`roles_1`.`id` = `model_has_roles_1`.`role_id`) on
	`model_has_roles_1`.`model_id` = `users`.`id`
left join `regions` as `regions_2` on
	`regions_2`.`cut_region` = `users`.`region`
group by
	`users`.`id`
order by
	`precedence` asc,
	`regions_2`.`order` asc,
	`users`.`name` asc

which results in 1 record instead of the 20 expected records. Then, when getting the records the query generated by paginate is

select
	MIN(CONCAT_WS(";;", `roles_1`.`precedence`, `roles_1`.`readable_name`)) as `precedence`,
	`regions_2`.`label` as `region_name`,
	`users`.`name` as `name`,
	`users`.`email` as `email`,
	`users`.`created_at` as `created_at`,
	`users`.`region` as `region`,
	`users`.`provider` as `provider`,
	`users`.`disabled_at` as `disabled_at`,
	`users`.`id` as `id`
from
	`users`
left join (`model_has_roles` as `model_has_roles_1`
inner join `roles` as `roles_1` on
	`roles_1`.`id` = `model_has_roles_1`.`role_id`) on
	`model_has_roles_1`.`model_id` = `users`.`id`
left join `regions` as `regions_2` on
	`regions_2`.`cut_region` = `users`.`region`
group by
	`users`.`id`
order by
	`precedence` asc,
	`regions_2`.`order` asc,
	`users`.`name` asc
limit 1 offset 0

Solution

Laravel's paginate computes the total of rows calling $this->toBase()->getCountForPagination() (it can be a function also, but that is out of the scope in our case). This function creates the proper query to get the total amount of rows, i.e.

select
	count(*) as aggregate
from
	(
	select
		MIN(CONCAT_WS(";;", `roles_1`.`precedence`, `roles_1`.`readable_name`)) as `precedence`,
		`regions_2`.`label` as `region_name`,
		`users`.`name` as `name`,
		`users`.`email` as `email`,
		`users`.`created_at` as `created_at`,
		`users`.`region` as `region`,
		`users`.`provider` as `provider`,
		`users`.`disabled_at` as `disabled_at`,
		`users`.`id` as `id`
	from
		`users`
	left join (`model_has_roles` as `model_has_roles_1`
	inner join `roles` as `roles_1` on
		`roles_1`.`id` = `model_has_roles_1`.`role_id`) on
		`model_has_roles_1`.`model_id` = `users`.`id`
	left join `regions` as `regions_2` on
		`regions_2`.`cut_region` = `users`.`region`
	group by
		`users`.`id`) as `aggregate_table`

which returns the expected number 20.

In order to use this value, I define $perPage = function ($total) {return $total;}, so paginate will copy $total to $perPage for the standard pagination. In the case of the simple pagination, we have to do the query before calling the function.

Warning

This change is for Laravel 9.X and 10.X, unfortunately for 8.X and older you have to compute $perPage = $this->getBuilder()->toBase()->getCountForPagination() for both standard and simple pagination as $perPage in paginate function can be only int or null, not a function. That means you will have a duplicate query.

[lrljoe]

As this will break L8, we'd need to look at a way around that. I know L8 will almost certainly be dropped for v3 of this package, where we'll be able to utilise this approach, but that's some way off!

[sebacastroh]

Something like app()->version() could help?

[lrljoe]

Most likely, feel free to have a tinker, and update the PR and I'll run it through my various demo environments. Really appreciate the contribution!

I do intend to get the tests and demo environment aligned in the near future to allow for easier testing!

[sebacastroh]

I added a workaround for Laravel 8.X. This will duplicate the query $this->getBuilder()->toBase()->getCountForPagination() but this is unavoidable in that version. In Laravel 9.X and later this was solved and no queries will be duplicated.

[lrljoe]

Thanks, I'll give this a review over the weekend, if you don't mind updating your README to reflect develop, then this will make the merge commit error go away too, which I'd appreciate.

[sebacastroh]

Done!

[lrljoe]

Looks okay initially. Going to do more testing this week and then merge this weekend if all okay to go into next release.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.