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

Add initial IDN support #515

Merged
merged 2 commits into from Dec 18, 2013

Conversation

Projects
None yet
2 participants
@soulseekah
Contributor

soulseekah commented Dec 17, 2013

The org.wordpress.android.ui.accounts.SetupBlog.getSelfHostedXmlrpcUrl method does not accept IDN names.
Punycode, however, is accepted. While URLUtil.isValidUrl says that it's a completely valid URL, the ApiHelper.getRSDMetaTagHrefRegEx method does not like it at all.
So I'm proposing the use of java.net.IDN to convert non-ASCII URLs to valid ASCII punycode for custom blogs.

If you'd like access to an IDN domain, I have one in production, and I'll be happy to give out credentials for testing.

Add initial IDN support
IDNs are not currently supported by the client, but punycode is. This
commit converts IDNs to punycode before attempting anything.
@roundhill

This comment has been minimized.

Contributor

roundhill commented Dec 17, 2013

This looks good, I would like to take you up on your offer to test it on an IDN domain. Can you email the details to android@automattic.com?

@soulseekah

This comment has been minimized.

Contributor

soulseekah commented Dec 18, 2013

@roundhill, e-mail sent. Let me know how it goes.

@roundhill

This comment has been minimized.

This block of code needs to be moved to before the code that you added, because if I don't enter the 'http://' in the URL box, then the URL is not converted to punycode.

This comment has been minimized.

Owner

soulseekah replied Dec 18, 2013

I'm missing an else condition when there's no http, true.
Moving the code lower will mess up the IDN.toASCII function, it does not like http in ther, so it has to be stripped beforehand if it exists.
This is why I'm getting the substring IDN.toASCII(url.substring(7));
I will also fix the pesky tabs vs. spaces issue I'm noticing.

This comment has been minimized.

roundhill replied Dec 18, 2013

OK sounds good.

IDN support bugfix
Missing else convert for when there's no http(s)
Fixed tabs vs. spaces
@soulseekah

This comment has been minimized.

Contributor

soulseekah commented Dec 18, 2013

See latest commit, it should take care of the issues.

You should probably be able to cherry pick it if needed. Or I can recreate the pull request with one clean commit once all iterations are verified here.

roundhill added a commit that referenced this pull request Dec 18, 2013

Merge pull request #515 from soulseekah/develop
Add initial IDN URL support to blog setup. Props @soulseekah.

@roundhill roundhill merged commit 735074a into wordpress-mobile:develop Dec 18, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment