I have a need to expose some of the core druapl pages (logs, users, comments etc)
in a view with limited space available. I noticed that the page size
is hard coded in most core modules that use drupals paging mechanism. A
quick glance reveals the following hard-coded pages sizes:

watchdog = 50 lines
user = 50 lines
statistics = 30 lines
comment = 50 lines
poll = 50 lines

I think it would be really usefull to provide a mechanism to override
these hard-coded values. I would suggest something simple like
appending a value to the url in the query string for page size. This
value could be interogated by each module that calls
theme_pager_link() to see if it should use it's hard-coded default or
use an override.

The comment module already makes use of this method to display it's
normal comment view (though not for it's admin view). I also found a
couple of instances where a variable was being used to determine the page
size as well. For these cases the variable could still be used but the query
string value could be used to override it.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dreed47’s picture

dreed47’s picture

I've got a patch for this but for some reason drupal.org is not letting me upload it. I get an error that says the directory is not writable.

dreed47’s picture

dreed47’s picture

dreed47’s picture

patch can be accessed here http://mycivicsite.com/page_size.patch

dreed47’s picture

FileSize
22.65 KB

Per the discussion on the developer list, I've redone this patch to use a variable to specify the page size for admin pages. Two new variables are used.

variable_get('admin_pager_long', 50)
variable_get('admin_pager_normal', 30)

The following modules are affected.

comment
node
path
profile
statistics
taxonomy
user
watchdog
locale (locale.inc actually)

New patch attached.

Jaza’s picture

FileSize
22.7 KB

Great patch, except that it doesn't use the new variables admin_pager_long and admin_pager_normal. All the pager lengths are still hard coded.

This attached patch replaces all hard-coded calls to:

drupal_get_page_size(50) with drupal_get_page_size(variable_get('admin_pager_long', 50))
drupal_get_page_size(30) or drupal_get_page_size(25) or drupal_get_page_size(20) with drupal_get_page_size(variable_get('admin_pager_long', 30))

Also made one more change:
drupal_get_page_size(15) with drupal_get_page_size(variable_get('admin_pager_search', 15))

I have added a new variable, 'admin_pager_search', as I feel that search results are different to all other pager results (since they alone are not displayed as a table in the admin section), and should have their own special setting.

Jaza.

dreed47’s picture

FileSize
14.79 KB

Yikes! I seemed to have uploaded the wrong patch in my last post. Here's a new one.

Thanks for the patch Jaza but when this was discussed on the developer list I think the concensus was to not have this overridable via a URL parameter. So my new patch only provides for the new variables.

discussion at http://lists.drupal.org/archives/drupal-devel/2005-10/msg00301.html

Uwe Hermann’s picture

FileSize
19.35 KB

Updated patch to work with HEAD. I did not change search result lengths in user.module and search.module. Should they get an extra variable, or just use admin_pager_normal?

dmitrig01’s picture

Version: x.y.z » 6.x-dev
Status: Needs review » Needs work

love the idea, but it's do for a re-roll

Pasqualle’s picture

Title: Hard coded page sizes » Hard coded pager limits
Status: Needs work » Needs review
FileSize
30.46 KB

reroll
used a new function for better readability/usage

test:
1. fresh install
2. enable "devel generate" module and generate some stuff (http://drupal.org/project/devel)
3. change variables pager_limit_long and pager_limit_short in {variables} table
4. go to admin/settings/performance and click "clear cached data"
5. visit pages where pager is used
6. check new pager_limit_* variables for taxonomy and poll modules at install/uninstall

problem:
I think upgrade from older drupal version does not create pager_limit_long and pager_limit_short variables, which result in that all pagers using these variables will be defaulted to limit 50

there is still a variable default_nodes_main which is used for similar purposes. i don't know if i can remove it..

Pasqualle’s picture

FileSize
30.4 KB

reroll

Pasqualle’s picture

I think there will be too many variables, I will try to store it into 1 variable like the theme_settings variable is stored.
Or is it an overkill?

Pasqualle’s picture

FileSize
30.97 KB

patch stores all pager limits inside variable pager_settings

Pasqualle’s picture

FileSize
8.51 KB
140 bytes
2.9 KB

and bonus: the module which provides easy way to change all your pager limits

inspired by String Overrides http://drupal.org/project/stringoverrides

moshe weitzman’s picture

Version: 6.x-dev » 7.x-dev
gpk’s picture

Priority: Normal » Critical

Having hard-coded limits is a real limitation, especially for the search page, so I'm marking this as must-have i.e. critical for 7.x.

Thanks to Pasqualle for getting it this far.

birdmanx35’s picture

This still applies cleanly to Drupal 6 HEAD.

JohnForsythe’s picture

I'd love to see this fixed in 6 if possible. Here's the cheap hack I'm using at the moment.. in search.module, this:

$result = pager_query("SELECT * FROM temp_search_results", 10, 0, $count_query);

becomes this:

$result = pager_query("SELECT * FROM temp_search_results", isset($GLOBALS['pager_override']) ? $GLOBALS['pager_override'] : 10, 0, $count_query);

Then you just need to set $GLOBALS['pager_override'] before calling a search. Not a good or comprehensive fix, but might get you by if you're looking for an easy way to change that number for some reason.

Pasqualle’s picture

If others would support the idea, I would like to change the second parameter in functions pager_query() and theme_pager().
Instead of $limit the $pager_type or $pager_id would be appropriate.
like:

pager_query($query, $pager_type = 'short', $element = 0, $count_query = NULL)
theme_pager($tags = array(), $pager_type = 'short', $element = 0, $parameters = array(), $quantity = 9)

Or change it to an array to support alpha pager like {'#limit' => 'short', '#type' => 'alpha', '#options' => array() }
Any suggestions?

cburschka’s picture

+1 for supporting default constants 'short', 'medium' and 'long' in pagers, whose value can be reconfigured. This would emulate the behavior of the date API.

Of course, sending in explicit numeric values should still be possible.

Note: '#hash prefixed keys' are only used in array trees of arbitrary depth, like forms. They're used to distinguish a property from a sub-item. We have no sub-items here.

MedicSean37’s picture

drupal.org really needs a true watchlist function

danieltome’s picture

Here is a solution better than using globals:
http://drupal.org/node/166172

use: variable_get('search_num_results',10)

I still don't know why it's not possible to get this "fix" into Drupal 5.
I wouldn't consider it a "feature request" but more of basic functionality that was left out.

Drupal seems quite serious, and even though you could just update line 871 to the nmber you prefer it feels dirty and "hackish" to update core files.

Is there a place were we can vote to include these type of "fixes" into 5.x?
thanks

lilou’s picture

Status: Needs review » Needs work
webchick’s picture

Priority: Critical » Normal
stBorchert’s picture

Status: Needs work » Needs review
FileSize
22.67 KB

Re-worked the complete patch. Now it uses variable_get('pager_limit_*') for each pager_query() and theme('pager') (and ->limit()).
With this patch its very simple to write a small contrib module that provides an UI to change each specific pager limit.
Complete list of used variable names:

pager_limit_locale_search_results
pager_limit_aggregator_items
pager_limit_comment_admin
pager_limit_dblog_items
pager_limit_dblog_top_items
pager_limit_node_admin
pager_limit_path_admin
pager_limit_profile_users
pager_limit_search_results
pager_limit_statistics_recent
pager_limit_statistics_top_pages
pager_limit_statistics_top_visitors
pager_limit_statistics_top_referrers
pager_limit_statistics_node_tracker
pager_limit_statistics_user_tracker
pager_limit_system_action
pager_limit_tracker
pager_limit_user_users
pager_limit_user_search_results
swentel’s picture

Impressive! patch applies cleanly. Ran some tests (use the php devel block) chaning some variables, works great!

JohnForsythe’s picture

One of the reasons I suggest using globals (see post #19), rather than variable_set, is that you can change the limit on the fly without affecting the behavior of other page requests running on the same site. Trying to do that with variable_set creates a race condition where you need to set the value back before any other process attempts a search.

Some of my modules programmatically run searches to find related articles, and need around 30 results to make good matches. But normal user searches don't need to return more than 10 results. By using the globals method, I can temporarily change to the limit without having the value change for other searches.

Using Drupal's variable system means any changes will affect all other searches being run, which is undesirable. At the least, a way to change this limit on a per query basis should be included.

stBorchert’s picture

At the least, a way to change this limit on a per query basis should be included.

In my opinion thats another issue.
Pager limits aren't hardcoded for search results only but for nearly *all* listings within drupal core.

JohnForsythe’s picture

Pager limits aren't hardcoded for search results only but for nearly *all* listings within drupal core.

That's true. And not being able to change them without making the change affect every request on your site is a serious limitation.

The current patch may cause a lot of unintended bugs for modules that are expecting to get a fixed number of results back.

Each module should be able to control how many results it wants, and attempting to use variable_set to do that will create race conditions.

stBorchert’s picture

Each module should be able to control how many results it wants, and attempting to use variable_set to do that will create race conditions.

For this reason I've used different variable names for each pager.

Btw.: how would you use $GLOBALS if the views API get into core and all of these listings are views?

JohnForsythe’s picture

For this reason I've used different variable names for each pager.

It's still a problem if multiple modules are calling these pagers and expecting different number of results, no? Many modules make calls to search module for example, and now they have no way of guaranteeing the number of results they're going to get. It used to be 10, but now it's whatever the variable was set to last, and this can change at any moment (even after you've checked the value, hence the race condition).

Btw.: how would you use $GLOBALS if the views API get into core and all of these listings are views?

Globals are a bad solution, I only used them because it presented an easy, one line workaround. A better solution is to incorporate a new argument into the function call.

In search.module:

function do_search($keywords, $type, $join1 = '', $where1 = '1 = 1', $arguments1 = array(), $columns2 = 'SUM(i.relevance) AS calculated_score', $join2 = '', $arguments2 = array(), $sort_parameters = 'ORDER BY calculated_score DESC') {
...
$result = pager_query("$select $sort_parameters", 10, 0, $count_select, $arguments);

becomes:

function do_search($keywords, $type, $join1 = '', $where1 = '1 = 1', $arguments1 = array(), $columns2 = 'SUM(i.relevance) AS calculated_score', $join2 = '', $arguments2 = array(), $sort_parameters = 'ORDER BY calculated_score DESC', $limit = FALSE) {
...
// If a limit wasn't specified, use the default setting.
if (!$limit) {
  $limit = variable_get('pager_limit_search_results', PAGER_LIMIT_SHORT);
}
$result = pager_query("$select $sort_parameters", $limit, 0, $count_select, $arguments);

Now modules can specify their own limit, or rely on the variables you've created. Best of both worlds.

stBorchert’s picture

Status: Needs review » Needs work

The extra parameter is a very good idea.
I will extend the patch to use $limit wherever possible.

stBorchert’s picture

Status: Needs work » Needs review
FileSize
24.74 KB

Hm, there were 3 functions only using a pager and returning the results: _locale_translation_seek (locale.inc), do_search (search.module) and user_search (user.module).
All other pagers are included in page building functions (e.g. dblog_overview()) so the extra parameter wouldn't make sense.

stBorchert’s picture

FileSize
33.91 KB

Btw: I've created a small module that provides an UI to set the pager limits. See attached screenshot.

JohnForsythe’s picture

The main goal of having the extra parameter is to allow contributed modules to reuse these pagers cleanly (with different limits).

Search is my main concern, because it's where I ran into this problem personally (writing a "recommended links" module), but I imagine the other pagers could be useful, too. It might make sense to include the $limit parameter further up the chain of functions in some cases where you want to display the entire page with a certain number of results. It should be easy then to write a module that lets each user control the number of results they want to see for each type of page, or create an overview page that just shows a couple of items from a bunch of different pages (see post #1), without affecting other users or modules.

Status: Needs review » Needs work

The last submitted patch failed testing.

stBorchert’s picture

Status: Needs work » Needs review
FileSize
23.83 KB

tracker.pages.inc has changed (now using PDO instead of plain SQL) so the old patch didn't apply anymore. Here is a re-roll.

stBorchert’s picture

@John: I don't know atm how we can add the limit to the other pagers. If you look into user_admin_account() in user.admin.inc: this is a page callback that builds the complete user administration page. Maybe we should extract the lines that get the results from DB into an additional function.

<?php 
function user_users($filter = FALSE, $limit = FALSE) {
  $header = array(
    array(),
    array('data' => t('Username'), 'field' => 'u.name'),
    array('data' => t('Status'), 'field' => 'u.status'),
    t('Roles'),
    array('data' => t('Member for'), 'field' => 'u.created', 'sort' => 'desc'),
    array('data' => t('Last access'), 'field' => 'u.access'),
    t('Operations')
  );
  
  // If no filter is given build a default filter.
  if (!$filter) {
    $filter = array('join' => '', 'where' => '', 'args' => array());
  }
  
  // If no limit is given use the default limit.
  if (!$limit) {
    $limit = variable_get('pager_limit_user_users', PAGER_LIMIT_LONG);
  }
  
  $sql = 'SELECT DISTINCT u.uid, u.name, u.status, u.created, u.access FROM {users} u LEFT JOIN {users_roles} ur ON u.uid = ur.uid ' . $filter['join'] . ' WHERE u.uid != 0 ' . $filter['where'];
  $sql .= tablesort_sql($header);
  $query_count = 'SELECT COUNT(DISTINCT u.uid) FROM {users} u LEFT JOIN {users_roles} ur ON u.uid = ur.uid ' . $filter['join'] . ' WHERE u.uid != 0 ' . $filter['where'];
  $result = pager_query($sql, $limit, 0, $query_count, $filter['args']);
  
  return $result;
}
?>
Dries’s picture

Status: Needs review » Needs work

With the new PDO stuff, we shouldn't have a need to introduce variable_get()s. Instead, you should be able to use the query alter-hook to change the hardcoded defaults. I think this patch now has become a "won't fix". However, it would be good to verify this, and to document this.

stBorchert’s picture

Well, the only query tags currently used are "node_access" and "term_access". Maybe we should focus then on documenting the use of hook_query_TAG_alter() and tag each pager query so it could be modified.

Hm, just noticed something strange in theme_pager(). The parameter $limit has no effect (its only handed over to theme_pager_first() and the other child functions).
@see #330748: Remove $limit from theme_pager*

Thought theme_pager() would be a blocker for using hook_query_TAG_alter() to alter the pager limits but it doesn't seem so :-).

stBorchert’s picture

Status: Needs work » Needs review
FileSize
6.77 KB

I've added some tags to pager queries. But how do you modify the limit with hook_query_alter() or hook_query_TAG_alter()?
I've tried

<?php
function pagertest_query_statistics_recent_hits_alter(QueryAlterableInterface $query) {
  // Set number of displayed items to 5.
  $query->extend('PagerDefault')->limit(5);
}

function pagertest_query_alter(QueryAlterableInterface $query) {
  // Set number of displayed items to 5.
  if ($query->hasTag('statistics_recent_hits')) {
    $query->extend('PagerDefault')->limit(5);
  }
}
?>

but none of them worked. ->condition worked but not ->limit.

Any hints?

Berdir’s picture

We are working on a solution to limit queries with hook_query_alter, see http://drupal.org/node/440310.

In short: It is currently not possible.

Berdir’s picture

Status: Needs review » Postponed
Crell’s picture

Status: Postponed » Needs work

Hm, I hadn't realized this was related to that issue.

I would actually propose something radically different here. With the extenders system, we now have the ability to have multiple types of pagers. Also, the pager system outside of the query object itself is still a trainwreck of suck. (Heck, the extender itself is still a trainwreck of suck IMO.)

What if we took this opportunity to provide a second pager type, one that allows a user-configurable number of records per page via a select box or something. That would then be implemented as a new extender class similar to PagerDefault, but named differently. (PagerVariable?) That would give us additional functionality in core, as well as the opportunity to refactor the pager system to make it less godawful. (Three global variables with non-self-documenting names mixed in with poking at $_GET? I still haven't a clue how that damned thing works.) Having a second implementation generally makes verifying the base system easier.

The ability to have multiple types of pagers was one of the goals for the extenders, so let's try and push that to its logical conclusion.

Thoughts?

moshe weitzman’s picture

@Crell's idea is great except for the fact that we may have no real need for such a pager in core? watchdog page? admin/content/node and admin/content/comment? we don't usually put things in without a clear use case. I know that phpmyadmin has such a pager but it is pretty uncommon.

webchick’s picture

Issue tags: +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. :)

Tagging with "Needs Documentation."

Crell’s picture

@ Moshe: I see no problem with making some of the core pages mentioned in the OP use the new pager. There are certainly reasons why that would make sense to do. Especially the taxonomy pages, since there was a thread early in the D6 cycle (I forget where it is now) that noted that the drag and drop really wasn't useful without much larger pages. At the time there was a patch to offer an "items per page" variable; I don't recall if it went in or not.

@webchick: Oh! Such timing! Get Earl into this thread. :-)

webchick’s picture

Um. Wow! That was a reply to the completely wrong issue. :D But um. Yay?

webchick’s picture

Issue tags: -Needs documentation

This issue might very well need documentation too, but it wasn't my intent to change it. :P

mitchell’s picture

+1 for doing this in views and merging it in #286665: SEE ISSUE #1805996 -- Moving Views into Drupal Core

stBorchert’s picture

Status: Needs work » Needs review
FileSize
14.69 KB

Re-rolled #42 to work with current HEAD.
This patch adds tags to all system queries that are extended by a pager. After #496516: Move query_alter() into a preExecute() method gets in one will be able to override all the system pager limits (e.g. on admin node overview or dblog) with the help of hook_query_alter() and hook_query_TAG_alter().
Examples:

<?php
/**
 * Implement hook_query_alter().
 */
function mymodule_query_alter(QueryAlterableInterface $query) {
  // Set number of displayed rows to 10 for recent hits page.
  if ($query->hasTag('statistics_recent_hits')) {
    $query->limit(10);
  }
}
  
/**
 * Implement hook_query_TAG_alter().
 *
 * Alter the number of displayed rows on dblog/overview.
 */
function mymodule_query_dblog_overview_alter(QueryAlterableInterface $query) {
  // Set number of displayed rows to 30.
  $query->limit(30);
}

?>

Note: this didn't have any effect until #496516: Move query_alter() into a preExecute() method is fixed.

stBorchert’s picture

Just a quick reminder: #496516: Move query_alter() into a preExecute() method is fixed so this one can do its work, too.
If it doesn't get it in we finally have another release with non-modifyable paging limits for pages generated by core.

Berdir’s picture

This is not true. We already have a pager tag, so you can use that one.

I'm not sure if a custom tag for every single query is a good idea, imho, it is a bit too much. If there is a need to customize a special query, I'm sure it is possible to figure out based on what has been selected, arg() and so on.

Crell’s picture

I'm not convinced this is necessary either. Admittedly using arg() is evil, but at the same time every tag does have a performance hit (moreso now that the registry is gone), so we shouldn't just add them quite so randomly.

stBorchert’s picture

They aren't added randomly. These are all the pages having a pager (and generated by core modules). If you want to change the number of displayed rows there is no cleaner way to use hook_query_alter() or hook_query_TAG_alter().
And to get this work we need the tags.

Status: Needs review » Needs work

The last submitted patch failed testing.

stBorchert’s picture

Status: Needs work » Needs review
FileSize
14.65 KB

Re-roll of the patch.
Question to Berdir and Crell: how would you change the number of results displayed while searching users?
The number is harcoded to a value of 15 so you would need real magic to change that.
But with a tag added to the base query (in user_search_execute) it will be a function call with only 1 line (see example in comment #52).

Berdir’s picture

I can't actually test it right now, but the following should work just fine. It is rather ugly code, but I think you could do the same check based on which tables/fields are selected but I can't write that out of my head.

function mymodule_query_pager_alter($query) {
if (arg(0) == 'search' && arg(1) == 'user') {
  $query->limit(20);
}
mattyoung’s picture

subscribe

Status: Needs review » Needs work

The last submitted patch failed testing.

stBorchert’s picture

Status: Needs work » Needs review
FileSize
13.81 KB

Re-roll

stBorchert’s picture

Last chance to get this in or send it to hell (at least until d8-development is started).

Kars-T’s picture

Status: Needs review » Reviewed & tested by the community

I applied the patch to latest D7 and could not find any errors. The code looks fine and is basically self describing. There are only tags added to the query object and I can't find any typos or problems with it.
As said before I believe this patch is an great addition to the core.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

stBorchert’s picture

Status: Needs work » Needs review
FileSize
13.82 KB

Reroll of this patch.
Function system_actions_manage has been moved to system.admin.inc so the old patch couldn't find it in system.module.

Kars-T’s picture

Status: Needs review » Needs work

I again got me D7 HEAD and patched it.
Tried this code:

function testmodule_query_dblog_overview_alter($query) {
  watchdog('PAGER', print_r($query, TRUE));
  $query->limit(2);
  watchdog('PAGER',print_r($query, TRUE));
}

And it works. Only two items ( in this case my debug prints) are shown in admin/reports/dblog

I still believe this would be a great addition. If you look at this thread it started with adding tons of variables and constants blowing up the code. This would be what I expected how this issue could be solved. The approach from stbochert is much more slick than just adding variables. It just adds tiny bits of code and makes the queries more accessible.

There was talk about random tag names in #55:

+++ includes/locale.inc	21 Sep 2009 17:07:52 -0000
@@ -2271,6 +2271,7 @@ function _locale_translate_seek() {
   }
 
   $sql_query = $sql_query->extend('PagerDefault')->limit(50);
+  $sql_query->addTag('locale_translate_seek');

The tag for the pager is the name of the function which I think is wise and relatively easy to understand.

+++ modules/profile/profile.pages.inc	21 Sep 2009 17:08:00 -0000
@@ -95,6 +96,7 @@ function profile_browse() {
+    $query->addTag('profile_browse_userlist');
+++ modules/system/system.admin.inc	21 Sep 2009 17:08:05 -0000
@@ -2324,6 +2324,7 @@ function system_actions_manage() {
+  $query->addTag('system_actions');
+++ modules/user/user.module	21 Sep 2009 17:08:16 -0000
@@ -803,6 +803,7 @@ function user_search_execute($keys = NUL
+  $query->addTag('user_search');

But there are three cases where the name of the tag differs from the function. Just now I decide to set this issue to "needs work" and talk to stbochert later about this.

Crell says in #55 as well there is a performance hit with adding new hooks but I can't say much about that.

This review is powered by Dreditor.

stBorchert’s picture

function profile_browse(): The function contains two separate queries. Therefore the tags needs to be named different.

'system_actions': The query actually returns system actions so it is more inuitive to name the tag 'system_actions' instead of 'system_actions_manage'.

''user_search': Should probably named 'user_search_results'.

I'll have to look at the other tags if they could be renamed according to the results the queries return.

Kars-T’s picture

Status: Needs work » Reviewed & tested by the community

I have chatted with stbochert and it seems I did misinterpreted Crells post. The patch from #66 works and I reviewed it so I change the status.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

stBorchert’s picture

Status: Needs work » Needs review

Ehm, bot? Patch applies cleanly without any errors.
Don't know why he suddenly complains about blog.pages.inc.

Kars-T’s picture

Status: Needs review » Reviewed & tested by the community

Testbot hiccup. I'm setting this to reviewed again.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Hm. Sorry, I really don't like the pattern this patch attempts to establish:

1. As Crell and Berdir have mentioned, tags should be added sparingly since there are (minor) performance implications for each.
2. There is nothing about these tags that implies that they are being added because they wrap a pager query; they're just the function name, but only on certain functions and not others. Developers will be terribly confused by this, and when it's appropriate and when it's not to add a tag for the function the calling code came from.
3. What happens if blog_page_user() calls two queries, each of which needs to be detectable by hook_query_alter()? How do we establish guidelines for this?

Is there any reason Berdir's suggestion @ #59 isn't a perfectly reasonable workaround for this issue? If so, I'm inclined to won't fix this. If not, then I would want to see a RTBC from Crell/Berdir on the approach here, and so far that has failed to happen, so this looks like a D8 issue indeed.

Crell’s picture

While #59 will work, it relies on arg() which is almost by definition "an evil ugly hack you should never use, fail!" However, I don't like the zillion-tags approach either so I am not going to RTBC this.

For D7, I'm comfortable leaving this as an evil-hack-edge-case, since I can't imagine changing pager limits being a very common pattern and there is an ugly-but-works approach (arg()). I am open to a better solution if someone can come up with one, but I can't think of one at the moment.

stBorchert’s picture

The better solution is to add tags to all paged queries so these queries can be modified by hook_query_TAG_alter().

@webchick: the tag names were only suggestions and can be changed to more handy names.

Kars-T’s picture

Sad thing. I still like the idea and everything that comes from the core should be easily accessible. The tag names imo are not that important as you have to look them up anyway. The only argument that counts for me would be "Views can do all this" and I see that the new admin_menu will bring overrides for nearly all internal views.

Status: Needs review » Needs work

The last submitted patch failed testing.

donquixote’s picture

Crell / #45:

That would give us additional functionality in core, as well as the opportunity to refactor the pager system to make it less godawful. (Three global variables with non-self-documenting names mixed in with poking at $_GET? I still haven't a clue how that damned thing works.)

Having a look at the pager code myself immediately made me search for "pager + refactor", and found this post.
The code as we have it is really a mess.

Some ideas for cleanup:
- Most importantly, the theme functions should have all necessary info passed in as arguments, so they don't have to look into $_GET or some other global vars, or pull global information from functions like pager_get_querystring().
- We need a more transparent and better documented way to build pager urls from function arguments. Right now this is all mixed up in theme_pager_link() and pager_load_array(), which pulls half of the information from other functions.
- Should we make it an array for drupal_render?

And for added flexibility:
- It could be useful to have an alter hook somewhere, so modules can change the way a pager is displayed.
- Theme and alter hook implementations should have the possibility to target only a specific pager. This means, they need additional info, such as a "pager id" and/or "pager categories".

I would love if I had the time to read all the rest of this thread, but I'm sorry I don't. But I see that the proposed patches don't do anything about use of $_GET, so I assume that there is no serious refactoring planned yet.

Maybe this should be a separate issue for D8 ? I hate feature freeze!

Crell’s picture

Title: Hard coded pager limits » Make pagers not suck
Version: 7.x-dev » 8.x-dev
Priority: Normal » Critical

Given that we're in feature freeze, I don't think there's much we can do here right now. Let's come back to this in D8 and do it right, which may mean a totally new pager system. (The current one is a trainwreck, full stop.)

donquixote’s picture

We can write a module that implements hook_theme_registry_alter, and replace theme_pager() with something nicer. Starting from there, we can provide a bunch of new module hooks and theme functions. Later this stuff can go into D8 core.

A nice ingredient would be url builder objects, that can be passed to theme functions as parameters - making them independent of global or static vars. The class below implements the default behavior, but could be easily replaced with something else.

I have not tested this code, but I will update the post when I notice a bug.

<?php
class PagerUrlBuilder {
  protected $_path;
  protected $_options;
  protected $_pager_element_id;
  
  /**
   * @param $path
   *   the url path to start with.
   * @param $options
   *   url options to start with.
   * @param $pager_element_id
   *   index/id of the pager we are dealing with.
   */
  public function __construct($path, array $options = array(), $pager_element_id = 0) {
    $this->_path = (string)$path;  // a bit of sanitizing..
    $this->_options = $options;
    $this->_pager_element_id = (int)$pager_element_id;  // a bit of sanitizing..
    // D8 implementations of url() will throw exceptions for bad parameters,
    // and we want to ring the alarm on construction time!
    // TODO: is this a good idea?
    url($path, $options);
  }
  
  /**
   * Build a pager url for a given page number.
   *
   * @param $i_page
   *   Page number
   * @return
   *   url of the new page.
   */
  public function pager_url($i_page) {
    $i_page = (int)$i_page;  // a bit of sanitizing..
    $options = $this->_options;
    $pager_param = isset($options['parameters']['page']) ? $options['parameters']['page'] : '';
    $pager_values = explode(',', $pager_param);
    // sanitize the pager parameter values
    for ($i=0; $i<count($pager_values); ++$i) {
      $pager_values[$i] = (int)$pager_values[$i];
      // TODO: we could filter for negative integers, or out of range integers.
      // But, is this really necessary?
      // $pager_values[$i] = ($pager_values[$i] < 0) ? 0 : $pager_values[$i];
    }
    // turn implicit zeros into explicit zeros.
    for ($i=count($this->_pager_values); $i<$this->_pager_element_id) {
      $pager_values[$i] = 0;
    }
    $pager_values[$this->_pager_element_id] = $i_page;
    $options['parameters']['page'] = implode(',', $pager_values);
    return url($this->_path, $options);
  }
}
?>
donquixote’s picture

FileSize
3.17 KB

And here is the module.

It's a bit unflexible at the moment, because it's all one big theme function. But in most cases this will not be necessary at all, thanks to hook_pt_pager_options_alter(). We could also make it a template instead, so that themes can implement a preprocess hook.

Most importantly, it moves the global dependencies out of the theme function.

The extra weight in theme_pt_pager() compared to theme_pager() is mostly due to new features.

donquixote’s picture

FileSize
3.15 KB

Removed some cruft from debug and testing.

stBorchert’s picture

See #33809-47: Make pagers not suck for a clean way on how to build a new pager system ...

donquixote’s picture

Yeah, I noticed this blog post.
Merlin's code solves a different part of the problem. It does not work as a contrib module, and won't make it into core before D8. And he doesn't talk much about the theming layer, and he doesn't mention a link builder object (what a nice idea, isn't it?). Instead, he proposes a lot of useful changes to core, which I did not (or not yet).

The implementation of theme('pager') is left as an exercise to the reader, or whoever decides to try and submit this as a patch.

Yes :)
Throw in the link builder object, and we are getting close.

First, there should be no globals

The use of globals is something Drupal has been moving away from, so this could support that effort. Globals are difficult to maintain, difficult to document, and entirely too easy to have namespace conflicts. Pagers could be stored in a static array with an accessor and a mutator (getter and setter) much like many other systems currently in Drupal.

In D8 we eliminate the globals, and in D9 we eliminate the statics ...
Statics (and singletons, global registry etc) are not more but encapsulated globals. Wrapping globals into functions and classes can make things more controlled, but it doesn't remove the dependencies. Where possible, I prefer to have things passed around as parameters.

One example would be if you want to build pagers in a cron run, with a variation of values for $current_page etc. All in one request. Thanks to #47, all the statics can be easily modified, but this does not protect us from global state side effects.

Now, maybe Drupal is just not made for that. So, "Make it easy to get and set common pager data" - ok...

I don't have a good solution either, unless I want to rewrite large portions of Drupal.

My own code from #82 does not eliminate any of the statics, but it moves the dependencies out of the theme function. Or, all that I found. There could be hidden ones in url() or l().

Crell’s picture

@donquixote: This issue is not for developing a contrib module. It's for fixing pagers in Drupal 8. Let's focus on that here, but not until after D7 is released as there's still a ton of work to do on that front.

donquixote’s picture

True, but a contrib module could help to prepare a D8 patch, don't you think?

catch’s picture

Priority: Critical » Major

Downgrading all D8 criticals to major per http://drupal.org/node/45111

joachim’s picture

Subscribing to this, but don't have time to delve.

Just want to say that I'm theming a pager right now and there's a flaw in the way theme_pager_link() adds title attributes based on link strings it *assumes* are passed to it. This means if you use theme_pager() to change the 'next' string, say, you lose the attribute without realizing.

mortendk’s picture

Issue tags: +html5

a little dream was if we could end up with markup like this

<nav class="pager">
  <h2 class="element-invisible">Pages</h2>
  <ul>
    <li class="firstpage">
      <a href="/node" title="Go to first page"><b>«</b> first</a>
    </li>
    <li class="previous">
      <a href="/node" rel="previous" title="Go to previous page">
        <b>‹</b> previous</a>
    </li>
    <li>
      <a href="/node" title="Go to page 1">1</a>
    </li>
    <li class="current">
        <em>2</em>
    </li>
    <li class="ellipsis">
      <i>...</i>
    </li>
    <li>
      <a href="/node?page=2" title="Go to page 3">3</a>
    </li>
    <li class="next">
      <a href="/node?page=2" rel="next" title="Go to next page">next <b>›</b></a>
    </li>
    <li class="lastpage">
      <a href="/node?page=2" title="Go to last page">last <b>»</b></a>
    </li>
  </ul>
</nav>
mgifford’s picture

What happened to this issue? Been quite now for almost a year now. Also related issue #115753: Pager rel attributes

salvis’s picture

The Pager Limits module makes all pagers configurable without requiring any changes in core.

It's D7 only at this point, but it should be portable to D8.

giorgio79’s picture

All admin lists will switch to Views, so I guess this is a no fix?

Crell’s picture

No, the pager API itself is still crap. It needs to rely on the request, not globals. Using Views doesn't help that at all.

donquixote’s picture

It needs to rely on the request, not globals.

I'd say, in pre-D8, the two are the same :) so this is actually a new dimension to the problem introduced in D8.

In #78, I complained that the theme functions should not have to look into globals, or indirectly pull their information from global state, but rather have their information from function arguments.

Following this up on D8, I think theme functions also should not look directly into the request object.

Proposed solution:
- One object on request level, that does all the url generation logic.
- One wrapper object per pager, that is exposed to the display layer, and can generate urls for this one specific pager. (or if needed, it could return some meta information that we want to render along with the pager links)

The display layer, to render one pager, will be given the url generator object, a range, and the current index.
The display layer decides which numbers to render or abbreviate, or whether to render just two arrow buttons. But, it should not concern itself about how many other pagers exist on the page. Also it does not need direct access to the request object, or to the request-level url generator.

Crell’s picture

don: Yeah, you're right. It should rely on information passed to it that 98% of the time comes from the request, but the pager system doesn't know that. :-)

donquixote’s picture

One thing I'm unsure about - should the range and current index be separate, or should it also be provided with the wrapper object?
If so, then the wrapper could also provide convenient next(), next($n), first() and last() methods. Or make it nextUrl(), prevUrl(), etc.

Display layer: Will this be a template? And how can twig talk to the wrapper object?
I always have the idea that pager rendering contains much more logic than html (I say logic, not business). All the if/else for whether to abbreviate or not, etc. Doing this as a template tends to look quite complex. Sometimes I like theme functions.

catch’s picture

Category: feature » task

How is this a feature?

YesCT’s picture

Is this still major?

Looks like there are some questions to be answered before someone can attempt a patch according to @Crell 's suggestion.

Also tagging, updating the issue summary with the new template to include ... a summary :) and updated proposed resolution will help. (see tips in doc: http://drupal.org/node/1427826)

mgifford’s picture

Last patch from Sept 2009 so this definitely needs a re-roll.

dawehner’s picture

It seems to be that a proper extending system in Drupal 8 should more look something like the following:

$select = db_select()...;

$extender = drupal_container()->get('database.extender.pager');
$extender->setSelect($select);
$extender->setConnection($select->getConnnection());

Some advantages:

  • People can switch out how they work.
  • Extenders can specify there dependencies and so don't have to rely on globals anymore

In general though we maybe should better figure out how to integrate the views side of pagers, as two totally different systems feel wrong.
Maybe a view pager could use another existing pager, but take into account that a view pager could work without the database at all.

Crell’s picture

I'm not sold on the exact syntax, but extenders as services gets a ++ from me as a concept.

tim.plunkett’s picture

Version: 8.x-dev » 9.x-dev

:(

Fabianx’s picture

Version: 9.x-dev » 8.x-dev

I am not convinced this is D9 ... Can't this be done without being an API change?

Anyway, this still needs goals defined in issue summary and a better title ...

klonos’s picture

I think we should postpone this on #2083717: Convert Search Results to Views (or close it as a duplicate or even make the other issue a META and this one one of its sub-issues).

andypost’s picture

Looks we need to fix this finally (8 years for now), also views probably have more flexible paging but it also sucks because pager ID should vary for dirrerent pages

PS: comment fields now affected too #2155387: Multiple comment fields per entity are supported, but break horribly when they need pagers

tim.plunkett’s picture

Priority: Major » Normal
Issue summary: View changes

This is not truly major, not at this stage of D8.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

joseph.olstad’s picture

jcisio’s picture

Status: Needs work » Closed (duplicate)

I guess we can close this issue because some pages are views now, some other pages (e.g. field list) already have their issues, and the linked issue is about generic pager. So nothing else to be done here.

joseph.olstad’s picture

#702940: Make the number of results per page configurable is not about a 'generic' pager, it's about the built in search module search pages results limit which was previously not configurable but thanks to the RTBC patch in the linked 11 year old issue it is now configurable.