Ajax: Support binary data (including FormData) #5197

mgol · 2023-01-16T13:46:34Z

Summary

NOTE: Please don't review the first commit; that one is #5196, it was just easier for me to depend on it. (that PR landed)

Two changes have been applied:

prefilters are now applied before data is converted to a string;
this allows prefilters to disable such a conversion
a prefilter for binary data is added; it disables data conversion
for non-string non-plain-object data; for FormData bodies, it
removes manually-set Content-Type header - this is required
as browsers need to append their own boundary to the header

Ref gh-4150

+39 bytes. We should discuss whether we want this fully in core or just the small change of running prefilters early enough for this to be possible. It is quite generic, though, with the small exception of special FormData treatment - which, I think, is not necessarily desired in all cases? Unless it is - then the PR overhead would be +20 bytes instead of +39.

Checklist

New tests have been added to show the fix or feature works
Grunt build and unit tests pass locally with these changes
If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com: Document binary data support for 4.0 api.jquery.com#1220

gibson042 · 2023-01-23T17:28:25Z

src/ajax.js

+		// Apply prefilters
+		inspectPrefiltersOrTransports( prefilters, s, options, jqXHR );
+
 		// Convert data if not already a string
 		if ( s.data && s.processData && typeof s.data !== "string" ) {
 			s.data = jQuery.param( s.data, s.traditional );
 		}

-		// Apply prefilters
-		inspectPrefiltersOrTransports( prefilters, s, options, jqXHR );


I think moving prefilters ahead of string coercion is a breaking change with respect to custom prefilters, cf. https://api.jquery.com/jQuery.ajaxPrefilter/ (emphasis mine):

originalOptions are the options as provided to the $.ajax() method, unmodified and, thus, without defaults from ajaxSettings

Is there a way to instead establish something like prefilter phases?

The line that now happens after the prefilters is only setting data, not options. Is there any change that I'm missing?

BTW, I would also consider this a breaking change regardless of the above, that's why I'm targeting it at v4.

timmywil

Good for 4.0. Might need a note in docs about the prefilter going before stringifying s.data, if that was ever documented.

mgol · 2023-01-23T17:51:13Z

src/ajax/binary.js

+	// `Content-Type` for requests with `FormData` bodies needs to be set
+	// by the browser as it needs to append the `boundary` it generated.
+	if ( s.data instanceof window.FormData ) {
+		s.contentType = false;


Note to self: check if this could be overridden in options if this is set to false for all binary data, not just FormData.

OK, this cannot be changed via settings as of now. 😕 I'll need another patch, I think.

Two changes have been applied: * prefilters are now applied before data is converted to a string; this allows prefilters to disable such a conversion * a prefilter for binary data is added; it disables data conversion for non-string non-plain-object `data`; for `FormData` bodies, it removes manually-set `Content-Type` header - this is required as browsers need to append their own boundary to the header Ref jquerygh-4150

PR jquerygh-5197 started treating all non-string non-plain-object `data` values as binary. However, `jQuery.ajax` also supports arrays as values of `data`. This change makes regular arrays no longer be considered binary data. Surprisingly, we had no tests for array `data` values; otherwise, we'd detect the issue earlier. This change also adds a few such missing tests. Ref jquerygh-5197

mgol · 2023-02-06T17:54:16Z

I missed handling array data values; a fix at #5203.

The way jquerygh-5197 implemented binary data handling, `processData` was being explicitly set to `false`. This is expected but it made it impossible to override it to `true`. The new logic will only set `processData` to `false` if it wasn't explicitly passed in original options. Ref jquerygh-5197

The way gh-5197 implemented binary data handling, `processData` was being explicitly set to `false`. This is expected but it made it impossible to override it to `true`. The new logic will only set `processData` to `false` if it wasn't explicitly passed in original options. Closes gh-5205 Ref gh-5197

PR jquerygh-5197 started treating all non-string non-plain-object `data` values as binary. However, `jQuery.ajax` also supports arrays as values of `data`. This change makes regular arrays no longer be considered binary data. Surprisingly, we had no tests for array `data` values; otherwise, we'd detect the issue earlier. This change also adds a few such missing tests. Ref jquerygh-5197

PR gh-5197 started treating all non-string non-plain-object `data` values as binary. However, `jQuery.ajax` also supports arrays as values of `data`. This change makes regular arrays no longer be considered binary data. Surprisingly, we had no tests for array `data` values; otherwise, we'd detect the issue earlier. This change also adds a few such missing tests. Closes gh-5203 Ref gh-5197

PR gh-5197 started treating all non-string non-plain-object `data` values as binary. However, `jQuery.ajax` also supports arrays as values of `data`. This change didn't land on `3.x-stable`; however... Surprisingly, we had no tests for array `data` values. This change backports a few such missing tests added in gh-5203. Ref gh-5197 Ref gh-5203

mgol added Ajax Needs review Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. labels Jan 16, 2023

mgol added this to the 4.0.0 milestone Jan 16, 2023

mgol self-assigned this Jan 16, 2023

mgol mentioned this pull request Jan 16, 2023

Make ajax understand other acceptable data #4150

Closed

mgol force-pushed the ajax-prefilters-before-data-gh-4150 branch from 3584c0b to 909c466 Compare January 16, 2023 13:51

gibson042 reviewed Jan 23, 2023

View reviewed changes

mgol added the Behavior Change label Jan 23, 2023

timmywil removed the Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. label Jan 23, 2023

timmywil approved these changes Jan 23, 2023

View reviewed changes

mgol commented Jan 23, 2023

View reviewed changes

mgol force-pushed the ajax-prefilters-before-data-gh-4150 branch from 909c466 to 5e8fc4d Compare January 23, 2023 23:51

mgol merged commit a7ed9a7 into jquery:main Feb 1, 2023

mgol removed the Needs review label Feb 1, 2023

mgol deleted the ajax-prefilters-before-data-gh-4150 branch February 1, 2023 12:48

mgol mentioned this pull request Feb 6, 2023

Ajax: Don't treat array data as binary #5203

Merged

2 tasks

mgol mentioned this pull request Feb 8, 2023

Document binary data support for 4.0 jquery/api.jquery.com#1220

Open

mgol mentioned this pull request Feb 8, 2023

Ajax: Allow processData: true even for binary data #5205

Merged

2 tasks

mgol mentioned this pull request Jan 16, 2024

Support dataType 'binary' in jQuery.ajax() #4337

Closed

manooweb mentioned this pull request Sep 5, 2024

trim() is deprecated in the forthcoming jQuery 4 polylang/polylang#1527

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ajax: Support binary data (including FormData) #5197

Ajax: Support binary data (including FormData) #5197

mgol commented Jan 16, 2023 •

edited

Loading

gibson042 Jan 23, 2023

mgol Jan 23, 2023

timmywil left a comment •

edited

Loading

mgol Jan 23, 2023

mgol Feb 8, 2023 •

edited

Loading

mgol Feb 8, 2023

mgol commented Feb 6, 2023

Ajax: Support binary data (including FormData) #5197

Ajax: Support binary data (including FormData) #5197

Conversation

mgol commented Jan 16, 2023 • edited Loading

Summary

Checklist

gibson042 Jan 23, 2023

Choose a reason for hiding this comment

mgol Jan 23, 2023

Choose a reason for hiding this comment

timmywil left a comment • edited Loading

Choose a reason for hiding this comment

mgol Jan 23, 2023

Choose a reason for hiding this comment

mgol Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

mgol Feb 8, 2023

Choose a reason for hiding this comment

mgol commented Feb 6, 2023

mgol commented Jan 16, 2023 •

edited

Loading

timmywil left a comment •

edited

Loading

mgol Feb 8, 2023 •

edited

Loading