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

IDN: Fix compile time detection of linidn2 TR46 #1207

Closed
wants to merge 1 commit into from

Conversation

rockdaboot
Copy link
Contributor

No description provided.

@mention-bot
Copy link

@rockdaboot, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @dfandrich and @rousskov to be potential reviewers.

Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I sort of noticed it when browsing but I didn't think it through all the way and didn't react either. Good catch.

@bagder bagder closed this in ba31574 Jan 13, 2017
@bagder
Copy link
Member

bagder commented Jan 13, 2017

Thanks!

@bagder
Copy link
Member

bagder commented Jan 13, 2017

Urgh! We can't use IDN2_TRANSITIONAL. It breaks the IDNA2008 feature for the .de TLD that is the primary reason we need IDNA2008 in the first place. It makes ß get converted to ss.

This is causes test 165 to fail.

@rockdaboot
Copy link
Contributor Author

It depends what you want. If you want TRANSITIONAL, you have to adjust the test. If you want NON-TRANSITIONAL, you are basically at IDNA2008 plus input mapping (NFC, lowercasing, ...).
If you want both, you have to make it selectable with a new option. I considered this for wget/wget2 as well... overhead, but may satisfy users. With an option the default should IMO be NON-TRANSITIONAL.

Suggestion: --idna=<2008|tr46t|tr46>

If you go with that (or similar), I would implement this for wget/wget2 as well.

@bagder
Copy link
Member

bagder commented Jan 13, 2017

Well, I want curl to work with IDN on the .de TLD as they require it. Users won't know what option to use for that, so we need to pick one to use. Changing the test case doesn't make us work with that, only changing the code will. Clearly the transitional option is not good enough for that use case. I took out the transitional again in commit ee35766.

I rather wait for someone to actually need an option to modify the IDNA level before we introduce that.

@rockdaboot
Copy link
Contributor Author

Wait... use NON-TRANSITIONAL. As IDNA2008 but much better...

@bagder
Copy link
Member

bagder commented Jan 13, 2017

Aha! Was that added in the same version as the IDN2_TRANSITIONAL one?

@rockdaboot
Copy link
Contributor Author

For example... German Umlauts (same goes for accents and special northern europe letters) may be in decomposed form (NFD/NFKD) or composed form (NFC/NFKC). TR46 accepts them both due to internal NFC transforming.

Yes, IDN2_NONTRANSITIONAL has been added with the same version as IDN2_TRANSITIONAL.

@bagder
Copy link
Member

bagder commented Jan 13, 2017

Excellent, thank you! Landed in 7d6e3f8 just now.

rockdaboot added a commit to rockdaboot/libpsl that referenced this pull request Jan 14, 2017
I changed my mind after talking with the cURL
maintainer Daniel Stenberg.
See curl/curl#1207
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants