[drupal-devel] [bug] Clean up pager code
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: Steven Status: patch 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. Steven
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: kbahey Status: patch "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. kbahey Previous comments: ------------------------------------------------------------------------ May 23, 2005 - 19: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.
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: Steven Status: patch 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 ;). Steven 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.
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: kbahey Status: patch 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. kbahey Previous comments: ------------------------------------------------------------------------ May 23, 2005 - 19: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 23, 2005 - 20: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 23, 2005 - 22: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 ;).
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: Eric Scouten Status: patch 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. Eric Scouten Previous comments: ------------------------------------------------------------------------ May 23, 2005 - 17: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 23, 2005 - 18: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 23, 2005 - 20: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 23, 2005 - 20: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.
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: Steven Status: patch 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. Steven 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.
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: Bèr Kessels Status: patch 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); } ?> Bèr Kessels Previous comments: ------------------------------------------------------------------------ May 24, 2005 - 01: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 - 02: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 - 04: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 - 04: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 - 05: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 - 06: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.
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: moshe weitzman Status: patch 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 ... moshe weitzman Previous comments: ------------------------------------------------------------------------ May 23, 2005 - 19: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 23, 2005 - 20: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 23, 2005 - 22: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 23, 2005 - 22: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 23, 2005 - 23: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 - 00: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 - 07: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); } ?>
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 ...
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: Junyor Status: patch Couldn't 'from' be from the first post, rather than the latest? I thought that was one of the problems with the existing pager. And it would once and for all solve problems with search engine results. Junyor Previous comments: ------------------------------------------------------------------------ May 24, 2005 - 01: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 - 02: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 - 04: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 - 04: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 - 05: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 - 06: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 - 13: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 - 16: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 ... ------------------------------------------------------------------------ May 24, 2005 - 20:58 : Dries 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.
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: Steven Status: patch Junyor: adding such logic to the pager would complicate the code a lot. The goal of this patch was to simplify the code. As has been pointed out, there are no real problems with renumbering the pager. Search engines will spider them out, manual links can be corrected (but they will continue to point to the last page if they go beyond the end). +1 on this patch going in as is. Steven 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 ... ------------------------------------------------------------------------ May 24, 2005 - 21:58 : Dries 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. ------------------------------------------------------------------------ May 25, 2005 - 00:56 : Junyor Couldn't 'from' be from the first post, rather than the latest? I thought that was one of the problems with the existing pager. And it would once and for all solve problems with search engine results.
participants (7)
-
Bèr Kessels -
Dries
-
Eric Scouten
-
Junyor
-
kbahey
-
moshe weitzman
-
Steven