Skip to content

Ajax: Overwrite s.contentType with content-type header value, if any #4650

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Apr 6, 2020
Merged

Ajax: Overwrite s.contentType with content-type header value, if any #4650

merged 5 commits into from
Apr 6, 2020

Conversation

wenz
Copy link
Contributor

@wenz wenz commented Mar 29, 2020

Summary

Overwrite s.contentType with content-type header value, if any
Fixes #4119

Checklist

Sorry, something went wrong.

@jsf-clabot
Copy link

jsf-clabot commented Mar 29, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@wenz
Copy link
Contributor Author

wenz commented Mar 29, 2020

Is there a way to re-run the jsf-clabot? My Git email settings were incorrect; now, the commit is correctly linked to my GitHub account.

@mgol
Copy link
Member

mgol commented Mar 29, 2020

@wenz You need to amend the existing commit with new author information & then git push --force-with-lease. You can also modify the commit message to be more precise.

@wenz wenz force-pushed the 4119-encode-percent-20 branch from fa8db2b to 3b7dab1 Compare March 30, 2020 05:27
@wenz
Copy link
Contributor Author

wenz commented Mar 30, 2020

@mgol done - thanks for the pointer!

mgol
mgol requested changes Mar 30, 2020
View reviewed changes
mgol
mgol reviewed Mar 30, 2020
View reviewed changes
wenz and others added 2 commits March 30, 2020 23:01
@wenz @mgol
Update src/ajax.js
48abff8 
Declare loop variable at the beginning of the prefilter

Co-Authored-By: Michał Gołębiowski-Owczarek <m.goleb@gmail.com>
@wenz
Ajax: Test for contentType override via header
8ee7197 
Fixes jquery#4119
mgol
mgol reviewed Mar 30, 2020
View reviewed changes
@mgol mgol added the Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. label Mar 30, 2020
@mgol mgol added this to the 3.5.0 milestone Mar 30, 2020
@mgol mgol mentioned this pull request Mar 30, 2020
Closed
@wenz @mgol
Update test/unit/ajax.js
efce159 
Break long line

Co-Authored-By: Michał Gołębiowski-Owczarek <m.goleb@gmail.com>
mgol
mgol approved these changes Mar 31, 2020
View reviewed changes
Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM! We need to think whether to include this in 4.0.0 or 3.5.0 (I'd opt for the latter, but we need to evaluate whether this should be treated as a breaking change or not) but the change looks good anyway.

@timmywil
Copy link
Member

Yes, this might change behavior, but I consider this a bug fix. You'd have to set the header and then expect that header to be ignored.

@mgol
Copy link
Member

mgol commented Apr 1, 2020

You'd have to set the header and then expect that header to be ignored.

You could have a header bag that you add to the request via a helper local to the project & then rely on being able to override that via dataType (although this override was never complete in the end). But yes, it does seem to be an edge case.

@mgol mgol mentioned this pull request Apr 6, 2020
Closed
4 tasks
@timmywil timmywil removed the Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. label Apr 6, 2020
@mgol mgol removed the Needs review label Apr 6, 2020
@mgol mgol changed the title Ajax: overwrite contentType property with header Ajax: Overwrite s.contentType with content-type header value, if any Apr 6, 2020
@mgol mgol merged commit 7fb90a6 into jquery:master Apr 6, 2020
mgol added a commit that referenced this pull request Apr 6, 2020
@wenz @gibson042 @mgol
Ajax: Overwrite s.contentType with content-type header value, if any
065143c 
This fixes the issue of "%20" in POST data being replaced with "+"
even for requests with content-type different from
"application/x-www-form-urlencoded", e.g. for "application/json".

Fixes gh-4119
Closes gh-4650

(cherry picked from 7fb90a6)

Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
Co-authored-by: Michał Gołębiowski-Owczarek <m.goleb@gmail.com>
@mgol
Copy link
Member

mgol commented Apr 6, 2020

Landed on master in 7fb90a6 & on 3.x-stable in 065143c.

Thanks for the PR & keeping up with our feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgol mgol

Assignees
No one assigned
Labels
Ajax
Milestone
3.5.0
Development

Successfully merging this pull request may close these issues.

Invalid encoding of %20 char sequence in $.ajax post, put body
4 participants
@wenz @jsf-clabot @mgol @timmywil