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

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

Closed
jonnott opened this issue May 12, 2020 · 19 comments · Fixed by #263
Closed

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

jonnott opened this issue May 12, 2020 · 19 comments · Fixed by #263

Comments

@jonnott
Copy link

jonnott commented May 12, 2020

FWIW, today the update to symfony/polyfill-intl-idn v1.16 (from 1.15) just completely broke all my Guzzle usages. This is on a server running CentOS 6 (and therefore old ICU - 4.2.1?) and PHP 7.3

@nicolas-grekas
Copy link
Member

Do you happen to know why? Can you send a fix or at least spot the PR at fault?

@jonnott
Copy link
Author

jonnott commented May 12, 2020

Don't know, but every single use is now failing with 'IDN conversion failed' exception message. Running latest guzzle/guzzle.

@stof
Copy link
Member

stof commented May 12, 2020

@jonnott do you have the intl extension installed or no ?

@nicolas-grekas I think it might be related to #252. By checking each constant separately, we will define missing IDNA_* constants, even when the native implementation of functions is used but with limited features (due to having an old version of ICU probably).

For constants related to new features, they should be defined only if our polyfill implementation is used, when they will impact the potential feature detection (guzzle does some feature detection on INTL_IDNA_VARIANT_UTS46)

@nicolas-grekas
Copy link
Member

@stof good catch! that's going to be fun to patch :)

@alex-dev
Copy link

alex-dev commented May 12, 2020

I can confirm the cause. idn_to_ascii is used by Guzzle\Utils and it detects INTL_IDNA_VARIANT_UTS46 to choose which conversion to use.

@jonnott
Copy link
Author

jonnott commented May 12, 2020

@stof @nicolas-grekas

System: Linux {hostname} 2.6.32-754.11.1.el6.x86_64 #1 SMP Tue Feb 26 15:38:56 UTC 2019 x86_64

PHP Version: 7.3.17

intl Enabled
ICU version | 4.2.1
ICU TZData version | 2009j
ICU Unicode version | 5.1

/opt/cpanel/ea-php73/root/etc/php.d/20-intl.ini

Composer:

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 certificate-b...
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                       6.5.3    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 use FPDI with...
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.16.0  Symfony polyfill for ctype functions
symfony/polyfill-intl-idn               v1.16.0  Symfony polyfill for intl's idn_to_ascii and idn_to_utf8 functions
symfony/polyfill-mbstring               v1.16.0  Symfony polyfill for the Mbstring extension
symfony/polyfill-php72                  v1.16.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.```

@stof
Copy link
Member

stof commented May 12, 2020

@nicolas-grekas We will have to identify constants related to configuring features of a function vs constants being a feature themselves. The second category can have independant checks. The first category need to be taken carefully (especially when the constants are not always all defined in PHP itself based on conditional features or PHP versions).
And yes, that's gonna be fun to patch 😄

@jonnott
Copy link
Author

jonnott commented May 12, 2020

Actually, having now force my versions of symfony/polyfill-* back to 1.15.0 via composer, the problems still persist. So goodness knows what's going on. guzzle/guzzle version hasn't been updated since 21/04/2020 (for me) - to latest 6.5.3

@alex-dev
Copy link

Actually, having now force my versions of symfony/polyfill-* back to 1.15.0 via composer, the problems still persist. So goodness knows what's going on. guzzle/guzzle version hasn't been updated since 21/04/2020 (for me) - to latest 6.5.3

I can confirm symfony/polyfill-intl-idn@1.15 and guzzlehttp/guzzle@6.5.3 works.

@jonnott
Copy link
Author

jonnott commented May 12, 2020

I can confirm symfony/polyfill-intl-idn@1.15 and guzzlehttp/guzzle@6.5.3 works.

@alex-dev ..but is that on CentOS 6 with ICU 4.2.1 ?

@alex-dev
Copy link

alex-dev commented May 12, 2020

CentOS 6.9.e19.12.3 x86_64
ICU 4.2.1-14.el6.x86_64
PHP 7.1.8

@nicolas-grekas
Copy link
Member

@alex-dev
Copy link

Well. That's a heavy handed fix. It works.

@jonnott
Copy link
Author

jonnott commented May 13, 2020

@fabpot @nicolas-grekas @alex-dev

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

Now using 1.17.0 of this lib, and latest Guzzle 6.5.3

Guzzle 6.5.1 fixed this issue so I could 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 polyfill-intl-idn :(

@jonnott
Copy link
Author

jonnott commented May 14, 2020

Issue is fixed by guzzle/guzzle#2626 btw

@nicolas-grekas
Copy link
Member

Thanks for the link. Did you try previous versions? What is causing the issue? Wrong code in guzzle?

@jonnott
Copy link
Author

jonnott commented May 14, 2020

I am puzzled as to the exact cause. Guzzle requests seemed to break (according to my application logs) when composer updated this polyfill lib from 1.15 to 1.16. Then 1.17 came out and didn't fix it. So I reverted to 1.15, but that didn't fix it either! So it may have been due to something else going on on the server (minor PHP 7.3.x update via WHM update maybe?) .. but all the while Guzzle was still on 6.5.3 and that's been out for a while now. Wish I knew the answer.

I wasn't the first time I'd had to use the ['idn_conversion'=>false] option for GuzzleHttp\Client to work around this, but Guzzle 6.5.1 seemed to fix the problem .. then somehow it's resurfaced (so I'm back to using that 'idn_conversion' option until a new release of Guzzle comes out with that PR merged).

@jonnott
Copy link
Author

jonnott commented May 14, 2020

Well. That's a heavy handed fix. It works.

It's in-fact TOO heavy-handed in the case of this particular polyfill class, because there are situations where the intl extension is loaded, but the ICU version available on the machine is unusable by the native PHP implementation (e.g. UCI 4.2.1). It should not be simply bypassing the polyfill if it detects the intl extension is loaded, rather it should be working out whether to use the native implementation or not based on the INTL_IDNA_VARIANT_UTS46 constant, as Utils::idnUriConvert() is currently doing in Guzzle.

@nicolas-grekas @alex-dev @fabpot @GrahamCampbell

@alex-dev
Copy link

alex-dev commented May 14, 2020

It should be working out whether to use the native implementation or not based on the INTL_IDNA_VARIANT_UTS46 constant.

That seems like a better approach for a polyfill. Everyone uses the latest API, and the polyfill check the actual implementation to know if it should shim itself in-between, replace it completely or leave it.
Problem is PHP. In PHP, without some monkey-patching in C, it is impossible to redefine a declared method or class. Therefore, we can't really do a javascript-style polyfill.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants