- Notifications
You must be signed in to change notification settings - Fork20.6k
Ajax: Support binary data (including FormData)#5197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
3584c0b to909c466Compare| // 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 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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):
originalOptionsare the options as provided to the$.ajax()method,unmodified and, thus, without defaults fromajaxSettings
Is there a way to instead establish something like prefilter phases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The line that now happens after the prefilters is only settingdata, 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 left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good for 4.0. Might need a note in docs about the prefilter going before stringifyings.data, if that was ever documented.
| // `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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Note to self: check if this could be overridden in options if this is set tofalse for all binary data, not justFormData.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK, this cannot be changed via settings as of now. 😕 I'll need another patch, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
PR:#5205
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 headerRefjquerygh-4150
909c466 to5e8fc4dComparePRjquerygh-5197 started treating all non-string non-plain-object`data` values as binary. However, `jQuery.ajax` also supportsarrays as values of `data`. This change makes regular arraysno longer be considered binary data.Surprisingly, we had no tests for array `data` values; otherwise,we'd detect the issue earlier. This change also addsa few such missing tests.Refjquerygh-5197
I missed handling array |
The wayjquerygh-5197 implemented binary data handling, `processData`was being explicitly set to `false`. This is expected but it madeit impossible to override it to `true`. The new logic will onlyset `processData` to `false` if it wasn't explicitly passedin original options.Refjquerygh-5197
PRjquerygh-5197 started treating all non-string non-plain-object`data` values as binary. However, `jQuery.ajax` also supportsarrays as values of `data`. This change makes regular arraysno longer be considered binary data.Surprisingly, we had no tests for array `data` values; otherwise,we'd detect the issue earlier. This change also addsa few such missing tests.Refjquerygh-5197
PRgh-5197 started treating all non-string non-plain-object`data` values as binary. However, `jQuery.ajax` also supportsarrays as values of `data`. This change makes regular arraysno longer be considered binary data.Surprisingly, we had no tests for array `data` values; otherwise,we'd detect the issue earlier. This change also addsa few such missing tests.Closesgh-5203Refgh-5197
PRgh-5197 started treating all non-string non-plain-object`data` values as binary. However, `jQuery.ajax` also supportsarrays 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 ingh-5203.Refgh-5197Refgh-5203
Uh oh!
There was an error while loading.Please reload this page.
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:
this allows prefilters to disable such a conversion
for non-string non-plain-object
data; forFormDatabodies, itremoves manually-set
Content-Typeheader - this is requiredas browsers need to append their own boundary to the header
Refgh-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 specialFormDatatreatment - which, I think, is not necessarily desired in all cases? Unless it is - then the PR overhead would be+20bytes instead of+39.Checklist