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.
Comment | File | Size | Author |
---|---|---|---|
#82 | pager_tools.zip | 3.15 KB | donquixote |
#81 | pager_tools.zip | 3.17 KB | donquixote |
#66 | 33809-pager_limits-66.patch | 13.82 KB | stBorchert |
#62 | 33809_pager_limits_62.patch | 13.81 KB | stBorchert |
#58 | 33809_pager_limits_58.patch | 14.65 KB | stBorchert |
Comments
Comment #1
dreed47 CreditAttribution: dreed47 commentedComment #2
dreed47 CreditAttribution: dreed47 commentedI'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.
Comment #3
dreed47 CreditAttribution: dreed47 commentedComment #4
dreed47 CreditAttribution: dreed47 commentedComment #5
dreed47 CreditAttribution: dreed47 commentedpatch can be accessed here http://mycivicsite.com/page_size.patch
Comment #6
dreed47 CreditAttribution: dreed47 commentedPer 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.
Comment #7
Jaza CreditAttribution: Jaza commentedGreat 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.
Comment #8
dreed47 CreditAttribution: dreed47 commentedYikes! 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
Comment #9
Uwe Hermann CreditAttribution: Uwe Hermann commentedUpdated 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?
Comment #10
dmitrig01 CreditAttribution: dmitrig01 commentedlove the idea, but it's do for a re-roll
Comment #11
Pasquallereroll
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..
Comment #12
Pasquallereroll
Comment #13
PasqualleI 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?
Comment #14
Pasquallepatch stores all pager limits inside variable pager_settings
Comment #15
Pasqualleand bonus: the module which provides easy way to change all your pager limits
inspired by String Overrides http://drupal.org/project/stringoverrides
Comment #16
moshe weitzman CreditAttribution: moshe weitzman commentedComment #17
gpk CreditAttribution: gpk commentedHaving 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.
Comment #18
birdmanx35 CreditAttribution: birdmanx35 commentedThis still applies cleanly to Drupal 6 HEAD.
Comment #19
JohnForsythe CreditAttribution: JohnForsythe commentedI'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.
Comment #20
PasqualleIf others would support the idea, I would like to change the second parameter in functions
pager_query()
andtheme_pager()
.Instead of
$limit
the$pager_type
or$pager_id
would be appropriate.like:
Or change it to an array to support alpha pager like
{'#limit' => 'short', '#type' => 'alpha', '#options' => array() }
Any suggestions?
Comment #21
cburschka+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.
Comment #22
MedicSean37 CreditAttribution: MedicSean37 commenteddrupal.org really needs a true watchlist function
Comment #23
danieltome CreditAttribution: danieltome commentedHere 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
Comment #24
lilou CreditAttribution: lilou commentedComment #25
webchickComment #26
stBorchertRe-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:
Comment #27
swentel CreditAttribution: swentel commentedImpressive! patch applies cleanly. Ran some tests (use the php devel block) chaning some variables, works great!
Comment #28
JohnForsythe CreditAttribution: JohnForsythe commentedOne 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.
Comment #29
stBorchertIn my opinion thats another issue.
Pager limits aren't hardcoded for search results only but for nearly *all* listings within drupal core.
Comment #30
JohnForsythe CreditAttribution: JohnForsythe commentedThat'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.
Comment #31
stBorchertFor 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?
Comment #32
JohnForsythe CreditAttribution: JohnForsythe commentedIt'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:
becomes:
Now modules can specify their own limit, or rely on the variables you've created. Best of both worlds.
Comment #33
stBorchertThe extra parameter is a very good idea.
I will extend the patch to use
$limit
wherever possible.Comment #34
stBorchertHm, 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.Comment #35
stBorchertBtw: I've created a small module that provides an UI to set the pager limits. See attached screenshot.
Comment #36
JohnForsythe CreditAttribution: JohnForsythe commentedThe 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.
Comment #38
stBorcherttracker.pages.inc has changed (now using PDO instead of plain SQL) so the old patch didn't apply anymore. Here is a re-roll.
Comment #39
stBorchert@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.Comment #40
Dries CreditAttribution: Dries commentedWith 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.
Comment #41
stBorchertWell, 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 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 :-).Comment #42
stBorchertI'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
but none of them worked.
->condition
worked but not->limit
.Any hints?
Comment #43
BerdirWe 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.
Comment #44
BerdirComment #45
Crell CreditAttribution: Crell commentedHm, 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?
Comment #46
moshe weitzman CreditAttribution: moshe weitzman commented@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.
Comment #47
webchickIncidentally, 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."
Comment #48
Crell CreditAttribution: Crell commented@ 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. :-)
Comment #49
webchickUm. Wow! That was a reply to the completely wrong issue. :D But um. Yay?
Comment #50
webchickThis issue might very well need documentation too, but it wasn't my intent to change it. :P
Comment #51
mitchell CreditAttribution: mitchell commented+1 for doing this in views and merging it in #286665: SEE ISSUE #1805996 -- Moving Views into Drupal Core
Comment #52
stBorchertRe-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:
Note: this didn't have any effect until #496516: Move query_alter() into a preExecute() method is fixed.
Comment #53
stBorchertJust 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.
Comment #54
BerdirThis 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.
Comment #55
Crell CreditAttribution: Crell commentedI'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.
Comment #56
stBorchertThey 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.
Comment #58
stBorchertRe-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).Comment #59
BerdirI 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.
Comment #60
mattyoung CreditAttribution: mattyoung commentedsubscribe
Comment #62
stBorchertRe-roll
Comment #63
stBorchertLast chance to get this in or send it to hell (at least until d8-development is started).
Comment #64
Kars-T CreditAttribution: Kars-T commentedI 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.
Comment #66
stBorchertReroll 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.Comment #67
Kars-T CreditAttribution: Kars-T commentedI again got me D7 HEAD and patched it.
Tried this code:
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:
The tag for the pager is the name of the function which I think is wise and relatively easy to understand.
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.
Comment #68
stBorchertfunction 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.
Comment #69
Kars-T CreditAttribution: Kars-T commentedI 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.
Comment #71
stBorchertEhm, bot? Patch applies cleanly without any errors.
Don't know why he suddenly complains about blog.pages.inc.
Comment #72
Kars-T CreditAttribution: Kars-T commentedTestbot hiccup. I'm setting this to reviewed again.
Comment #73
webchickHm. 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.
Comment #74
Crell CreditAttribution: Crell commentedWhile #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.
Comment #75
stBorchertThe 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.
Comment #76
Kars-T CreditAttribution: Kars-T commentedSad 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.
Comment #78
donquixote CreditAttribution: donquixote commentedCrell / #45:
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!
Comment #79
Crell CreditAttribution: Crell commentedGiven 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.)
Comment #80
donquixote CreditAttribution: donquixote commentedWe 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.
Comment #81
donquixote CreditAttribution: donquixote commentedAnd 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.
Comment #82
donquixote CreditAttribution: donquixote commentedRemoved some cruft from debug and testing.
Comment #83
stBorchertSee #33809-47: Make pagers not suck for a clean way on how to build a new pager system ...
Comment #84
donquixote CreditAttribution: donquixote commentedYeah, 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).
Yes :)
Throw in the link builder object, and we are getting close.
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().
Comment #85
Crell CreditAttribution: Crell commented@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.
Comment #86
donquixote CreditAttribution: donquixote commentedTrue, but a contrib module could help to prepare a D8 patch, don't you think?
Comment #87
catchDowngrading all D8 criticals to major per http://drupal.org/node/45111
Comment #88
joachim CreditAttribution: joachim commentedSubscribing 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.
Comment #89
mortendk CreditAttribution: mortendk commenteda little dream was if we could end up with markup like this
Comment #90
mgiffordWhat happened to this issue? Been quite now for almost a year now. Also related issue #115753: Pager rel attributes
Comment #91
salvisThe 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.
Comment #92
giorgio79 CreditAttribution: giorgio79 commentedAll admin lists will switch to Views, so I guess this is a no fix?
Comment #93
Crell CreditAttribution: Crell commentedNo, the pager API itself is still crap. It needs to rely on the request, not globals. Using Views doesn't help that at all.
Comment #94
donquixote CreditAttribution: donquixote commentedI'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.
Comment #95
Crell CreditAttribution: Crell commenteddon: 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. :-)
Comment #96
donquixote CreditAttribution: donquixote commentedOne 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.
Comment #97
catchHow is this a feature?
Comment #98
YesCT CreditAttribution: YesCT commentedIs 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)
Comment #99
mgiffordLast patch from Sept 2009 so this definitely needs a re-roll.
Comment #100
dawehnerIt seems to be that a proper extending system in Drupal 8 should more look something like the following:
Some advantages:
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.
Comment #101
Crell CreditAttribution: Crell commentedI'm not sold on the exact syntax, but extenders as services gets a ++ from me as a concept.
Comment #102
tim.plunkett:(
Comment #103
Fabianx CreditAttribution: Fabianx commentedI 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 ...
Comment #104
klonosI 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).
Comment #105
andypostLooks 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
Comment #106
tim.plunkettThis is not truly major, not at this stage of D8.
Comment #115
joseph.olstadComment #116
jcisio CreditAttribution: jcisio as a volunteer and at Axess Open Web Services commentedI 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.
Comment #117
joseph.olstad#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.