#378 ✓resolved
Tobias Lütke

Form#serializeElements fails to honor html order

Reported by Tobias Lütke | October 8th, 2008 @ 12:16 AM | in 1.7

This causes nested array syntax to fail:



<input name="facts[][key]" type="text" value="a" />
<input name="facts[][value]" type="text" value="a" />
<input name="facts[][key]" type="text" value="b" />
<input name="facts[][value]" type="text" value="b" />

Will be submitted as



facts[][key]=a&facts[][key]=b&facts[][value]=a&facts[][value]=b

which produces totally wrong results on the server side in a rails or php application.

Comments and changes to this ticket

  • Tobie Langel

    Tobie Langel October 8th, 2008 @ 07:20 PM

    • State changed from “new” to “bug”
  • Juriy Zaytsev

    Juriy Zaytsev October 10th, 2008 @ 06:12 PM

    Tobias,

    What's the proper way to serialize a form as in your example? Sequentially, in document order?

    
    facts[][key]=a&facts[][value]=a&facts[][key]=b&facts[][value]=b
    

    The thing is that serialization happens into an object first, and only then translated into a string (if true was not passed as Form#serialize first argument). Names are represented as "keys" in an object and are, of course, grouped together:

    
    {
      'facts[][key]': ['a', 'b'],
      'facts[][value]': ['a', 'b']
    }
    

    If document order matters, representing serialized form as an object doesn't seem like the best solution.

  • Tobias Lütke

    Tobias Lütke October 11th, 2008 @ 08:51 PM

    Unfortunately the core of the problem is that the form is first serialized to a hash. Because of the way it makes arrays of duplicated keys the value order will be unlike the actual form in the HTML and will lead to different server side behavior.

    It seems that to address the problem Form#serialize needs to emit an array of hashes instead and toQueryParam() has to be made aware of this format:

    
    
    [{'facts[][key]':a},{'facts[][value]':a}, ..]
    
    
  • Juriy Zaytsev

    Juriy Zaytsev October 12th, 2008 @ 02:06 AM

    Yeah, this is a waste of memory : )

    
    [
      { 'facts[][key]': 'a' },
      { 'facts[][value]': 'a' },
      { 'facts[][key]': 'b'  },
      { 'facts[][value]': 'b' }
    ]
    

    P.S. Don't want to sound pedantic, but "those" are not really "hashes" - just simple objects (there are also prototype.js' Hash objects which are different from native objects)

  • Juriy Zaytsev

    Juriy Zaytsev October 12th, 2008 @ 02:07 AM

    • Assigned user set to “Juriy Zaytsev”
    • Milestone set to 1.7
  • Tobie Langel

    Tobie Langel October 12th, 2008 @ 03:10 AM

    As per specs :

    The control names/values are listed in the order they appear in the document.

    That's definitely something we should fix.

  • Juriy Zaytsev

    Juriy Zaytsev October 12th, 2008 @ 04:52 AM

    Changing Form.serialize to respect elements order (and so return an array instead of an object) is a pretty serious back-compat issue, isn't it? : /

    We could work around it by returning an array augmented with key-named properties (as we do in some of the offset/dimensions-related methods). Besides poorer performance, this wouldn't work with forms that have numeric-named controls (e.g. <input type="text" name="23">). Are those even valid values?

    Perhaps push it to 2.0 (and not worry about back-compat)?

  • Diego Perini

    Diego Perini November 4th, 2008 @ 04:33 PM

    Isn't this problem related to the selectors returning randomly ordered results sets ?

    I mean, are we sure the order of "elements" passed to Form#serialize are really "document ordered" ?

  • Juriy Zaytsev

    Juriy Zaytsev November 4th, 2008 @ 06:05 PM

    Diego,

    Fortunately, result is ordered properly, as it is collected with getElementsByTagName('*') and then filtered afterwards:

    
    getElements: function(form) {
        return $A($(form).getElementsByTagName('*')).inject([],
          function(elements, child) {
            if (Form.Element.Serializers[child.tagName.toLowerCase()])
              elements.push(Element.extend(child));
            return elements;
          }
        );
      },
    
  • Tobie Langel

    Tobie Langel July 24th, 2009 @ 02:26 AM

    • Tag changed from needs_patch, needs_tests, serialize to missing:tests, needs_patch

    [not-tagged:"needs_tests" tagged:"missing:tests" bulk edit command]

  • Tobie Langel

    Tobie Langel July 24th, 2009 @ 02:28 AM

    • Tag changed from missing:tests, needs_patch to missing:patch, missing:tests

    [not-tagged:"needs_patch" tagged:"missing:patch" bulk edit command]

  • Tobie Langel

    Tobie Langel July 24th, 2009 @ 03:36 AM

    • Tag changed from missing:patch, missing:tests to missing:patch, needs:tests

    [not-tagged:"missing:tests" tagged:"needs:tests" bulk edit command]

  • Tobie Langel

    Tobie Langel July 24th, 2009 @ 03:37 AM

    • Tag changed from missing:patch, needs:tests to needs:patch, needs:tests

    [not-tagged:"missing:patch" tagged:"needs:patch" bulk edit command]

  • subimage

    subimage September 1st, 2009 @ 11:55 AM

    Here's my crack at fixing this problem, since I really need this done for the project I'm working on at the moment.

    I touched prototype.js in a couple places...

    Form.serializeElements

    If a name like "things[][foo]" is encountered I'm adding an associative array keyed on "things[]" to the results object.

    Thus, you'll get results looking something like this for a form...

    
    results {
      'name': 'my name',
      'things[]': [
        {'name': 'abc', 'weight': '100lbs'},
        {'name': 'xyz', 'weight': '25lbs'}
      ]
    }
    

    Object.toQueryPair

    My second modification allows toQueryPair to accept this new format from serializeElements.

    It's not too heavily tested, but it's working on my setup right now. I've included a quickie test HTML file with an example of the fix. You're all welcome to beautify it as necessary.

    I ran into an issue of the query string not being properly ordered if I don't pass the hash object itself to Ajax.request as params. As long as you call Form.serialize(form, true) it works beautifully. Someone might want to investigate the cause of that.

    I'm not sure if this is 100% done but I wanted to motivate someone to look at this and help get it finished. This is a huge bug in an otherwise flawless library.

  • subimage

    subimage September 7th, 2009 @ 10:55 PM

    What's the protocol for providing tests?

  • Samuel Lebeau

    Samuel Lebeau September 8th, 2009 @ 12:19 AM

    @subimage : Just take a look at test/unit/object_test.js and test/unit/dom_test.js and you should be all set. You can find further information on how to run tests on the Contribute page.

    Thanks for taking time to contribute !

  • T.J. Crowder

    T.J. Crowder November 16th, 2009 @ 04:50 PM

    • Assigned user cleared.

    [responsible:none bulk edit command]

  • Tobie Langel

    Tobie Langel March 1st, 2010 @ 12:41 AM

    • Tag changed from needs:patch, needs:tests to needs:patch, needs:tests, section:dom
  • duncanbeevers

    duncanbeevers May 6th, 2010 @ 04:38 AM

    Built in-order tuple-based serialization, backwards-compatible with existing form serialization.

    http://github.com/duncanbeevers/prototype/commit/cbe500765a543960cb...

  • duncanbeevers

    duncanbeevers May 6th, 2010 @ 05:54 PM

    Updating to apply against 1.7_rc1

  • duncanbeevers

    duncanbeevers May 6th, 2010 @ 06:18 PM

    New commit applies against 1.7_rc1
    Serialization to final output format is performed in initial traversal of form elements.

    http://github.com/duncanbeevers/prototype/commit/2223163c832cc46c5d...

  • subimage

    subimage May 6th, 2010 @ 06:26 PM

    Hey Duncan...how's your patch differ from mine?

  • duncanbeevers

    duncanbeevers May 6th, 2010 @ 06:43 PM

    My patch doesn't reify the Rails nested-parameters convention and instead just deals with the issue of duplicate field names explicitly.

    My patch also has tests, and is applied against the source files used to compile Prototype, not to the sprocketized output.

    I think your patch makes sense for people who want to transform nested objects into Rails-style parameters, but my patch simply aims to preserve the correct ordering and naming of the original form inputs.

  • duncanbeevers

    duncanbeevers May 11th, 2010 @ 06:28 PM

    Created separate tests for ordering and serialization.

  • Andrew Dupont

    Andrew Dupont October 17th, 2010 @ 08:30 AM

    • State changed from “bug” to “resolved”
    • Importance changed from “” to “”

    This was resolved in 876ece.

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

The Prototype JavaScript library.

Shared Ticket Bins

Referenced by

Pages