Skip to content
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

Paginator component ignores limit for custom finders #6047

Closed
PhantomWatson opened this issue Mar 10, 2015 · 3 comments
Closed

Paginator component ignores limit for custom finders #6047

PhantomWatson opened this issue Mar 10, 2015 · 3 comments
Assignees
Milestone

Comments

@PhantomWatson
Copy link
Contributor

The docs say that options can be set for a custom finder (e.g. $this->paginate['finder']['tagged']['tags'] = $tags) and José Lorenzo's answer to a StackOverflow question suggests that limit can be part of these options.

However, the following code

$this->paginate['finder']['recentActivity'] = ['limit' => 7];
$recent = $this->paginate($this->ModelName);

results in a limit of 20 (the default) being used.

In PaginatorComponent::paginate(), the line

list($finder, $options) = $this->_extractFinder($options);

discards $options['finder']['recentActivity']['limit'], preventing it from being applied to the query. This is because in PaginatorComponent::_extractFinder(), the line

$options = $options + (array)current($type);

is allowing any options with a key in PaginatorComponent::_defaultConfig (page, limit, maxlimit, or whitelist) to overwrite options set inside the finder definition (e.g. $this->paginate['finder']['recentActivity']['limit']) with the value set outside of the finder definition (e.g. $this->paginate['limit']) or with the default value.

It seems like either

  • that line in _extractFinder() should be flipped around to (array)current($type) + $options so that finder options aren't overwritten,
  • or the docs should be updated to say something like "most query modifiers like fields and conditions can be included in $options, but limit will be ignored".

Since the docs don't explicitly say whether or not things like limit should be used as options for a custom pagination finder, I wasn't sure if this was intentional or if a pull request is in order. Would allowing the use of $this->paginate['finder']['finderName']['limit'] cause any unintended consequences?

I'm using CakePHP 3.0.0-RC1.

@markstory markstory added this to the 3.0.0 milestone Mar 11, 2015
@markstory
Copy link
Member

I don't think letting limit be defined in the paginate settings would be a problem. I think your proposal of flipping how the arrays are merged sounds like a good idea.

@markstory markstory self-assigned this Mar 11, 2015
PhantomWatson added a commit to PhantomWatson/cakephp that referenced this issue Mar 11, 2015
@PhantomWatson
Copy link
Contributor Author

Awesome. Here's a pull request: #6055

@markstory
Copy link
Member

Closing as there is a pull request now.

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

No branches or pull requests

2 participants