-
Notifications
You must be signed in to change notification settings - Fork 20.5k
Serialize: handle arrays with null values #2436
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
Conversation
783ab70 to
af7c1ca
Compare
af7c1ca to
9f4f7ba
Compare
|
Thanks for the PR. If we want to change it, we should just use Besides that, this sounds fine to me barring any back compat problems. Could you sign our CLA? |
test/unit/serialize.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use spaces between elements, as well as after { & [, before } & ] etc. Yes, the surrounding style is wrong, we'll fix it but it doesn't seem worth to make it bad for new code as well. ;) For more, read https://contribute.jquery.org/style-guide/js/#spacing.
|
This change looks good to me too. There are only a handful of places where we treat This issue has probably existed so long because it's a rare case not defined by any standard. Serializing JavaScript objects is best done with JSON and not form encoding, this case never occurs with HTML forms. Might as well make it do something reasonable though! It seems unlikely the current behavior is being depended upon by anyone. |
784bc01 to
4e14263
Compare
|
I've signed the CLA with full name DanielNill and email daniellnill@gmail.com Not sure why the bot is still marking this unsigned. |
test/unit/serialize.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, you should put a tab instead of spaces to match the other indentation.
|
You're missing a space between your first & last name. That's probably why the CLA check still fails. |
|
@mzgol a little confused, does the bot check for a space? I assumed that as long as my name signed on the cla matched my name in my git config it would authorize it. Since, at this point I'd have multiple commits that would need to be amended what is the best way to handle that? |
I think it does. It definitely doesn't accept everything as a name.
You can squash your commits first, then amend the remaining commit via: and: |
|
The CLA check is fine now. |
|
Sorry, that was a false positive. We don't consider "DanielNill" a valid name; that should probably be "Daniel Nill"? |
…ry into serialize_arrays_of_null
|
Landed, thanks! |
Arrays with null values are treated as nested objects. This causes something like
{foo: ["bar", null, "buzz"]}to be serialized atfoo[]=bar&foo[1]=&foo[]=buzzwhen really it should be serialized asfoo[]=bar&foo[]=&foo[]=buzzThis error is caused because javascript's typeof operator treats null as an object.
It might be worth reviewing http://www.ecma-international.org/ecma-262/5.1/#sec-11.4.3 to make sure there aren't other cases of other poorly typed entities causing similar problems.