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

Fix #10202. Don't modify data property in ajax, regression from #9682. #544

Closed
wants to merge 1 commit into from

Conversation

JangoSteve
Copy link

This fixes a regression (see #10202 for discussion), which was introduced in #9862 (present in v1.6.3 and 1.6.4). I included a test and a fix.

@@ -664,10 +664,10 @@ jQuery.extend({
if ( !s.hasContent ) {

// If data is available, append data to url
if ( s.data ) {
// Only append once, do not re-append if data is already in url
var dataAppendedToUrl = ( new RegExp( "(&|\\?)" + s.data + "(&|$)" ) ).test( s.url );
Copy link
Member

Choose a reason for hiding this comment

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

Don't declare variables inside of conditions. Declare it at the top of the function body and assign it here

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I intentionally did it that way because it was done that way already on line 679, right below this. I'll update it if you want. I thought it was weird, but I figured if that's the way you guys were doing it, I'd follow along.

EDIT: It's also done that way on line 605. Should all of these be changed?

EDIT 2: Also, on line 437.

Copy link
Member

Choose a reason for hiding this comment

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

You'd need to escape every regexp special characters (think . for instance) in s.data so that you don't have false negative. Talk about a "simple" hack gone wrong in the convoluted department (not to mention performance and memory issues in older browsers).

{ type: "GET", url: "path/to/page", data: { p: 6 } }
// and
{ type: "GET", url: "path/to/page?p=6" }

are strictly equivalent. It's up to the particular plugin you wish to fix for to handle that situation (ie. understand that data is part of the URL in a GET request).

Again: this is not a regression, the data is still there, the requests are strictly equivalent.

Relying on data not being part of the URL in the context of a GET request is the actual bug and it doesn't happen within jQuery core.

@jaubourg jaubourg closed this Oct 12, 2011
@JangoSteve
Copy link
Author

That's fine, we can go with a different solution, this was just one approach.

I don't think it's quite as simple as saying they're equivalent and thus it's not a regression. The problem is this real-world scenario: we have a handler that does something with xhr.data, expecting the object that we just passed to .ajax() in the first place. This has been the case since forever, up until 1.6.3. Do {p: 6} and "p=6" contain the same data? Yes. However, jQuery makes it incredibly easy to convert the former to the latter, but no way to convert the latter to the former. We'd have to use our own function to convert back (I've done this and it's about 30 lines of code).

Since conversion process between the two forms ({p: 6} and "p=6") is trivial one way and difficult the other way, then they're not equivalent in practice.

If adding 30 lines of code to every plugin and app that needs this can be easily avoided by changing one already-existing line of code in here, why not do it? I understand not wanting to use this solution, rightfully so; but there are a number of other ways to go about it, and I'm more than willing to figure it out.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
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.

None yet

3 participants