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 🐛 Prevent _wrapper_format and ajax_form parameters from bleeding through to generated URLs Active which will remove all the whack-a-mole anyway?

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

Yeah, doesn't really makes sense but they did fail. Some of the failure messages don't make any sense though so going to do some manual testing to get to the bottom of things.

Investigating something yesterday I think I figured this out. It might require referencing Wim's D8 render pipeline flow chart and understanding that `ajax_page_state` actually happens in a AjaxResponseSubscriber though. It boils down to, since we're _entirely_ removing the query from the request in the controller, later when it gets to the response subscriber its also gone. The subscriber then thinks it never existed and doesn't recompute the libraries to return correctly. This then can cause problems for interactions with the page since the libraries that previously existed on the page have been removed from Drupal's list its tracking.

There's _probably_ an argument to be made that AjaxResponseSubscriber should capture the query during the response event and hold it to be more resilient to things happening during the rest of the request handling. I'm not sure if that's the correct way or if there's an appetite to change it and its definitely a big change of scope from this issue so going to try avoid addressing it here.

That said, since this code is already specifically trying to handle this sort of request hackery, we can do a little more to work around this limitation. The merge request has been updated to hopefully handle handle things. If it works it could probably use some documentation on why it needs to work the way it does and maybe a link to an additional follow up.

Random test failure 🐛 [random test failure] TimestampFormatterWithTimeDiffTest::testTimestampFormatterWithTimeDiff Active in Javascript tests. Nightwatch looks like chrome crashing?

Seems that change fixed the problem.

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

So I started doing that and then I'm like "Well, this isn't really different then '_drupal_ajax' so we should probably use a similar constant so we can track dependencies in the same way.

Unfortunately, this was like pulling up the proverbial rock and all the bugs rushed out. The reason it didn't "affect" core was it seems like there are a number of tests and workarounds placed directly in various core systems. Specifically Pager and medial_library filter this and only this parameter out of some URL building.

I've gone ahead removed the work arounds, cleaned up the documentation related to these query filters, and consolidated the filtering list to hopefully make systems that need similar workarounds in the future easier to implement.

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 🐛 Prevent _wrapper_format and ajax_form parameters from bleeding through to generated URLs Active .

Thanks catch! I was definitely thinking about moving towards that so glad you agree.

As far as the 10.1 patch, you should be able to use the earlier version. it doesn't have the docs but behaves the same.:

https://git.drupalcode.org/project/drupal/-/commit/827b13b4bd8beaf77861b...

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 🐛 Prevent _wrapper_format and ajax_form parameters from bleeding through to generated URLs Active

Looks like this code is behaving correctly but maybe media is being naughty?

Basically, media has media_library_views_post_render which calls MediaLibraryState::fromRequest($view->getRequest()).

You might think this is generating a media state from the request but it _also_ mangles the request on the view.

    $query = $request->query;

    // ...

    // Remove ajax_page_state as it is irrelevant.
    // @todo: Review other parameters passed
    // See https://www.drupal.org/project/drupal/issues/3396650
    if ($query->has('ajax_page_state')) {
      $query->remove('ajax_page_state');
    }

Because the views request is the _actual_ request and not the cloned version, this is mangling was able to remove the actual page state. But when this patch adds the state back it breaks this little hack which apparently doesn't have a test. Or more accurately, it kinda had a test we are now removing in this patch. However it didn't test that it's hack was actually changing view's ajax behavior so it didn't see this change as breaking.

So... maybe this is blocked on 🐛 MediaLibraryState passes all query parameters around, not just the ones it needs Active ? I'm not sure what the next steps are because I'm not really familiar with what media is trying to accomplish and why it is fighting with asset management like this.

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 🐛 ajax_page_state leaks through request in Views Ajax Needs work 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 an issue that arose because of previous commits introduced in the MR (see #19 🐛 ajax_page_state leaks through request in Views Ajax Needs work ). It's expected that it will pass without the changes. To see it fail, you'd need to remove changes mentioned in #23 🐛 ajax_page_state leaks through request in Views Ajax Needs work .

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

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 🐛 ajax_page_state leaks through request in Views Ajax Needs work and #22 🐛 ajax_page_state leaks through request in Views Ajax Needs work . As mentioned in #23 🐛 ajax_page_state leaks through request in Views Ajax Needs work , the issue in #19 🐛 ajax_page_state leaks through request in Views Ajax Needs work 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 🐛 Prevent _wrapper_format and ajax_form parameters from bleeding through to generated URLs Active 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: https://www.drupal.org/project/drupal/issues/3413820 📌 Add functional test for media library appearance after filtering and paging Active

Thanks!

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

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

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 ?