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.

Sign up for GitHub

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

Jump to bottom

[6.5] Fix various intl icu issues #2626

Merged
merged 6 commits into from May 23, 2020
Merged

[6.5] Fix various intl icu issues #2626

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8d3709f
Fix issues when compiled against old intl icu
GrahamCampbell Apr 24, 2020
44de375
Fixes
GrahamCampbell Apr 24, 2020
18d89d7
Bumped minimum polyfill-intl-idn version
GrahamCampbell May 14, 2020
6606165
Skip conversion for ASCII domains
GrahamCampbell May 14, 2020
05965c0
Lock version to symfony/polyfill-intl-idn:1.17.0
Nyholm May 23, 2020
0758da5
Merge pull request #1 from Nyholm/pr-2626
GrahamCampbell May 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion composer.json
View file
Open in desktop
Expand Up @@ -23,7 +23,7 @@
"require": {
"php": ">=5.5",
"ext-json": "*",
"symfony/polyfill-intl-idn": "^1.11",
"symfony/polyfill-intl-idn": "^1.17",
"guzzlehttp/promises": "^1.0",
"guzzlehttp/psr7": "^1.6.1"
},
Expand Down
10 changes: 10 additions & 0 deletions phpstan-baseline.neon
View file
Open in desktop
Expand Up @@ -495,6 +495,11 @@ parameters:
count: 1
path: src/Handler/CurlFactory.php

-
message: "#^Parameter \\#1 \\$filename of function is_dir expects string, string\\|false given\\.$#"
count: 1
path: src/Handler/CurlFactory.php

GrahamCampbell marked this conversation as resolved.
Show resolved Hide resolved
-
message: "#^Method GuzzleHttp\\\\Handler\\\\CurlFactory\\:\\:retryFailedRewind\\(\\) has no return typehint specified\\.$#"
count: 1
Expand Down Expand Up @@ -1310,6 +1315,11 @@ parameters:
count: 1
path: src/UriTemplate.php

-
message: "#^Method GuzzleHttp\\\\Utils\\:\\:idnToAsci\\(\\) has parameter \\$info with no value type specified in iterable type array\\.$#"
count: 1
path: src/Utils.php

-
message: "#^Function GuzzleHttp\\\\uri_template\\(\\) has parameter \\$variables with no value type specified in iterable type array\\.$#"
count: 1
Expand Down
26 changes: 22 additions & 4 deletions src/Utils.php
View file
Open in desktop
Expand Up @@ -3,6 +3,7 @@

use GuzzleHttp\Exception\InvalidArgumentException;
use Psr\Http\Message\UriInterface;
use Symfony\Polyfill\Intl\Idn\Idn;

final class Utils
{
Expand Down Expand Up @@ -30,10 +31,7 @@ public static function currentTime()
public static function idnUriConvert(UriInterface $uri, $options = 0)
{
if ($uri->getHost()) {
$idnaVariant = defined('INTL_IDNA_VARIANT_UTS46') ? INTL_IDNA_VARIANT_UTS46 : 0;
$asciiHost = $idnaVariant === 0
? idn_to_ascii($uri->getHost(), $options)
: idn_to_ascii($uri->getHost(), $options, $idnaVariant, $info);
$asciiHost = self::idnToAsci($uri->getHost(), $options, $info);
if ($asciiHost === false) {
$errorBitSet = isset($info['errors']) ? $info['errors'] : 0;

Expand Down Expand Up @@ -64,4 +62,24 @@ public static function idnUriConvert(UriInterface $uri, $options = 0)

return $uri;
}

/**
* @param string $domain
* @param int $options
* @param array $info
*
* @return string|false
*/
private static function idnToAsci($domain, $options, &$info = [])
{
if (\preg_match('%^[ -~]+$%', $domain) === 1) {
return $domain;
}

if (\extension_loaded('intl') && defined('INTL_IDNA_VARIANT_UTS46')) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

we're not :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

return Idn::idn_to_ascii($domain, $options, Idn::INTL_IDNA_VARIANT_UTS46, $info);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jonnott jonnott May 14, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...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) .. ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jonnott jonnott May 14, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@gmponos gmponos May 14, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}