Batch is dependent on the order of query string parameters

The problem occurs because the request handler overwrites query parameters if they appear further on in the request. This may be a bug in Symfony.

The response then returns the full page html as the 'json' request. As this is not json, the response can not be decoded when it is received by the browser — resulting in the {"message":"Not acceptable format: json"} error.

I've made a patch that replaces op=do_nojs with op=do instead of appending the new query parameter.

This will need testing on a site with CloudFlare and the "Query String Sort" feature enabled.

Hi,

I can confirm I bumped into this issue on a 8.5.6 site and the attached patch has fixed this.

Marking at RTBC and requesting a back port to the 8.5 branch.

Cheers

Tom

Re-roll of patch from #13.

After talking to @dawehner at Drupal Europe, I've added a check and enforced that 'op=do' is added if that query parameter is not present.

I'm not sure how (or even if) this can be tested. The problem only occurs if the query parameters are rearranged, and a third party proxy is being used to do this.

+++ b/core/misc/batch.es6.js
@@ -36,7 +36,12 @@
+        if (~batch.uri.indexOf(`op=do_nojs`)) {

I would suggest to not use ~ here. It is quite an unknown feature of JS. It feels like doing !== -1 would be more readable for most people, but that's maybe just me.

I've changed the conditional as suggested by @dawehner in #21.

I can confirm this problem as well. To fix, I just created a Page Rule to specifically turn off Query String Sort for /batch*
Works like a charm :)

I can confirm as well that I encountered this issue as well on a site behind CloudFlare with default page rules in place. To help people find this issue, here's the visible page output for the error:

An AJAX HTTP error occurred.
HTTP Result Code: 406
Debugging information follows.
Path: https://www.mydrupalsite.com/batch?id=12345&op=do_nojs&op=do
StatusText: Not Acceptable
ResponseText:

{"message":"Not acceptable format: json"}

Adding the following custom page rule in CloudFlare immediately fixed the issue across all subdomains:

*mydrupalsite.com/batch*
Cache Level: Bypass, Query String Sort: Off

Yep, this one bit us too - exact same cause. I think we should also fix the "finished" operation. Even if it doesn't bite us under these specific circumstances, it seems bad to have multiple "op" query parameters in general.

The problem is still present on Drupal 9.1.x / 9.2.x
Here is a patch for it.

The patch style guide violations and we need testing to make sure that the fix fixes the problem and that it will stay fixed in future code updates.

Rerolled #34 as a fork for Drupal 10. (Note that one can download patches from Gitlab MRs or commits, e.g. https://git.drupalcode.org/issue/drupal-2879023/-/commit/c68aa376a7fc262886cb56d9ac762e4b06e6b174.patch - do be sure to download to your actual local/code repo, especially in the case of MRs, so that an online version doesn't get changed under you.)

Still needs tests before it can be MR/review ready.

I've hidden the patch since we now have a merge request, I also updated the MR to fix linting issues and updated the issue summary.

Thanks 👍 but it still needs a test

Yes, I left that tag and left it in needs work.

I also asked for advice on writing batch tests in the contribute channel if you have any guidance.

Hi @m4olivei Good to see your batch test (setup) code. But I think your code could go in the existing MR !9284 and hide your new branch. Basically merge the two branches and hide the other one?

Yep, thanks @oily. I'm first attempting to establish a test that fails without the patch. That's why I opened a separate branch and MR.

At the minute, it doesn't, all tests pass. My initial approach is flawed in that it operates against the current request object, which is built from the $_GET super global, which PHP puts together for us at the beginning of the request. It's here, in the PHP engine, where a duplicate query string param, like op in our case, gets de-duped. So we'll have to find some other way to simulate what Cloudflare is doing. My next thought is to use $_SERVER['REQUEST_URI'], as I think that will have the full initial query string preserved with two op params. I'll parse that out and modify the request object.

Hum, it still passed on https://git.drupalcode.org/project/drupal/-/merge_requests/13277 where I hoped that an existing test in the suite would fail. I'm fairly confident that the event listener that I added to the batch_test module does indeed simulate the behavior of CloudFlare's Query String Sort feature. I verified this by adding the same event listener to some other active module, which does trigger the error in the description. It could be that there are no tests in the test suite that exercise AJAX Batch processing, but I find that hard to believe. Going to leave this for now.

Hid MR !13277 as code is now merged into MR !9284

It could be that there are no tests in the test suite that exercise AJAX Batch processing..

I have reviewed the code. I understand that we now have an event listener to mimic what Cloudfare does but do we need to create functional code coverage in addition? The equivalent of Examples module batch module test?

Where it states in the IS,

"So what happens with batch processing, if the query string sorting is enabled? Let's use real batch request:

/batch?id=18&op=do_nojs&op=do&_format=json

When sorting query string parameters, the request is turned to:

/batch?_format=json&id=18&op=do&op=do_nojs"

do we not now need to create functional test coverage to simulate this? Then to test for the output, which should be in the correct form, not /batch?_format=json&id=18&op=do&op=do_nojs which is incorrect, but the correct form?

Then can also test for the error message, which should not appear? Am I misunderstanding? Do we also need to create test batch processes (2 are defined in batch_example.module). The batch_test core test module includes core/modules/system/tests/modules/batch_test/src/BatchTestDefinitions.php where test batch operations are configured. So perhaps we need to crew new batch definition(s) similar to the ones described in the IS in there?

I think the IS title is confusing. It sounds like 'Batch' ie the Drupal Batch api itself is dependent on blah blah. But the intended meaning seems to be when we/ I run a batch process, not just any batch process, but a batch process that is specifically associated with Cloudfare and also specifically utilises the query string sorting functionality that is used in connection with Cloudfare.

Understood that way it throws up the question, this query string sorting is it specific only Cloudflare and its related operations or is it some kind of standard that could be used in batch processes unconnected with Cloudflare, also? Useful to know the scope of the issue. Does it just fix a Cloudflare problem or could it solve problems related to eg various contrib modules (not just Cloudflare-related contrib modules)?

Drupal batch processing breaks when you use the query string sort feature in cloudflare.

Batch should not depend on the order of the parameters, just the existence.

The event subscriber is to simulate cloudflare.

We probably could do it differently but batch testing is painful.

However the test only isn't failing so test coverage isn't sufficient.

@nicxvan Ok, interesting. Does this mean that say the examples batch_example module were installed. It contains a couple of batch processes, would they be potentially affected if the batch_example module was added to the MR?

If the answer is 'yes' then do we need to create new test batch processes and in such a way that they are affected by the Cloudflare simulation?

We really just need to trigger a batch that alphabetically sorts the parameters and successfully finishes with the patch and fails without.

Honestly if we use the event subscriber we should rename it to alpha sort and remove the company's name.

I ran into this on Acquia Cloud, Drupal 11.3 and Cloudflare Enterprise. All batch operations site-wide were failing.

What I observed:

• Browser sends: /batch?id=536&op=do_nojs&op=do&_format=json
• Drupal logs: /batch?_format=json&id=536&op=do_nojs

I applied the latest patch/MR from this issue to replace op=do_nojs instead of appending a duplicate op=do and it fixed the problem.

The core problem is that batch.js creates URLs with duplicate query parameters: op=do_nojs&op=do. This is bad practice regardless of Cloudflare. Any reverse proxy or CDN that sorts query strings (Varnish, Fastly, Akamai, CloudFront) would trigger it.

Something like QueryStringSortSubscriber or SortedQueryStringSubscriber would be more accurate and wouldn't tie the test to a single vendor. Otherwise this patch is simple and RTBC in my opinion.

Looking at this more, I think there may be a simpler and more fundamental fix available server-side.

The batch controller in _batch_page() switches on the op query parameter to decide what to do. The do_nojs operation exists as a fallback for browsers without JavaScript. It renders an HTML progress page with a meta refresh tag that reloads the page to process the next chunk. The do operation is the AJAX path that processes a chunk and returns a JsonResponse.

I wonder if a JSON request will ever need the do_nojs code path? If the request has _format=json, JavaScript is making that request, and the intended operation is always do.

If so, the server can simply correct for this? Something like:

// JSON requests always use the 'do' operation. The 'do_nojs' fallback
// only applies to HTML requests where JavaScript is not available.
if ($request->getRequestFormat() !== 'html' && $op === 'do_nojs') {
  $op = 'do';
}

Basically: if someone asks for do_nojs on a JSON request, treat it as do, because that combination makes no sense.

If that works, it would make the batch system resilient to any CDN or reverse proxy that normalizes query strings without relying on JavaScript to construct the right URL. I've not tested this, but I wanted to share the idea as it would fix the root cause more elegantly (I think).

I implemented the approach proposed in #63 and tested it on my site, where I'm able to reproduce the original bug.

I applied the attached patch using composer-patches, deployed it to my site, and verified that it works both with and without JavaScript enabled. 👍

I also started reviewing the existing Batch API tests. I don't think there are any tests that cover the op dispatch logic. As a next step, I'll try to create a MR.

@dries re: #62-64 I was going to add tags, Needs IS update but may be worth waiting until approach #62-64 is given the thumbs up by one or two others (ce n'est pas encore un fait accompli). An MR and test will help others to review it.

If #62-64 proves good the IS title could be changed and the content edited to reflect that although Cloudflare config was how the issue was discovered, the issue is not focused either on Cloudflare or on CDN's. It is a Batch API issue.

Would it be appropriate to create a new issue based on #62-64 to fix bug in Batch API and that fixes this issue (as it was conceived prior to #62-64)?

I added two basic tests. Without the fix testDoNojsCorrectedForJsonRequest fails. It gets a render array instead of a JsonResponse because op=do_nojs falls through to the HTML progress page path.

testDoNojsPreservedForHtmlRequest passes in both cases, confirming the fix doesn't break the no-JS HTML fallback.

I created a merge request: https://git.drupalcode.org/issue/drupal-2879023/-/merge_requests/1. I hope I did that right.

Re: #66 Running manual TEST-ONLY test. Output:

https://git.drupalcode.org/issue/drupal-2879023/-/jobs/8935363

The TEST-ONLY request leads to only 1 of the 2 tests failing.

The one passing test is intentional. As explained in comment #66, it adds missing test coverage for the path that works. I wanted to make sure there are no regressions.

Ok, no longer confident that my code review comment is helpful, either!

The reason for my comment is that if this issue is only triggered when the url contains the query parameter _format=json i.e. the format is json, then instead of !== 'html' why not simply == 'json' in the if statement? I am also speculating on whether other formats like xml and api_json are in anyway relevant to this issue (beginning to think they are not) so need to be handled by the if condition?

Turns out that updating the MR was a bit more work than I expected, this fork was missing main since it was so old so I had to push main then start from scratch, I moved @oily's comment over and pushed up the changes from @dries.

I have not fully reviewed the code yet, but it seems a bit more robust than the other approach.

@nicxvan I think #62:

The core problem is that batch.js creates URLs with duplicate query parameters: op=do_nojs&op=do. This is bad practice regardless of Cloudflare. Any reverse proxy or CDN that sorts query strings (Varnish, Fastly, Akamai, CloudFront) would trigger it.

Something like QueryStringSortSubscriber or SortedQueryStringSubscriber would be more accurate...

describes the issue in more depth than so far. This stuff could be used to update the IS soon. The creative 1 passing and 1 failing test seems like it ticks the 'Needs tests' box for this issue.

I have limited experience with CDN's so not sure if you would ever get a url containing _format='xml' or suchlike. But if you do then since there only seems to be about 4 possible different values for _format in a url, hopefully all possibilities can be handled easily in the if statement. If the if statement is good as is then could be close to RTBTC?

Oh, and if it is possible to get _format='xml' etc in the url's then could add a couple of new test scenarios to cover it?

Perhaps I misunderstand? Is it that this bug only affects routes and their query params when a batch operation is taking place. When batch operations are taking place _format query param will only ever be equal to 'json' or 'html'?

Looking at the batch route definitions in system.routing.yml, only two formats can reach _batch_page():

system.batch_page.html:                                                                                                 
    requirements:                                                                                                                                                                           
      _format: 'html'                                             
                                                                                                                                                                                            
  system.batch_page.json:                                         
    requirements:                                                                                                                                                                           
      _format: 'json'

As a result, _format=xml or _format=api_json should result in an error. I tested it with curl -s "https://localhost/batch?id=1&op=do_nojs&_format=api_json", which returns a 'No route found'. Because xml/api_json is blocked by the routing layer, we don't need tests here, in my opinion.

This means that in our new code !== 'html' and === 'json' are functionally equivalent. We could change !== 'html' to === 'json' if preferred.

I initially added a suggestion on the MR to apply,

But on second thought I lean towards !== html since this path likely would not work with xml and api_json even if we were to add those routes at a later date.

Thank you for #77 and #78 and for copying my comment to the MR!

Given #77 I agree !== 'html' is fine. I just tried to resolve the 1 comment but I think only you can resolve it, @nicxvan? Seems ready for RTBTC?

I resolved it, I actually have a couple of other things to check, I will comment back in a few minutes

Thank you for your patience, I finally had a chance to review this properly and I have a couple of comments.

First I checked out the branch then applied the fix from the other MR to see if the tests pass with the original fix.
The tests do not pass, I think they are trying to fix the same issue, but the newer MR is better since it's server side, fixing it JS side is another round trip and get a loop if the CDN does something weird, so I think we can close that MR and focus on this one.

I don't have a site I can test this on at the moment manually though.

I have a couple of questions on the test too I'll post directly on the MR setting them to needs work for those questions which I'll post momentarily.

Finally, I wonder where we should document the fact that the op will automatically change? Maybe a change record is enough?

I tried to refactor the test, but got stuck. The challenge is setting up a real batch with a valid ID that persists across requests while working with redirects.

Looking at how Core tests batch internals (e.g. BatchKernelTest), kernel tests that directly call batch functions seem to be the norm.

Maybe there is a reason batch tests don't use functional test style like StyleSerializerEntityTest?

I agree it's best to drop Cloudflare from the code comments. I made that change.

I looked into adding a finished callback to verify the batch completes, but I don't think it applies. In progressive mode (which is the code path that op=do uses), _batch_do() calls _batch_process(), which returns [$percentage, $message, $task_message] directly. It never calls _batch_finished() ...

The existing assertInstanceOf(JsonResponse::class, $response) is actually a strong assertion for what we're testing. Without the fix, op=do_nojs falls through to the do_nojs switch case, which calls _batch_progress_page() and returns a render array. With the fix, it gets corrected to op=do, which calls _batch_do() and returns a JsonResponse.

So, yeah, the finished callback simply can't fire in this code path and I think the test might be sufficient.

Slightly off topic but possible important to decide the best path forward: while working on this, I started questioning whether the no-JS fallback is still worth maintaining in 2026.

Deprecating the no-JS path would simplify the batch API and eliminate bugs and complexity, including this one.

We introduced the no-JS batch API 19 years ago in May 2007, when JavaScript support in browsers was still unreliable. That has clearly changed.

I can't think of a strong reason to have a no-JS path in 2026, but I'm not up to speed on core's policy. It seems like in 2026, it’s about personal preference, not browser support. I understand people’s desire to block intrusive JavaScript (e.g blocking ads), but this is non-intrusive and could be allowed.

Either way, if we were to decide that the no-JS path can be removed, it probably does not make sense to keep investing in the current issue’s tests.

Worth creating an issue for (if one doesn't already exists)?

I did some research on #86 from a taxi to the airport after DrupalCon Chicago (so not ideal), but I can’t find an official Drupal Core policy requiring a no-JS fallback path.

In practice, it appears that several Core subsystems (e.g. Media Library, Toolbar) require JavaScript and don’t currently provide a fallback. See #3396174: The toolbar should be usable without JS, for example.

I understand this is slightly off-topic, so I’ll stop here, and can create a new issue later, if desired.

So, yeah, the finished callback simply can't fire in this code path and I think the test might be sufficient.

Looking at the code that seems right, something is still nagging me in the back of my mind but I don't see anything so I don't think this is worth holding up further. I might ask in slack.

Slightly off topic but possible important to decide the best path forward: while working on this, I started questioning whether the no-JS fallback is still worth maintaining in 2026.

That is an interesting question, I would assume batches run through drush use a no js route, so we'd probably keep them for that alone.

Either way that is a separate issue for sure, John Cook is the subsystem maintainer for batches, we could ask him.

PS I love this:

    case 'do':
      // JavaScript-based progress page callback.
      $response = _batch_do();
      break;

Thank you all for working in this.

I’m happy to proceed with the current changes.

I’ve looked through the changes and everything looks good.

Whether we need a no-JS fallback is not for me to decide. But without official policy, I’d like to keep it. If a policy is created then we’d need a separate issue for removing the fallback.

I’ve checked the source of Drush (https://github.com/drush-ops/drush/blob/14.x/includes/batch.inc) and Drush doesn’t use the changed code. It has its own batch processing system.

A separate issue might be to change batch.js so that it doesn’t append ops, but replaces the existing values. But that is not needed here.

I’m leaving as “needs review” as manual testing is still required.

Re #88: It seems Drush has its own batch implementation: see drush/includes/batch.inc. Looking at that code for a bit (and for the first time ever), it seems Drush uses parts of Core's batch API but replaces the HTTP orchestration (_batch_page(), _batch_do(), the do_nojs meta refresh path) with its own process spawning. That makes sense.

Interestingly, grepping around, it looks like BrowserTestBase functional tests rely on the no-JS meta refresh path to drive batches. Not sure why, but as a starting point, see UiHelperTrait::checkForMetaRefresh() and it's caller around line 117).

So: it would probably not impact Drush, but likely impact browser tests. 😬

Re #89: I have been using the latest patch in production. I understand and support the need for more testers, but wanted to share that it has been working for me.

I will try test this manually, I will create a custom module locally and call _batch_populate_queue with a batch from it, then use curl to trigger in different parameter orders.

Edit: I'm not having much luck unfortunately, I got the queue populated using the code in the test in a custom module, but trying to trigger it without the batch.js is proving a bit more involved than I have time for at the moment.

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

Yeah I wasn't sure how to move this forward, I wasn't able to manually test as requested, but the code looks right and it's tested, I just couldn't trigger it manually with curl.

This fix feels like it is in the wrong layer. I think we can use vanilla JS so that we only send one op.

I.e. let's do

diff --git a/core/misc/batch.js b/core/misc/batch.js
index 2a055157da7..0cf953361c7 100644
--- a/core/misc/batch.js
+++ b/core/misc/batch.js
@@ -19,7 +19,10 @@
       function updateCallback(progress, status, pb) {
         if (progress === '100') {
           pb.stopMonitoring();
-          window.location = `${batch.uri}&op=finished`;
+          const url = new URL(batch.uri, window.location.origin);
+          url.searchParams.set('op', 'finished');
+          const finalUri = url.toString();
+          window.location = finalUri;
         }
       }
 
@@ -36,7 +39,10 @@
           errorCallback,
         );
         progressBar.setProgress(-1, batch.initMessage);
-        progressBar.startMonitoring(`${batch.uri}&op=do`, 10);
+        const url = new URL(batch.uri, window.location.origin);
+        url.searchParams.set('op', 'do');
+        const finalUri = url.toString();
+        progressBar.startMonitoring(finalUri, 10);
         // Remove HTML from no-js progress bar.
         $progress.empty();
         // Append the JS progressbar element.

I've tested this locally and it works a treat.

Here's why I yhink #99 is the correct fix. The additional op is added in batch.js - it's not the code we're chaging in the MR that was rtbc the nojs thing is always present on the first response to start the batch. It's the JS implementation that has to be responsible for changing it.

I've pushed a test up that covers the change in suggested in #99.

Which MR should be reviewed?

@smustgrave in my mind MR 15594 is the correct fix here as it fixes the code that is responsible for adding the duplicate op query string. It could be argued that the other code makes the system a bit more robust if it is used incorrectly but I think just not using the system incorrectly is the better fix.

Rerolled the patch from MR !15103 against Drupal 10.6.9.
The original patch fails to apply because the _batch_populate_queue() function in form.inc has moved from line ~582 to line ~1001, causing a context mismatch on the @param $set_id hunk.