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.
Problem/Motivation
Views' pager "first" and "previous" links have incorrect URLs
Beta phase evaluation
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:
- Clean install of Drupal 8
- Add 100 items of content (ie:
drush en -y devel_generate && drush generate-content 100
) - Navigate to the site home page
- 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
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.
Comment | File | Size | Author |
---|---|---|---|
#32 | 2539472-28-32.interdiff.txt | 3.8 KB | mikeker |
#32 | 2539472-32-views-pager-links.patch | 8.48 KB | mikeker |
#29 | pager_first_and-2539472-29.patch | 589 bytes | harings_rob |
#28 | interdiff_25-28.txt | 3.05 KB | mondrake |
#28 | 2539472-28.patch | 6.46 KB | mondrake |
Comments
Comment #1
mikeker CreditAttribution: mikeker as a volunteer commentedComment #2
mikeker CreditAttribution: mikeker as a volunteer commentedThis 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.
Comment #3
mikeker CreditAttribution: mikeker as a volunteer commentedGrrr....
Comment #4
mikeker CreditAttribution: mikeker as a volunteer commentedAdded beta eval and bumped issue priority to Major since this has no reasonable workaround and affects any site with a pager.
Comment #5
mondrakeMy suggestion for tests (test-only == interdiff)
Comment #7
mikeker CreditAttribution: mikeker as a volunteer commented@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.Comment #9
mondrakeFully makes sense.
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.Comment #10
YesCT CreditAttribution: YesCT commentedComment #11
mikeker CreditAttribution: mikeker as a volunteer commented#9:
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! :)
Comment #12
mondrake#11 shouldn't it be after the two if blocks?
Also changed title and component since this issue is about the core pager.
Comment #14
mikeker CreditAttribution: mikeker as a volunteer commentedYup.
Comment #15
mondrake#14 thanks @mikeker.
Code and tests look good to me, only the comment is now misplaced
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."
Comment #16
Wim LeersCan you explain why this fixes the problem?
Comment #17
mondrake#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.Comment #18
mikeker CreditAttribution: mikeker as a volunteer commented@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!
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).
Comment #19
Wim LeersThanks 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?
Comment #20
Fabianx CreditAttribution: Fabianx as a volunteer commentedWhat 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.
Comment #21
mondrake#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:
oh yeah I missed that, thanks.
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.
Comment #22
mondrakeComment #23
Wim LeersAhhh!
So we went from
href="/foo/bar"
tohref=""
for the first page. So the link to the first page doesn't actually do anything anymore.Comment #24
alexpottNice additional text coverage.
$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.
Comment #25
mikeker CreditAttribution: mikeker as a volunteer commentedBlame @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.Comment #26
mikeker CreditAttribution: mikeker as a volunteer commentedGrrr... Always forget that...
Comment #27
mikeker CreditAttribution: mikeker as a volunteer commentedAdded a new meta to cleanup pager.inc and convert it to a service: #2547833: Pager.inc -- add tests, clean it up, convert to a service, use it!
Comment #28
mondrakeI 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).Comment #29
harings_rob CreditAttribution: harings_rob as a volunteer commentedPlease fill up the comments to a maximum.
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
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.
Comment #30
mikeker CreditAttribution: mikeker as a volunteer commentedJust a quick comment re #29:
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.Comment #31
othermachines CreditAttribution: othermachines commentedMessy or no, patch in #28 works great for me.
Edit: Too much coffee, not enough brain.
Comment #32
mikeker CreditAttribution: mikeker as a volunteer commentedUpdated 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.
Comment #33
mikeker CreditAttribution: mikeker as a volunteer commentedComment #36
martin107 CreditAttribution: martin107 commentedI 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!
Comment #37
alexpottCommitted 5f705aa and pushed to 8.0.x. Thanks!