Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Pager links are just links, no need for separate theme functions.
Their specialized use-case can be indicated through theme function sub-patterns; e.g., 'link__pager__first'
.
Blocked by
- #1598886: Clean up pager theme functions
- #1410574: Make l() respect the URL query string before adding the 'active' CSS class
- #891112: Replace theme_item_list()'s 'data' items with render elements
Related to
Comment | File | Size | Author |
---|---|---|---|
#46 | theme.pager_.46.patch | 21.52 KB | jwilson3 |
#46 | interdiff.txt | 0 bytes | jwilson3 |
#50 | interdiff.txt | 719 bytes | jwilson3 |
#50 | theme.pager_.50.patch | 21.59 KB | jwilson3 |
#45 | theme.pager_.45.patch | 21.85 KB | jwilson3 |
Comments
Comment #1
bleen CreditAttribution: bleen commentedonce #1598886: Clean up pager theme functions lands I'll take this on
Comment #2
mondrakeWhen this takes off, please take a look at #1588138: pager_query_add_page() [in D7, theme_pager_link()] overrides parameters passed programmatically also. Thanks
Edit:
same issue reported also in #1786724: theme_pager does not always respect parameters option while generating pager links.
Comment #3
larowlanRelated #1805678: Expand theme_pager_link() so it can accept html input or specify invisible elements
Comment #4
sunThere are some dependencies here, so I had to start deeper down in the rabbit hole:
- #891112: Support render elements for theme_item_list().
Moved heavy theme variable processing into new template_preprocess_pager().
Comment #6
sunNow on to the guts...
Comment #7
sunIncorporated #1805678: Expand theme_pager_link() so it can accept html input or specify invisible elements to avoid needless conflicts.
Comment #8
larowlannote @_wdm_ worked on the change over in #1805678: Expand theme_pager_link() so it can accept html input or specify invisible elements so lets make sure he gets some commit credits :)
Comment #10
sunThis patch is blocked on #1410574: Make l() respect the URL query string before adding the 'active' CSS class, which looks RTBC to me.
With that, the entire theme_pager_link() function will cease to exist, so #1805678: Expand theme_pager_link() so it can accept html input or specify invisible elements is completely obsolete.
Done so:
Fixed template_preprocess_search_results() passes bogus 'tags' variable to theme_pager().
Fixed Node and User admin pages call theme_pager() instead of assigning #theme.
Incorporated essential fix of #1805678: Expand theme_pager_link() so it can accept html input or specify invisible elements
Eliminated theme_pager_link().
Comment #11
mondrakeHi sun,
did you have a chance to review #1588138: pager_query_add_page() [in D7, theme_pager_link()] overrides parameters passed programmatically as per #2 above?
At first glance it looks like the order in the array_merge is left unchanged. Thanks
Comment #12
sun@mondrake: That's an independent bug and won't be touched, changed, or fixed here.
Fact is, this patch only moves code around. Likewise, the code logic that is going on in these pager functions is totally, completely beyond me - I'm not able to understand what the current non-theme functions are doing and I was very tempted to add documentation to the pager API functions, but did not do so, since that is an entirely different can of worms.
Just have a look at this nonsense. Ranks pretty high on my Top 10 List of Undocumented Insanity™. Cleaning up that horrible mess is left for another issue.
Comment #13
mondrake@sun: got it. Pity. But I like your concept of independent bugs :), gives the idea of life going on...
So I'll wait for this to be committed and re-roll a patch for #1588138: pager_query_add_page() [in D7, theme_pager_link()] overrides parameters passed programmatically afterwards, hoping some other exterminator will eventually pick it up...
Cheers
Comment #14
jenlamptonTagging for Twig so our sprinters can help you SLAY this Undocumented Insanity™.
Comment #15
sunThe first dependency landed, #1410574: Make l() respect the URL query string before adding the 'active' CSS class
Merged and re-rolled for that.
We're still blocked on #891112: Replace theme_item_list()'s 'data' items with render elements
Comment #16
jenlamptonchanging status and updating title.
see #1778610: Remove the check for a link template from l(), have l() always output just a string.
If we're unable to use l() in pagers because l() doesn't work with query strings on pagers, we should fix l(). We should not invent a new theme function as a work-around.
Comment #17
sunSorry, I don't really get that change in direction.
I had many situations in the past, in which had no need to override the entire pager theme function, but only wanted to output customized first/previous/next/last links, and didn't want to deal with the whole pager theming.
By consolidating those links into
theme_link()
by leveraging theme suggestions, that focused override is still possible. If we replace all the links with straight calls tol()
, you will have to override the entiretheme_pager()
.Comment #18
bleen CreditAttribution: bleen commentedI agree with @sun here 100% ... the theme_link function (as it will be used by pager) provides a lot more flexibility. Not all links are created equal...
Comment #19
jenlamptonNo, not all links are created equal, but they do all have one thing in common: none of them need to go through the theme layer.
We don't need a theme_link(), since it will never be the case that a front-end developer needs to change every single link on a site. We don't have a theme_paragraph function or a theme_blockquote for a reason, and we shouldn't have a theme_link function for the same reason.
We just need to be smarter about where and how the contents of links can be overridden (I agree that we always need to leave front-end devs the ability to override this markup). But, so far, in every example I've come across in core, the links are (or can be) exposed to the front-end dev at a higher template level. Whatever template has the link in it can change it there, we don't need to change it in a link template.
I don't think the entire theme_pager will need to be overridden if we can add theme_hook_suggestions for pager__first, etc.
Please see #1778610: Remove the check for a link template from l(), have l() always output just a string.
and #1833920: [META] Markup Utility Functions
and #1751194: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter()
For the record, I think the approach in the patch above is much, much better than what we currently have and I'm all for things getting better a little bit at a time. Would you prefer that I leave this issue alone and open a new one for replacing theme_link() with l() in respect to pagers? I'd be happy to do that so this can get in. :)
Comment #20
Fabianx CreditAttribution: Fabianx commented#19: Lets continue the cleanup till we find a good solution for using l() or a Link component.
This is currently just creating confusion, but not helping getter cleaner markup.
Need to give this a review as well.
Comment #21
sunThanks for reviewing, but we're still blocked on #891112: Replace theme_item_list()'s 'data' items with render elements
Comment #22
sunComment #23
Fabianx CreditAttribution: Fabianx commentedWow, this is so perfectly matching what I am currently working on as followup from #891112: Replace theme_item_list()'s 'data' items with render elements.
Looks very much RTBC to me. Will give this a proper review later if no one beats me to it.
Comment #25
mgiffordNow that #891112: Replace theme_item_list()'s 'data' items with render elements what else should be addressed by this issue?
@Fabianx was almost ready to mark it RTBC in November, but the patch failed shortly after that.
I expect it will need a re-roll now.
Comment #26
jwilson3Re-roll, removes some snippets from #22 that are already into core, sorry, no interdiff.txt because I couldn't get the original patch to apply, had to apply by hand.
This was no longer needed since theme_link can now handle rendering HTML, done with #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig.
This is already in, via #80855: Add element #type table and merge tableselect/tabledrag into it.
Comment #27
jwilson3Ah, patchutils is nice.
Here's another re-roll to clear up some additional whitespace accidentally introduced. (detected only after seeing the interdiff from #22 - #26 ;)
The interdiff here is between #22 and #27, but it cant really be trusted too much... some of the changes are just offset differences.
Comment #27.0
jwilson3Updated issue summary.
Comment #29
mondrakeIsn't this a duplicate of #1898474: pager.inc - Convert theme_ functions to Twig at this stage?EDIT: actually not, this is about removing theme_pager_link
Comment #30
mondrakeOops
Comment #31
jwilson3So..... this fails because a renderable array doesn't make sense as the value of '#markup'. This brings up the question of *when and how* should we render each individual link__pager.
There are three possible places
For this re-roll I chose B, because it took the least amount of code changes (see interdiff), though, now that I write this out, not entirely sure B is the right way to go.
I think C makes the most sense because it then allows a themer to simply implement template_item_list__pager() or template_preprocess_item_list__pager() to change the entire structure including the un-rendered sub-elements.
Edit: fixed code snippet in example A.
Comment #32
jwilson3I'd love some validation on #31, before doing another re-roll using method C above.
Comment #34
jwilson3Its failing on views mini pagers.
Comment #35
mondrakeJust note that #1898474: pager.inc - Convert theme_ functions to Twig is due to remove theme_ functions in pager.inc overall, and replace with Twig templates. So I doubt that in the long term options B and C will do... :(
Comment #36
jwilson3Why exactly?
I thought the whole purpose of moving to twig was to be able to just do
{{ mythingy }}
and the twig back end would be able to handle the rendering of whatever object, array, or simple type that thingy may be. Does this exclude render arrays?Comment #37
star-szrTwig can indeed print render arrays, and that's what is recommended to pass to templates for performance reasons. See http://drupal.org/node/1920746 for more information. However, at this time the fate of theme_ functions is uncertain, mainly due to performance of theme functions vs. templates (PHPTemplate or Twig). See #1986116: Improve performance by replacing very small and simple templates and theme function with "Markup Utility Functions".
Short answer: from my perspective I like C best. Postponing rendering makes things much more flexible and alterable along the way.
Comment #38
jwilson3Thanks Cottser... the Twig best practices page was just what I needed to know, and it says everything.
Strange I hadn't see that one before, so I linked to them also on some of the twig meta issues.
going to re-roll this using method C in comment #31.
Comment #39
mondrakeVery strange... I am sure I didn't touch tags in #35. Re-adding.
Comment #40
jwilson3Comment #41
jwilson3Blasted!
Comment #43
jwilson3The pager "tags" (the text used for various pagers, next,prev,first,last, etc) has 5 elements -- not four -- and at the very least, views explicitly uses that and has code that explicitly explains this...
and who knows how many other contrib modules are using this... so in order not to break the API, I suppose we should keep five tags.
Part of this was fixed in #38, but the other part of it is here too, cause i missed a piece.
Comment #44
jwilson3t('') is useless. #facepalm
Comment #45
jwilson3This should finally fix the views mini pagers issues noted above on #34.
Comment #46
jwilson3EXPERIMENTAL PATCH:
So, doing method C (from comment #31) essentially just wraps in some default classes to the existing render arrays. And what's more, the code is overly complex because only *some* of the elements are render arrays (eg 'first', 'previous', 'item', 'next', and 'last') while *others* are just simple text markup (eg, 'less', 'more', and the 'item' of the current page). This means our template preprocess is too tightly coupled to the default theme implementation and a lot of the code could be simplified if we just said that all of the items are render arrays.
The ones that were just plain text are converted to render arrays in the preprocess like this:
(Edit: I've now coverted example code ^ to use a proper ellipse '…' just like the actual code does ;)
This makes everything way more consistent, the code more concise, and easier to modify for themes implementing their own preprocess functions.
If and when the theme_ function gets ported into a twig template (see the pagination dreammarkup issue for more on that), we're going to likely need to swap out all of this code anyway, so i'd just as soon go "all in" with this issue, in the direction of doing a proper theme_link() implementation.
If this isn't a good idea we should be able to start again from the patch in #45. Let me know what you think.
Comment #48
bleen CreditAttribution: bleen commentedCan someone remind me again why we dont use
…
?Comment #49
jwilson3Good question, maybe its for same reason we don't use
» « ” and “
; ie, for consistency (??)Comment #50
jwilson3This should fix the failing tests from #46 (forgot to copy a line for the "pager-item" wrapper class for pages listed after the current page.
Comment #51
jwilson3Comment #52
mondrakeElaborating on #50, and taking cues from #1898474: pager.inc - Convert theme_ functions to Twig, would it make sense to move the entire logic of preparation of the render array to the template_preprocess_pager function, and leave just the final call to theme('item_list__pager',...) in the theme_pager function?
This way we could avoid passing through variables like
also, allow for simpler replacement of theme_pager if this is finally moved to Twig, and allow contrib modules to override any element of the pager.
Thoughts?
Comment #53
jwilson3The
items
array is currently prepared by the preprocess to be the list of pages keyed by page number. To use the theme('item_list__pager') we would need to prepare an array of all comprehensive items for the list (which is what the theme_ function currently serves to do for us). So to move this to the preprocess, we'd need to either:1) re-purpose the
items
element prepared by the preprocess as it currently works, or2) come up with another array that produces a mixture of keys: 'first','prev','less',0,1,2,3,4,5,6,7,8,9,'more','next','last' (anyone know if its possible to do an array with mixed keys like this, or is it just setting us up for problems?
I'm not sure which of these two ways would be best.
Comment #54
jwilson3It appears that theme_link() is being removed from Drupal 8: #1985470: Remove theme_link()
Talk about pulling the rug right out from under me. Totally frustrating.
If anyone is interested in following, we're discussing what to do about this on the sister issue, #1898474: pager.inc - Convert theme_ functions to Twig where Mondrake recently tried to incorporate the code from this issue, but also got bitten. :-/
I suppose, we can hang this one up to dry now?
Comment #55
mondrake:-)
Comment #55.0
mondrakeAdd references to more recent related issues.