Skip to content

Conversation

@DanielNill
Copy link
Contributor

Arrays with null values are treated as nested objects. This causes something like {foo: ["bar", null, "buzz"]} to be serialized at foo[]=bar&foo[1]=&foo[]=buzz when really it should be serialized as foo[]=bar&foo[]=&foo[]=buzz

This 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.

@DanielNill DanielNill force-pushed the serialize_arrays_of_null branch from 783ab70 to af7c1ca Compare June 29, 2015 20:41
@DanielNill DanielNill force-pushed the serialize_arrays_of_null branch from af7c1ca to 9f4f7ba Compare June 29, 2015 20:43
@mgol
Copy link
Member

mgol commented Jun 29, 2015

Thanks for the PR. If we want to change it, we should just use jQuery.type that already returns "null" for null, it was created specifically to workaround faulty typeof behavior.

Besides that, this sounds fine to me barring any back compat problems. Could you sign our CLA?

Copy link
Member

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.

@dmethvin
Copy link
Member

This change looks good to me too. There are only a handful of places where we treat undefined and null equally and we do that via an == null comparison. In a quick scan of the jQuery code there seem to be some simple tests like .data(null, ...) that also pass the object test but those are already outside the domain of legal values and we don't guarantee behavior there. If you find cases that need work though, please open another issue.

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.

@DanielNill DanielNill force-pushed the serialize_arrays_of_null branch from 784bc01 to 4e14263 Compare July 3, 2015 05:26
@DanielNill
Copy link
Contributor Author

I've signed the CLA with full name DanielNill and email daniellnill@gmail.com
and my git config shows:

[user]
    name = DanielNill
    email = daniellnill@gmail.com

Not sure why the bot is still marking this unsigned.

Copy link
Contributor

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.

@mgol
Copy link
Member

mgol commented Jul 3, 2015

You're missing a space between your first & last name. That's probably why the CLA check still fails.

@DanielNill
Copy link
Contributor Author

@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?

@mgol
Copy link
Member

mgol commented Jul 5, 2015

@mzgol a little confused, does the bot check for a space?

I think it does. It definitely doesn't accept everything as a name.

Since, at this point I'd have multiple commits that would need to be amended what is the best way to handle that?

You can squash your commits first, then amend the remaining commit via:

git commit --amend --author="FirstName LastName <login@domain>"

and:

git push --force

@jzaefferer
Copy link
Member

The CLA check is fine now.

@jzaefferer
Copy link
Member

Sorry, that was a false positive. We don't consider "DanielNill" a valid name; that should probably be "Daniel Nill"?

@mgol mgol self-assigned this Aug 16, 2015
@mgol mgol added this to the 3.0.0 milestone Sep 7, 2015
@mgol mgol removed their assignment Sep 7, 2015
@mgol mgol added the Serialize label Sep 7, 2015
@mgol mgol closed this in 3d7ce0a Sep 7, 2015
mgol pushed a commit that referenced this pull request Sep 7, 2015
@mgol
Copy link
Member

mgol commented Sep 7, 2015

Landed, thanks!

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Development

Successfully merging this pull request may close these issues.

6 participants