[BUG] polyfill-intl-idn breaks on large hoster installation

[bmack]

We use the intl-idn package in TYPO3 CMS and it helps a lot! Thank you for providing such a package.

However there is one large TYPO3 hoster (having roughly 200.000 TYPO3 installations), where they have a messed up Server setup regarding to ICU version on their system. In short: They provide the php-intl extension, but do not define all constants as needed for PHP 7.2+. We had solved this issue in the past by a hard workaround, but with the update to polyfill-intl-idn 1.17.1 and 1.18.0 then (respective change) symfony/polyfill-intl-idn@3bff59e (or f968149#diff-eaa55ac0b04a1928e5455b4f48efe417R14-R17) our setups don't work on the hoster anymore, as they clearly have a misconfiguration.

Still, the previous solution when not checking on the extension_loaded() was more robust for these kind of weird setups.

In any case, I'm (personally) fine if you decide to keep the extension_loaded() check in your library, but wanted to let you know that this actually is an issue for a lot of people. It would be great to know if you're able to fix it or planning to fix/revert the three lines or if we should find a workaround for ourselves in our project.

[stof]

Well, the extra checks being added are necessary: if the extension is loaded and provides the functions, defining the constants will still not make them work (as the function being called would be the one from the extension, not the one from the polyfill).
And declaring the constants only when the implementation supports them means that it is possible to do feature detection to know whether the feature can be used (using defined('INTL_IDNA_VARIANT_UTS46') for instance). This is much better than having to deal with a non-working feature without being able to detect such case (that's why we did the change).

If your project has to support runtimes providing ext-intl but with totally outdated ICU versions, the polyfill won't save you and you will have to deal with the fact that some features (for instance, the UTS46 variant) will be missing. PHP makes it impossible to polyfill incomplete implementations (we cannot replace functions coming from extensions as we cannot replace an already declared function).

IMO, reverting the check would actually make things worse than today, as the feature will still not work but you won't be able to feature-detect it (maybe you did not realized before that the feature was not working for such setup even though the constant was defined).

[nicolas-grekas]

@bmack do the missing constants still work as expected when passed as values with the functions provided by this strange build of the extension? Are there also missing functions or does this only apply to constants?

We could polyfill the constants if they work as expected when passed to the functions.

[stof]

Well, the discussion in #262 which made us add this check tells me that the INTL_IDNA_VARIANT_UTS46 variant does not work with old ICU versions.

[bmack]

Yes. I'm (personally) fine with locking to 1.17.0 for the time being until the hoster has sorted out their issues, as this polyfill should IMHO contain a "full replacement" and not a "partial replacement", which would make tests much more complicated - plus I don't even know how they compiled it in the first place (that's what makes it so hard for me to reproduce in general).

Thanks for replying and supporting, I appreciate your time on this, but I'm pretty sure you have more important things to do.

[stof]

@bmack as said before, we can only polyfill missing functions, not incomplete implementations of functions. As PHP does not allow replacing existing functions, it is simply impossible to build such a polyfill in userland (and if we were to build a C extension to polyfill another C extension, that would be insane). Deciding that we delete the polyfill entirely because we can only solve the case of not having ext-intl but not the case of having an outdated ext-intl would not improve the ecosystem.

Locking to 1.17.0 will probably not solve the issue for your case: the constant is defined in 1.17.0, but the feature still does not work when using idn_to_ascii(INTL_IDNA_VARIANT_UTS46) as that would use the incomplete implementation.

[bmack]

Got some more details:

We previously checked for the constant INTL_IDNA_VARIANT_UTS46 to be defined in our code. If defined, we used the \idn_to_ascii($domain, IDNA_DEFAULT, INTL_IDNA_VARIANT_UTS46); call, otherwise we called the polyfill's Idn::idn_to_ascii method directly. Now that IDNA_CHECK_BIDI is checked since 1.17.1 (through the update of the underlying polyfill lib) as well, this fails as well

polyfill/src/Intl/Idn/Idn.php

Line 148 in aa691e9

'CheckBidi' => self::INTL_IDNA_VARIANT_2003 === $variant || 0 !== ($options & \IDNA_CHECK_BIDI),

.

[stof]

Well, calling the Idn::idn_to_ascii method directly is an unsupported usage of the polyfill, as the Idn class is internal.

But we might want to use self::IDNA_CHECK_BIDI instead to reference the options

[bmack]

@stof so true! I hope it becomes clear, but just saying it again, I didn't expect this to be a "Hey, this is a BC break" discussion, I'm grateful for all the work you've but in there, so calling Idn::idn_to_ascii is ofc our existing workaround already and no good solution for the broader world.

[stof]

@nicolas-grekas should we update the Idn class to use a class constant for the IDNA_CHECK_BIDI value rather than using the ext-intl constant, to avoid breaking the case of projects accessing the internal API directly ?

[bmack]

That would be great. I can provide a PR (and test it on this hoster setup) if you agree on pushing this forward.

[nicolas-grekas]

Works for me, PR welcome.

[TRowbotham]

Correct, the INTL_IDNA_VARIANT_UTS46 was only introduced in ICU 4.6 along with the new option constants IDNA_CHECK_BIDI, IDNA_CHECK_CONTEXTJ, IDNA_NONTRANSITIONAL_TO_ASCII, IDNA_NONTRANSITIONAL_TO_UNICODE, and probably some of the IDNA_ERROR_* constants. If this host is using an ICU version < 4.6 none of these constants will be defined.