Ticket #2452 (closed bug: fixed)
$.post and $.get are probably not handling the abcense of data well
| Reported by: | adsmart | Owned by: | flesler |
|---|---|---|---|
| Priority: | major | Milestone: | 1.4 |
| Component: | ajax | Version: | 1.2.3 |
| Keywords: | Cc: | adsmart, nathanhammond | |
| Blocking: | Blocked by: |
Description
I was looking for the post and get code today (I'm planning on replicating them for the DELETE and PUT verbs... those should be added) and I noticed a funny problem with the following (GET has the same problem):
post: function( url, data, callback, type ) {
if ( jQuery.isFunction( data ) ) {
callback = data;
data = {};
}
return jQuery.ajax({
type: "POST",
url: url,
data: data,
success: callback,
dataType: type
});
},
You make a check to see if the second argument is a function, indicating that no data is being sent. That's fine but it ignores the fact that the type might also be passed. As a result, the following would have unintended effects:
$.post("/sample/test", do_stuff_function, "text/html");
It also doesn't take into account
$.post("/sample/test", "text/html");
Which, while probably silly in most cases, is legitimate within the published docs for the functions.
I suspect the following would be a better solution (depending on how you feel about using arguments.length, you could use other methods to identify things):
if (arguments.length == 2 ) {
type = data;
callback = function () {};
data = {};
} else if (arguments.length == 3) {
type = callback;
callback = data;
data = {};
};
Attachments
Change History
comment:2 Changed 5 years ago by ygirouard
A fullproof solution would probably be to force users to pass an array of options (array object or json), much like you do for $.ajax(); That way it avoids having to deal with omited arguments since you reference each as an object property.
For example:
$.get({url: "/sample/test", callback: do_stuff_function, type: "html"});
Then you can test each parameter individually, and if set to 'undefined' you can ignore it or set a default.
i.e.:
post: function(params) {
if (params.data == undefined || params.data == ""){params.data = {};}
// Do other testing here if needed...
return jQuery.ajax({
type: "POST",
url: params.url,
data: params.data,
success: params.callback,
dataType: params.type
});
},
Changed 5 years ago by nathanhammond
-
attachment
argshift.diff
added
Makes sure the argument shift is complete.
comment:4 Changed 5 years ago by flesler
- Status changed from new to assigned
- Cc adsmart, nathanhammond added
- Priority changed from critical to major
- Owner set to flesler
- Milestone changed from 1.2.4 to 1.3
- need changed from Review to Commit
Ok, thanks for the patch. I'll review this asap.
comment:5 Changed 4 years ago by paul.irish
Ticket #3775 has gotten some attention on this issue, too. I think we can close it as a dupe.
Ariel, can we drop in Nathan's non-intrusive patch?
comment:6 Changed 4 years ago by JDay
A patch is included with dupe #4384 with a test case and a fix for the comment typo.
Please follow the bug reporting guidlines and use jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.
