#10202 closed bug (invalid)
`data` attribute disappears from settings for ajax GET requests
Reported by: | Owned by: | ||
---|---|---|---|
Priority: | low | Milestone: | 1.next |
Component: | ajax | Version: | 1.6.3 |
Keywords: | Cc: | jaubourg | |
Blocked by: | Blocking: |
Description
This is specifically for v1.6.3, everything worked prior to that, from at least 1.4 onward.
If you create an ajax request with jQuery.ajax(type: 'GET', data: someData)
, and then bind handler to the beforeSubmit
event, the data
attribute disappears from the settings object. This only happens for GET requests in 1.6.3. The data attribute does not disappear for POST requests, or for any request types in 1.6.2 and earlier.
I don't know if this was intensional or not (since GET requests also happen to serialize the data
attribute into the URL), but I didn't see it in the changelog, and it messed up the jquery-ujs in Rails.
See http://jsfiddle.net/HnVjX/1/
If you change the JS in the above example to be a non-GET type it works, or alternatively if you change it to jquery 1.6.2, it works.
Change History (18)
comment:1 Changed 11 years ago by
Component: | unfiled → ajax |
---|---|
Priority: | undecided → low |
Resolution: | → worksforme |
Status: | new → closed |
comment:2 Changed 11 years ago by
Please read the ticket description. You changed the type
to "POST". From the original ticket description:
This only happens for GET requests in 1.6.3.
Please re-open this ticket. It is not fixed, and it broke our well-tested jquery-ujs library in core rails.
comment:3 Changed 11 years ago by
Resolution: | worksforme |
---|---|
Status: | closed → reopened |
Whoops! Thanks for noting and thanks for your patience and persistence - greatly appreciated.
comment:4 Changed 11 years ago by
No prob, I know how it goes ;-) If I can get some time, and if nobody else can get to it, I could probably come up with a test case and fix in the next week or so.
comment:5 Changed 11 years ago by
Cc: | jaubourg added |
---|
I'll cc jaubourg on this, maybe he has some insight, in the meantime, yes a test case would be awesome.
comment:6 Changed 11 years ago by
http://bugs.jquery.com/ticket/9682
So, yes, data is removed on purpose for requests with no body.
comment:7 Changed 11 years ago by
Hmm, so it looks like data
was purposely removed from the settings
object, so that if you retry the ajax request with $.ajax(this)
, it won't re-append the data
to the url.
I can definitely see that use-case, but it seems at least equally likely (if not more) that we have some handler bound to *any* of the event hooks that needs access to settings.data
. I'm not sure how breaking one situation to fix the other is a good solution.
Also, in order to workaround the bug in that other scenario, all we had to do is: delete this.data; $.ajax(this)
. But now, in order to workaround *this* bug, and access the data
, we need an entire script to deserialize the url?
comment:8 follow-up: 9 Changed 11 years ago by
For that other bug, if we're going to alter the settings
object anyway, wouldn't have made way more sense to change the processData
attribute to false
rather than delete the data
attribute?
comment:9 Changed 11 years ago by
Replying to [email protected]…:
For that other bug, if we're going to alter the
settings
object anyway, wouldn't have made way more sense to change theprocessData
attribute tofalse
rather than delete thedata
attribute?
Nope, nevermind, that just affects translating the data from an array to a string, it doesn't actually change whether or not it gets appended to the query string.
comment:10 follow-up: 11 Changed 11 years ago by
Milestone: | None → 1.next |
---|---|
Status: | reopened → open |
I'll mark this as a valid regression.
comment:11 Changed 11 years ago by
Replying to timmywil:
I'll mark this as a valid regression.
I completly disagree. This is not a regression.
{ url: "mypath", data: { a: 1 } }
and
{ url: "mypath?a=1" }
are stricly equivalent from an ajax point of view.
If one need to keep track of the original data then:
- use the original options map (never changed)
- create a prefilter that "saves" the data in the complete options map
We're just paying the price for giving access to the internal options map in the callbacks, knowing that the exact behaviour of such and such options in that scenario has never ever been documented.
comment:12 follow-up: 13 Changed 11 years ago by
It seems easy to avoid this problem in the first place. Right now, jQuery always appends the serialized data
attribute to url
. So, to fix that other ticket, jQuery was modified to remove the data
attribute after the first time through, so that there would not be any data left to append on subsequent attempts.
Rather than deleting the data attribute (which seems destructive), why not just check to see if it's already been done?
// If data is available, append data to url if ( s.data && s.url.indexOf(s.data) === -1 ) { s.url += ( rquery.test( s.url ) ? "&" : "?" ) + s.data; }
comment:13 Changed 11 years ago by
Replying to Steve Schwartz <[email protected]…>:
It seems easy to avoid this problem in the first place. Right now, jQuery always appends the serialized
data
attribute tourl
. So, to fix that other ticket, jQuery was modified to remove thedata
attribute after the first time through, so that there would not be any data left to append on subsequent attempts.Rather than deleting the data attribute (which seems destructive), why not just check to see if it's already been done?
// If data is available, append data to url if ( s.data && s.url.indexOf(s.data) === -1 ) { s.url += ( rquery.test( s.url ) ? "&" : "?" ) + s.data; }
Because I could very well do something like this:
$.ajax({ url: "myPath?hello=world", data: { field: value } });
comment:14 follow-up: 15 Changed 11 years ago by
I don't understand. That would result in url: myPath?hello=world&field=value
the first time around, and then it wouldn't change on subsequent tries. This is exactly as jQuery currently does, except that the code I posted doesn't delete the data
attribute.
comment:15 Changed 11 years ago by
Replying to Steve Schwartz <[email protected]…>:
I don't understand. That would result in
url: myPath?hello=world&field=value
the first time around, and then it wouldn't change on subsequent tries. This is exactly as jQuery currently does, except that the code I posted doesn't delete thedata
attribute.
What about:
$.ajax({ url: "myPath?hello=world", data: { ello: worl } });
?
comment:16 Changed 11 years ago by
Ah, that's a good point. Replace my original code example with this:
var dataAppendedToUrl = (new RegExp("(&|\\?)" + s.data + "(&|$)")).test(s.url); if ( s.data && !dataAppendedToUrl ) { s.url += ( rquery.test( s.url ) ? "&" : "?" ) + s.data; }
I have this coded up, complete with test, and all tests passing. See pull request 544: https://github.com/jquery/jquery/pull/544.
comment:17 Changed 11 years ago by
Resolution: | → invalid |
---|---|
Status: | open → closed |
This is not a regression. Rationale and possible adaptation to the now correct behaviour ajax exhibits are given in http://bugs.jquery.com/ticket/10202#comment:11 (I'd go with a prefilter personally).
comment:18 Changed 11 years ago by
I posted this on the github ticket, but I'll repost here, in response to comment 11 and why {p: 6}
and "p=6"
are not technically equivalent, and thus why this is still needed.
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.
Works for me... http://jsfiddle.net/rwaldron/HnVjX/2/