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
base: master
from

Conversation

Projects
None yet
3 participants
@rockdaboot
Contributor

rockdaboot commented Jan 13, 2017

No description provided.

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Jan 13, 2017

@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.

mention-bot commented Jan 13, 2017

@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.

@bagder

bagder approved these changes Jan 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jan 13, 2017

Member

Thanks!

Member

bagder commented Jan 13, 2017

Thanks!

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jan 13, 2017

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@rockdaboot

rockdaboot Jan 13, 2017

Contributor

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.

Contributor

rockdaboot commented Jan 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jan 13, 2017

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@rockdaboot

rockdaboot Jan 13, 2017

Contributor

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

Contributor

rockdaboot commented Jan 13, 2017

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

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jan 13, 2017

Member

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

Member

bagder commented Jan 13, 2017

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

@rockdaboot

This comment has been minimized.

Show comment
Hide comment
@rockdaboot

rockdaboot Jan 13, 2017

Contributor

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.

Contributor

rockdaboot commented Jan 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jan 13, 2017

Member

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

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

Use TR46 non-transitional with libidn2 >= 0.14
I changed my mind after talking with the cURL
maintainer Daniel Stenberg.
See curl/curl#1207
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment