Views pagers include ajax metadata

Adding related issues.

Adding related issues.

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

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

+++ b/core/modules/views/src/Controller/ViewAjaxController.php
@@ -130,9 +130,27 @@ public function ajaxView(Request $request) {
+      // Add all POST data, because AJAX is always a post and many things,

Nitpick: Can "such" move up a line and be in 80 chars?

Otherwise it looks and works great! Thanks!!!

Example:

Before patch:

?sort_bef_combine=delta%20ASC&sort_by=delta&sort_order=ASC&ajax_page_state[theme]=external&ajax_page_state[theme_token]=&ajax_page_state[libraries]=addtoany/addtoany%2Cadmin_toolbar/toolbar.tree%2Cadmin_toolbar_tools/toolbar.icon%2Cadminimal_admin_toolbar/adminimal-admin-toolbar%2Cbetter_exposed_filters/auto_submit%2Cclassy/base%2Ccommerce/toolbar%2Ccore/drupal.active-link%2Ccore/html5shiv%2Ccore/normalize%2Cds/ds_2col%2Centity_ref_tab_formatter/tab_formatter%2Cexternal/font-awesome%2Cexternal/global-styling%2Cexternal/protip-trigger%2Cexternal/typekit%2Cgo_utility/utility.filter_dropdown%2Clayout_discovery/onecol%2Csystem/base%2Ctoolbar/toolbar%2Ctoolbar/toolbar.escapeAdmin%2Cuser/drupal.user.icons%2Cviews/views.ajax%2Cviews/views.module%2Cwebform/webform.element.options&_wrapper_format=drupal_ajax&page=2

After patch:

?sort_bef_combine=delta%20ASC&sort_by=delta&sort_order=ASC&page=2

which is correct because those are the exposed filters. This matches the pager link when AJAX is disabled.

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

This also occurs in the admin views associated with Workbench. (subscribing)

Here 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

$preview = $this->renderer->executeInRenderContext($context, function() use ($view, $display_id, $args) {
  return $view->preview($display_id, $args);
 });

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

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

Thank you SOOO much netdreamer!! #14 fixed my issues with views ajax and pagers.

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

Actually, this was marked as needs tests so moving back to needs work.

Patch #14 works very well. Thank you @netdreamer.

Patch #14 worked for me as well.

Added a test.

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

+++ b/core/modules/views/src/Controller/ViewAjaxController.php
@@ -130,9 +130,26 @@ public function ajaxView(Request $request) {
+      // Add all POST data, because AJAX is always a post and many things,
+      // such as tablesorts, exposed filters and paging assume GET.
+      $request_all = $request->request->all();
+      $query_all = $request->query->all();
+      $request->query->replace($request_all + $query_all);

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

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

Are we sure it's not needed or are we just missing test coverage?

It was moved befere to keep it above the logic

+++ b/core/modules/views/src/Controller/ViewAjaxController.php
@@ -165,6 +176,7 @@ public function ajaxView(Request $request) {
         // @todo Remove this parsing once these are removed from the request in
         //   https://www.drupal.org/node/2504709.
         unset($used_query_parameters[FormBuilderInterface::AJAX_FORM_REQUEST], $used_query_parameters[MainContentViewSubscriber::WRAPPER_FORMAT], $used_query_parameters['ajax_page_state']);
+        $request->query->remove('ajax_page_state');

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?

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

As @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.

Nice, looks good to me.

Committed and pushed 48726e6908 to 8.6.x and 05eab2037e to 8.5.x. Thanks!