Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CJK characters are broken. #731

Closed
wants to merge 1 commit into from
Closed

Conversation

cheolhee
Copy link

@cheolhee cheolhee commented Sep 2, 2015

CJK letters were broken when I sent strings with POST method.
I think this charset should be a default set because the NSData is made with NSUTF8StringEncoding.

mutableURLRequest.HTTPBody = query(parameters!).dataUsingEncoding(NSUTF8StringEncoding, allowLossyConversion: false)

@cnoon
Copy link
Member

cnoon commented Sep 2, 2015

Is there any way you could provide a failing test for this change? That would help us debug to make a more informed decision.

@cnoon
Copy link
Member

cnoon commented Sep 6, 2015

Thanks for your PR @cheolhee!

After much investigation, I've decided to merge this change in 21040ab. I've also added a test around the POST behavior in ba6d3dd.

With that said, I only made this change due to the fact you're seeing issues without the change. I cannot see any downsides to making the change. This thread makes me believe though that the charset should not really be required. Additionally, no actual unicode characters will ever make it into the HTTP body since all parameter key/value pairs are percent escaped prior to be encoded into UTF-8.

I'm assuming your server is requiring the charset when it actually shouldn't. Either way, as I mentioned, I cannot see any downsides to adding it.

For future reference, there's a good amount of information about the encoding algorithm here.

@cnoon cnoon closed this Sep 6, 2015
@cnoon cnoon added this to the 2.0.0 milestone Sep 6, 2015
@cnoon cnoon self-assigned this Sep 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants