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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cdale’s picture

Posted 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.

mrfelton’s picture

Hmm.. 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().

stBorchert’s picture

Status: Active » Needs review
FileSize
19.41 KB

Here's a patch that removes the limit attribute from theme_pager and all of its occurences.

Dries’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me, but I'd like to have one other person confirm this to make sure I'm not mistaken.

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs work

Sounds reasonable, but we also need to change the signature in drupal_common_theme().

merlinofchaos’s picture

Status: Needs work » Reviewed & tested by the community

I 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.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Thanks, Earl! Marking back to needs work for Damien's comment.

merlinofchaos’s picture

Status: Needs work » Reviewed & tested by the community

One 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.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

This is still needs work; for #5 and now #8.

merlinofchaos’s picture

Whoops, I crossposted not once but TWICE. Sorry!

stBorchert’s picture

Status: Needs work » Needs review
FileSize
21.09 KB

According to Damien's note I've re-rolled the patch and updated the signature in drupal_common_theme().

Dries’s picture

Status: Needs review » Needs work

Looks great, and tests succeed. I've committed this to CVS HEAD. Please update the upgrade documentation in the handbook, and mark this 'fixed'. Thanks!

webchick’s picture

Issue tags: +Needs documentation

Tagging 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.

jhodgdon’s picture

Regarding 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?

jhodgdon’s picture

Title: Remove $limt from theme_pager* » Remove $limit from theme_pager*
joelwx’s picture

Status: Needs work » Needs review
Issue tags: -Needs documentation +tcdocsprint09

Documented 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.

Status: Needs review » Needs work

The last submitted patch failed testing.

jhodgdon’s picture

Status: Needs work » Fixed

The 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.)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Gábor Hojtsy’s picture

Uhm, 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.

NancyDru’s picture

Status: Closed (fixed) » Active

The docs (see #14) have not been updated.

jhodgdon’s picture

Just 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.

jhodgdon’s picture

Recommend that we don't update the rest of the documentation until #514914: Re-add removed feature of $limit "passing" for themers is resolved.

jhodgdon’s picture

Status: Active » Fixed

Doc has apparently now been updated, see http://drupal.org/node/514914#comment-1913654

Status: Fixed » Closed (fixed)
Issue tags: -tcdocsprint09

Automatically closed -- issue fixed for 2 weeks with no activity.