Respect $config['cur_page'] variable for pagination #4384
Conversation
| @@ -497,7 +497,7 @@ public function create_links() | |||
| { | |||
| $this->cur_page = $this->CI->input->get($this->query_string_segment); | |||
| } | |||
| else | |||
| else if (! ctype_digit($this->cur_page)) | |||
There was a problem hiding this comment.
This will fail if the cur_page value is an integer.
It also doesn't fit with our styleguide.
There was a problem hiding this comment.
Maybe is_numeric would be more appropriate here. I took this check from the same source file, so then I suppose I'll have to change https://github.com/bcit-ci/CodeIgniter/blob/develop/system/libraries/Pagination.php#L518 to validate integers as well. Otherwise pagination would fallback to the default page again.
There was a problem hiding this comment.
Regarding style guide, I copied the statement from the aformentioned code line, just to be certain that the change would be compliant. Regarding the styleguide I suppose there should be one space before and after the negation sign.
Will send a corrected version later today.
There was a problem hiding this comment.
is_numeric() won't be sufficient either, because it accepts floats (and IIRC some other weird stuff). The problem is that you're trying to satisfy multiple conditions by adding just one line of code ... it can't work that way. I'd do this:
elseif (empty($this->cur_page))
{
// the current auto-detection from uri
}
else
{
$this->cur_page = (string) $this->cur_page;
}
The string cast is necessary to keep the following checks working.
In addition to spaces around operators, we also write elseif no space.
jekkos
force-pushed
the
pagination-fix
branch
3 times, most recently
from
6ab80b8
to
626d1d7
Compare
|
Hope the patch is compliant with the styleguide now. |
|
Sadly not, we put braces on separate lines ... |
After upgrading to CI3 I noticed that developers are able to determine the current page counter for pagination based on
* Explicit query string parameters
* URI segment configuration
In earlier versions a developer could still set the current page counter in the pagination lib directly which is useful if you want to use pagination with HTTP POST instead of GET. This could be done by passing
$config['cur_page'] = '10';
to the pagination function for link generation. Currently this code has changed and will always try to check whether the uri segment is a valid number or not, even if the cur_page variable was passed in the associative array, and fallback to zero if it fails to validate that result.
This can be easily resolved by checking whether the counter was already set with a valid number from the $config array before trying to resolve it from the uri segment.
This fix give a developer more flexibility and stop CI from overwriting the externally set value with an incorrect one.
Signed-off-by: jekkos <jeroen.peelaerts@gmail.com>
jekkos
force-pushed
the
pagination-fix
branch
from
626d1d7
to
356ad58
Compare
|
Ok should be good now. |
Respect $config['cur_page'] variable for pagination
|
For future reference - please don't put that whole description in the commit message. I just now noticed that you did this. Stating just that cur_page was erroneously ignored would be fine, but personal thoughts should be avoided as they can be misleading and/or not actually in line with why the patch was merged. For example, utilizing this feature for POST requests is something that I'd rather discourage, but a user reading your commit message may take it as a "cool" idea instead. I do like that you've overwritten previous commits and it's just one that appears in the log - I wish more people did that. :) |
|
Thanks for the merge, using POST surely has some disadvantages. In this comment it was only used to clarify the need for more flexibility. I also like the fact that commit history shows only what is relevant, thats why I rewrote the first commits afterwards :) |
After upgrading to CI3 I noticed that developers are able to determine the current page counter for pagination based on
In earlier versions a developer could still set the current page counter in the pagination lib directly which is useful if you want to use pagination with HTTP POST instead of GET. This could be done by passing
to the pagination function for link generation. Currently this code has changed and will always try to check whether the uri segment is a valid number or not, even if the cur_page variable was passed in the associative array, and fallback to zero if it fails to validate that result.
This can be easily resolved by checking whether the counter was already set with a valid number from the $config array before trying to resolve it from the uri segment.
This fix give a developer more flexibility and stop CI from overwriting the externally set value with an incorrect one.
Signed-off-by: jekkos jeroen.peelaerts@gmail.com