Use libidn2 #133
Use libidn2 #133
Conversation
The define IDNA_ACE_PREFIX is not defined in the idn2.h header. Therefore we use another define IDN2_OK to test that the lib is properly loaded.
ghost
requested review from
And not libidn11-dev anymore
|
Since #27 Travis always runs the tests with the network. Should I fix the test to be aligned with the live data? Or should we make the test run without network by default (so that on installation they are run in a close environment)? |
|
I went with updating the tests. I realize that the |
The `se` and `nic.se` zones have evolved a little. The tests made over the network using these zones have been slightly updated to fix errors and be aligned with current zone configuration.
|
Did you consider using https://metacpan.org/pod/Net::LibIDN2 instead? That way maybe we could simply drop some of our own XS code. |
No I haven't. Thanks for the pointer. |
|
I think it would be a good idea to use Net::LibIDN2 instead. There is some magic that the IDN libraries do, such as down casing and making NFC. With Net::LibIDN2 we would have control of that. We could have a test case that verifies that labels that look like A-labels also are valid A-labels. Would we loose anything by using Net::LibIDN2 instead? |
| @@ -18,8 +18,8 @@ to_idn(...) | |||
|
|
|||
| if (SvPOK(ST(i))) | |||
| { | |||
| status = idna_to_ascii_8z(SvPVutf8_nolen(obj), &out, IDNA_ALLOW_UNASSIGNED); | |||
| if (status == IDNA_SUCCESS) | |||
| status = idn2_to_ascii_8z(SvPVutf8_nolen(obj), &out, IDN2_ALLOW_UNASSIGNED); | |||
There was a problem hiding this comment.
The flags we se are important. Is IDN2_ALLOW_UNASSIGNED really what we want here? The features that we select are important. This has to be investigated first.
There was a problem hiding this comment.
The IDNA_ALLOW_UNASSIGNED has been used since the beginning of the to_idn() function. I've just tried to keep the same flag thinking it would give the same behavior. Are you saying that there is a change in behavior?
|
Should we have any discussion about this PR? I thought that this would be replaced by #144. |
ghost
mentioned this pull request
|
I've seen I forgot to update the Dockerfile and some references to "libidn". This is fixed. Please re-review. |
Purpose
Replace libidn with libidn2 following https://libidn.gitlab.io/libidn2/manual/libidn2.html#Converting-from-libidn.
Context
#131
Changes
libidn2instead oflibidnlibidn2header ininclude/LDNS.hsrc/LDNS.xsHow to test this PR
The tests should pass.
Discussion
Old code was using
IDNA_ALLOW_UNASSIGNEDinidna_to_ascii_8z. Even if this flag exists in IDN2 (IDN2_ALLOW_UNASSIGNED) it seems it has been kept for compatibility reason and is unused. I haven't dig any further, but do we need to keep this flag, or should we update some logic in the code to keep the expected behavior we had with the old code?