Convert theme_views_mini_pager to twig

Mh, this would make it actually worse to theme, mhh.

+++ b/core/modules/views/views.theme.inc
@@ -1098,6 +1098,70 @@ function theme_views_mini_pager($vars) {
+function template_preprocess_views_mini_pager(&$vars) {

I was instructed that the variable passed by reference should be changed for consistency to:

function template_preprocess_views_mini_pager(&$variables) {

The last patch didn;t remove the previous theme function, as well as the item list render array being incorrect.

See how this gets on.

Sorry, also the last patch changed the output of the pager too, if we are converting, I don't think we should change too much what the mini pager is doing. Interdiff from last patch.

Nice job @damiankloip getting that to pass on this one. I have a few questions and nitpiky little things.

+++ b/core/modules/views/views.theme.inc
@@ -1036,23 +1036,24 @@ function theme_views_form_views_form($variables) {
 
-  // Current is the page we are currently paged to.
+  // current is the page we are currently paged to

Should be keeping the period at the end.

+++ b/core/modules/views/views.theme.inc
@@ -1086,13 +1087,13 @@ function theme_views_mini_pager($vars) {
+  $vars['items'] = $items;

We could probably replace $vars with $variables for consistency here, no? I've been doing that on these functions. @bzitzow mentioned this above as well.

+++ b/core/modules/views/views.theme.inc
@@ -1036,23 +1036,24 @@ function theme_views_form_views_form($variables) {
   if ($pager_total[$element] > 1 && $pager_page_array[$element] > 0) {
     $li_previous = array(
       '#theme' => 'pager_link__previous',
       '#text' => (isset($tags[1]) ? $tags[1] : t('‹‹')),
-      '#attributes' => array('title' => t('Go to previous page')),
       '#page_new' => pager_load_array($pager_page_array[$element] - 1, $element, $pager_page_array),
       '#element' => $element,
       '#interval' => 1,
@@ -1065,7 +1066,6 @@ function theme_views_mini_pager($vars) {

@@ -1065,7 +1066,6 @@ function theme_views_mini_pager($vars) {
     $li_next = array(
       '#theme' => 'pager_link__next',
       '#text' => (isset($tags[3]) ? $tags[3] : t('››')),
-      '#attributes' => array('title' => t('Go to next page')),
       '#page_new' => pager_load_array($pager_page_array[$element] + 1, $element, $pager_page_array),
       '#element' => $element,
       '#interval' => 1,
@@ -1073,6 +1073,7 @@ function theme_views_mini_pager($vars) {

Why are we removing the title attributes here?

Why are we removing the title attributes here?

Ask the last patch author, The patch I fixed was from that :) Looks like they should probably stay though.

Here's an updated patch, thought I might as well quickly reroll it.

Thanks for the review.

Valid php is usually good...

+++ b/core/modules/views/views.theme.incundefined
@@ -1036,16 +1036,18 @@ function theme_views_form_views_form($variables) {
+/**
+ * Display a views mini-pager.
+ */

This doesn't display anything, it just implements preprocess.

Tagging.

+/**
+ * Display a views mini-pager.
+ */

Should be "Prepares variables for a Views mini-pager."

+ * - items: An array of pager elements.

We're not talking about PHP data types in Twig templates.

For the preprocess you also need:

Default template: views-mini-pager.html.twig.

and an @param for the variables.

Thanks @thedavidmeister! Tagging the tweaks in #17 and #18 as a novice task, with only one correction for the first item mentioned in #17:

Per #1913208: [policy] Standardize template preprocess function documentation preprocess function names should end in 'templates', so maybe 'Prepares variables for Views mini-pager templates.'.

@ronnienorwood just jumping on this to get it done. You are welcome to do more doc cleanup or manual testing to help get this in. Or if you would like another like this, I'm sure I could find one for you, just jump on IRC #drupal-twig and ping me.

hmm, I really got to watch out for that save button...

Goal was to manually test that the mini pager is using twig template

How I tested

• test mini pager first
• reviewed patch
• installed patch

Verified the mini pager patch using twig was being used by adding a test string in the twig file

Tested as part of Florida Drupal Camp 2013

To just print the pager seems to be not really useful for theming. I'm wondering whether #1898474: pager.inc - Convert theme_ functions to Twig would improve the situation? In general it seems to be useful to generalize as much as possible from the normal pager.

@dawehner - the goal for now is to convert everything, and then consolidate later when it makes sense. Converting and consolidating simultaneously would be too much. See #1804614: [meta] Consolidate theme functions and properly use theme suggestions in core.

The minipage is not displayed at all.
- I added 3 nodes
- On admin/structure/views/view/frontpage/edit
- Set pager side to 1
- Full pager is visible
- Mini pages is not. There is no code in the html according to

$ diff --suppress-common-lines --side-by-side /tmp/a /tmp/b | less
    <div class="view view-frontpage view-id-frontpage view-di |     <div class="view view-frontpage view-id-frontpage view-di
                                                              |       <div class="item-list"><ul class="pager"><li class="pag
<input type="hidden" name="form_build_id" value="form-RTfs6wt | <input type="hidden" name="form_build_id" value="form-pMdwLNk

where /tmp/a (patch applied) and /tmp/b clean 8.x

Unlike clemens.tolboom manual test, for me #23 worked. I did the same as karang did. Template is used properly

#28 - did you clear the cache after applying the patch? this is required to make the new template appear.

@thedavidmeister: tnx ... it was a cache clear :-/ ... sorry

I did not manage to move forward get past page 2: http://drupal.d8/node?page=1
And from http://drupal.d8/node?page=3 cannot move previous (http://drupal.d8/node?page=2) moves immediately to first page. Has this to do with #1625248: Mini Pager ("tags") are broken?

I have a pager size of 1 and 4 nodes so I expected good rendering on:
- http://drupal.d8/node
- http://drupal.d8/node?page=0
- http://drupal.d8/node?page=1 : No previous link
- http://drupal.d8/node?page=2 : No previous link
- http://drupal.d8/node?page=3

Reported #1998330: Minipager is broken on page size == 1 due to ceil(PHP_MAX_INT / 1) mentioned in #32

@clemens.tolboom I belief this is related to #1898474: pager.inc - Convert theme_ functions to Twig, like dawehner mentioned in #26. I remember that the first time I tested the patch, I had the same behavior you described. Then I applyed the patch from #1898474: pager.inc - Convert theme_ functions to Twig and the pager worked for me, maybe you'll give this a try.

The code I refer to in #1998330: Minipager is broken on page size == 1 due to ceil(PHP_MAX_INT / 1) comes from #1901290: Replace the mini pager with a lite pager (which does not run a count query) which is rather new Feb 18, 2013.

#23: 1912604-22-twig-views-mini-pager.patch queued for re-testing.

Number #23 doesn't apply any longer.
Here's a re-roll and some doc cleanup.

#38: 1912604-12-twig-views-mini-pager.patch queued for re-testing.

#38 needed a re-roll after #1898474: pager.inc - Convert theme_ functions to Twig. Attached.

However, what's the purpose to have a Twig template like that here? Can't we just re-use pager.html.twig, and pass $variables['items'] instead of $variables['pager']?

#41: 1912604-41-twig-views-mini-pager.patch queued for re-testing.

Needs reroll after #1963942: Change all instances of $vars to $variables got committed.

re-rolled from #41

+++ b/core/modules/views/templates/views-mini-pager.html.twigundefined
@@ -0,0 +1,16 @@
+ * @see template_preprocess()

We no longer need this

Thanks @mondrake.

#47: 1912604-48-twig-views-mini-pager.patch queued for re-testing.

Needed re-roll, see comment #15 in #2049723: Mini pager shows "Page 1" if there is non item at all available.

#50 failed because now we need to take into account, in the Twig template, that the preprocess may no longer return the pager items. Patch attached - to align with the main pager I also:

• changed the name of the variable changed by preprocess from 'pager' to 'items'
• changed the template to include the visually hidden 'Pages' text

While testing this patch I spotted #2055993: Ajax for mini pagers in the preview does not work but it also happens on HEAD, so this is fine.

This patch itself is totally fine.

#55 - this is broader than the mini pager, in fact occurs any time any 'navigational' link/button is clicked (any link/button that requires an update to the preview itself). See #2048309: Views UI Preview - navigation is broken, there is a patch there that longs for review :)

+++ b/core/modules/views/templates/views-mini-pager.html.twigundefined
@@ -0,0 +1,17 @@
+  <h2 class="visually-hidden">{{ 'Pages'|t }}</h2>

Why are we adding this in this issue with no discussion of accessibility? This should be debated in another issue and this issue should be the straight conversion to Twig.

Also no profiling has been done.

Agreed - we should not be changing markup in this conversion. We can look at consolidating with the main pager template afterwards.

Ah sorry, just thought that copying/pasting from the main pager template would be ok.

... also created follow up #2058279: Align mini pager accessibility markup to full pager..

Nice!

Needs profiling still, I can work on it.

Profiling looks great and I also did a markup comparison and it matches up perfectly.

Profiling results

=== 8.x..8.x compared (5204417bc7ab7..5204421e7a2a2):

ct  : 77,828|77,828|0|0.0%
wt  : 375,892|375,026|-866|-0.2%
cpu : 337,380|335,475|-1,905|-0.6%
mu  : 19,233,144|19,233,144|0|0.0%
pmu : 19,260,448|19,260,448|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5204417bc7ab7&...

=== 8.x..views-mini-pager-1912604-59 compared (5204417bc7ab7..5204425923c81):

ct  : 77,828|77,910|82|0.1%
wt  : 375,892|375,971|79|0.0%
cpu : 337,380|338,161|781|0.2%
mu  : 19,233,144|19,280,944|47,800|0.2%
pmu : 19,260,448|19,308,296|47,848|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5204417bc7ab7&...

Markup comparison

Before:

<div class="item-list"><ul class="pager"><li class="pager-previous odd first"><a href="/node?page=1" title="Go to previous page" rel="prev">‹ previous</a></li><li class="pager-current even">Page 3</li><li class="pager-next odd last"></li></ul></div>

After:

<div class="item-list"><ul class="pager"><li class="pager-previous odd first"><a href="/node?page=1" title="Go to previous page" rel="prev">‹ previous</a></li><li class="pager-current even">Page 3</li><li class="pager-next odd last"></li></ul></div>

I should add, the scenario was the default homepage view set to use the mini pager. I added 50 nodes via devel generate to get the pager to show up.

This looks good, but unfortunately no longer applies.

Had a branch from the profiling so here is a quick reroll.

Reroll was needed because of #2062315: Remove unnecessary 'pattern' lines in views_theme(). Back to RTBC.

Committed 88b192c and pushed to 8.x. Thanks!