#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

    Alex,

    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.

    @Juriy

    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

Attachments

Pages