[6.5] Fix various intl icu issues #2626

GrahamCampbell · 2020-04-24T10:10:14Z

This fixes the following issues:

Whenever PHP has been compiled against a too old version of intl icu, we should use the polyfill implementation (Fixes INTL_IDNA_VARIANT_2003 is deprecated #2620).
A minimum version of the polyfill of 1.17.0 should be imposed to fix issues with constants being defined in error (Fixes [6.5] Fix various intl icu issues #2626 (comment)).
When a domain is only ASCII, we should skip conversion entirely (Fixes IDN conversion fails with domains containing multiple hyphens #2640).

mbabker · 2020-04-27T19:04:57Z

Rolled this to production, not getting the deprecation notice anymore.

jonnott · 2020-05-12T11:17:27Z

+1 for getting this merged and released!

jonnott · 2020-05-12T16:29:53Z

Is symfony/polyfill#262 related?

GrahamCampbell · 2020-05-12T16:31:47Z

Hmmm, not really. There were issues with the older version of the polyfill too.

jonnott · 2020-05-13T08:31:19Z

@GrahamCampbell @fabpot

Still getting 'IDN conversion failed' on all Guzzle Client requests, unless I specifically set 'idn_conversion=>false'

Now using 1.17.0 of symfony/polyfill-intl-idn, and latest Guzzle 6.5.3

Guzzle 6.5.1 fixed this issue so I was able to drop 'idn_conversion=>false', but now the problem is back and I don't know why. Upping to 6.5.3 didn't cause this regression I don't think, but the problem seems to be there whether I'm using 1.15, 1.16 or 1.17 of symfony/polyfill-intl-idn :(

GrahamCampbell · 2020-05-13T08:36:26Z

You haven't tried applying this PR though...

GrahamCampbell · 2020-05-13T08:36:39Z

This will fix the issue for you.

jonnott · 2020-05-13T08:39:06Z

@GrahamCampbell Waiting for it to be in a release ... I tend to be happier to leave work-arounds in my code (like ['idn_conversion' => false]) than have forked/bodged-in-vendor-dir versions of dependencies.

GrahamCampbell · 2020-05-13T11:50:54Z

The only way for this PR to make it into a release is if people (like yourself) to test it so we know if it works.

phpstan-baseline.neon

jonnott · 2020-05-13T12:11:32Z

The only way for this PR to make it into a release is if people (like yourself) to test it so we know if it works.

@GrahamCampbell I would love to be able to help by directly testing the change .. but unfortunately the only environment in which I'm experiencing the issue is in production (which is CentOS 6 / ICU 4.2.1 / cPanel WHM / PHP 7.3) .. whereas in my local dev environment (MAMP 5.7, PHP 7.3, macOS Catalina, ICU 56.1) there's no problem at all.

So the only thing I can do (so production isn't broken), after researching the issue, is to set idn_conversion=>false for every new GuzzleHttp\Client().

I would love to be able to help more, but my production environment is very busy running a constantly-used API + app, and is using Guzzle to handle all communication with external services and APIs.

I understand that I could somehow perhaps deploy the fixed branch into production to test it, but that's a bit out of my league re: composer skills. Anyone got a link to a simple guide on how to do this?

What would help (as much of the detail of this IDN/ICU stuff is way over my head) .. would be to have a clear idea of how somewhere between guzzle 6.5.1 and 6.5.3, and/or symfony/polyfill-intl-idn 1.15.0 to 1.16.0 .. the 'IDN conversion failed' error (which had gone away) .. has returned ?

jonnott · 2020-05-14T10:42:17Z

@GrahamCampbell Can you advise on how I could temporarily put your icu-fixes branch into my production environment (using composer) in order to test this?

jonnott · 2020-05-14T10:44:00Z

@GrahamCampbell Can you advise on how I could temporarily put your icu-fixes branch into my production environment (using composer) in order to test this?

Like this? https://stackoverflow.com/questions/13498519/how-to-require-a-fork-with-composer

jonnott · 2020-05-14T11:14:07Z

@GrahamCampbell I can confirm this FIXES the problem for me, after testing in production..

(new GuzzleHttp\Client())->get('https://some.url/here'); now works without needing 'idn_conversion'=>false option setting for the Client.

WHM/cPanel (latest on 'release' channel)
CentOS 6
PHP 7.3.17

Internationalization support | enabled (intl)
-- | --
ICU version | 4.2.1
ICU TZData version | 2009j
ICU Unicode version | 5.1

composer.json file:

{
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/GrahamCampbell/guzzle"
        }
    ],
    "require": {
        "stripe/stripe-php": "^7.0",
        "drewm/mailchimp-api": "^2.4",
        "tijsverkoyen/akismet": "^1.1",
        "facebook/graph-sdk": "^5.4",
        "guzzlehttp/guzzle": "dev-icu-fixes",
        "calcinai/xero-php": "^1.7",
        "vlucas/phpdotenv": "^4.0",
        "edamov/pushok": "^0",
        "mpdf/mpdf": "^8.0",
        "plasticbrain/php-flash-messages": "^1.0",
        "symfony/cache": "^5.0"
    }
}

composer show

calcinai/xero-php                       v1.9.1                A client implementation of the Xero API, with a cleaner OAuth interface and ORM-like abstraction.
drewm/mailchimp-api                     v2.5.4                Super-simple, minimum abstraction MailChimp API v3 wrapper
edamov/pushok                           0.11.1                PHP client for Apple Push Notification Service (APNs) - Send push notifications to iOS using the new APNs HTTP/2 protocol with token-based (JWT with p8 private key) or ...
facebook/graph-sdk                      5.7.0                 Facebook SDK for PHP
fgrosse/phpasn1                         v2.1.1                A PHP Framework that allows you to encode and decode arbitrary ASN.1 structures using the ITU-T X.690 Encoding Rules.
guzzlehttp/guzzle                       dev-icu-fixes 6606165 Guzzle is a PHP HTTP client library
guzzlehttp/promises                     v1.3.1                Guzzle promises library
guzzlehttp/psr7                         1.6.1                 PSR-7 message implementation that also provides common utility methods
mpdf/mpdf                               v8.0.5                PHP library generating PDF files from UTF-8 encoded HTML
myclabs/deep-copy                       1.9.5                 Create deep copies (clones) of your objects
paragonie/random_compat                 v9.99.99              PHP 5.x polyfill for random_bytes() and random_int() from PHP 7
phpoption/phpoption                     1.7.3                 Option Type for PHP
plasticbrain/php-flash-messages         v1.0.1                A modern take on PHP session-based flash messages
psr/cache                               1.0.1                 Common interface for caching libraries
psr/container                           1.0.0                 Common Container Interface (PHP FIG PSR-11)
psr/http-client                         1.0.0                 Common interface for HTTP clients
psr/http-factory                        1.0.1                 Common interfaces for PSR-7 HTTP message factories
psr/http-message                        1.0.1                 Common interface for HTTP messages
psr/log                                 1.1.3                 Common interface for logging libraries
ralouphie/getallheaders                 3.0.3                 A polyfill for getallheaders.
setasign/fpdi                           v2.3.3                FPDI is a collection of PHP classes facilitating developers to read pages from existing PDF documents and use them as templates in FPDF. Because it is also possible to ...
spomky-labs/base64url                   v2.0.1                Base 64 URL Safe Encoding/Decoding PHP Library
stripe/stripe-php                       v7.32.0               Stripe PHP Library
symfony/cache                           v5.0.8                Symfony Cache component with PSR-6, PSR-16, and tags
symfony/cache-contracts                 v2.0.1                Generic abstractions related to caching
symfony/polyfill-ctype                  v1.17.0               Symfony polyfill for ctype functions
symfony/polyfill-intl-idn               v1.17.0               Symfony polyfill for intl's idn_to_ascii and idn_to_utf8 functions
symfony/polyfill-mbstring               v1.17.0               Symfony polyfill for the Mbstring extension
symfony/polyfill-php72                  v1.17.0               Symfony polyfill backporting some PHP 7.2+ features to lower PHP versions
symfony/service-contracts               v2.0.1                Generic abstractions related to writing services
symfony/var-exporter                    v5.0.8                A blend of var_export() + serialize() to turn any serializable data structure to plain PHP code
tijsverkoyen/akismet                    1.1.1                 Akismet is a wrapper-class to communicate with the Akismet API.
vlucas/phpdotenv                        v4.1.5                Loads environment variables from `.env` to `getenv()`, `$_ENV` and `$_SERVER` automagically.
web-token/jwt-core                      v2.1.6                Core component of the JWT Framework.
web-token/jwt-key-mgmt                  v2.1.6                Key Management component of the JWT Framework.
web-token/jwt-signature                 v2.1.6                Signature component of the JWT Framework.
web-token/jwt-signature-algorithm-ecdsa v2.1.6                ECDSA Based Signature Algorithms the JWT Framework.

GrahamCampbell · 2020-05-14T11:19:10Z

Yeh. Great. Thanks for confirming. I think this PR is good to be merged now. I'll contact the Guzzle core team.

nicolas-grekas · 2020-05-14T11:59:58Z

src/Utils.php

+            return \idn_to_ascii($domain, $options, INTL_IDNA_VARIANT_UTS46, $info);
+        }
+
+        return Idn::idn_to_ascii($domain, $options, Idn::INTL_IDNA_VARIANT_UTS46, $info);


Can this skip using the polyfill class directly? It's marked internal and it shouldn't be used.
If not, why did you do it this way?

Because we need to be able to call the polyfill even if the original extension is installed to work around the extension using a too old version of the C library. Symfony doesn't have a way for us to do this, other to call code that is formally internal.

And this commit (included in polyfill-intl-idn 1.17 release) totally stops Guzzle doing anything other than calling the class method directly now..
symfony/polyfill-intl-idn@3bff59e

...which to me suggests it should in fact be polyfill-intl-idn which should be verifying whether the ICU version available on the host machine is usable, or if the polyfill should be used instead, and it not be left to Guzzle to be looking at this and deciding for itself (therefore 'breaking the rules' by using code marked internal directly) .. ?

@jonnott not possible. Userland implementations cannot replace functions defined by PHP.

Oh well, looks like this PR is the best solution we have then.

@jonnott this commit in the polyfill fixes detecting wrongly that INTL_IDNA_VARIANT_UTS46 is supported by idn_to_ascii. It does not change the fact that idn_to_ascii is defined by the extension when defined (and so the polyfill could not define it).

Because we need to be able to call the polyfill even if the original extension is installed to work around the extension using a too old version of the C library. Symfony doesn't have a way for us to do this, other to call code that is formally internal.

I wonder if it would be best to remove the polyfill. Library users that see a deprecation can set idn_conversion to false and use their own custom solutions with a middleware or something like that.

That's not always possible. For me, the trigger on the deprecation notice came from a third party library that uses yet another library which is instantiating its own Guzzle\Client instance without any sane way for me to reach it to add middleware or set the idn_conversion configuration.

stof · 2020-05-14T13:49:17Z

src/Utils.php

+            return $domain;
+        }
+
+        if (\extension_loaded('intl') && defined('INTL_IDNA_VARIANT_UTS46')) {


you should remove the extension_loaded check. That way, if you have only the polyfill installed, you will still call idn_to_ascii (from the polyfill) rather than the internal API.

There will always be a case where using the polyfill directly will be needed, so anyway...

BUT it's very possible that ppl forced the polyfill to be NOT installed, by using a "replace" for "intl" in their composer.json.

This "if" should add a || !class_exists(Idn::class) to safeguard against this.

Hmm, if anyone has done this, we'd expect a fatal error. I think that would be fine, since it indicates the class doesn't exist, and anyone who has forced non-installation will know that that means.

require ext-intl + replace symfony/polyfill-intl is going to be very common.
We recommend it when possible for other polyfilled extensions.
A fatal error would be totally WTF I think...

Another option would be to copy the polyfill source code into guzzle I guess, and not require the package.

interested in creating a reuseable package both the polyfill and guzzle could depend on

we're not :)

There will always be a case where using the polyfill directly will be needed, so anyway...

but that case would be the pure legacy one (having ext-intl installed with a legacy ICU version). Currently, polyfill internals are also used for the non-legacy case of not having ext-intl.

And for that legacy case, I would also vote for using the variant 2003 rather than using the polyfill internals.

To support something that dates back to 2011?

Not actually 2011. Some distros from 2019 ship with old versions of the icu library that don't have this constant. This is one of the points of this PR and the older PR this fixes.

To support something that dates back to 2011?

Not actually 2011. Some distros from 2019 ship with old versions of the icu library that don't have this constant. This is one of the points of this PR and the older PR this fixes.

For example CentOS 6, which is still supported until 2020-11-30, has ICU 4.2.1.

But do recent PHP versions trigger a deprecation for the variant 2003 even when it was compiled without support for the uts46 variant ? that looks weird to me.

nicolas-grekas · 2020-05-14T16:04:33Z

src/Utils.php

+            return $domain;
+        }
+
+        if (\extension_loaded('intl') && defined('INTL_IDNA_VARIANT_UTS46')) {


There will always be a case where using the polyfill directly will be needed, so anyway...

BUT it's very possible that ppl forced the polyfill to be NOT installed, by using a "replace" for "intl" in their composer.json.

This "if" should add a || !class_exists(Idn::class) to safeguard against this.

ahilles107 · 2020-05-21T10:40:12Z

Hey, i confirm that with this fix i cannot reproduce issue anymore. It will be merged and released? Or there is another approach for this bug?

Nyholm · 2020-05-23T18:28:59Z

Thank you for all reviews and confirming that this works.

Im happy with this PR after some small changes. See: https://github.com/GrahamCampbell/guzzle/pull/1

Lock version to symfony/polyfill-intl-idn:1.17.0

Nyholm

Thank you

nicolas-grekas · 2020-05-26T06:20:14Z

composer.json

@@ -23,7 +23,7 @@
    "require": {
        "php": ">=5.5",
        "ext-json": "*",
-        "symfony/polyfill-intl-idn": "^1.11",
+        "symfony/polyfill-intl-idn": "1.17.0",


Sorry to chime in after merge: locking to a specific version of the polyfill effectively prevents installing any new(/old) versions ppl might want to use in their app. This is taking control away from users.

I'd recommend using ^1.17.

I suppose this was done as a measure against using an @internal class. But the "risk" this guards against is not strong enough to justify locking all users.

The bigger risk I mentioned in my review is someone using "replace": "symfony/polyfill-intl".
In order to guard from this risk, I'd recommend adding a class_exists(Idn::class) check before using the class instead.

We will allow more versions after they are released and we can confirm comparability. Just like someone would write 5.x interested of >=5, we have to bound our versions.

In order to guard from this risk, I'd recommend adding a class_exists(Idn::class)

That is insufficient. What if the method is renamed, etc. No point in guessing everything possible. If someone has "replaced" the polyfill then this won't matter anyway.

Fix issues when compiled against old intl icu

8d3709f

GrahamCampbell mentioned this pull request Apr 24, 2020

INTL_IDNA_VARIANT_2003 is deprecated #2620

Closed

Fixes

44de375

GrahamCampbell closed this Apr 24, 2020

GrahamCampbell reopened this Apr 24, 2020

GrahamCampbell mentioned this pull request Apr 24, 2020

Small docblock fix. #2624

Closed

GrahamCampbell mentioned this pull request Apr 30, 2020

IDN conversion failed (errors: IDNA_ERROR_EMPTY_LABEL) #2600

Closed

guilliamxavier mentioned this pull request May 13, 2020

Avoid PHPStan error #2637

Closed

guilliamxavier reviewed May 13, 2020

View reviewed changes

phpstan-baseline.neon Show resolved Hide resolved

Bumped minimum polyfill-intl-idn version

18d89d7

Skip conversion for ASCII domains

6606165

GrahamCampbell changed the title ~~[6.5] Fix issues when compiled against old intl icu~~ [6.5] Fix various intl icu issues May 14, 2020

GrahamCampbell mentioned this pull request May 14, 2020

IDN conversion fails with domains containing multiple hyphens #2640

Closed

jonnott mentioned this pull request May 14, 2020

symfony/polyfill-intl-idn 1.16 broken Guzzle for CentOS 6 (old ICU) servers? symfony/polyfill#262

Closed

nicolas-grekas reviewed May 14, 2020

View reviewed changes

stof reviewed May 14, 2020

View reviewed changes

nicolas-grekas suggested changes May 14, 2020

View reviewed changes

gmponos added this to the 6.5.x milestone May 14, 2020

ahilles107 mentioned this pull request May 18, 2020

Heroku compatibility superdesk/web-publisher#1109

Closed

Lock version to symfony/polyfill-intl-idn:1.17.0

05965c0

Merge pull request #1 from Nyholm/pr-2626

0758da5

Lock version to symfony/polyfill-intl-idn:1.17.0

Nyholm approved these changes May 23, 2020

View reviewed changes

Nyholm merged commit d3f2c17 into guzzle:6.5 May 23, 2020

nicolas-grekas reviewed May 26, 2020

View reviewed changes

GrahamCampbell deleted the icu-fixes branch June 16, 2020 08:48

LorbusChris mentioned this pull request Nov 30, 2020

Guzzle exception: IDN conversion failed nextcloud/server#24448

Closed

[6.5] Fix various intl icu issues #2626

[6.5] Fix various intl icu issues #2626

Conversation

GrahamCampbell commented Apr 24, 2020 • edited Loading

mbabker commented Apr 27, 2020

jonnott commented May 12, 2020

jonnott commented May 12, 2020

GrahamCampbell commented May 12, 2020

jonnott commented May 13, 2020

GrahamCampbell commented May 13, 2020

GrahamCampbell commented May 13, 2020

jonnott commented May 13, 2020

GrahamCampbell commented May 13, 2020 • edited Loading

jonnott commented May 13, 2020 • edited Loading

jonnott commented May 14, 2020

jonnott commented May 14, 2020 • edited Loading

jonnott commented May 14, 2020

GrahamCampbell commented May 14, 2020

nicolas-grekas May 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonnott May 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonnott May 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmponos May 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas May 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahilles107 commented May 21, 2020

Nyholm commented May 23, 2020

Nyholm left a comment

Choose a reason for hiding this comment

nicolas-grekas May 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrahamCampbell commented Apr 24, 2020 •

edited

Loading

GrahamCampbell commented May 13, 2020 •

edited

Loading

jonnott commented May 13, 2020 •

edited

Loading

jonnott commented May 14, 2020 •

edited

Loading

nicolas-grekas May 14, 2020 •

edited

Loading

jonnott May 14, 2020 •

edited

Loading

jonnott May 14, 2020 •

edited

Loading

gmponos May 14, 2020 •

edited

Loading

nicolas-grekas May 14, 2020 •

edited

Loading

nicolas-grekas May 26, 2020 •

edited

Loading