EllisLab/CodeIgniter — pull request #2199 by cryode (2013-01-28 10:35:56+00:00)
Title: Multiple pagination bug fixes & optimizations.

I was unable to come up with a way to fix various Pagination library issues through multiple pull requests. Since a few issues are closely related in how they interact with each other, separating them was tedious, and I ended up starting over repeatedly. I'll document everything here as best as possible for future reference.

---

One bug unmentioned so far is the inconsistent handling of the first URL -- in most scenarios, the `< First` link did not match the links for page one, or `Previous` when it pointed to page one. That is fixed in this update.

Here's a list of everything modified in the Pagination library, in roughly top to bottom order, to help follow the diff:

1. Moved the `_parse_attributes()` call into the `if` check to see if any attributes have been set. No point in calling it if none exist. *No related bug.*

2. Moved the `$this->num_links` check to the top of the method. Anything that will error out should be checked as soon as possible. *No related bug.*

3. `reuse_query_string` is now checked first, and a `$get` array is put together of any existing values. This array will be used to generate the appropriate query strings in a bit, including the query string for page numbers. *Part of fix for #2166 & inconsistent handling of query strings.*

4. `base_url` and `first_url` are then sanitized / assigned.
- If `page_query_string = TRUE` and `first_url` is empty, the first URL inherits the base URL, and reused query strings when applicable. `per_page` is then appended to the base_url only. *Part of fix for #2166 & inconsistent handling of query strings.*
- For segment-based URLs, any reused query strings are appended after the page number and any user-defined suffix. (Prefixes and suffixes are not used for the first URL. *See: #1414*)
- If the `first_url` config option is ever set by the dev, it will be used as-is; no query strings or prefix/suffix will be added.

5. `Pagination::$uri_segment` is set to `0`, and the method defaults to the number of the last URI segment from `URI::segment_array()`. *Fixes: #2124, #1909; Discussed: #2166 (which is now outdated)*

6. The `Render the Pages` part was reorganized a bit, remove some unnecessary nested `if`'s and whatnot.

7. A bunch of comment updates, for both grammar and prevent redundancy.

---

##### Related:

- CI's `$config['url_suffix']` is not used at all by the class, and I'm not sure if it should (see #1887). First, it could mean weird URLs. If you have *ci.dev/controller/method.html*, you could end up with any of these depending on how things were set up:
- ci.dev/controller/method.html/10
- ci.dev/controller/method/10.html
- ci.dev/controller/method.html/10.html

A lot of this boils down to developer preference. And since you can generate suffixed URLs easily with the pagination library, I'd consider it a non-issue for the time being. Making the class play nice with CI's suffix brings in a whole new ball game.

- Language files (see #1589) -- I'm not overly concerned with implementing this now, but I think it's a good idea. It makes i18n a little easier, and prevents needing to pass the language through all the config options upon init.

---

I did my best to throw whatever use cases I could think of at this. In some situations it did produce some funny results, but it was usually because things like `page_query_string` and `suffix` were mixed together - a *highly* unlikely scenario that anyone would want that, and similar to using CI's suffix, a pain in the ass to "fix". Testing is always appreciated -- you should be able to throw this into a 2.1.x application and have it work out of the box (at least I think so). With a lot of rearranging like this, a bug is probably inevitable. Let's squash it early, yo.

If this looks good and doesn't have any arguments, I will update the documentation and change log to accompany it.

Cheers! :beers: