Hard coded pager limits
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
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.
#1
#2
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.
#3
#4
#5
patch can be accessed here http://mycivicsite.com/page_size.patch
#6
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.
#7
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.
#8
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
#9
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?
#10
love the idea, but it's do for a re-roll
#11
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..
#12
reroll
#13
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?
#14
patch stores all pager limits inside variable pager_settings
#15
and bonus: the module which provides easy way to change all your pager limits
inspired by String Overrides http://drupal.org/project/stringoverrides
#16
#17
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.
#18
This still applies cleanly to Drupal 6 HEAD.
#19
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.
#20
If others would support the idea, I would like to change the second parameter in functions
pager_query()andtheme_pager().Instead of
$limitthe$pager_typeor$pager_idwould 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?
#21
+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.
#22
drupal.org really needs a true watchlist function
#23
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
#24
#25
#26
Re-worked the complete patch. Now it uses
variable_get('pager_limit_*')for eachpager_query()andtheme('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_resultspager_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
#27
Impressive! patch applies cleanly. Ran some tests (use the php devel block) chaning some variables, works great!
#28
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.
#29
In my opinion thats another issue.
Pager limits aren't hardcoded for search results only but for nearly *all* listings within drupal core.
#30
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.
#31
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?
#32
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).
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.
#33
The extra parameter is a very good idea.
I will extend the patch to use
$limitwherever possible.#34
Hm, there were 3 functions only using a pager and returning the results:
_locale_translation_seek(locale.inc),do_search(search.module) anduser_search(user.module).All other pagers are included in page building functions (e.g.
dblog_overview()) so the extra parameter wouldn't make sense.#35
Btw: I've created a small module that provides an UI to set the pager limits. See attached screenshot.
#36
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.
#37
The last submitted patch failed testing.
#38
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.
#39
@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;
}
?>
#40
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.
#41
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$limithas no effect (its only handed over totheme_pager_first()and the other child functions).@see #330748: Remove $limit from theme_pager*
Thought
theme_pager()would be a blocker for usinghook_query_TAG_alter()to alter the pager limits but it doesn't seem so :-).#42
I've added some tags to pager queries. But how do you modify the limit with
hook_query_alter()orhook_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.
->conditionworked but not->limit.Any hints?
#43
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.
#44
#45
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?
#46
@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.
#47
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."
#48
@ 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. :-)
#49
Um. Wow! That was a reply to the completely wrong issue. :D But um. Yay?
#50
This issue might very well need documentation too, but it wasn't my intent to change it. :P
#51
+1 for doing this in views and merging it in #286665: Moving Views into Drupal Core
#52
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()andhook_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.
#53
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.
#54
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.
#55
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.
#56
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()orhook_query_TAG_alter().And to get this work we need the tags.
#57
The last submitted patch failed testing.
#58
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).#59
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.
<?phpfunction mymodule_query_pager_alter($query) {
if (arg(0) == 'search' && arg(1) == 'user') {
$query->limit(20);
}
?>
#60
subscribe
#61
The last submitted patch failed testing.
#62
Re-roll
#63
Last chance to get this in or send it to hell (at least until d8-development is started).
#64
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.
#65
The last submitted patch failed testing.
#66
Reroll of this patch.
Function
system_actions_managehas been moved to system.admin.inc so the old patch couldn't find it in system.module.#67
I again got me D7 HEAD and patched it.
Tried this code:
<?phpfunction 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.
#68
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.
#69
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.
#70
The last submitted patch failed testing.
#71
Ehm, bot? Patch applies cleanly without any errors.
Don't know why he suddenly complains about blog.pages.inc.
#72
Testbot hiccup. I'm setting this to reviewed again.
#73
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.
#74
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.
#75
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.
#76
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.
#77
The last submitted patch failed testing.