New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

3.5.* - Controller::paginate() doesn't pass all options correctly #11330

Closed
sdustinh opened this Issue Oct 16, 2017 · 15 comments

Comments

Projects
None yet
5 participants
@sdustinh
Contributor

sdustinh commented Oct 16, 2017

This is a (multiple allowed):

  • bug
  • enhancement
  • feature-discussion (RFC)
sdustinh@sdustinh:~/github/cakephp/app$ php --version
PHP 7.0.22-0ubuntu0.16.04.1 (cli) ( NTS )
Copyright (c) 1997-2017 The PHP Group
Zend Engine v3.0.0, Copyright (c) 1998-2017 Zend Technologies
    with Zend OPcache v7.0.22-0ubuntu0.16.04.1, Copyright (c) 1999-2017, by Zend Technologies
    with Xdebug v2.4.0, Copyright (c) 2002-2016, by Derick Rethans
sdustinh@sdustinh:~/github/cakephp/app$ mysql --version
mysql  Ver 14.14 Distrib 5.7.19, for Linux (x86_64) using  EditLine wrapper
sdustinh@sdustinh:~/github/cakephp/app$ composer show cakephp/*
cakephp/bake                1.4.2  Bake plugin for CakePHP 3
cakephp/cakephp             3.5.2  The CakePHP framework
cakephp/cakephp-codesniffer 3.0.1  CakePHP CodeSniffer Standards
cakephp/chronos             1.1.2  A simple API extension for DateTime.
cakephp/debug_kit           3.11.1 CakePHP Debug Kit
cakephp/elastic-search      0.3.5  An Elastic Search datasource and data mapper for CakePHP 3.0
cakephp/migrations          1.7.1  Database Migration plugin for CakePHP 3.0 based on Phinx
cakephp/plugin-installer    1.0.0  A composer installer for CakePHP 3.0+ plugins.

What you did

Created a controller with default pagination options in the $paginate property and used $this->paginate($query).

What happened

When calling $this->paginate($query) the pagination options were only partially being used in the Cake\Datasource\Paginator object.

What you expected to happen

Here is a copy/paste of my controller almost exactly as it is:

/**
 * Products Controller
 *
 * @property \Facilities\Model\Table\FacilitiesTable $Facilities
 * @property \Products\Model\Table\ProductsTable $Products
 * @property \Products\Model\Table\ProductTypeGroupsTable $ProductTypeGroups
 * @property \Products\Model\Table\ProductTypesTable $ProductTypes
 * @property \Products\Model\Table\ProductManufacturersTable $ProductManufacturers
 */
class ProductsController extends AppController
{
    /**
     * Default Model Class
     *
     * @var string
     */
    public $modelClass = 'Products.Products';
    /**
     * Default Pagination Options
     *
     * @var array
     */
    public $paginate = [
        'Products' => [
            'contain' => [
                'ProductTypes',
                'ProductManufacturers',
                'Assets.AssetLocations.Facilities',
            ],
            'limit' => 50,
            'maxLimit' => 100,
            'sortWhitelist' => [
                'Facilities.code',
                'Products.id',
                'ProductCapacities.bytes',
                'ProductManufacturers.name',
                'ProductTypes.name',
                'total_assets',
            ],
        ],
    ];
    /**
     * Product Availability Report
     *
     * This report will show the total number of assets by product. You can apply
     * filters to it for more specific numbers.
     *
     * @return void
     */
    public function availability()
    {
        $getFilter = function ($filter) {
            $query = $this->request->getQueryParams();

            if (isset($query[$filter]) === false) {
                return null;
            }

            return array_filter((array)$query[$filter]) ?: null;
        };

        $products = $this->Products->find();
        $products
            ->innerJoinWith('Assets', function ($query) {
                return $query->find('tracked');
            })
            ->innerJoinWith('Assets.AssetStatuses', function ($query) use ($getFilter) {
                $assetStatuses = $getFilter('assetStatus');

                if ($assetStatuses !== null) {
                    $query->where(['AssetStatuses.id IN' => (array)$assetStatuses]);
                }

                return $query;
            })
            ->innerJoinWith('Assets.AssetLocations.Facilities', function ($query) use ($getFilter) {
                $facilities = $getFilter('facility');

                if ($facilities !== null) {
                    $query->where([
                        'OR' => [
                            ['Facilities.id IN' => (array)$facilities],
                            ['Facilities.code IN' => (array)$facilities],
                        ],
                    ]);
                }

                return $query;
            })
            ->select([
                'facility_code' => 'Facilities.code',
                'total_assets' => $products->func()->count('Assets.id'),
            ])
            ->contain([
                'ProductCapacities',
                'ProductManufacturers' => function ($query) use ($getFilter) {
                    $manufacturers = $getFilter('manufacturer');

                    if ($manufacturers !== null) {
                        $query->where(['ProductManufacturers.id IN' => (array)$manufacturers]);
                    }

                    return $query;
                },
                'ProductTypes' => function ($query) use ($getFilter) {
                    $productTypes = $getFilter('productType');

                    if ($productTypes !== null) {
                        $query->where(['ProductTypes.id IN' => (array)$productTypes]);
                    }

                    return $query;
                },
                'ProductTypes.ProductTypeGroups' => function ($query) use ($getFilter) {
                    $productTypeGroups = $getFilter('productTypeGroup');

                    if ($productTypeGroups !== null) {
                        $query->where(['ProductTypeGroups.id IN' => (array)$productTypeGroups]);
                    }

                    return $query;
                },
            ])
            ->group(['Products.id', 'ProductTypes.id', 'Facilities.id'])
            ->enableAutoFields(true);
        $this->set('products', $this->Paginator->paginate($products, $this->paginate));

        // Grab filtering data
        $assetStatuses = $this->AssetStatuses->find('list')->order(['AssetStatuses.name' => 'ASC']);
        $facilities = $this->Facilities->find('list', ['valueField' => 'codename']);
        $manufacturers = $this->ProductManufacturers->find('list')->order(['ProductManufacturers.name' => 'ASC']);
        $productTypeGroups = $this->ProductTypeGroups->find()
            ->contain(['ProductCategories'])
            ->find('list', ['groupField' => 'category.name'])
            ->order([
                'ProductCategories.name' => 'ASC',
                'ProductTypeGroups.name' => 'ASC',
            ]);
        $productTypes = $this->ProductTypes->find()
            ->contain(['ProductTypeGroups'])
            ->find('list', ['groupField' => 'group.name'])
            ->order([
                'ProductTypeGroups.name' => 'ASC',
                'ProductTypes.name' => 'ASC',
            ]);
        $this->set(compact(
            'assetStatuses',
            'facilities',
            'manufacturers',
            'productTypeGroups',
            'productTypes'
        ));
    }
    /**
     * Initialize
     *
     * @return void
     */
    public function initialize()
    {
        parent::initialize();

        $this->loadComponent('Paginator');

        if ($this->request->getParam('action') === 'availability') {
            $this->loadModel('Assets.AssetStatuses');
            $this->loadModel('Facilities.Facilities');
            $this->loadModel('Products.ProductManufacturers');
            $this->loadModel('Products.ProductTypeGroups');
            $this->loadModel('Products.ProductTypes');
            $this->loadModel('Products.Products');
        }
    }
}

This has been a constant problem I've had when using pagination in the framework but I was finally able to make it work so I wanted to open a bug issue with you guys to confirm it's a framework problem instead of something I've messed up in my application code.

As you can see in the code above, I make a call to $this->Paginator->paginate($products, $this->paginate). This was the only way I could get it to pagination correctly. When I didn't call the component directly (e.g. $this->paginate($products)) and dumped the paginator object I noticed that the limit field was set correctly but none of the other properties were even being set. I was able to confirm this problem in the view with $this->Paginator->sort() not generating the correct direction.

I did a step by step on Cake\Datasource\Paginator::paginate() and the first spot that failed was the actual call to validateSort(). It was returning an empty array every time. Naturally, this led me to check out Cake\Controller\Controller::paginate() as the problem since it was the only place I was able to reproduce the issue. Looking over how that method works, the only thing I could see that might cause a problem would be when the default options are merged with the incoming here. I don't know why that would be a problem, but the fact that limit made its way to the paginator object and sortWhitelist did not would make me think that a array_merge() call would be the fix.

Anyway, if you guys can't reproduce it and it works for you then just close this issue. For my application it seems I'll just have to use the component directly because I can't find another way around it. If you need me to do any more confirmation tests I'd be happy to do it.

Thanks for your hard work!

@VarCI-bot VarCI-bot added the Defect label Oct 16, 2017

@VarCI-bot

This comment has been minimized.

Show comment
Hide comment
@VarCI-bot

VarCI-bot Oct 16, 2017

Collaborator

sdustinh, please include the CakePHP version number you are using in your description. It helps us debug your issue.

--
Automated response by Var.CI 🤖

Collaborator

VarCI-bot commented Oct 16, 2017

sdustinh, please include the CakePHP version number you are using in your description. It helps us debug your issue.

--
Automated response by Var.CI 🤖

@markstory markstory added this to the 3.5.5 milestone Oct 16, 2017

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Oct 16, 2017

Member

Does a similar issue happen if you pass a Table instance or model name to Controller::paginate()?

Member

markstory commented Oct 16, 2017

Does a similar issue happen if you pass a Table instance or model name to Controller::paginate()?

@sdustinh

This comment has been minimized.

Show comment
Hide comment
@sdustinh

sdustinh Oct 24, 2017

Contributor

Sorry for the delay. It doesn't seem to matter if I pass a string, the instance, or the query. The only thing that consistently gets set is limit, contain, or fields. When I dump the paginator object I don't see the settings in it unless I call the component directly. Then it works every time. I'm really at a loss for what could be causing it. The only thing I could see as a potential would be that $settings += $this->paginate; maybe reading from the initial state?

Contributor

sdustinh commented Oct 24, 2017

Sorry for the delay. It doesn't seem to matter if I pass a string, the instance, or the query. The only thing that consistently gets set is limit, contain, or fields. When I dump the paginator object I don't see the settings in it unless I call the component directly. Then it works every time. I'm really at a loss for what could be causing it. The only thing I could see as a potential would be that $settings += $this->paginate; maybe reading from the initial state?

@markstory markstory self-assigned this Oct 24, 2017

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Oct 24, 2017

Member

I'll see if I can reproduce this problem.

Member

markstory commented Oct 24, 2017

I'll see if I can reproduce this problem.

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Oct 25, 2017

Member

I wasn't able to reproduce this issue in a small app. My controller's paginate property and index method looked like:

    public $paginate = [
        'Bookmarks' => [
            'limit' => 3,
            'maxLimit' => 10,
            'sortWhitelist' => ['title', 'Bookmarks.title']
        ]
    ];

    /**
     * Index method
     *
     * @return void
     */
    public function index()
    {
        $this->set('bookmarks', $this->paginate($this->Bookmarks));
        $this->set('_serialize', ['bookmarks']);
    }

The maxLimit and sortWhitelist were respected and I was not able to set a limit above 10, nor sort on a non-title field. Have I missed something?

Member

markstory commented Oct 25, 2017

I wasn't able to reproduce this issue in a small app. My controller's paginate property and index method looked like:

    public $paginate = [
        'Bookmarks' => [
            'limit' => 3,
            'maxLimit' => 10,
            'sortWhitelist' => ['title', 'Bookmarks.title']
        ]
    ];

    /**
     * Index method
     *
     * @return void
     */
    public function index()
    {
        $this->set('bookmarks', $this->paginate($this->Bookmarks));
        $this->set('_serialize', ['bookmarks']);
    }

The maxLimit and sortWhitelist were respected and I was not able to set a limit above 10, nor sort on a non-title field. Have I missed something?

@markstory markstory removed their assignment Oct 31, 2017

@markstory markstory modified the milestones: 3.5.5, 3.5.6 Nov 2, 2017

@markstory markstory modified the milestones: 3.5.6, 3.5.7 Nov 18, 2017

@markstory markstory modified the milestones: 3.5.7, 3.5.8, 3.5.9 Dec 6, 2017

@markstory markstory modified the milestones: 3.5.9, 3.5.10 Dec 28, 2017

@markstory markstory closed this Dec 28, 2017

@ideagroup-dev

This comment has been minimized.

Show comment
Hide comment
@ideagroup-dev

ideagroup-dev Jan 10, 2018

considering the following instruction :
$this->set('bookmarks', $this->paginate($this->Bookmarks->find()->limit(2)));

the limit set in the pagintation options array overrrides the limit set within the query builder
in the case one wants to limit the results set to xx records
instead of limiting the number of results per page

ideagroup-dev commented Jan 10, 2018

considering the following instruction :
$this->set('bookmarks', $this->paginate($this->Bookmarks->find()->limit(2)));

the limit set in the pagintation options array overrrides the limit set within the query builder
in the case one wants to limit the results set to xx records
instead of limiting the number of results per page

@ADmad

This comment has been minimized.

Show comment
Hide comment
@ADmad

ADmad Jan 10, 2018

Member

the limit set in the pagintation option overrrides the limit set in the query builder
in the case one wants to limit the results set to xx records
instead of limiting the number of results per page

I don't see a problem with that. The whole point of paginator is to set limit and offset for the query.

Member

ADmad commented Jan 10, 2018

the limit set in the pagintation option overrrides the limit set in the query builder
in the case one wants to limit the results set to xx records
instead of limiting the number of results per page

I don't see a problem with that. The whole point of paginator is to set limit and offset for the query.

@ideagroup-dev

This comment has been minimized.

Show comment
Hide comment
@ideagroup-dev

ideagroup-dev Jan 10, 2018

What if one wants to paginate 10 rows per page but no more than 1000 records paginated that way?
how would one set the 1000 records limit?

ideagroup-dev commented Jan 10, 2018

What if one wants to paginate 10 rows per page but no more than 1000 records paginated that way?
how would one set the 1000 records limit?

@ADmad

This comment has been minimized.

Show comment
Hide comment
@ADmad

ADmad Jan 10, 2018

Member

You will have to implement your own paginator for that :)

Member

ADmad commented Jan 10, 2018

You will have to implement your own paginator for that :)

@ideagroup-dev

This comment has been minimized.

Show comment
Hide comment
@ideagroup-dev

ideagroup-dev Jan 10, 2018

in fact i just need a maxPage property that limits the number of pages (companion of maxLimit) !!
modify the cake paginator component ?

ideagroup-dev commented Jan 10, 2018

in fact i just need a maxPage property that limits the number of pages (companion of maxLimit) !!
modify the cake paginator component ?

@ADmad

This comment has been minimized.

Show comment
Hide comment
@ADmad

ADmad Jan 10, 2018

Member

You could easily restriction the max page number in your app itself. The paginator would still be returning all counts based on total records/pages so a "maxPage" wouldn't make sense in PaginatorComponent but could can extend the core component and do so in your custom one.

Member

ADmad commented Jan 10, 2018

You could easily restriction the max page number in your app itself. The paginator would still be returning all counts based on total records/pages so a "maxPage" wouldn't make sense in PaginatorComponent but could can extend the core component and do so in your custom one.

@ideagroup-dev

This comment has been minimized.

Show comment
Hide comment
@ideagroup-dev

ideagroup-dev Jan 10, 2018

as far as I understand
in the core paginator component, limit is intended for limiting the number of rows per page
on the other hand in the query builder limit sets the mysql LIMIT value whenever the app is connecting a mysql database
You suggest that I should create a controller component that extends the core paginator component
Appreciate the advise
will certainly give it a try unless a fix is milestoned for next release ..

ideagroup-dev commented Jan 10, 2018

as far as I understand
in the core paginator component, limit is intended for limiting the number of rows per page
on the other hand in the query builder limit sets the mysql LIMIT value whenever the app is connecting a mysql database
You suggest that I should create a controller component that extends the core paginator component
Appreciate the advise
will certainly give it a try unless a fix is milestoned for next release ..

@ADmad

This comment has been minimized.

Show comment
Hide comment
@ADmad

ADmad Jan 10, 2018

Member

The feature you want is unlikely to land in the core anytime soon. You could implement it though and submit a patch for it later on :)

Member

ADmad commented Jan 10, 2018

The feature you want is unlikely to land in the core anytime soon. You could implement it though and submit a patch for it later on :)

@ideagroup-dev

This comment has been minimized.

Show comment
Hide comment
@ideagroup-dev

ideagroup-dev Jan 10, 2018

I definitly need to figure out something
althought I don't really have any experience on how to contribute or submit any kind of code to the cake community
if there was any guideline available i will defintly check it out...

ideagroup-dev commented Jan 10, 2018

I definitly need to figure out something
althought I don't really have any experience on how to contribute or submit any kind of code to the cake community
if there was any guideline available i will defintly check it out...

@ideagroup-dev

This comment has been minimized.

Show comment
Hide comment
@ideagroup-dev

ideagroup-dev Jan 10, 2018

considering the following instruction :
$this->set('bookmarks', $this->paginate($this->Bookmarks->find()->limit(2)));

considering the offset value

$this->set('bookmarks', $this->paginate($this->Bookmarks->find()->limit(0)->page(1000)));
should retrieve the 1000 first rows

when debuging the query object the offset value within the generated sql string is set to zero
"LIMIT 0 OFFSET 0"
while the pagination occurs in the rendered view

ideagroup-dev commented Jan 10, 2018

considering the following instruction :
$this->set('bookmarks', $this->paginate($this->Bookmarks->find()->limit(2)));

considering the offset value

$this->set('bookmarks', $this->paginate($this->Bookmarks->find()->limit(0)->page(1000)));
should retrieve the 1000 first rows

when debuging the query object the offset value within the generated sql string is set to zero
"LIMIT 0 OFFSET 0"
while the pagination occurs in the rendered view

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment