Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ajax: Don't process data property on no-entity-body requests #3781

Closed
wants to merge 2 commits into from

Conversation

dmethvin
Copy link
Member

@dmethvin dmethvin commented Sep 12, 2017

Fixes gh-3438

Summary

The description in gh-3438 is pretty detailed. Basically, don't process settings.data if settings.processData is false. This allows a settings.beforeSend hook to have its way with the original data and, for example, serialize it to JSON on the URL.

I'd consider this a long-standing bug and the current documentation would imply it should have worked this way all along. It should be safe to include in a minor release.

Checklist

@mention-bot
Copy link

@dmethvin, thanks for your PR! By analyzing the history of the files in this pull request, we identified @markelog, @jaubourg and @gibson042 to be potential reviewers.

@jaubourg
Copy link
Member

👎 see my comment on the issue

(and sorry to be late to the party)

@gibson042
Copy link
Member

I'm still in favor of this change, but I observed while combing through the code that modifying an ajax URL in beforeSend (as in the example) breaks our caching logic. We may want to open an issue for ensuring that cacheURL always corresponds to the request as issued.

P.S. It's great to see you here, @jaubourg. And you're not late at all.

@jaubourg
Copy link
Member

Hey @gibson042, nice to see you too :)

@timmywil
Copy link
Member

What @jaubourg said makes sense to me. It's not that the data should be ignored, it's that it should not be processed. It should still get used if passed. Otherwise, why pass it a GET request at all? Also, I don't see a use case for passing a non string in a !hasContent request.

@gibson042
Copy link
Member

This is my interpretation of desired behavior as discussed in the meeting. It should be implemented in this PR and documented in https://github.com/jquery/api.jquery.com/blob/master/entries/jQuery.ajax.xml#L145 .

processData (default: true)
Type: Boolean
When true, non-string request data is serialized with $.param, and data is additionally re-encoded for payloads with contentType "application/x-www-form-urlencoded" (the default) or moved into URL query parameters for methods which are issued without a payload (e.g., "GET" and "HEAD"). When false, such processing is skipped (except for the special case of string data, which is still moved into the URL of no-content requests).

};
} );

ajaxTest( "jQuery.ajax() - data - process string with GET", 2, function( assert ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation?

@dmethvin dmethvin closed this in d723789 Jan 16, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Ajax always processes data for requests without entity body
6 participants