pager_query() vs. theme('pager') argument handling
i was testing a bugfix to the project module (removing tablesort_pager() from the call to theme('pager')) and noticed that the $limit and $element args for theme('pager') vs. the ones to pager_query() are duplicated. in IRC, steven (unconed) pointed me to: http://drupal.org/node/5371#comment-89191 if theme('pager') is smart enough to remember the query (which is why y'all got rid of tablesort_pager() in the first place), why does theme('pager') need to take these other args, either, since they're also passed to pager_query()? we definitely want these to match, or else the prev/next and numbered links generated by theme('pager') won't match the data being displayed. i know this isn't 6.7 critical, but i'm trying to understand why these args to theme('pager') are there at all. seems like a recipe for confusing bugs when the calls to pager_query() and theme('pager') disagree on these args... any other insights on this? thanks, -derek (dww)
Derek Wright wrote:
i was testing a bugfix to the project module (removing tablesort_pager() from the call to theme('pager')) and noticed that the $limit and $element args for theme('pager') vs. the ones to pager_query() are duplicated. in IRC, steven (unconed) pointed me to:
http://drupal.org/node/5371#comment-89191
if theme('pager') is smart enough to remember the query (which is why y'all got rid of tablesort_pager() in the first place), why does theme('pager') need to take these other args, either, since they're also passed to pager_query()? we definitely want these to match, or else the prev/next and numbered links generated by theme('pager') won't match the data being displayed. 'all these other' args?
There are 4 args. 1) Tags. This is something that should probably be handled more by theming, but still. Has nothing to do with the query. Not passed to pager_query(). 2) Limit. Yes, I grant that pager_query() should probably (and possibly does) remember this number. 3) Element ID. THis is critical. You can't not have this one. Passed to pager_query() but since you can have more than one pager per page, you have to have this to identify which pager you're using. 4) parameters. Part of the links; not important to pager_query(). So yea, you could eliminate argument 2 there. Perhaps that's worthwhile in 4.8. I'm not so sure.
On Fri, 21 Apr 2006 15:21:51 -0700 Earl Miles wrote:
2) Limit. Yes, I grant that pager_query() should probably (and possibly does) remember this number.
doesn't look like it does.
3) Element ID. THis is critical. You can't not have this one. Passed to pager_query() but since you can have more than one pager per page, you have to have this to identify which pager you're using.
yeah, sorry, i didn't look closely at this. i assumed it was something like which page we were on. my bad.
4) parameters. Part of the links; not important to pager_query().
righto.
So yea, you could eliminate argument 2 there. Perhaps that's worthwhile in 4.8. I'm not so sure.
ok, fair enough. i think it'd be worth doing, since it seems that the calls to pager_query and theme('pager') are often quite separate in the code, and so if someone wants to change one they could easily forget to change the other. encouraging the use of constants for this would help, but it seems better to just have pager_query() remember it, if it's already remembering stuff for the pager links themselves... i'll submit an issue about this once 4.8 development opens up. thanks, -derek (dww) p.s. i wish there was a good way to submit 4.8 issues already -- submitting as version: cvs, status: postponed seems lame.
participants (2)
-
Derek Wright -
Earl Miles