laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.43k stars 10.99k forks source link

[5.6.27] Pagination pages count incorrect #25804

Closed StAngerGordun closed 6 years ago

StAngerGordun commented 6 years ago

Description:

Pagination returns an incorrect number of pages. Eloquent collection has 32 items ( dd($collection->get()) and dd(count($collection->get())) show it). But when I use $collection->paginate(10) in back and $collection->links() in front it shows 24 pages. $collection->paginate(10)->count() return 234.

When I use simplePaginate method - it works fine.

jmarcher commented 6 years ago

You clearly need to provide more info in order to name to follow this further.

StAngerGordun commented 6 years ago

You clearly need to provide more info in order to name to follow this further.

I have a query something like this: $products = Product::where($filters)->join('categories', 'categories.id', '=', 'products.category_id')->distinct('products.id');. This is a simplified query, but the essence, I think, is understandable.

I want to return in the response products with pagination: return view('products')->with(['products' => $products->paginate(10)])

laurencei commented 6 years ago

It doesnt make any sense to count the pagination.

You want to do $collection->total() to get the total number of records

staudenmeir commented 6 years ago

What exactly is $collection in your case?

StAngerGordun commented 6 years ago

What exactly is $collection in your case?

Something like this: $collection = Product::where($filters)->join('categories', 'categories.id', '=', 'products.category_id')->distinct('products.id');.

screenshot_2

In this screen: 1) dd($collection->get()) 2) dd($collection->paginate(10))

staudenmeir commented 6 years ago

paginate() doesn't support distinct(). Use groupBy() instead:

Product::select('products.*')
    ->join('categories', 'categories.id', '=', 'products.category_id')
    ->where($filters)
    ->groupBy('products.id', ...)
    ->paginate(10);

Since you are using MariaDB, you have to add all columns from select() to groupBy().

StAngerGordun commented 6 years ago

paginate() doesn't support distinct(). Use groupBy() instead:

Product::select('products.*')
    ->join('categories', 'categories.id', '=', 'products.category_id')
    ->where($filters)
    ->groupBy('products.id', ...)
    ->paginate(10);

Since you are using MariaDB, you have to add all columns from select() to groupBy().

But why? If I use a custom paginator LengthAwarePaginator - everything works fine! Something like this:

$products = Product::select('products.*')
    ->join('categories', 'categories.id', '=', 'products.category_id')
    ->where($filters)
    ->distinct('products.id')
    ->get();
$sliced = $products->slice($perPage * ($currentPage - 1), $perPage);
$finalResult =  new LengthAwarePaginator($sliced,
            \count($products),
            $perPage,
            $currentPage
        );
staudenmeir commented 6 years ago

What is your actual query? The query you posted doesn't require a DISTINCT clause. Also: The distinct() method doesn't accept arguments.

StAngerGordun commented 6 years ago

What is your actual query? The query you posted doesn't require a DISTINCT clause. Also: The distinct() method doesn't accept arguments.

Yes you are right. distinct() in my query does not take arguments. I posted a little wrong query. My original query is to long and has multiple left and rigt joins and filters (where() and whereIn()). So, when I use my query without distinct() - it returns dublicate records. The number of rows that is returned in this case is 234. The exact same number is returned when using pagination. As I see, pagination does not work with distinct(), but work fine with groupBy(). I can't understand why.

alberthuang24 commented 6 years ago

Distinct cannot be used for multiple fields @StAngerGordun

laurencei commented 6 years ago

I'll be closing this - as it seems the issue has been answered by the comments above. Feel free to let me know if otherwise....

yopop666 commented 6 years ago

I have faced same issue having next configuration:

Pagination provide empty pages when is used with join and distinct. My query is someting like

 Package::query()
    ->select("packages.*")
    ->leftJoin("barcodes", "barcodes.scan_id", "=", "packages.scan_id");
    ->orderBy("scan_id", "desc")
    ->distinct()
    ->paginate(20, ["packages.scan_id"]);

The problem is at this line https://github.com/laravel/framework/blob/5.6/src/Illuminate/Database/Eloquent/Builder.php#L710. Instead of:

    $results = ($total = $this->toBase()->getCountForPagination())

should be:

    $results = ($total = $this->toBase()->getCountForPagination($columns))

This issue was fixed already for Database/Query/Builder https://github.com/laravel/framework/blob/5.6/src/Illuminate/Database/Query/Builder.php#L1980.

I have fixed this for my project creating Custom Eloquent Builder

class CustomEloquentBuilder extends Builder {

    /**
     * Paginate the given query.
     *
     * @param  int  $perPage
     * @param  array  $columns
     * @param  string  $pageName
     * @param  int|null  $page
     * @return \Illuminate\Contracts\Pagination\LengthAwarePaginator
     *
     * @throws \InvalidArgumentException
     */
    public function paginate($perPage = null, $columns = ['*'], $pageName = 'page', $page = null)
    {
        $page = $page ?: Paginator::resolveCurrentPage($pageName);

        $perPage = $perPage ?: $this->model->getPerPage();

        $results = ($total = $this->toBase()->getCountForPagination($columns))
            ? $this->forPage($page, $perPage)->get($columns)
            : $this->model->newCollection();

        return $this->paginator($results, $total, $perPage, $page, [
            'path' => Paginator::resolveCurrentPath(),
            'pageName' => $pageName,
        ]);
    }
}

and in my model I have added:

/**
     * Create a new Eloquent query builder for the model.
     *
     * @param QueryBuilder $query Query Builder
     *
     * @return CustomEloquentBuilder|static
     */
    public function newEloquentBuilder($query)
    {
        return new CustomEloquentBuilder($query);
    }
StAngerGordun commented 6 years ago

@yopop666, excellent! Thank you so much!

askolt commented 5 years ago

@yopop666 , very nice. Work for Laravel 5.5

MahmoudDorghamy commented 4 years ago

@yopop666 , Thank you