Really URL encode and decode strings

[dv]

The old way doesn't create URL-safe strings, if characters such as & or / or : appear, which makes the methods useless to parse URL query params.

As you can see I've updated them so that the old NSxxxStringEncoding constants would still be valid, even though Foundation uses different constants - they get automatically converted.

[markrickert]

Looks like there's an issue with the specs on OSX.

[dv]

Yep, apparently on OSX NSxxxStringEncoding is a valid encoding and won't be changed to a kCFStringEncodingxxx, whereas on iOS it is not valid and would be converted. I've updated the test to be more relaxed about whether it is converted or not, as long as it doesn't throw an error and has the same content it's all good.

[clayallsopp]

Looks like there's still some sort of error on OS X

I believe this will affect existing usage of BubbleWrap, right? ie if you previously expected BW to not escape your slashes, this will now escape them?

[dv]

Indeed, the test doesn't work if the NS UTF16 encoding isn't available on the platform. Hopefully third time's the charm :)

Correct, the output will be different from before. In my opinion this PR fixes it to the correct output, and before it was missing quite a few characters. If you roam the interwibbles searching for "url encode osx" or something like that, you'll note a few people give the original solution (stringByAddingPercentEscapesUsingEncoding) and then others warning them that this does not escape all characters, and instead to use the new solution (CFURLCreateStringByAddingPercentEscapes).

It's a giant mess with lots of confusing answers - a perfect situation for bubblewrap to bring peace :)

Personally I can't imagine any use cases where you'd want to use to_url_encoded without escaping characters that would interfere with a URL. In every other language url_encoded means with all these characters escaped.

[clayallsopp]

Sweet, tests pass. Yeah, I agree that this is a better solution to the original problem, just unsure on whether or not this should be deferred to BW 2.0 when we can break backwards compatibility in accordance with semver. FWIW I think it can go either way, since this can also be viewed as a bug fix

any opinions @markrickert @colinta

[colinta]

I lean towards calling the old behavior "broken". I'm going to update SugarCube's escape_url method to behave correctly.

[colinta]

FWIW, my REAL opinion is that BW shouldn't pollute, but I don't think I'm winning that argument...

[clayallsopp]

@dv an you add an extra argument to to_url_encoded and to_url_decoded, which when true will preserve the old behavior? & default it to false.

we'll keep it temporarily, until 2.0 (in case folks make new issues about their URLs being broken etc). then will merge this :+1:

[dv]

Sure thing @clayallsopp, how does this look?

Let me know if you want me to squash the commits.

[dv]

The Travis fails on a bug that's showed up a couple of times now, but also disappears completely at random. I can't reproduce it locally, and I'm pretty sure if Travis would redo the test, there's a 50% chance the bug would not show up.

There's something wrong, somewhere, but I don't know what or where :)

[clayallsopp]

Hm yeah I forced travis to rebuild and it passed. Very strange. Will merge and address the issue later if it comes up