#126 enhancement
Peter Gyongyosi

toQueryString fails with Arrays

Reported by Peter Gyongyosi | May 29th, 2008 @ 06:46 PM | in 2.0

If you try to use toQueryString on a Hash that was created from an Array, the results include the built-in functions. See the following test case I added to the Hash unit test:

  testArrayToQueryString: function(){
    var test = new Array();
    test = ["a", "b", "c"];
    this.assertEqual("0=a&1=b&2=c", $H(test).toQueryString());
  },

The answer we actually got is

0=a&1=b&2=c&each=function%20(iterator.....

This is especially annoying if you try to pass an Array as a parameter in an Ajax.Request, like in this test case:

  testArrayRequest: function() {
    this.assertEqual("", $("content").innerHTML);
    
    this.assertEqual(0, Ajax.activeRequestCount);

    var request = new Ajax.Request("../fixtures/hello.js", {
                        asynchronous: false,
                        parameters: new Array(),
                        method: 'GET',
                        evalJS: 'force'
    });

    this.assertEqual("../fixtures/hello.js", request.url);

  },

Even though we passed an empty array as parameters, the GET url is filled with the code gibberish as seen above, which is no surprise as Ajax.Request::request() ultimately uses toQueryString from Hash.

I could only test this with FF2 and Konqueror on Linux at the moment, but it should be easy to reproduce this on other platforms using the test cases above.

Comments and changes to this ticket

  • Peter Gyongyosi

    Peter Gyongyosi May 29th, 2008 @ 06:49 PM

    I failed to add that this happens with the latest version from the git repo at the moment, namely:

    0e7b2f42b56d11e777a7f68d276ead8aef2976ea

  • Juriy Zaytsev

    Juriy Zaytsev May 29th, 2008 @ 07:47 PM

    • State changed from “new” to “invalid”

    Peter,

    Hash is only guaranteed to work "reliably" with objects (i.e. objects created by an Object function). Values of any other "type" (e.g. number, string, array, etc.) all produce undesired effect.

    This happens because #toQueryString is based on #each method (mixed into a Hash klass from Enumerable) which in its turn is based on plain for/in iteration. for/in essentially enumerates over members from the prototype chain of an object. Since Prototype.js extends native objects (Array, Number, String, etc.) with helper methods, those methods are all enumerated-over.

    As far as Ajax.Request, parameters are treated as an object, not an array - therefore gibberish output.

    All you need to do is pass an empty object literal:

    ...
    parameters: {},
    ...
    
  • Peter Gyongyosi

    Peter Gyongyosi May 30th, 2008 @ 12:13 AM

    Juriy,

    thank you for your detailed answer. As a matter of fact, I actually did look around and found out what you've told about the Hash#each method before opening this ticket -- that's what I've reported as a bug :) Maybe I wasn't specific enough, sorry for the confusion. (And obviously my problem is not that I cannot pass an empty "parameters" parameter to Ajax.Request...)

    In my opinion, converting an Array to a Hash or an Object can be done quite seamlessly and naturally: the numerical indexes should be converted into hash keys or property names. That's what the user expects and even the syntax (stuff[key]) is similar. And this conversion, if applied before saving the object into Hash's internal _object property, solves our problems with the improper #each iterations.

    I've attached a patch that enables us to do this through an Array#toObject method. I also changed some small things in the Ajax and Hash units to make use of it, and this did eliminate my previous problems. I also added some basic test cases for these changes. Please consider adding it to the upstream code, as it would solve some really confusing problems.

    If for whatever reasons you decide not to, I have two suggestions regarding the documentation:

    1) at the description of Hash, include your remarks from above about Hash not working "reliably" when initialized with anything but an Object

    2) at Ajax.Options, the description for parameters says:

    This can be provided either as a URL-encoded string

    or as any Hash-compatible object (basically

    anything),

    For me, "basically anything" does include an Array, and most certeanly does not mean "Object, and nothing else", which, according to you, is the case as it uses toQueryString() internally :)

  • Juriy Zaytsev

    Juriy Zaytsev June 2nd, 2008 @ 06:44 AM

    • Milestone set to 1.7
    • State changed from “invalid” to “enhancement”
    • Assigned user set to “Juriy Zaytsev”

    ok, points taken.

    Couple of notes:

    1) I like the idea of having Array#toObject as it seems to be quite useful on its own.

    2) I don't see a reason to use "this" in #toObect

    ...
    toObject: function(){
      var results = {};
      this.each(function(value, i) {
        results[i] = value;
      })
      return results;
    }
    ...
    

    or even using a plain loop (for performance reasons):

    ...
    toObject: function(){
      var results = {};
      for (var i=0, l=this.length; i<l; i++) {
        results[i] = this[i];
      }
      return results;
    }
    ...
    

    3) If we were to implement #toObject, then Hash cnstructor should probably check for #toObject existence, rather than checking object's type, so:

    ...
    this._object = (object && Object.isFunction(object.toObject)) ? object.toObject() : Object.clone(object);
    ...
    

    same with Ajax.Base - it's better to go the duck-typing way rather than check for type:

    ...
    else if (Object.isFunction(this.options.parameters.toObject))
      this.options.parameters = this.options.parameters.toObject();
    ...
    
  • Peter Gyongyosi

    Peter Gyongyosi June 2nd, 2008 @ 10:07 AM

    Thank you for considering this, it will sure save people some headaches.

    Your notes are absolutely valid -- my code was just a scetch-up of what I thought should be added, I am in no way a diehard JS or Prototype hacker :) Do you need a patch with those changes or my duty here is done?

  • T.J. Crowder

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

    • Assigned user cleared.

    [responsible:none bulk edit command]

  • Matti Järvinen

    Matti Järvinen January 19th, 2010 @ 06:08 PM

    Debugged code because of this issue for a while today.

    Any news on this?

  • Tobie Langel

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

    • Milestone changed from 1.7 to 2.0
  • Tobie Langel

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

    • Tag set to section:dom
  • Dan Dean

    Dan Dean March 1st, 2010 @ 04:09 AM

    Shouldn't this be tagged "section:lang" not "section:dom" ?

  • Tobie Langel

    Tobie Langel March 1st, 2010 @ 07:36 AM

    • Tag changed from section:dom to section:lang

    Yeah. I was just half-asleep when I tagged it. TY.

  • yopai

    yopai April 12th, 2010 @ 11:41 PM

    See my post in #445 (marked as invalid !?).

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

Attachments

Referenced by

Pages