Merge theme_pager_link*() theme functions into theme_link()

once #1598886: Clean up pager theme functions lands I'll take this on

When 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.

Related #1805678: Expand theme_pager_link() so it can accept html input or specify invisible elements

note @_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 :)

Hi 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

@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

Tagging for Twig so our sprinters can help you SLAY this Undocumented Insanity™.

changing 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.

I 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...

No, 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. :)

#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.

Wow, 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.

Now 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.

Re-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.

--- a/core/includes/theme.inc
+++ b/core/includes/theme.incundefined

@@ -1645,7 +1645,8 @@ function theme_status_messages($variables) {
  * @see l()
  */
 function theme_link($variables) {
-  return '<a href="' . check_plain(url($variables['path'], $variables['options'])) . '"' . new Attribute($variables['options']['attributes']) . '>' . ($variables['options']['html'] ? $variables['text'] : check_plain($variables['text'])) . '</a>';
+  $html = !empty($variables['options']['html']);
+  return '<a href="' . check_plain(url($variables['path'], $variables['options'])) . '"' . new Attribute($variables['options']['attributes']) . '>' . ($html ? $variables['text'] : check_plain($variables['text'])) . '</a>';
 }
 

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.

--- a/core/modules/node/node.admin.inc
+++ b/core/modules/node/node.admin.incundefined

@@ -605,7 +605,7 @@ function node_admin_nodes() {
     );
   }
 
-  $form['pager'] = array('#markup' => theme('pager'));
+  $form['pager'] = array('#theme' => 'pager');
   return $form;

This is already in, via #80855: Add element #type table and merge tableselect/tabledrag into it.

Ah, 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.

Updated issue summary.

Isn'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

Oops

So..... 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

• a) in the preprocess_ , render each $item, as it is added to $variables.
  

  // Eg, in template_preprocess_pager()
  $variables['last'] = drupal_render(array(
    '#theme' => 'link__pager__last',
    [...]
  ));
  

• b) in the theme_ function, render each $item as it is added add it as #markup.
  

  // Eg, in theme_pager()
  $items[] = array(
    '#wrapper_attributes' => array('class' => array('pager-last')),
    '#markup' => drupal_render($variables['last']),
  );
  

• c) in the theme_ function, don't use #markup at all, just merge each $item render array in with it's #wrapper_attributes, and let the single call to theme('item_list__pager') do all the recursive work.
  

  // Eg, in theme_pager()
  $items[] = array_merge($variables['last'], array('#wrapper_attributes' =>  array('class' => array('pager-last'))));
  

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.

I'd love some validation on #31, before doing another re-roll using method C above.

Its failing on views mini pagers.

Just 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... :(

I doubt that in the long term options B and C wil do

Why 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?

Twig 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.

Thanks 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.

Very strange... I am sure I didn't touch tags in #35. Re-adding.

Blasted!

The 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...

// from core/modules/views/lib/Drupal/views/Plugin/views/pager/Full.php
   85      // The 0, 1, 3, 4 index are correct. See theme_pager documentation.
   86      $tags = array(
   87:       0 => $this->options['tags']['first'],
   88:       1 => $this->options['tags']['previous'],
   89:       3 => $this->options['tags']['next'],
   90:       4 => $this->options['tags']['last'],
   91      );

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.

t('') is useless. #facepalm

This should finally fix the views mini pagers issues noted above on #34.

EXPERIMENTAL 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:

//before
$variable['less'] = '…';
//after
$variable['less'] = array('#markup' => '…');

(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.

+++ b/core/includes/pager.incundefined
@@ -191,193 +205,245 @@ function theme_pager($variables) {
+      '#markup' => '…',

Can someone remind me again why we dont use … ?

Good question, maybe its for same reason we don't use » « ” and “; ie, for consistency (??)

This 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.

Elaborating 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

  $variables['first'] = NULL;
  $variables['previous'] = NULL;
  $variables['next'] = NULL;
  $variables['last'] = NULL;

  $variables['less'] = FALSE;
  $variables['more'] = FALSE;

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?

The 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, or
2) 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.

It 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?

:-)

Add references to more recent related issues.