Compress ajax_page_state

However, we need to figure out whether bc is necessary - some contrib modules might inspect the array. One possible way would be to decompress the query string and replace it in the incoming request, essential upcast it, but... not sure what that looks like yet.

I think that should indeed be feasible!

Here's an attempt at the upcasting idea.

BigPipe and system_js_settings_alter() were suffering a bit - see the comment about multiple calls. However with a bit of manual testing it seems like it's mostly working.

Fixing commit checks..

+++ b/composer.json
@@ -59,7 +59,8 @@
+            "php-http/discovery": true

is that required?

@andypost no that's cruft that crept into the patch somehow.

This should help at least slightly with the tests - need to move the is_array() check earlier.

This time fixing the composer.json syntax error, and doing the compression at the very last minute, which hopefully means only one conversion to/from string to array.

CommentCSSTest and similar ones in tracker and history are using ajaxPageState to check the existence of JavaScript. We can probably either drop that assertion or rework it to check something else. Will worry about any more actual test failures first.

OK most, maybe all, of the remaining test failures are test assertions looking for things in ajaxPageState now.

There doesn't seem to be a lot of contrib use. Not sure about custom modules.

See http://grep.xnddx.ru/search?text=ajax_page_state&filename=

Found one real bug via the test failures, haven't addressed the test expectation issues yet but this should narrow down a bit more properly.

Removing some stray debug and fixing an attempt to decompress the query parameter when it's not a compressed query parameter.

Only doing the processing on GET requests doesn't work either, now checking whether we're dealing with an array or a string.

The last submitted patch, 14: 3348789-14.patch, failed testing. View results →

#14 looks like it's down to test expectations trying to inspect ajax_page_state now.

Two further things to sort out:
1. Need a before/after comparison of ajax_page_state size to make sure the compression is making a dent, an AJAX-enabled admin/content view in Umami/Claro is probably a decent candidate. We could also compare this against only compressing ajax_page_state['libraries'] which wouldn't require json_encode - that might end up smaller than needing to json_encode the whole array, but it might not.

2. At the moment, ajax_page_state is compressed for asset requests but not everywhere else (mostly form API I think). We don't have to compress it for POST requests, but maybe we should anyway for consistency? And it wouldn't hurt to make POST requests slightly smaller.

1. Agreed!
2. I could be convinced either way 🤓 I think I have a slight preference for consistency?

+++ b/core/misc/ajax.js
@@ -827,10 +827,7 @@
-    const pageState = drupalSettings.ajaxPageState;
-    options.data['ajax_page_state[theme]'] = pageState.theme;
-    options.data['ajax_page_state[theme_token]'] = pageState.theme_token;
-    options.data['ajax_page_state[libraries]'] = pageState.libraries;
+    options.data.ajax_page_state = drupalSettings.ajaxPageState;

We probably now want an @see \Drupal\Core\Ajax\AjaxPageStateSubscriber?

Ran some numbers. Test steps:

Install umami
Uninstall bigpipe (because it does complicated things with ajax_page_state)
Set admin/content to 'use ajax' in the views configuration, also reduce the number of items per page to 5 so the pager kicks in.

Open chromium network tab, hit a pager link, look at request URL for the AJAX request, extract the bits of the URL relevant to ajax_page_state.

Lovely to see AJAX GET requests in HEAD for the first time!

HEAD:

571 characters

ajax_page_state[libraries]=claro/drupal.nav-tabs,claro/global-styling,contextual/drupal.contextual-links,contextual/drupal.contextual-toolbar,core/drupal.active-link,core/drupal.dropbutton,core/drupal.tableheader,core/drupal.tableresponsive,core/drupal.tableselect,core/normalize,demo_umami/toolbar-warning,shortcut/drupal.shortcut,system/admin,system/base,toolbar/toolbar,toolbar/toolbar.escapeAdmin,tour/tour,user/drupal.user.icons,views/views.ajax,views/views.module&ajax_page_state[theme]=claro&ajax_page_state[theme_token]=cHnSbuz5ebauPOoZ0unr_J3ILU7IZiPVz9smGQC4Npc

with patch:
400 chars

ajax_page_state=eJyFkU9LAzEQxb_LntNdQUX0Jh60IloRPZSFMtkd2tgks8xM-k_87m6X7Uqx4CnM7715M0m-Mu8sAzuU7CarPDCVRc2pAZ9HWI0UrJgezz1Z8CPRrXdxbiqKihtN4IeOXzRqLUv5x6NE3gK3LsZBh0rdCrv-Y6FmamxSpXjM2xU9LhBq5BMCozQUpY08IQp6rLQXInEA73Zoagw0SwGCK4t-x9EaOO5vLQtirZIOQQdgZCuKoSygDi4OlQVB04cMaX9AjlJBg7ddq1LqlMQmCfIwaV_krn0_MSuHaymL7sjhEzbHJFCdPGYm0wUGPHzsoZ4pLTHu6UN8s2l3iRbS5IWmZyny7PF8_PR-NZ66ycfuWsL9693Fc1Nl3z9mdtJq

Also modified the patch to only compress ajaxPageState['libraries']:

545 chars

ajax_page_state[theme]=claro&ajax_page_state[theme_token]=cHnSbuz5ebauPOoZ0unr_J3ILU7IZiPVz9smGQC4Npc&ajax_page_state[libraries]=eJxlyctygjAAQNEf0sojiizFAhILAZSKbJgQgjyiIAR5fH1numrHzV2cS-qWrtK2bzD7SNu6SXrO68eC_GGOE0ZzilPavntLu6Z-dMWLvr-OMkr4gsJRrQ5iP0w7ezypelW6HYS29pSCGbnwIPWKYIkPnxkhdocyll9otnzd-hLZVovqJIGOJsIdooWJPOjim_BUcvvsbE2AHHeKI_RE51jOrt0QeEkDiV14rTmCFKrAYJvyLusKSAABfpDXyjh539IaE1URfQ9WsTyM7KhNOTVLGGavwOKTjNYbLzmpXfZk5HKShFuUpvuDya8GKURuaHowzE4OXYlHqqcfFZBmBC_3k6IZl9CWkDpvzsv79cSBsAxAFBe75cCCuVLApx_mrW4tXgUdutVvP3CJx39wr9Oe0R_ZGI50

So the json_encode() is worthwhile after all and we're getting a nearly 1/3rd reduction in string length compared to HEAD. Attaching the libraries-only version of the patch as a .txt just for reference.

Checked out the get vs. post behaviour a bit. I think I have it working always compressing, manual testing of both views pagers and layout builder UI both work as well as layout builder UI tests.

Added the @see from #17.

No interdiff due to fatfingering something (and the patch is still tiny for now).

The last submitted patch, 19: 3348789-19.patch, failed testing. View results →

OK #19 passes even more tests now. Haven't gone through the remaining failures individually to make sure they're 100% assertions, but I'm happy enough that this will work generically with the compression and decompression in one place each, and that those places are early/late enough it will be transparent for everything else.

This is going to make 📌 Dynamically determine ajaxPageState based on libraries Active harder to do, because that wanted to get rid of ajaxPageState['libraries'] whereas we're compressing the whole thing, but it also becomes less necessary. Even if we do that issue, we'd definitely want to compress because this is about the size of the outgoing network request.

This should fix CommentCssTest and BigPipeTest assertions.

I added a helper to WebTestBase to get the uncompressed drupalSettings, next to the existing getDrupalSettings() method. We could maybe do a trait instead, but this is used in a fair amount of places.

Weird whitespace issue.

CCF fix #23.

#24 is changing a lot of unrelated code in the same files, any code style changes like that should be moved to a separate issue. Fixing the one stray extra line.

Alright this should get us down to 3-4 remaining test failures, which look like genuine test fails rather than assertions on ajaxPageState.

For the test changes there are two types:

1. Tests that want to check if a particular library is in ajaxPageState for whatever reason, these can just use getAjaxPageStateUncompressed().

2. Tests that are messing with the contents of ajaxPageState because they're testing it directly- they have to compress/uncompress the string, which is fair enough I think (also a reason not to auto-uncompress for tests).

Should this be removed from the 10.1 target list?

Reviving this one.

Instead of ::getAjaxPageStateUncompressed(), adding an optional parameter defaulting to TRUE to uncompress ajaxPageState, that should mean a lot less test breakage/changes.

testAjaxWithAdminRoute() 

This seems to be testing the wrong thing?!!?!?! I manually clicked through the test route and it shows the admin theme, which is claro, then I looked again the test, and it's asserting stable9, but saying that should be the admin theme, which is wrong. But if this is really the case, then how is it passing now?

I tried to manually reproduce the issue in Drupal\Tests\views\FunctionalJavascript\ExposedFilterAJAXTest but that all appears to be working OK for me.

So still expecting some test failures but uploading to see overall progress.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Missing a use statement.

The last submitted patch, 31: 3348789-31.patch, failed testing. View results →

The comments in testAjaxWithAdminRoute are wrong - it's supposed to get the front end theme even when requesting a page with the admin theme, that's the whole point.

I think we need to adapt SettingsCommand - draft code for that. This doesn't fix the ajax theme negotiation though yet. Uploading progress.

Moved the ajax_page_state decompression to a middleware so it runs as early as possible.

Grumble grumble.

This might be green, or at least closer to green, main changes since #26:

1. Move the uncompress logic to a MiddleWare because that's the earliest we can do anything and we're trying to make this transparent to everything outside the AJAX system. Doesn't have a functional impact as far as I can tell, just seemed like a good idea to move it earlier.

2. AjaxBasePageNegotiator only changes the theme if ajax_page_state['theme_token'] is set, but if the default theme is in ajax_page_state we don't need the theme token. I haven't figured out exactly why theme_token isn't null, maybe a side-effect of json_encode/json_decode - however, I think the new logic is actually more correct: if we've got a theme, negotiate, if we've got a theme_token, validate the token.

++ b/core/lib/Drupal/Core/Theme/AjaxBasePageNegotiator.php
@@ -66,7 +66,7 @@ public function __construct(CsrfTokenGenerator $token_generator, ConfigFactoryIn
    */
   public function applies(RouteMatchInterface $route_match) {
     $ajax_page_state = $this->requestStack->getCurrentRequest()->get('ajax_page_state');
-    return !empty($ajax_page_state['theme']) && isset($ajax_page_state['theme_token']);
+    return !empty($ajax_page_state['theme']);
   }
 
   /**

3. SettingsCommand replaces the entirety of ajax_page_state with a fresh, compressed, value.

The last submitted patch, 36: 3348789-36.patch, failed testing. View results →

Should fix FieldTypeCategoriesIntegrationTest - just the test assumption that the literal library strings will be in the HTML, which is no longer the case, but can use ::getDrupalSettings().

The last submitted patch, 38: 3348789-37.patch, failed testing. View results →

Remaining fails:

Drupal\FunctionalTests\Bootstrap\UncaughtExceptionTest
Have a feeling this is just because we're adding a middleware so need to change the test expectation.

Drupal\Tests\media_library\FunctionalJavascript\WidgetViewsTest
No idea yet, seems to work in manual testing unless I'm missing a step.

Drupal\Tests\Core\Command\QuickStartTest
Think this bogus/random/unrelated.

Thanks for this work on this so far everyone. If anyone else needs it for D9 I have created the patch based on 3348789-37.patch. We have been looking at the various patches for D9 to allow Ajax to make get requests so need this to wrangle the query parameters into a nicer looking (and shorter) string.

Re #41
Yes because I am already adding the patch https://www.drupal.org/files/issues/2023-03-30/956186-174-reroll-95-185.... → from 📌 Allow AJAX to use GET requests Fixed the patch doesn't work so attached is an additional version that is dependent on that patch from 📌 Allow AJAX to use GET requests Fixed .

I must have missed something that is required for D9.5.x that isn't needed in the D10 & D11 patches as trying to use it causes js errors in AjaxResponse due to all of the libraries being included, which throws the "LoadJS" in core/assets/vendor/loadjs/loadjs.min.js, this stops the other js files from loading that did actually need loading.

Worked out that my backport patch was simply missing the new core/lib/Drupal/Core/StackMiddleWare/AjaxPageState.php file and service config, sorry.

I have also added a tweak to the core/lib/Drupal/Core/EventSubscriber/AjaxResponseSubscriber.php that is already part of D10 core to use $event->getRequest()->get(static::AJAX_REQUEST_PARAMETER).

Attached are the new versions of the D9.5.x patches.

Thanks for the work Altcom! Super good.

When attaching patches, it's very useful to keep the same patch name and increment the patch number to match the comment number. This makes it easier when applying and testing patches to keep them straight, I can do 37,38,44 etc.

Moving the 11.x code in #38 to and MR so it's easier to know where the active work against core is.

@Altcom_Neil good that you're trying to use this on 9.5.x, it would be great if you could try to reproduce the test failure in Drupal\Tests\media_library\FunctionalJavascript\WidgetViewsTest and confirm whether it's a real bug or a test expectation issue. That's the main blocker to getting this committed to core.

Ajax Views pagers are not working in 10.1. That issue links here, are we intending to fix views Ajax paging with the work here or should I look into it separately in 3384852?

https://www.drupal.org/project/drupal/issues/3384852 🐛 Ajax Pager broken after upgrade 10.0.9 to 10.1.2 Needs work

This issue is the one to fix it, it works on most setups but not on those that have restrictions on very long URLs, which is what this addresses.

Re #40

Drupal\Tests\media_library\FunctionalJavascript\WidgetViewsTest
No idea yet, seems to work in manual testing unless I'm missing a step.

If I first click "Apply filters", then select media and click "Insert selected" I see the POST /media-library request URL includes uncompressed ajax_page_state and response code is 500:

Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "ajax_page_state" contains a non-scalar value. in Symfony\Component\HttpFoundation\InputBag->get() (line 37 of vendor/symfony/http-foundation/InputBag.php)

#0 core/lib/Drupal/Core/StackMiddleware/AjaxPageState.php(34): Symfony\Component\HttpFoundation\InputBag->get()
#1 core/lib/Drupal/Core/StackMiddleware/StackedHttpKernel.php(51): Drupal\Core\StackMiddleware\AjaxPageState->handle()
#2 core/lib/Drupal/Core/DrupalKernel.php(704): Drupal\Core\StackMiddleware\StackedHttpKernel->handle()
#3 index.php(19): Drupal\Core\DrupalKernel->handle()
#4 {main}

@Olli thanks that does it, not sure what I was trying last time.

Tracked it down I think, see if this breaks anything else.

The last submitted patch, 44: 3348789-compress-ajax_page_state-9_5_x-37.patch, failed testing. View results →

Bumping this to major since certain hosting environment don't allow long enough query strings for views AJAX to succeed.

Added a change record https://www.drupal.org/node/3389367 →

One remaining thing here is that it's technically an API break to add an argument to WebDriverTestCase::getDrupalSettings and WebTestCase::drupalSettings, we might need to comment the new argument out and use func_get_args(). Having said that if no contrib overrides the method (and I doubt it does), maybe we can just add it directly.

I am unable to open modal window :
<a class="use-ajax" href="/node/2/edit" data-dialog-options="{&quot;width&quot;:800}" data-dialog-type="modal">First node displayed in modal dialog.</a>

after I upgraded to 10.1.3. However this patch is not compatibile with D10.1.3.
Is there any progress for D10 release also ?

@coaston define 'not compatible', have you tried it? All of the progress is on this issue, there is no other issue with a different patch. You can help by testing or reviewing the changes here.

Seemed to cause a number of failures. May all be related

ValueError: func_get_arg(): Argument #1 ($position) must be less than the number of the arguments passed to the currently executed function

We are having the same issue as reported in https://www.drupal.org/project/drupal/issues/3348789#comment-15242707 📌 Compress ajax_page_state Fixed
We applied the updated patch in https://www.drupal.org/project/drupal/issues/3348789#comment-15242845 📌 Compress ajax_page_state Fixed
But now, the Media Modal will not open.
It appears that the array is getting passed and causing a scalar issue.
Thank you for your work on this issue.

@catch Just going back to #36
I included that change in the 9.5.x patch but I'm not sure that change removing the isset($ajax_page_state['theme_token']) should be there.

The reason that the 'theme_token' var is missing in the ajax requests is because the System module deliberately excludes it when building the 'ajaxPageState' settings in system_js_settings_alter():

      // The theme token is only validated when the theme requested is not the
      // default, so don't generate it unless necessary.
      // @see \Drupal\Core\Theme\AjaxBasePageNegotiator::determineActiveTheme()
      $active_theme_key = \Drupal::theme()->getActiveTheme()->getName();
      if ($active_theme_key !== \Drupal::service('theme_handler')->getDefault()) {
        $settings['ajaxPageState']['theme_token'] = \Drupal::csrfToken()
          ->get($active_theme_key);
      }

So for the default theme the theme token is not added, and so

  public function applies(RouteMatchInterface $route_match) {
    $ajax_page_state = $this->requestStack->getCurrentRequest()->get('ajax_page_state');
    return !empty($ajax_page_state['theme']) && isset($ajax_page_state['theme_token']);
  }

returns false for the default theme which I guess is to 1. remove some unnecessary data from the ajax object; and 2. skip calling the determineActiveTheme method which is just going to return the default theme. This means that ThemeNegotiator::determineActiveTheme fallsback all the way to the DefaultNegotiator::determineActiveTheme which just returns the default theme.
When we remove the theme_token check from the conditional in AjaxBasePageNegotiator::applies it leads to a php notice as the AjaxBasePageNegotiator::determineActiveTheme expects it to be set when it does $token = $ajax_page_state['theme_token'];.

Attached are new Drupal 9.5.x versions without the change to the condition in AjaxBasePageNegotiator::applies

@pelicani try the MR, the patch is out of date.

@Altcom_Neil restored the isset() to see if things still pass tests.

The last submitted patch, 61: 3348789-compress-ajax_page_state-9_5_x-61.patch, failed testing. View results →

@catch
I have pulled in the latest changes you have committed to your 11.x issue branch except the changes in https://git.drupalcode.org/project/drupal/-/merge_requests/4853/diffs?co... as the comment says '@todo Uncomment new method parameters before drupal:11.0.0.' so I have left them as they were.

Re your restoring of the isset - not sure if you intended to write it that way or not but the change at https://git.drupalcode.org/issue/drupal-3348789/-/commit/a1c7a99b0d2eb2c... is wrapping the isset() within the !empty():
return !empty($ajax_page_state['theme'] && isset($ajax_page_state['theme_token']))
Original code was:
return !empty($ajax_page_state['theme']) && isset($ajax_page_state['theme_token']);

Removing the code change from AjaxBasePageNegotiator has the happy by product of removing the dependency on Allow AJAX to use GET requests [#956186] 📌 Allow AJAX to use GET requests Fixed as the patch no longer trying to change the line under the one this issue patches in D9.5.x

Also - confirming that the latest changes fix the issue reported in #50 with the uncompressed ajax_page_state values being passed in the Media Library view. On D9.5.x I was getting an error trying to use the bulk operation form but that is now fixed.

The last submitted patch, 64: 3348789-compress-ajax_page_state-9_5_x-63.patch, failed testing. View results →

I remember why the change now.

If you look at the theme negotiator, it does an || with the default theme, this means it's supposed to work when that's set but the token is not. The reason the change is necessary is because of how the encoding/decoding works - i.e. token used to be passed as an explicitly null query parameter but now it can not be in the array at all when it's decoded. As you can see from https://www.drupal.org/pift-ci-job/2772097 → this is handled by the test coverage.

    if ($theme === $this->configFactory->get('system.theme')->get('default') || $this->csrfGenerator->validate($token, $theme)) {
      return $theme;
    }

Thanks @catch,

Applied the MR as a patch for D10.
Only patch that would not apply for D10 was the test in a/core/modules/field_ui/tests/src/Functional/FieldTypeCategoriesIntegrationTest.php because this file does not exist in D10.
all the other patches applied fine.

However, we are still having an issue in the Media Library.
Specifically, in the url, we are seeing compressed ajax_page_state variable, but we are also seeing the uncompressed version for theme and libraries when we insert the image found after filtering.
Note : this only happens after filtering the media library then inserting.

We are now looking at the Media Library to see if it's adding the uncompressed version.

@pelicani

If you are using Cloudflare and since the ajax_page_state is very long, the response of the media pager will be 500 when you click the pager number second.

@pelicani make sure you're using the latest version of the MR diff.

If you're still getting some uncompressed query strings it's unlikely to be the media_library itself, but the fact that the library combines AJAX POST and GET requests for different operations. Clear steps to reproduce would help.

https://git.drupalcode.org/project/drupal/-/merge_requests/4853#note_212060 needs review.

Thanks all.

@catch

Attaching the patch we created from the MR.
https://www.drupal.org/files/issues/2023-09-27/KAL-1754-compress-ajax-pa... →

I'll walk through it today to double check I grabbed the latest changes.

Our issue is the same as reported in https://www.drupal.org/project/drupal/issues/3348789#comment-15242707 📌 Compress ajax_page_state Fixed
My initial comment is here ... https://www.drupal.org/project/drupal/issues/3348789#comment-15244675 📌 Compress ajax_page_state Fixed

Better steps to reproduce involve adding a console.log to the ajax.js file.

// If no Ajax callback URL was given, use the link href or form action.
if (!this.url) {
const $element = $(this.element);
if ($element.is('a')) {
this.url = $element.attr('href');
} else if (this.element && element.form) {
this.url = this.$form.attr('action');
}
console.log('url built: '+this.url);
} else {
console.log('url exists: '+this.url);
}

https://www.drupal.org/files/issues/2023-09-27/Screenshot%202023-09-27%2... →

This occurs when we add an image using the media library and filter the images then select one to insert.
On insert, we get the issue.
I hope this is clear, but I can post additional screenshots if necessary.

@aloneblood
Our issue is on a privately hosted environment with no Cloudflare

Thank you for all the work you do.

+++ b/lib/Drupal/Core/StackMiddleware/AjaxPageState.php
@@ -0,0 +1,42 @@
+    if ($request->query->has('ajax_page_state')) {

This is out of date, you're missing an elseif, so definitely try again with the latest from the MR.

@catch

Thank you for looking at this.

I see the else if is in there, line 354, it's a patch to the patch.

@catch

I hear you, that you don't think the issue is in Media Library, but, well, we are still going to poke around in there.
No offense.

In the MediaLibraryUiBuilder.php on line 335 the view request is set...

// Make sure the state parameters are set in the request so the view can
// pass the parameters along in the pager, filters etc.
$view_request = $view_executable->getRequest();

This creates a request parameter for ajax_page_state that does contain the uncompressed libraries that are breaking the URL.

I'm having a hard time removing it.
We set the library to null, but that broke other things.
Good times.

@pelicani happy to be proved wrong!

I've pushed two commits, the first compresses ajax_page_state if it's in there. This presumably will shorten the URL and passes tests.

   $all = $state->all();
    // Make sure the ajax_page_state query parameter is compressed.
    if (isset($all['ajax_page_state'])) {
      $all['ajax_page_state'] = UrlHelper::compressQueryParameter(json_encode($all['ajax_page_state']));
    }
    $view_request->query->add($all);

I am not that familiar with the media library code, but I also wondered if we need ajax_page_state at all there, so pushed a second commit that unsets it instead, including both code snippets here in case you want to compare and contrast:

 $all = $state->all();
    // Remove ajax_page_state from the query parameters since it's not relevant
    // to the view.
    // @todo: potentially remove more irrelevant query parameters.
    if (isset($all['ajax_page_state'])) {
      unset($all['ajax_page_state']);
    }
    $view_request->query->add($all);

@catch

Thank you for being so active on this issue with us!
We applied the latest but still got the uncompressed ajax page state in the URL.

Then, @markie found another file to adjust, MediaLibraryState.php and added a change to line 116

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

// Once we have validated the required parameters, we restore the parameters
// from the request since there might be additional values.
$state->replace($query->all());
return $state;

This removed the ajax page state in the url for good.
OMG, yay, w00t, huzzah, giggity!

I've been looking at this for a week and am so ready to NOT be looking at it any more.

Updated the MR with a change in the MediaLibraryState.php file. This resolved our issue. I think it might remove the need to change the MediaLibraryUiBuilder.php, but why not have both. We found that when opening the media library, and then doing a filter, the un-compressed ajax_page_state was still getting passed when inserting the selected item, causing a 400 error yet again. This change to the state resolved it. Hope this helps.

@pelicani that might be the only change needed - in which case could revert my last 2-3 commits and only handle removing ajax_page_state in MediaLibraryState - seems like a better place to do it.

> vendor/bin/phpcs --standard=core/phpcs.xml.dist --parallel=$(nproc) -- 'core'

FILE: /workspace/drupal-3348789/core/modules/media_library/src/MediaLibraryUiBuilder.php
----------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------
 5 | WARNING | [x] Unused use statement
----------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------------------

Time: 1 mins, 0.44 secs; Memory: 10MB

Resolved

Made one more commit to remove my changes from MediaLibraryUiBuilder. Last sentence of #79 was a dead-end, I think we're probably fine as-is now.

Added test coverage to the MediaLibraryState changes and once I remembered how dataProviders worked, everything passes.

New test coverage looks good. We should open a follow-up to exclude more query parameters here, could clean things up a fair bit. There is similar code for views pagers, although no standard list or API anywhere.

@catch Re the change in #66 to the Drupal\Core\Theme\AjaxBasePageNegotiator::applies, if the && isset($ajax_page_state['theme_token']) is removed then we have to do something about line 78 in Drupal\Core\Theme\AjaxBasePageNegotiator::determineActiveTheme because every call to this will cause a PHP warning.

"Warning: Undefined array key "theme_token" in Drupal\Core\Theme\AjaxBasePageNegotiator->determineActiveTheme() (line 78 of .../core/lib/Drupal/Core/Theme/AjaxBasePageNegotiator.php)

It could just be a simple as changing it to

   public function determineActiveTheme(RouteMatchInterface $route_match) {
     $ajax_page_state = $this->requestStack->getCurrentRequest()->get('ajax_page_state');
     $theme = $ajax_page_state['theme'];
-    $token = $ajax_page_state['theme_token'];
+    $token = $ajax_page_state['theme_token'] ?? '';
 
     // Prevent a request forgery from giving a person access to a theme they
     // shouldn't be otherwise allowed to see. However, since everyone is

as $this->csrfGenerator->validate() expects the $token var to be a string? So if the theme is not the default AND the theme_token is not set the var is an empty string.

We were also getting errors

TypeError: Drupal\Component\Utility\UrlHelper::uncompressQueryParameter(): Argument #1 ($compressed) must be of type string, array given, called in .../core/lib/Drupal/Core/StackMiddleware/AjaxPageState.php on line 35

due to the ajax_page_state already being uncompressed. This was coming from an old custom ajax callback that did ajax get requests before issue #956186 📌 Allow AJAX to use GET requests Fixed that we have now changed but just for defensiveness how about a condition before the code tries to uncompress the var:

   public function handle(Request $request, $type = self::MASTER_REQUEST, $catch = TRUE): Response {
     if ($request->request->has('ajax_page_state')) {
       $ajax_page_state = $request->request->get('ajax_page_state');
-      $ajax_page_state = json_decode(UrlHelper::uncompressQueryParameter($ajax_page_state), TRUE);
+      if (is_string($ajax_page_state)) {
+        $ajax_page_state = json_decode(UrlHelper::uncompressQueryParameter($ajax_page_state), TRUE);
+      }
       $request->request->set('ajax_page_state', $ajax_page_state);
     }
     elseif ($request->query->has('ajax_page_state')) {
       $ajax_page_state = $request->query->get('ajax_page_state');
-      $ajax_page_state = json_decode(UrlHelper::uncompressQueryParameter($ajax_page_state), TRUE);
+      if (is_string($ajax_page_state)) {
+        $ajax_page_state = json_decode(UrlHelper::uncompressQueryParameter($ajax_page_state), TRUE);
+      }
       $request->query->set('ajax_page_state', $ajax_page_state);
       $request->request->set('ajax_page_state', $ajax_page_state);
     }
-- 

not sure if it is necessary but there might be other ajax code floating around in contrib modules doing a similar thing to what we used to do?

Attached is another 9.5.x backport with the latest commits to MediaLibraryState and AjaxBasePageNegotiator, plus the above defensive bits of code to stop any potential PHP warnings being thrown. D9.5.x patch is now again dependant on the patch here https://www.drupal.org/project/drupal/issues/956186#comment-14991435 📌 Allow AJAX to use GET requests Fixed

then we have to do something about line 78 in Drupal\Core\Theme\AjaxBasePageNegotiator::determineActiveTheme

Pushed a commit that does this:

diff --git a/core/lib/Drupal/Core/Theme/AjaxBasePageNegotiator.php b/core/lib/Drupal/Core/Theme/AjaxBasePageNegotiator.php
index 80025daa6a..e9001ef3ba 100644
--- a/core/lib/Drupal/Core/Theme/AjaxBasePageNegotiator.php
+++ b/core/lib/Drupal/Core/Theme/AjaxBasePageNegotiator.php
@@ -75,14 +75,13 @@ public function applies(RouteMatchInterface $route_match) {
   public function determineActiveTheme(RouteMatchInterface $route_match) {
     $ajax_page_state = $this->requestStack->getCurrentRequest()->get('ajax_page_state');
     $theme = $ajax_page_state['theme'];
-    $token = $ajax_page_state['theme_token'];
 
     // Prevent a request forgery from giving a person access to a theme they
     // shouldn't be otherwise allowed to see. However, since everyone is
     // allowed to see the default theme, token validation isn't required for
     // that, and bypassing it allows most use-cases to work even when accessed
     // from the page cache.
-    if ($theme === $this->configFactory->get('system.theme')->get('default') || $this->csrfGenerator->validate($token, $theme)) {
+    if ($theme === $this->configFactory->get('system.theme')->get('default') || $this->csrfGenerator->validate(ajax_page_state['theme_token'], $theme)) {
       return $theme;
     }
   }

It's only optional if the first condition fails, so that way we still get a PHP notice when something is wrong. There's also no point assigning the variable for something used once.

just for defensiveness how about a condition before the code tries to uncompress the var

If we do something like that, we should also add a trigger_error('...', E_USER_WARNING) so people know there's something wrong.

@commit That commit for Drupal\Core\Theme\AjaxBasePageNegotiator::determineActiveTheme is a better solution.

For the Drupal\Core\StackMiddleware\AjaxPageState how about something like this?

  /**
   * {@inheritdoc}
   */
  public function handle(Request $request, $type = self::MASTER_REQUEST, $catch = TRUE): Response {
    if ($request->request->has('ajax_page_state')) {
      $request->request->set('ajax_page_state', $this->parseAjaxPageState($request->request->get('ajax_page_state')));
    }
    elseif ($request->query->has('ajax_page_state')) {
      $ajax_page_state = $this->parseAjaxPageState($request->query->get('ajax_page_state'));
      $request->query->set('ajax_page_state', $ajax_page_state);
      $request->request->set('ajax_page_state', $ajax_page_state);
    }
    return $this->httpKernel->handle($request, $type, $catch);
  }

  /**
   * Parse the ajax_page_State variable in the request.
   *
   * Decompresses and decodes the json to return an array.
   *
   * @param mixed $ajax_page_state
   * This should be a string containing the compressed json query parameters.
   *
   * @return array
   */
  private function parseAjaxPageState(mixed $ajax_page_state) {

    if (is_string($ajax_page_state)) {
      $ajax_page_state = json_decode(UrlHelper::uncompressQueryParameter($ajax_page_state), TRUE);
    }
    else {
      /**
       * @deprecated ajax_page_state should be a compressed json encoded string
       * in all POST and GET in Drupal 10.2.0 and so the passed in variable will
       * be a string only in 11.x
       */
      trigger_error('From 10.2.0 Drupal\Core\StackMiddleware\AjaxPageState::handle() expects the ajax_page_state to be compressed using UrlHelper::compressQueryParameter() in all post and get requests, this check will be removed in drupal:11.x and generate a TypeError instead. See https://www.drupal.org/node/3389367.', E_USER_DEPRECATED);
    }
    return $ajax_page_state;
  }

In Drupal 10.2.x the code would allow the uncompressed ajax_page_state to pass through while logging the deprecated error. In Drupal 11.x the method could be change to private function parseAjaxPageState(string $ajax_page_state) and then it would throw a type error if the ajax_page_state variable is not the expected compressed string? Or 11.x could remain as it is now and just throw the TypeError from UrlHelper::uncompressQueryParameter()?

The new method parseAjaxPageState() to does all of the checking, then decompressing, then decoding to return the expected array to remove any duplicated code in the main handle() method and allows the adding of a single call to trigger_error()?

New version of the D9.5.x with your latest change to AjaxBasePageNegotiator::determineActiveTheme plus the parseAjaxPageState method to handle code not compressing the ajax_page_state variable attached.

@catch One thing with the commit https://git.drupalcode.org/project/drupal/-/merge_requests/4853/diffs?co...

It looks like the $ has gone awol in $ajax_page_state['theme_token']:

    if ($theme === $this->configFactory->get('system.theme')->get('default') || $this->csrfGenerator->validate(ajax_page_state['theme_token'], $theme)) {

Fixed the typo and also implemented #86 (with a tweak to the deprecation message to (hopefully) fit the deprecation message coding standard, never sure when it's not a method deprecation).

The fact that Media Library was (mistakenly) setting ajax_page_state as a query parameter itself shows that this is possible, even if there's no legitimate use-case to set it on purpose, so worth doing.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

@catch Media browser navigation is not working see video it's always in page 0 but if you filter by alphabetic order works.

I also adjust the patch to Drupal 10.1.x

@saidatom the media browser is working for me - are you getting any PHP or JavaScript errors? If you can reproduce the error on your own site, please also try a clean install of Drupal 11.x (umami is the easiest way to test) and check whether you can still reproduce it with that before marking needs work. We also have extensive test coverage of the media library browser which is passing tests (and failed with earlier iterations of the change).

@catch yes, you are right i have facets enabled and that causes the issue without console or php errors.

For me its RTBC.

CC failure in #91

#91 is a 10.1.x backport patch, the MR needs review.

Hiding patches to make it more clear.

claudiu.cristea → made their first commit to this issue’s fork.

We landed here, coming from 💬 Extremely long Views AJAX query string triggers 403 in AWS Postponed: needs info . This compression solves our issue. RTBC for the MR, which is now green after rebasing.

Added a suggestion for the deprecation notice and a question.

Applied the deprecation message suggestion, good rewording. Made a change for the other situation but maintains the current HEAD behaviour (silently don't return anything) rather than throwing an exception. I can see why we'd consider throwing BadRequestException there instead, but that should probably be a follow-up.

Appears feedback has been addressed.

Opened a follow-up for potentially throwing a BadRequestException when the theme token isn't provided and a non-default theme is requested. 📌 Consider throwing BadRequestException in AjaxPageStateNegotiator Active .

We do a lot of manual JSON encoding/decoding around this:

    $ajax_page_state = json_decode(UrlHelper::uncompressQueryParameter($ajax_page_state), 1);
    $ajax_page_state['libraries'] = $ajax_page_state['libraries'] . $fake_library;
    $ajax_page_state = UrlHelper::compressQueryParameter(json_encode($ajax_page_state));

Wonder if we should add helper methods to UrlHelper, e.g. [de]compressQueryArray(), which take/return an array instead of a string and handles the JSON encoding for us? However this wouldn't be a breaking change so we could do it in a followup.

Also not sure if the new argument to getDrupalSettings() is worth it, we don't need it here and unsure why anyone would need the compressed string; they can extract it from the content if they really need it? We shouldn't add API unless we have an actual use for it.

Other than these two points I think this is ready to go.

Thanks @catch! That addresses the concerns I raised in https://drupal.slack.com/archives/C1BMUQ9U6/p1698224910195289 and the concerns raised by @longwave in #104.

I updated the CR to reflect the new approach.

Looks good to me!

The new approach looks cleaner but there is a little bit of cleanup to do in the middleware compared to the previous version.

You're right, good catch! Applied the suggestion 👍

One more question about ajax.js, otherwise this looks ready.

Seems to have 1 open thread to revert some changes.

No that's outdated, see https://git.drupalcode.org/project/drupal/-/merge_requests/5123#note_222448 which already made the suggested change five days ago, marked as resolved though.

I'm not eligible to RTBC this as such, but I think I can RTBC the one commit to remove newly out-of-scope changes since #109, so restoring the previous RTBC.

Reviewed the changes and added questions about code that seems unnecessary.

Resolved two of these and answered the others, since the actual code changes are extremely minor, taking the liberty of re-RTBCing.

lauriii → closed merge request !5123

lauriii → closed merge request !4853

Committed b2a606d and pushed to 11.x. Also cherry-picked to 10.2.x. Thanks!

Is there any chance this would be rerolled for Drupal 10.1 also? We noticed the same issue in our staging environment which is behind a proxy. No issues in local.

I tried the latest 10.1 version of the patch from #91 but I get mixed results. It seems to fix the pagination issue of the views Ajax calls but exposed forms are broken. Previously also the exposed filters used GET with requests but now the pagination only loads the content with Ajax and exposed filters actually reload the page and pass the exposed values to the URL.

We had the same error on a Media pagination but on version 10.1.x.
Why this is not applied to that version?

We add a temporary patch if any had the same issue in the 10.1.x without the tests classes.

I had the same issue with Drupal 10.1.x and Media Directories and it has been resolved with #122.

#122 resolved same issue Drupal 10.1.6 and media_library view.

To my comment

We had the same error on a Media pagination but on version 10.1.x.
Why is this not applied to that version?

We got the response from Catch on the Slack contribute

because it's potentially a breaking change if a module is doing something strange with ajax_page_state.

Well, some drupal 10.1.x are reporting that the paginators are broken, so the "breaking change" was already introduced.

Thank you, Eduardo
The patch in #122 fixed the issue with Media Library and pagers.
After upgrading to latest Drupal ~10.1.0

Hoping that the committed fix in 11.0.x, and 10.2.x to be back-ported to 10.1.x

As explained in #125, because there is some potential for breaking custom code and contrib modules that depend upon ajax_page_state, there is no chance of this being committed to 10.1.x. However, sites can use patch #122 until 10.2.0 is released in a few weeks.

Noted;
Thanks, Damien, for the quick comment.
Happy with that, I will wait for Drupal ~10.2.0 stable tag release.
Doing more testing, after updating our custom ajax_page_state

Well, some drupal 10.1.x are reporting that the paginators are broken, so the "breaking change" was already introduced.

Yes but this is restricted to 10.1.x sites with some kind of CDN or varnish rule preventing long URLs. If we backported this and broke several contributed modules in a patch release of 10.1.x, then that would break things for another subset of 10.1.x sites (and the ones affected would still need to update to the patch release to get the benefit).

10.2.0 will be out in a couple of weeks.

While the patch in #122 definitely helped, the request is still going well over our Varnish limit size. Do we need to figure out how to let these requests through or is there some way to reduce this?? Loading the Media Browser and clicking through to a second page (with #122 applied) looks like this (2,390 chars not including headers):

https://dev.www.my-test-site.com/testing/views/ajax?ajax_form=1&_wrapper...

@dreg one of the earlier approaches on this issue compressed all of ajax_page_state instead of just the libraries, but was considered too much of an API change. I'm not sure if that would have reduced things enough for your varnish settings though.

Looking at the URL you posted, this bit seems a bit egregious and comes in at 1499 characters, so could open a follow-up issue to take a look at that.

media_library_opener_id=media_library.opener.field_widget&media_library_allowed_types%5Bimage%5D=image&media_library_selected_type=image&media_library_remaining=1&media_library_opener_parameters%5Bfield_widget_id%5D=field_media_image%3A-field_paragraph_band-0-subform&media_library_opener_parameters%5Bentity_type_id%5D=paragraph&media_library_opener_parameters%5Bbundle%5D=page_header&media_library_opener_parameters%5Bfield_name%5D=field_media_image&media_library_opener_parameters%5Bentity_id%5D=725&media_library_opener_parameters%5Brevision_id%5D=797&hash=MdCytfJWxVGpEfN1LYH6YKwhAuiPKz_o-u8JaIl7hyY&_wrapper_format=drupal_ajax&view_name=media_library&view_display_id=widget&view_args=image&view_path=%2Fnode%2F105%2Fedit&view_base_path=admin%2Fcontent%2Fmedia-widget&view_dom_id=5d0e1f479c58d847ab8977967424d7ed258a264e1a11b05cc3b3abb68e179991&pager_element=0&ajax_form=1&_wrapper_format=drupal_ajax&media_library_opener_id=media_library.opener.field_widget&media_library_allowed_types%5Bimage%5D=image&media_library_selected_type=image&media_library_remaining=1&media_library_opener_parameters%5Bfield_widget_id%5D=field_media_image%3A-field_paragraph_band-0-subform&media_library_opener_parameters%5Bentity_type_id%5D=paragraph&media_library_opener_parameters%5Bbundle%5D=page_header&media_library_opener_parameters%5Bfield_name%5D=field_media_image&media_library_opener_parameters%5Bentity_id%5D=725&media_library_opener_parameters%5Brevision_id%5D=797&hash=MdCytfJWxVGpEfN1LYH6YKwhAuiPKz_o

I think media library should probably be using the same approach as is being used here.

I wonder if we cannot pass this as a var in the private temp store, or something similar, instead of GET. It looks unusual big for something to be delivered in the URL. I know we’re doing it in other places (autocomplete?). But honestly, I haven’t looked into to see whether it’s doable

@catch
Gotcha. Would you suggest a new issue thread or piggy backing here? We were able to get around the Varnish issue but def still an ugly request. Also in debugging saw the request vars have a big duplicate section. All of this is in there twice:

media_library_opener_id=media_library.opener.field_widget
&media_library_allowed_types%5Bimage%5D=image
&media_library_selected_type=image
&media_library_remaining=1
&media_library_opener_parameters%5Bfield_widget_id%5D=field_media_image%3A-field_paragraph_band-0-subform
&media_library_opener_parameters%5Bentity_type_id%5D=paragraph
&media_library_opener_parameters%5Bbundle%5D=page_header
&media_library_opener_parameters%5Bfield_name%5D=field_media_image
&media_library_opener_parameters%5Bentity_id%5D=725
&media_library_opener_parameters%5Brevision_id%5D=797
&hash=MdCytfJWxVGpEfN1LYH6YKwhAuiPKz_o-u8JaIl7hyY

@dreg I went ahead and opened 🐛 media_library_opener leads to massive GET requests that break varnish etc. Active

After applying #122, I appear to be getting a JS error after each AJAX call in Drupal.behaviors.fieldGroup:

Uncaught TypeError: Cannot read properties of undefined (reading 'Effects')

@beerendlauwers please open a separate issue with steps to reproduce, it looks like you at least have field_group module installed?

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

Still figuring out why for creating a new issue, but we've found that in some edge case TBD, it tries to decompress an already decompressed state, which ends up producing an ajax error because ajax_page_state[libraries] ends up being FALSE.

I suspect what I'm seeing is another variant of 🐛 Warning: Undefined array key 1 in Drupal\Core\Asset\LibraryDependencyResolver->doGetDependencies() after update to Drupal 10.2.2 and when views ajax is enable. Active , so will follow-up there.

Ran into this on 10.1 and the patch at #122 helped.

Noting here that we have the Field Group module installed and I was unable to reproduce the Field Group errors mentioned above. Wondering if those errors are related to something else in the set up or maybe an issue with this patch and Field Group specifically on 10.2.

I vote that this should be reopened given that it was only closed due to inactivity and is still a major issue against 10.1 which is currently a supported minor version.

Hi,
this is still an issue, right? The views pager is not working on my site when AJAX is enabled. Tested with Drupal 10.2.0, 10.2.1, 10.2.2 and 10.2.3.

The most likely cause of a broken pager is that you need to upgrade facets to the dev release. The latest stable release of Facets does not support the new pager AJAX.

+1 for re-opening

#122 patch did not fix the issue for our page that's hosted on aws and we're getting 403 from the server. To make it harder to debug, the media library pagination works locally and on our CI.

I was thinking about writing an http_middleware that is checking the request size and creates a watchdog warning if request is to big. Then at least we'd know we're going to have problems on production environments.

This is how our request looks like:

GET /views/ajax?media_library_opener_id=media_library.opener.editor&media_library_allowed_types%5B0%5D=image&media_library_allowed_types%5Bdocument%5D=document&media_library_allowed_types%5Bgallery%5D=gallery&media_library_allowed_types%5Bvideo%5D=video&media_library_selected_type=image&media_library_remaining=1&media_library_opener_parameters%5Bfilter_format_id%5D=restricted_html&media_library_opener_parameters%5Btenant%5D=<id>&media_library_view=media_library&media_library_display=widget_table&hash=<hash>&_wrapper_format=drupal_modal&_wrapper_format=drupal_ajax&view_name=media_library&view_display_id=widget_table&view_args=image&view_path=%2Fmedia-library&view_base_path=admin%2Fcontent%2Fmedia-widget-table&view_dom_id=d9cf4f37403929685b2da3338f20b4af842d6a45f430e626c1acfaa79e4abc69&pager_element=0&media_library_opener_id=media_library.opener.editor&media_library_allowed_types%5B0%5D=image&media_library_allowed_types%5Bdocument%5D=document&media_library_allowed_types%5Bgallery%5D=gallery&media_library_allowed_types%5Bvideo%5D=video&media_library_selected_type=image&media_library_remaining=1&media_library_opener_parameters%5Bfilter_format_id%5D=restricted_html&media_library_opener_parameters%5Btenant%5D=<id>&media_library_view=media_library&media_library_display=widget_table&hash=<hash>&_wrapper_format=drupal_modal&page=1&_drupal_ajax=1&ajax_page_state%5Btheme%5D=<theme>&ajax_page_state%5Btheme_token%5D=<token>Q&ajax_page_state%5Blibraries%5D=<924chars-of-hash> HTTP/2
Host: cms.int.infoportal.cloud.wholesaleservices.de
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:109.0) Gecko/20100101 Firefox/115.0
Accept: application/json, text/javascript, */*; q=0.01
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate, br
Referer: https://<domain>/node/add/article/<id>
X-Requested-With: XMLHttpRequest
Connection: keep-alive
Cookie: SSES<session-cookie> AWS<220-chars of was cookies>
Sec-Fetch-Dest: empty
Sec-Fetch-Mode: cors
Sec-Fetch-Site: same-origin
DNT: 1
Sec-GPC: 1
TE: trailers

and response is pretty simple:

HTTP/2 403 Forbidden
server: awselb/2.0
date: Thu, 20 Jun 2024 10:52:57 GMT
content-type: text/html
content-length: 118
X-Firefox-Spdy: h2