#15 enhancement
Alex Potsides

String.toQueryParams() not handling % properly

Reported by Alex Potsides | April 15th, 2008 @ 05:07 PM | in 1.7

I think. At least, it doesn't handle % very well. I've just downloaded the latest version of the Prototype source and added the following test case to testToQueryParams in string.html, which it currently fails with a "Malformed URI" error:

this.assertHashEqual({'key1': 'va%lue1'}, 'key1=va%lue1'.toQueryParams(), 'rogue percent symbol test');

I've attached a patch file which makes the above test pass although it could probably be implemented in a less kludgy way. Apply the patch file to /sstephenson-prototype-master/src/string.js

There's a little write up here: http://www.achingbrain.net/alex/...

And some demo pages here: http://www.achingbrain.net/files...

I've tested it on Safari 3, Firefox 2, Opera 9 and IE 7.

Thanks for an excellent framework.

Comments and changes to this ticket

  • Juriy Zaytsev

    Juriy Zaytsev April 15th, 2008 @ 07:01 PM

    How about just decodeURIComponent(encodeURIComponent(string)) ?

    This also affects getHeaderJSON, btw.

  • Alex Potsides

    Alex Potsides April 15th, 2008 @ 07:19 PM

    Because it then fails the test case named 'proper decoding' with the message:

    expected <#<Hash:{'a b': 'c', 'd': 'e f', 'g': 'h'}>>, actual: <#<Hash:{'a%20b': 'c', 'd': 'e%20f', 'g': 'h'}>>

    Anyway, without wishing to sound rude, if you were going to do

    var key = decodeURIComponent(encodeURIComponent(string));

    you might as well just do

    var key = string;
  • Juriy Zaytsev

    Juriy Zaytsev April 15th, 2008 @ 07:44 PM


    but the point of such wrapping is to pass already encoded string into decodeURIComponent. I don't see how this is the same. Though, of course, I might be missing something.

  • Andrew Dupont

    Andrew Dupont April 15th, 2008 @ 09:47 PM

    How about

    var key = decodeURIComponent(string.gsub('%', encodeURIComponent));

    That'll do it, right?

  • Juriy Zaytsev

    Juriy Zaytsev April 15th, 2008 @ 09:59 PM

    Isn't gsub kindof unnecessary here?

    decodeURIComponent(string.replace(/%/g, encodeURIComponent))
  • Tobie Langel

    Tobie Langel April 15th, 2008 @ 11:25 PM

    Juriy, afaik, some versions of safari don't support passing a method as a second argument to String#replace.

  • Alex Potsides

    Alex Potsides April 16th, 2008 @ 10:09 AM

    I tried adding the above ideas to my local copy of Prototype and they both fail the unit test I pasted in the initial post.

    They both cause the 'proper decoding' unit test to fail with the message:

    Failure: proper decoding
    expected <#<Hash:{'a b': 'c', 'd': 'e f', 'g': 'h'}>>, actual: <#<Hash:{'a%20b': 'c', 'd': 'e%20f', 'g': 'h'}>>

    I suppose the problem is that the methods above indiscriminately encode all % symbols, rather than just ones that are not part of an encoded character.


    If the process performed by encodeURIComponent is essentially the reverse of decodeURIComponent, then the action you suggest appears redundant.

    To break it out onto multiple lines:

    var string = "my string";
    string = encodeURIComponent(string); // "my%20string"
    string = decodeURIComponent(string); // "my string"

    Even with pre-encoded characters:

    var string = "my%20string";
    string = encodeURIComponent(string); // "my%2520string"
    string = decodeURIComponent(string); // "my%20string"
    // call it again for kicks
    string = decodeURIComponent(string); // "my string"

    Do you know of a case where this will not be true?

  • Tobie Langel

    Tobie Langel April 16th, 2008 @ 10:22 PM

    • State changed from “new” to “enhancement_patch”
    • Title changed from “[PATCH] [TEST] String.toQueryParams() not handling % properly” to “String.toQueryParams() not handling % properly”
  • Tobie Langel

    Tobie Langel April 18th, 2008 @ 01:16 PM

    • State changed from “enhancement_patch” to “enhancement”
  • John-David Dalton

    John-David Dalton May 29th, 2008 @ 04:48 AM

    • Milestone set to 1.7
    • Assigned user set to “Juriy Zaytsev”
  • T.J. Crowder

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

    • Assigned user cleared.

    [responsible:none bulk edit command]

  • Radoslav Stankov

    Radoslav Stankov March 2nd, 2010 @ 09:49 AM

    • Tag set to needs:patch, section:lang

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