I searched for an issue about this, but came up trumps..
Is there a reason that $limit is needed in theme_pager? As far as I can see, it is not used by theme_pager or any of the theme_pager_* functions.
It seems to give a false sense of what is happening, as if you were to set limit to 10, when the pager_query was given a limit of 20, then you would expect that only 10 results are themed. This is not the case.
I propose the removal of $limit from these functions, or at the very least a change in the docs as having $limit The number of query results to display per page.
in the docs for theme_pager is misleading.
Patch attached for removal of $limit from pager theme functions.
Comment | File | Size | Author |
---|---|---|---|
#11 | 330748_remove_limit_from_pager_9.patch | 21.09 KB | stBorchert |
#3 | 330748_remove_limit_from_pager_3.patch | 19.41 KB | stBorchert |
theme-pager-remove-limit.patch | 5.71 KB | cdale | |
Comments
Comment #1
cdale CreditAttribution: cdale commentedPosted without thinking. I just realized that this patch will need to be slightly larger before it will work. :)
At the very least, all theme_pager calls in core will need to be checked.
I'll leave this as active and see what the general consensus is before going ahead with the changes required.
Comment #2
mrfelton CreditAttribution: mrfelton commentedHmm.. I'd like to see this fixed. It's very misleading. For example, I'm trying to theme the Search results page, and I want to show more than 10 results per page. One would think that I could alter the parameters to theme_pager, since it includes a $limit parameter. But this has no effect, and it seems like I actually have to completely replace do_search with my own version as the search limit is hard coded into the call to pager_query().
Comment #3
stBorchertHere's a patch that removes the limit attribute from
theme_pager
and all of its occurences.Comment #4
Dries CreditAttribution: Dries commentedThis looks good to me, but I'd like to have one other person confirm this to make sure I'm not mistaken.
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedSounds reasonable, but we also need to change the signature in drupal_common_theme().
Comment #6
merlinofchaos CreditAttribution: merlinofchaos commentedI would like to see a complete rewrite of the internals of the pager, which are not at all friendly to using outside of pager_query() and there's definitely uses for it. There are also some options that more advanced paging needs that this system doesn't support and a more flexible API could add more options without creating any new problems. And probably make the code easier to read, to boot.
That said, this patch is 100% correct in my review, and modernizing the pager system should not hold something like this up. $limit is of no value on theme('pager') and even if it *were* used, which it's not, it would be wrong because the proper limit was set in pager_query().
I agree, this is RTBC.
Comment #7
webchickThanks, Earl! Marking back to needs work for Damien's comment.
Comment #8
merlinofchaos CreditAttribution: merlinofchaos commentedOne note: The $limit is not actually stored in pager_query(). It should be stored (maybe global $pager_limit) in case it needs to be referenced by a theme function. For example, some theme function that wants to put "displaying 10 items per page..." can't do it without that.
Comment #9
webchickThis is still needs work; for #5 and now #8.
Comment #10
merlinofchaos CreditAttribution: merlinofchaos commentedWhoops, I crossposted not once but TWICE. Sorry!
Comment #11
stBorchertAccording to Damien's note I've re-rolled the patch and updated the signature in drupal_common_theme().
Comment #12
Dries CreditAttribution: Dries commentedLooks great, and tests succeed. I've committed this to CVS HEAD. Please update the upgrade documentation in the handbook, and mark this 'fixed'. Thanks!
Comment #13
webchickTagging with "Needs Documentation."
Incidentally, if anyone in this issue has interest in further improving the pager system, Earl wrote up a great document at http://www.angrydonuts.com/modernizing-the-drupal-pager-system on some concrete ways to do so. :)
Also, #33809: Make pagers not suck looks like a good issue to help out with.
Comment #14
jhodgdonRegarding this needing documentation, see #454798: Theme_pager changes should be mentioned in update guide - separate doc issue to add it to the Upgrading page(s). What other doc change(s) does it need?
Comment #15
jhodgdonComment #16
joelwx CreditAttribution: joelwx commentedDocumented removal of $limit parameter to the 5 pager functions on the "Converting 6.x to 7.x themes" page (http://drupal.org/node/254940).
Work done by zzolo, joelwx, bryan kennedy at Twin Cities documentation sprint.
Comment #18
jhodgdonThe upgrade doc section looks good to me. (Auto-tester bot tried to test a patch above; this was not a patch that needed testing though, but a Handbook section update that needed a review.)
Comment #20
Gábor HojtsyUhm, Earl pointed out in #8 that this patch actually removed a feature for themes. Themes could do "displaying 10 of 2355" before this patch, but not after. This was dismissed not only in the patch reviews, but also in the docs. I mean the docs should have identified what should themers do if they used the $limit param before. Well, they cannot do anything. You cannot reverse engineer the number of items on a page from the number of total items and the total number of pages, because you don't know how many are displayed on the last page. When you have 10 items on 2 pages for example, these can be either 9, 8, 7, 6 or 5 items displayed per page.
Opened #514914: Re-add removed feature of $limit "passing" for themers as follow-up to add this feature back. Via globals as Earl wrote.
Comment #21
NancyDruThe docs (see #14) have not been updated.
Comment #22
jhodgdonJust to clarify, someone did update http://drupal.org/node/254940 (6.x to 7.x theme update guide) with this information.
Probably it needs to be also added to the module update guilde http://drupal.org/update/modules/6/7 and http://drupal.org/node/394070 ... It doesn't appear to be there.
Comment #23
jhodgdonRecommend that we don't update the rest of the documentation until #514914: Re-add removed feature of $limit "passing" for themers is resolved.
Comment #24
jhodgdonDoc has apparently now been updated, see http://drupal.org/node/514914#comment-1913654