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

crash maybe due to incorrect array length #18

Open
younes0 opened this issue Mar 16, 2014 · 7 comments
Open

crash maybe due to incorrect array length #18

younes0 opened this issue Mar 16, 2014 · 7 comments
Labels
Milestone

Comments

@younes0
Copy link

younes0 commented Mar 16, 2014

I got this form, with the following json structure:

[{"name":"slots[1395086400]","value":"0"},{"name":"slots[1395086400]","value":"1"},{"name":"slots[1395090000]","value":"0"},{"name":"slots[1395090000]","value":"1"},{"name":"slots[1395172800]","value":"0"},{"name":"slots[1395172800]","value":"1"},{"name":"slots[1395176400]","value":"0"},{"name":"slots[1395176400]","value":"1"},{"name":"slots[1395259200]","value":"0"},{"name":"slots[1395259200]","value":"1"},{"name":"slots[1395262800]","value":"0"},{"name":"slots[1395262800]","value":"1"},{"name":"slots[1395345600]","value":"0"},{"name":"slots[1395345600]","value":"1"},{"name":"slots[1395349200]","value":"0"},{"name":"slots[1395349200]","value":"1"},{"name":"slots[1395432000]","value":"0"},{"name":"slots[1395432000]","value":"1"},{"name":"slots[1395435600]","value":"0"},{"name":"slots[1395435600]","value":"1"},{"name":"slots[1395518400]","value":"0"},{"name":"slots[1395518400]","value":"1"},{"name":"slots[1395522000]","value":"0"},{"name":"slots[1395522000]","value":"1"}]"

when calling serializeObject() on this form , this is what I get logged :

Object {slots: Array[1395522001]}
  slots: Array[1395522001]
    1395086400: "1"
    1395090000: "1"
    1395172800: "1"
    1395176400: "1"
    1395259200: "1"
    1395262800: "1"
    1395345600: "1"
    1395349200: "1"
    1395432000: "1"
    1395435600: "1"
    1395518400: "1"
    1395522000: "1"
    length: 1395522001 // really?!
  __proto__: Array[0]
  __proto__: Object

Trying to access the slots property causes immediate crash with Chrome, not in Firefox, but they both have the incorrect length value.

Edit: the form consists of multiple checkboxes, and each has its checkbox with negative value so it can be sent even if it's unchecked. See: http://stackoverflow.com/questions/1809494/post-the-checkboxes-that-are-unchecked

@younes0
Copy link
Author

younes0 commented Mar 16, 2014

Try it by yourself: http://pastebin.com/ZpzgbVS4

NB: I don't get this problem with Tobias Cohen' plugin

@macek
Copy link
Owner

macek commented Mar 16, 2014

The "problem" here is that numerical keys are serialized as indices on an array. An illustration:

var arr = [];
arr[3] = 1;
arr.length; // 4

Arrays are going to behave the same, even if you use larger numbers

var arr = [];
arr[1395522000] = 1;
arr.length; // 1395522001

One solution for you would be to have data like

<input name="slots[0][id]" value="1395518400">
<input name="slots[0][value]" value="1">

<input name="slots[1][id]" value="1395522000">
<input name="slots[1][value]" value="1">

<!-- slot[2][id], slot[2][value], etc -->

As an alternative, I have considered changing how numerical keys are serialized using objects instead. That would make the code read something more like this

var arr = {};
arr[1395522000] = 1;
arr.length; // undefined

The primary difference here is we're sort of cheating using an object, but at least it doesn't blow up the "array" when we use large indices.

Advantages:

  • use large indices
  • API for accessing specific key stays the same: arr[n];

Disadvantages:

  • no .length property
  • iterating becomes more complex

However, to make this workable for everyone, I am considering opening up the serializing functions so that they could be overridden when you have corner cases such as your own.

Here's what the default would be. This is the current implementation

FormSerializer.factories.fixed = function(key, value, build) {
  return build([], key, value);
};

You could override this in your own code as such

FormSerializer.factories.fixed = function(key, value) {
  var obj = {};
  obj[key] = value;
  return obj;
};

Or to be a little more elegant, this would do the same thing

FormSerializer.factories.fixed = function(key, value, build) {
  return build({}, key, value);
};

To tie it all together, here's a complete implementation example you might use

<script src="jquery.js"></script>
<script src="jquery.serialize-form.js"></script>
<script>
  // override numeric key factory
  FormSerializer.factories.fixed = function(key, value, build) {
    return build({}, key, value);
  };

  // serialize form
  $("form").submit(function() {
    console.log( $(this).serializeObject() );
  });
</script>

Let me know your thoughts on this.

@macek
Copy link
Owner

macek commented Mar 16, 2014

And to be fair, you're right, you won't get this "problem" with Tobias Cohen's code, but his code will also not serialize your nested keys. You'll just get {"slots[1395522000]":1} with which you will have to do additional parsing/manipulation on the server side.

@macek macek added the feature label Mar 16, 2014
@younes0
Copy link
Author

younes0 commented Mar 16, 2014

Okay, I get it. The main problem is that I overrided numerical key arrays which may be a bad practice.
Your first solution looks fine to me, I'm more willing to change the HTML.

The design flaw in this case beeing the unsent unchecked checkoxes and the solution I set up, you could simply add this very particular case as a notice in the documentation instead, this would be enough. I personally never encountered other similar issue.

@macek
Copy link
Owner

macek commented Mar 16, 2014

@younes0 thanks following up.

I'm glad the first solution seems to work for you.

I will still probably end up inverting the control of the factories so that people can override behavior where desired. It's a non-urgent feature, but I'll leave this issue open until I've implemented it.

Please let me know if I can help you in any other way.

@younes0
Copy link
Author

younes0 commented Mar 17, 2014

That sounds good, thanks.

@macek macek added this to the 3.x milestone Sep 23, 2014
@YOzaz
Copy link

YOzaz commented May 5, 2016

@macek please bring back possibility to overwrite factories, as I'm facing same issue as well. Lots of the frameworks makes life much easier if you're using CRUD object ID as form data key.
I understand that it doesn't comply with W3C directive(s), however - post-processors like PHP do not generate additional NULL values in arrays when submitting a form.

Alternatively, maybe, you can just add a config flag to force casting arrays to objects? Something like:

// foo[]
if (patterns.push.test(k)) {
   var idx = incrementPush(root.replace(/\[\]$/, ''));
   value = build( config['cast_arrays'] ? {} : [], idx, value);
}
// foo[n]
else if (patterns.fixed.test(k)) {
   value = build( config['cast_arrays'] ? {} : [], k, value);
}

Configuration can be done through params in serializeObject() function, or on global prototype I suppose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants