When on a view with an ajax pager, and after at least one ajax load, the links to the next pages will include a bunch of extra metadata related to ajax requests. Specifically ajax_page_state
and _wrapper_format
are included as query parameters. If you try to load this link, Drupal will return a bunch of JSON instead of rendering the page.
Steps to reproduce:
- Install latest Drupal 8.2.x
- Create 10 dummy articles
- Create a view of content with a page display at
/test-ajax-pager
- Add a full pager, limit to 2 items shown per page
- Under advanced, set
Use AJAX
toYes
- Go to
/test-ajax-pager
and click the link for page 2 - Copy the link for page 3, and open in a new browser window
Here's an example of what the link looks like /test-ajax-pager?ajax_page_state%5Btheme%5D=bartik&ajax_page_state%5Btheme_token%5D=&ajax_page_state%5Blibraries%5D=bartik%2Fglobal-styling%2Cclassy%2Fbase%2Cclassy%2Fmessages%2Ccontextual%2Fdrupal.contextual-links%2Ccontextual%2Fdrupal.contextual-toolbar%2Ccore%2Fdrupal.active-link%2Ccore%2Fhtml5shiv%2Ccore%2Fnormalize%2Cquickedit%2Fquickedit%2Cshortcut%2Fdrupal.shortcut%2Csystem%2Fbase%2Ctoolbar%2Ftoolbar%2Ctoolbar%2Ftoolbar.escapeAdmin%2Ctour%2Ftour%2Cuser%2Fdrupal.user.icons%2Cviews%2Fviews.ajax%2Cviews%2Fviews.module&_wrapper_format=drupal_ajax&page=2
and here's what it should look like /test-ajax-pager?page=2
Comment | File | Size | Author |
---|---|---|---|
#31 | 2798947-31.patch | 3.96 KB | Lendude |
#31 | interdiff-2798947-29-31.txt | 2.1 KB | Lendude |
#29 | 2798947-29.patch | 2.94 KB | Lendude |
#29 | interdiff-2798947-27-29.txt | 1.66 KB | Lendude |
#23 | views_pagers_include-2798947-23.patch | 2.8 KB | loopduplicate |
Comments
Comment #2
mondrakeAdding related issues.
Comment #3
mondrakeAdding related issues.
Comment #4
rocketeerbkw CreditAttribution: rocketeerbkw at Amazee Labs commentedI found a similar issue with the destination query. Based on the fix in that issue, I've come up with a fix for this one as well. I think all the code is there, just things happen out of order.
The current order is:
ajax_page_state
and_wrapper_format
for destinationThis patch makes the order:
ajax_page_state
and_wrapper_format
Comment #5
dawehnerDo you understand why changing the order fixes the issue exactly? Maybe putting that into a comment in the file would be great for other people to understand WHY something was done in the particular way. We struggled with this exact implementation for a long time.
Comment #7
LoMo CreditAttribution: LoMo as a volunteer commentedThe patch would no longer apply, so re-rolled it... looks like the main changes since the patch was written were simple changes to the short-form array syntax.
Comment #9
Kristen PolNitpick: Can "such" move up a line and be in 80 chars?
Otherwise it looks and works great! Thanks!!!
Example:
Before patch:
After patch:
which is correct because those are the exposed filters. This matches the pager link when AJAX is disabled.
Comment #10
rwam CreditAttribution: rwam commentedThe patch #7 works for the url params indeed, but it has critical side effects: after each ajax call the view stylesheet was added and all javascripts (vendor, module, themes) were injected in the page too. And this again and again and again… (see screenshot)
I did a rollback.
Comment #11
keithdoyle9 CreditAttribution: keithdoyle9 commentedThis also occurs in the admin views associated with Workbench. (subscribing)
Comment #13
dawehnerHere is an idea, I'm not sure how easy/feasible it actually is.
We have a problem of two one state of the request we want to have during rendering the view (without all the additional parameters),
and one afterwards, when we interact with the ajax state, the ajax parameters etc.
Given that I'm suggesting to create a new request object, kind of similar to the existing patch, but a new cloned object, instead of the old one.
$view_request = clone $request
.This object then should be pushed onto the request stack so that everything knows about about this temporary new request:
\Drupal::requestStack()->push($view_request)
.Once the rendering is done in
this request could be dropped again, so that for example the ajax state system could see the original request again.
I don't know whether this idea will actually work and how tricky it would be to actually implement, but it might be worth giving it a try :)
Comment #14
netdreamer CreditAttribution: netdreamer commentedI reworked the patch #2798947-7: Views pagers include ajax metadata, based on findings by @rwam and I think I've fixed that problem.
I'm using it now on some pages with multiple ajax pagers without any issue, but please test it!
As already stated in the sourcecode, everything is related to #2504709: Prevent _wrapper_format and ajax_form parameters from bleeding through to generated URLs, so I think that this fix will be no more useful when parent issue will be fixed.
Comment #15
netdreamer CreditAttribution: netdreamer commentedComment #17
Coufu CreditAttribution: Coufu at iFactory commentedThank you SOOO much netdreamer!! #14 fixed my issues with views ajax and pagers.
Comment #18
Kristen PolI tested per steps in the issue summary and it worked as expected. Note that the link for the first page is:
/test-ajax-pager?page=0
and it would be better if it was just:
/test-ajax-pager
but that is not the focus of this issue. The code is easy to read. It still has the one nitpick I mentioned in #9 (moving "such" up to the previous line) but that shouldn't hold this up. Marking RTBC.
Comment #19
Kristen PolComment #20
Kristen PolActually, this was marked as needs tests so moving back to needs work.
Comment #21
rwam CreditAttribution: rwam commentedPatch #14 works very well. Thank you @netdreamer.
Comment #22
keithdoyle9 CreditAttribution: keithdoyle9 commentedPatch #14 worked for me as well.
Comment #23
loopduplicateAdded a test.
Comment #25
BerdirConfirmed that this helps with our project.
There is a deeper issue for us, but I don't know if that is something specific to our site or possibly a generic issue. Every few clicks on the pager, the ajax action is not triggered and as a result, the browser just normally fetches that URL. Previously, you then get the raw JSON response, with this page, it just loads that page as a normal non-ajax link. Which is not perfect, but so much better than the alternative.
Figuring out what is going on with the ajax action is a separate issue, I'll open an issue if I ever track down what exactly causes it.
Comment #26
alexpottThis section makes me feel uneasy. Moving everything from POST to GET feels very odd. Also if I remove this section the test still passes so whatever this is here for is not tested.
Comment #27
LendudeThe piece of code pointed out in #26 was originally something that was moved from further down in
\Drupal\views\Controller\ViewAjaxController::ajaxView
but the deletion got removed in a reroll in #14 so we were left with an addition instead of a move.But as @alexpott pointed out, this code move isn't needed at all to pass the test so lets remove it.
Comment #28
BerdirAre we sure it's not needed or are we just missing test coverage?
It was moved befere to keep it above the logic
Right, before it was moved because we also removed the part here.
Keeping it below is OK because above we unset both request and query, so it doesn't make a difference that we merge them together afterwards.
However, the unset here seems bogus then because $used_query_parameters is $request->query and they were already unset above. So possibly we can remove that and move the @todo up?
I guess we are doing the query remove here because it has been merged in from the POST request above. There is no reason for ajax_page_state to ever be present in the query, so what if we move the unset above to unset it on $request_all, so we never merge it into $request->query in the first place?
Comment #29
Lendude@Berdir looking at the failing test in #23 it covers the steps and result in the IS nicely, so if we are doing things that need extra test coverage, we also need to expand on this in the IS. I think the test coverage is fine, but you are absolutely right, we can reshuffle some more code to make all this much clearer. It seems like we were stripping things from the request in two different places for the same reason, so lets do that in one place.
Had a quick shot at @dawehners proposal in #13 but couldn't get it to work quickly (still sounds like a good idea).
Comment #31
LendudeAs @Berdir rightly pointed out to me in slack, removing 'ajax_page_state' from the POST data lead to the situation in #10, so we need to make sure it never gets added to GET but not remove it from POST.
Added a new test assertion to test for the double loading of assets as described in #10 and this indeed fails with the patch in #29.
Lets see how this does.
Comment #32
BerdirNice, looks good to me.
Comment #33
alexpottCommitted and pushed 48726e6908 to 8.6.x and 05eab2037e to 8.5.x. Thanks!