[drupal-devel] [bug] Clean up pager code

[Dries]

Issue status update for http://drupal.org/node/23495

 Project:      Drupal
 Version:      cvs
 Component:    base system
 Category:     bug reports
 Priority:     normal
 Assigned to:  Steven
 Reported by:  Steven
 Updated by:   Dries
 Status:       patch

IMO, we don't need a legacy handler for this.  You can't install one in
the legacy.module because the  from-parameter is not part of
$_GET['q'].  Because most (if not all) pages with pagers show content
in reverse chronological order, it is very unlikely that they are used
as permanent links.


Additional comments:



The code comments in that file start with capital letters and end with
a point.  Some of your comments appear to be inconsistent.  Though, it
looks like some of these are existing comments that have been moved
around.
If from means 'page number', why don't we rename it to page?  It makes
for a better URL scheme and improves readability of the code. 
Something to consider.

UnConeD: I'm not committing this patch because you might want to
respond, and revise it once more based on people's comments.  Either
way, I'm OK with the patch as is so feel free to commit once you think
it is ready.




Dries



Previous comments:
------------------------------------------------------------------------

May 24, 2005 - 02:54 : Steven

Attachment: http://drupal.org/files/issues/pager_1.patch (20.22 KB)

FactoryJoe recently asked me about theming the pager. Looking at the
code, I decided it was way too much uncommented voodoo. So here's the
cleanup patch.


The biggest change is that before this patch, the pager would count
items. When there are 5 items per page, the urls would go from=0,
from=5, from=10, etc. The code also handled in between values, e.g.
from=2 or from=13. IMO this 'in between' feature is useless (you can
only use it by typing in the url yourself, and once you do the pager
remembers the in-page offset) and it complicates the code.


Instead of this, I made the pager count page numbers. 'from' is simply
an integer per pager: 0, 1, 2, 3... This makes the code simpler. I also
commented it some more, and cleaned up some inconsistencies in the code.
Finally, the code had a lot of (int) casts in it. Most of these were
unnecessary as the variable was either the result of a calculation, or
used in a calculation. I got rid of those that had no effect.


Functionally, nothing changes except the meaning of "from" in the pager
URLs. I hardly think anyone would link to a specific pager page.




------------------------------------------------------------------------

May 24, 2005 - 03:19 : kbahey

"I hardly think anyone would link to a specific pager page.

"
Not humans perhaps, but search engines would for sure.


Now this will cause 404.


This does not mean I am against the change. It makes sense. Just
pointing out a potential consequence.




------------------------------------------------------------------------

May 24, 2005 - 05:05 : Steven

I'm not sure what you mean... a search engine can simply spider out the
new indices like any other page change.


In fact, now the from numbers are sequential, a really smart search
engine might take advantage of that ;).




------------------------------------------------------------------------

May 24, 2005 - 05:15 : kbahey

What I mean is that sites will already have from=25, from=50, ...etc.
indexed in Google and other search engines. So these pages are
bookmarked in a way, though not by humans.


These will now give a 404 (at least some of them) if someone clicks on
the old links.




------------------------------------------------------------------------

May 24, 2005 - 06:44 : Eric Scouten

That's probably not a major concern. Such search links are pretty
fragile anyway, since new items will invariably get posted and push
older items down the list and onto the next ?from= batch.


I wonder if there's a way to discourage Google from scanning the search
pages, but still following the links on those pages.




------------------------------------------------------------------------

May 24, 2005 - 07:21 : Steven

Actually, old links will continue to work partially. Part of my cleanup
patch was ensuring you cannot go beyond the last or before the first
page due to such bad links. If you use a value of from= that is too
large, you end up on the last page.




------------------------------------------------------------------------

May 24, 2005 - 14:31 : Bèr Kessels

can we not add a few lines to legacy.module?


I suggtest something like: if the from value is larger then the amount
of pages, then translate it yo a new version.


in psuedocode:

<?php
if ($currentpage > $lastpage) {
 $from = round($currentpage / amount_per_page);
}
?>






------------------------------------------------------------------------

May 24, 2005 - 17:28 : moshe weitzman

Steven - I have noticed that the pager will issue a query if if its
COUNT query found 0 rows. Seems like we could optimize out the 2nd
query, no? This is a long standing issue, unrelated to your patch. But
as long as you are there ...