Problem/Motivation

Views' pager "first" and "previous" links have incorrect URLs

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug
Issue priority Major because there is no workaround and this affects every Drupal site with a content listing, including the default home page
Prioritized changes Yes: followup on a recent critical issue
Disruption None

Fallout from #2351015: URL generation does not bubble cache contexts.

Steps to reproduce:

  1. Clean install of Drupal 8
  2. Add 100 items of content (ie: drush en -y devel_generate && drush generate-content 100)
  3. Navigate to the site home page
  4. Click the "next >>" link at the bottom of the page

The "<< first" and "previous" links will show a URL of /node?page=1. If you click "next >>" again, the "previous" link will be correct, but the "<< first" link will still be wrong.

Proposed resolution

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Add automated tests Instructions DONE
Update the patch to incorporate feedback from reviews (include an interdiff) Instructions
Add steps to reproduce the issue Novice Instructions DONE
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

User interface changes

Should be no UI changes (except the link goes to the correct url)

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeker’s picture

mikeker’s picture

This fixes the issue, but I'm not sure it's the correct fix considering the huge change that was #2351015: URL generation does not bubble cache contexts. There is strange code in pager_query_add_page as indicated by the @todo.

Regardless this should've been caught by tests.

mikeker’s picture

Status: Active » Needs review

Grrr....

mikeker’s picture

Priority: Normal » Major
Issue summary: View changes

Added beta eval and bumped issue priority to Major since this has no reasonable workaround and affects any site with a pager.

mondrake’s picture

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

The last submitted patch, 5: 2539472-5-test-only.patch, failed testing.

mikeker’s picture

@mondrake: Thanks for starting the tests!

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

The last submitted patch, 7: 2539472-7-tests-only.patch, failed testing.

mondrake’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

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.

YesCT’s picture

Issue summary: View changes
mikeker’s picture

Status: Needs work » Needs review
FileSize
4.48 KB
1020 bytes

#9:

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.

Good call. I've moved it outside the if-block so it could be for other tests and added a comment.

Also, thanks for removing the needs-tests tag. I never remember to clean those things up! :)

mondrake’s picture

Title: Views' pager "first" and "previous" links have incorrect URLs » Pager "first" and "previous" links have incorrect URLs
Component: views.module » base system
Issue tags: +Regression

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

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

Status: Needs review » Needs work

The last submitted patch, 11: 2539472-11-views-pager-links.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review
FileSize
4.28 KB
1.15 KB

shouldn't it be after the two if blocks?

Yup.

mondrake’s picture

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

Wim Leers’s picture

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

mondrake’s picture

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

mikeker’s picture

Status: Needs work » Needs review

@mondrake #15, 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.

#17: Thank you for explaining that so well, @mondrake!

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.

I thought about that earlier. We had (more) full URLs in the link before #2351015: URL generation does not bubble cache contexts landed -- I believe they included the path, but not the schema or domain. After that change, we have only the query string. But I figured it was better to be cautious as this code is a little funky (see the @todo in pager.inc@300).

Wim Leers’s picture

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?

Fabianx’s picture

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.

mondrake’s picture

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

mondrake’s picture

Status: Needs review » Reviewed & tested by the community
Wim Leers’s picture

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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.

mikeker’s picture

$page_new and $new_page - really?

Blame @catchpole for that! :P

Though, considering the state of the docs in this file, it's difficult to know what to call new variables... Fortunately, there is cleanup work going on in #2530296: Fix up docs in core/includes/pager.inc. Anyhow, I've updated the vars to have more meaning. I didn't touch $page_new because I have no idea what it really is, which brings me to...

I believe pager.inc needs a rewrite -- it uses global variables, doesn't following coding standards, and loops through pager_load_array far more times than is needed. There are several issues that have started this, but none that have finished it. And I don't think we should try to finish it here, I'll add a meta later.

mikeker’s picture

Status: Needs work » Needs review

Grrr... Always forget that...

mikeker’s picture

mondrake’s picture

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

harings_rob’s picture

Status: Needs review » Needs work
Issue tags: +DUGBE0609
FileSize
589 bytes
  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.

mikeker’s picture

Just a quick comment re #29:

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

The change from <current> to <none> was part of #2351015: URL generation does not bubble cache contexts and reverting that would affect caching of these pages (I believe...). Because of that we can't use the (admittedly cleaner) empty query string option. @mondrake offers some more details in #21.

othermachines’s picture

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

Edit: Too much coffee, not enough brain.

mikeker’s picture

Updated patch to address the feedback in #29 from @harings_rob. Thanks for the review!

Also cleaned up a few other doc errors while I was in there -- mostly end-of-line periods.

mikeker’s picture

Status: Needs work » Needs review

The last submitted patch, 29: pager_first_and-2539472-29.patch, failed testing.

The last submitted patch, 29: pager_first_and-2539472-29.patch, failed testing.

martin107’s picture

Status: Needs review » Reviewed & tested by the community

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!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed 5f705aa on 8.0.x
    Issue #2539472 by mikeker, mondrake, harings_rob: Pager "first" and "...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.