ajax_page_state leaks through request in Views Ajax

This looks good. Not sure how to add test coverage and wondering if we should commit it as-is and defer that to #2504709: Prevent _wrapper_format and ajax_form parameters from bleeding through to generated URLs which will remove all the whack-a-mole anyway?

Reran the tests 3 times so appears the test failure is legit.

Reran the failing nightwatch and javascript and they passed. @catch mentioned moving tests to another ticket in #5.

So think this is good.

I'm waffling about descoping the test coverage. The code already tried to fix this and already references the followup we're proposing descoping the test coverage to, so how do we actually know that it is and will stay any better this time around?

That said, I couldn't think of what to test either.

But maybe we could add inline comments to the current change set, at least, to reduce the risk of further regressions before we fix this properly?

For #11

I experience the same issue in Drupal 10.1.6 using Facets 2.0.6 and the MR does not apply here.
Is there also a version of the Patch for Drupal 10 somewhere?

This looks great, and probably as good as we're going to get things before #2504709: Prevent _wrapper_format and ajax_form parameters from bleeding through to generated URLs.

Thanks neclimul!
I use
"3399951: Fix ajax_page_state leaks through request in Views Ajax": "https://git.drupalcode.org/project/drupal/-/commit/827b13b4bd8beaf77861b86b8ace48500ecdf6d1.patch"
in my composer patches section. That seems to resolve the issue for us.

Thanks again for the hint.

Looks like there's a regression where some styles get lost after applying filters in Media Library:

Nice find - we should at least be able to add a test to catch that issue, even if more generic tests will be easier in #2504709: Prevent _wrapper_format and ajax_form parameters from bleeding through to generated URLs

The partial history is that whatever media is doing was 'working' because all AJAX requests were POST requests prior to 10.1.0. When we made AJAX requests for views use GET, it started breaking, but we didn't have sufficient test coverage for all of the behaviour to notice in the original patch/MR. What media is doing is definitely weird, but like you I also don't understand why it is doing it, nor do I have a clear idea of what it should be doing instead.

The regression seen in #19 isn't specific to media. Doing this shows a similar issue:

1. Check out MR
2. Install Drupal with standard
3. Configure the Content admin (/admin/content) view to use AJAX
4. Apply filters on /admin/content
5. Observe exposed form field styling changes

What's happening is that because ajax_page_state is temporarily removed from the Request object, when $view->preview() is executed, AjaxBasePageNegotiator does not apply because ajax_page_state['theme'] and ajax_page_state['theme_token'] are unset, so instead of the admin theme, the AJAX content is rendered in the default theme.

Since $request->get() also checks the contents of the attributes parameter bag, adding this to the MR diff worked for me, but I don't know if it's ideal:

diff --git a/core/modules/views/src/Controller/ViewAjaxController.php b/core/modules/views/src/Controller/ViewAjaxController.php
index 9b2042947f..d5fc6096bf 100644
--- a/core/modules/views/src/Controller/ViewAjaxController.php
+++ b/core/modules/views/src/Controller/ViewAjaxController.php
@@ -199,7 +199,17 @@ public function ajaxView(Request $request) {
         // Reuse the same DOM id so it matches that in drupalSettings.
         $view->dom_id = $dom_id;

+        // Populate request attributes temporarily with ajax_page_state theme
+        // and theme_token for theme negotiation.
+        $theme_keys = [
+          'theme' => TRUE,
+          'theme_token' => TRUE,
+        ];
+        if (($temp_attributes = array_intersect_key($existing_page_state, $theme_keys))) {
+          $request->attributes->set(AjaxResponseSubscriber::AJAX_PAGE_STATE_REQUEST_PARAMETER, $temp_attributes);
+        }
         $preview = $view->preview($display_id, $args);
+        $request->attributes->remove(AjaxResponseSubscriber::AJAX_PAGE_STATE_REQUEST_PARAMETER);
         $response->addCommand(new ReplaceCommand(".js-view-dom-id-$dom_id", $preview));
         $response->addCommand(new PrependCommand(".js-view-dom-id-$dom_id", ['#type' => 'status_messages']));
         $request->query->set('ajax_page_state', $existing_page_state);

Added the changes mentioned in the previous comment along with a test to the MR and rebased.

I think the constant is out of scope here, and it's also on an event subscriber which shouldn't be relied on as part of the API. Bumping this to major since it can cause problems with complex views / ajax combinations.

Reverted constant use and rebased.

I ran the test-only job and locally but the testExposedFilteringThemeNegotiation seems to be passing without the changes.

I ran the test-only job and locally but the testExposedFilteringThemeNegotiation seems to be passing without the changes.

That test is for an issue that arose because of previous commits introduced in the MR (see #19). It's expected that it will pass without the changes. To see it fail, you'd need to remove changes mentioned in #23.

So I have to set up my local to get a failing test? How come we aren’t testing the scenario described?

Sorry if something isn’t clicking for me

So I have to set up my local to get a failing test? How come we aren’t testing the scenario described?

The test added is for coverage that was originally missing, per #20 and #22. As mentioned in #23, the issue in #19 is not specific to the media library, so the test is a more generic Views Ajax use case. If others feel it's necessary to test media library, that test can be added, but it seems redundant to me.

But we currently don't have coverage for this specific bug?

I think the added generic test coverage here is fine, that's going to catch more issues than a test specific to the media library hopefully.

Will rely on yall for that decision! just seems like could leave us open for this bug to come back because this edge case isn't covered.

We've still got #2504709: Prevent _wrapper_format and ajax_form parameters from bleeding through to generated URLs lined up after this issue and will probably add more coverage there too. It would be good to add an extra media library functional test (or extend an existing one) with filtering + paging but I think that could be done in its own issue as a task.

I will say that downloading and applying the MR diff fixes a weird issue on our project with a Facets-driven AJAX view. The server is set up to send "no-cache, no-store" headers on every response (I'm not sure why). After applying Facet-ed filters on the view, clicking on one of the results to view the node, and then using browser back navigation to return to the results listing, the listing page was completely missing CSS/JS even though the correct theme was being used.

I haven't tried to see whether this is reproducible with a core-only view, because this server set up seems very edge-casey. But if others think it's a worthwhile thing to try to test, I can give it a go.

It would be good to add an extra media library functional test (or extend an existing one) with filtering + paging but I think that could be done in its own issue as a task.

Could we open a follow up for this one?

Then probably good to mark, leaning on that @catch has done a review less then 2 weeks ago. And I don't personally see anything in the code

Follow up: #3413820: Add functional test for media library appearance after filtering and paging

Thanks!

Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!

If I open the media library under Drupal 10.2.2 and then - still within the media library modal dialog - cause any AJAX request e.g. by changing the view exposed filters, all(!) page assets (like drupal.js etc) are loaded again causing many follow-up problems. This seems to happen because of:

  if ($query->has('ajax_page_state')) {
      $query->remove('ajax_page_state');
  }

in MediaLibraryState.php. Will this problem also be solved by this bug and won't appear in Drupal 10.2.3 ?