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

Improve punycode handling with libidn #63

Merged
merged 3 commits into from
Nov 11, 2015
Merged

Improve punycode handling with libidn #63

merged 3 commits into from
Nov 11, 2015

Conversation

walro
Copy link
Contributor

@walro walro commented Nov 5, 2015

Requires libidn.

We need to verify that it works in the current locations:

  • Travis
  • Heroku (will try in production) 
  • Datacenter (will try in production) 

Fixes #48.

Should be somewhat more performant as a regex check is cheaper than
PublicSuffix´s parse. It should also help with avoiding running into
exceptions in PublicSuffix´s parse (I am guessing a URL with with a bad
scheme is more likely to have a “bad” domain).
@walro walro changed the title WIP: Fix #48 Fix #48 Nov 5, 2015
@walro
Copy link
Contributor Author

walro commented Nov 11, 2015

@dentarg think this a viable solution?

@dentarg dentarg changed the title Fix #48 Improve punycode handling with libidn Nov 11, 2015
@dentarg
Copy link
Collaborator

dentarg commented Nov 11, 2015

Yes, sure

@dentarg
Copy link
Collaborator

dentarg commented Nov 11, 2015

I wonder if apt-get install libidn11 works, doesn't look like a big difference here:

http://packages.ubuntu.com/precise/idn
http://packages.ubuntu.com/trusty/idn

vs

http://packages.ubuntu.com/precise/libidn11
http://packages.ubuntu.com/trusty/libidn11

@walro
Copy link
Contributor Author

walro commented Nov 11, 2015

Yeah, I wrote about it in the issue #48 (comment)

@dentarg
Copy link
Collaborator

dentarg commented Nov 11, 2015

My guess is that "idn" is just a "wrapper package", i.e. it does nothing except requiring the other package (libidn)

@dentarg
Copy link
Collaborator

dentarg commented Nov 11, 2015

I also wonder what idn2 and IDNA2008 is, seen on http://packages.ubuntu.com/search?keywords=idn

@walro
Copy link
Contributor Author

walro commented Nov 11, 2015

My guess is that "idn" is just a "wrapper package", i.e. it does nothing except requiring the other package (libidn)

Think so too. They both require libc6, so I think just libidn11 is fine. I checked one of our app-servers and it (libidn11) was already installed, so I think this will work just fine.

@dentarg
Copy link
Collaborator

dentarg commented Nov 11, 2015

Yeah, but I think we should add it to ansible playbooks anyways

@walro
Copy link
Contributor Author

walro commented Nov 11, 2015

Yeah, but I think we should add it to ansible playbooks anyways

👍

@walro
Copy link
Contributor Author

walro commented Nov 11, 2015

Yeah, but I think we should add it to ansible playbooks anyways

Fixed in https://github.com/twingly/ansible/commit/c8d999f8464fc7a81a51d18d54c690069a7719fc

@walro
Copy link
Contributor Author

walro commented Nov 11, 2015

I think easiest path is to merge this, release bugfix release and try it out on a Heroku app that sees a lot of urls (Twitter?) and see how it goes.

@walro
Copy link
Contributor Author

walro commented Nov 11, 2015

I also wonder what idn2 and IDNA2008 is, seen on http://packages.ubuntu.com/search?keywords=idn

Yeah, but I think that's "nice to have knowledge", I think we should proceed with what we have going now.

I think it will work fine.
@dentarg
Copy link
Collaborator

dentarg commented Nov 11, 2015

Yes, I agree with all comments above

@walro
Copy link
Contributor Author

walro commented Nov 11, 2015

Great!

walro added a commit that referenced this pull request Nov 11, 2015
Improve punycode handling with libidn
@walro walro merged commit f033062 into master Nov 11, 2015
@walro walro deleted the fix/48 branch November 11, 2015 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants