Add IDN support for redirects also by gmponos · Pull Request #2424 · guzzle/guzzle

gmponos · 2019-12-08T12:44:38Z

No description provided.

gmponos · 2019-12-08T12:45:48Z

if the corrent PR is a solution for issue #2423 then we should consider centralizing this logic..

gmponos · 2019-12-08T20:48:07Z

btw I am targeting 6.5 as a bugfix.

alexeyshockov

LGTM

alexeyshockov

Please see my comments related to #2448. IMO it makes sense to include a fix for it here too, because you are changing the same piece of code.

alexeyshockov · 2019-12-11T08:54:24Z

+ * @return UriInterface
+ * @internal
+ */
+function _idn_uri_convert(UriInterface $uri, $options = IDNA_DEFAULT)


What do you think about changing IDNA_DEFAULT constant to 0 here? I'm think when intl is not available it can cause an error...

See roundcube/roundcubemail#6244 for details, also related to #2448.

alexeyshockov · 2019-12-11T08:55:55Z

+function _idn_uri_convert(UriInterface $uri, $options = IDNA_DEFAULT)
+{
+    if ($uri->getHost()) {
+        $asciiHost = idn_to_ascii($uri->getHost(), $options, INTL_IDNA_VARIANT_UTS46, $info);


It make sense to use $variant = defined('INTL_IDNA_VARIANT_UTS46') ? INTL_IDNA_VARIANT_UTS46 : null; workaround here, because it seems that the constant is not always available.

gmponos · 2019-12-11T17:50:16Z

@alexeyshockov thank you for the review.. can you check again?

alexeyshockov

Still the UTS46 issue (read my comment and suggestion). Thanks a lot for your work.

alexeyshockov · 2019-12-11T17:57:41Z

+        $variant = defined('INTL_IDNA_VARIANT_UTS46') ? INTL_IDNA_VARIANT_UTS46 : 0;
+        $asciiHost = idn_to_ascii($uri->getHost(), $options, $variant, $info);


Just checked locally, it raises a deprecation error (idn_to_ascii(): INTL_IDNA_VARIANT_2003 is deprecated).

The only way is to call the function without the third argument at all, like here:

$asciiHost = defined('INTL_IDNA_VARIANT_UTS46') ? idn_to_ascii($uri->getHost(), $idnOptions, INTL_IDNA_VARIANT_UTS46, $info) : idn_to_ascii($uri->getHost(), $idnOptions);

thanks again.. done..

OK, so it's night and too many versions to keep in the head.

It works on my machine, because I have PHP 7.4 with already right defaults.

On 7.3 it raises a warning by default, if you don't specify INTL_IDNA_VARIANT_UTS46 manually. So there is basically no way to get rid of this deprecation error in PHP 7.2+ if you have an old version of ICU library. Looks like we have to disable idn_conversion by default in this case (PHP 7.2+ and INTL_IDNA_VARIANT_UTS46 not defined).

https://3v4l.org/9MPJh

@gmponos, now I think it's better to open a separate PR for this change. Please take a look at #2453.

better we avoid the back and forth :)

Agree, but now it seems really out of scope here

sagikazarmark · 2019-12-14T15:35:36Z

So, how does this relate to #2454 ?

Should the two be shipped in 6.5.1?

alexeyshockov · 2019-12-14T15:57:28Z

So, how does this relate to #2454 ?

Initially (when the issue with INTL_IDNA_VARIANT_UTS46 was discovered) I thought it can be fixed quickly here, in this PR. After additional information from affected devs and some checks I ended up with creating a separate PR for this change.

So the last changes (connected to INTL_IDNA_VARIANT_UTS46) should be removed from here. Sorry again for confusion, @gmponos.

Should the two be shipped in 6.5.1?

Don't see reasons not to do that. Or ship #2454 first and this one after, in 6.5.2. Whatever works for you.

sagikazarmark · 2019-12-14T17:30:55Z

Thanks @alexeyshockov !

If @gmponos can make the required changes we can release these fixes together.

gmponos · 2019-12-14T17:40:26Z

I will later today or tomorrow...

gmponos · 2019-12-18T14:02:20Z

@alexeyshockov I merged your changes from 6.5 branch.

Would appreciate if you could re-validate this.

alexeyshockov

Thank you for this fix, looks good

sagikazarmark · 2019-12-19T22:11:17Z

@gmponos Can you please enable the intl extension in GH actions ci.yaml? I just saw that there is a condition for the intl extension being loaded.

gmponos · 2019-12-20T06:52:51Z

done on #2478 since current branch has tests under travis

gmponos mentioned this pull request Dec 8, 2019

IDNs are not properly converted to ASCII on automatic redirects #2423

Closed

alexeyshockov reviewed Dec 8, 2019

View reviewed changes

Comment thread src/RedirectMiddleware.php Outdated

gmponos force-pushed the issue#2423 branch from 3aded4a to e884a47 Compare December 8, 2019 19:30

gmponos changed the base branch from master to 6.5 December 8, 2019 19:30

gmponos marked this pull request as ready for review December 8, 2019 20:49

alexeyshockov approved these changes Dec 10, 2019

View reviewed changes

alexeyshockov suggested changes Dec 11, 2019

View reviewed changes

Fix issue 2423 and issue 2448

1996437

gmponos force-pushed the issue#2423 branch from ef6ae01 to 1996437 Compare December 11, 2019 17:52

alexeyshockov suggested changes Dec 11, 2019

View reviewed changes

Fix idn according to suggestions from review

f280749

Nyholm added this to the 6.5.1 milestone Dec 12, 2019

gmponos added 2 commits December 15, 2019 10:08

Revert a check that is not needed

d56612f

Resolved conflicts with 6.5

a5a84c3

alexeyshockov approved these changes Dec 19, 2019

View reviewed changes

sagikazarmark approved these changes Dec 20, 2019

View reviewed changes

sagikazarmark merged commit 283a724 into guzzle:6.5 Dec 20, 2019

gmponos deleted the issue#2423 branch December 20, 2019 12:15

gmponos added the area/intl-icu label May 23, 2020

		$variant = defined('INTL_IDNA_VARIANT_UTS46') ? INTL_IDNA_VARIANT_UTS46 : 0;
		$asciiHost = idn_to_ascii($uri->getHost(), $options, $variant, $info);

Uh oh!

Conversation

gmponos commented Dec 8, 2019

Uh oh!

gmponos commented Dec 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

gmponos commented Dec 8, 2019

Uh oh!

alexeyshockov left a comment

Choose a reason for hiding this comment

Uh oh!

alexeyshockov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmponos commented Dec 11, 2019

Uh oh!

alexeyshockov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexeyshockov Dec 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Dec 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexeyshockov commented Dec 14, 2019

Uh oh!

sagikazarmark commented Dec 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmponos commented Dec 14, 2019

Uh oh!

gmponos commented Dec 18, 2019

Uh oh!

alexeyshockov left a comment

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Dec 19, 2019

Uh oh!

gmponos commented Dec 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gmponos commented Dec 8, 2019 •

edited

Loading

alexeyshockov Dec 11, 2019 •

edited

Loading

sagikazarmark commented Dec 14, 2019 •

edited

Loading

sagikazarmark commented Dec 14, 2019 •

edited

Loading