Pager "first" and "previous" links have incorrect URLs

My suggestion for tests (test-only == interdiff)

I would prefer to check the page query string argument specifically as this doesn't seem to be covered in any other tests

Fully makes sense.

+++ b/core/modules/system/src/Tests/Pager/PagerTest.php
@@ -118,20 +118,27 @@ protected function assertPagerItems($current_page) {
     if ($current_page != count($elements)) {
       $last = array_pop($elements);
       $next = array_pop($elements);
+      $total_pages = count($elements);
     }

Just wondering why $total_pages = count($elements); inside the if branch? I see it won't be needed on a last page, but still I think it would be more readable if it's after the two if branches that remove the first/previous/next/last elements. A comment would also help.

#11 shouldn't it be after the two if blocks?

Also changed title and component since this issue is about the core pager.

#14 thanks @mikeker.

Code and tests look good to me, only the comment is now misplaced

+++ b/core/modules/system/src/Tests/Pager/PagerTest.php
@@ -120,18 +120,28 @@ protected function assertPagerItems($current_page) {
+    // We remove elements from the $elements array in the following code, so
+    // we store the total number of pages for verifying the "last" link.
+    $total_pages = count($elements);
+

I'd suggest "We have removed first/previous/next/last elements from the $elements array, so the total number of pages in the pager, to verify the "last" link, is given by the count of the remaining elements."

+++ b/core/includes/pager.inc
@@ -308,8 +308,9 @@ function pager_query_add_page(array $query, $element, $index) {
-  if ($new_page = implode(',', pager_load_array($page_new[$element], $element, explode(',', $page)))) {
-    $query['page'] = $new_page;
+  $new_page = pager_load_array($page_new[$element], $element, explode(',', $page));
+  if (!empty($new_page)) {
+    $query['page'] = implode(',', $new_page);

Can you explain why this fixes the problem?

#16:

Because pager_load_array($page_new[$element], $element, explode(',', $page)) returns an array [0 => 0] for the first page, which once imploded is converted to '0' string. This is associated to $new_page. But '0' fails the if condition and $query['page'] = $new_page; is not called. This leads to a blank querystring (and URL link) that browsers interpret as a request to reload current page.

With the patch, the test is on the value returned by pager_load_array() which is a not empty array (there's an element in it), and implosion to string is deferred to the assignment to $query['page']. So URL generated is ?page=0 which is what we need.

EDIT: maybe we do not need the if at all here anymore. It looks like it was useful when we had full URLs in the link, so that link to first page was 'special' in the sense that no querystring was needed, just the base URL. But now we have only querystring, and we need explicit ?page=x URL for all links.

Thanks for that explanation in #17! :)

However, that code was not changed in #2351015: URL generation does not bubble cache contexts. In fact, it has not been changed in a *long* time (in no small part due to that @todo). So there must be a subtle interaction with #2351015 that caused this problem. Do we know what exactly?

Or am I missing something?

What we changed in #2351015: URL generation does not bubble cache contexts was that the pager links had been from absolute / relative urls, which would have implied a full 'url' cache context to just the query parameters on whatever page the view is currently implemented.

#19: the (not so) subtle interaction with #2351015: URL generation does not bubble cache contexts is that before it, we were always passing '<current>' to the URL generator whereas now we pass '<none>' in most of the cases. The href in the pager link is built, now like before, by url + querystring. The effect of <none> + blank querystring for page 0 in HEAD now is a blank href, so when clicking the browser just reloads the current page, not page 0. The patch here produces href ?page=0 which is what we need. The tests are a good addition also because we were not actually testing the href of the pager links, and a good reason for missing to spot this bug in the first place.

#18:

I believe the comment is still valid: we remove elements from $elements at line 146: unset($elements[--$page]); and then verify that there are no remaining items left to ensure we don't have too many pager links.

oh yeah I missed that, thanks.

But I figured it was better to be cautious as this code is a little funky

indeed, let's not touch that here.

I think I can set this to RTBC, my actual coding here is very limited. The patch fixes the bug and introduces useful tests. Also tested manually.

Ahhh!

So we went from href="/foo/bar" to href="" for the first page. So the link to the first page doesn't actually do anything anymore.

Nice additional text coverage.

  $page_new = pager_load_array($index, $element, $pager_page_array);

  $page = \Drupal::request()->query->get('page', '');
  $new_page = pager_load_array($page_new[$element], $element, explode(',', $page));

$page_new and $new_page - really? Also do we think that we could improve the documentation in the method as to what is actually going on.

I see the need for the rewrite, but in the meantime (I doubt it can fit 8.0 now), maybe we can clean up a bit pager_query_add_page, rather than trying to document the as-is (that is pretty 'funky' as you said :)).

This patch is taking a bit of #2044435: Convert pager.inc to a service, adds documentation, and removes pager_load_array (grepping the code base I see it's only used in pager_query_add_page).

1. +++ b/core/includes/pager.inc
   @@ -285,32 +285,38 @@ function template_preprocess_pager(&$variables) {
   + *   The index of the target page, for the given element, in the pager
   

   Please fill up the comments to a maximum.

2. +++ b/core/includes/pager.inc
   @@ -285,32 +285,38 @@ function template_preprocess_pager(&$variables) {
   +  // Build the 'page' query parameter.
   

   Please try to write the full comment as one line.
   Also, this comment block is a bit unclear to me, maybe this can be simplified

3. +++ b/core/modules/system/src/Tests/Pager/PagerTest.php
   @@ -120,18 +120,28 @@ protected function assertPagerItems($current_page) {
   +    // We remove elements from the $elements array in the following code, so
   

   Please fill up the comments to a maximum.

Extra notes:
It might be a more clean solution to remove the ?page=0 when the pager is "not used".

Suggestion:
Attached you can find an alternative fix, I see a few comments regarding this change, but it might have been overlooked.

Messy or no, patch in #28 works great for me.

Edit: Too much coffee, not enough brain.

I have looked over the patch -- every thing is in order...

The documentation fixes are appropriate
The code changes fix the problem.
The tests are a simple elegant extension that will catch any future problems.

In addition I have manually tested the pager by visiting /admin/reports/dblog with 8 pages of of logs entries ...
I flicked backwards and forward trying to push it over .. every first/previous/next/last link was appropriate for the page I visited.

Nice patch!

Committed 5f705aa and pushed to 8.0.x. Thanks!