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

Add IDN support for redirects also #2424

Merged
merged 4 commits into from Dec 20, 2019
Merged

Conversation

gmponos
Copy link
Member

@gmponos gmponos commented Dec 8, 2019

No description provided.

@gmponos
Copy link
Member Author

gmponos commented Dec 8, 2019

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

@gmponos gmponos changed the base branch from master to 6.5 December 8, 2019 19:30
@gmponos
Copy link
Member Author

gmponos commented Dec 8, 2019

btw I am targeting 6.5 as a bugfix.

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

@alexeyshockov alexeyshockov left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@alexeyshockov alexeyshockov left a comment

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@gmponos
Copy link
Member Author

gmponos commented Dec 11, 2019

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

Copy link
Collaborator

@alexeyshockov alexeyshockov left a comment

Choose a reason for hiding this comment

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

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

Comment on lines 361 to 362
$variant = defined('INTL_IDNA_VARIANT_UTS46') ? INTL_IDNA_VARIANT_UTS46 : 0;
$asciiHost = idn_to_ascii($uri->getHost(), $options, $variant, $info);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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);

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks again.. done..

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

better we avoid the back and forth :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree, but now it seems really out of scope here

@Nyholm Nyholm added this to the 6.5.1 milestone Dec 12, 2019
@sagikazarmark
Copy link
Member

sagikazarmark commented Dec 14, 2019

So, how does this relate to #2454 ?

Should the two be shipped in 6.5.1?

@alexeyshockov
Copy link
Collaborator

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
Copy link
Member

sagikazarmark commented Dec 14, 2019

Thanks @alexeyshockov !

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

@gmponos
Copy link
Member Author

gmponos commented Dec 14, 2019

I will later today or tomorrow...

@gmponos
Copy link
Member Author

gmponos commented Dec 18, 2019

@alexeyshockov I merged your changes from 6.5 branch.

Would appreciate if you could re-validate this.

Copy link
Collaborator

@alexeyshockov alexeyshockov left a comment

Choose a reason for hiding this comment

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

Thank you for this fix, looks good

@sagikazarmark
Copy link
Member

@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
Copy link
Member Author

gmponos commented Dec 20, 2019

done on #2478 since current branch has tests under travis

@sagikazarmark sagikazarmark merged commit 283a724 into guzzle:6.5 Dec 20, 2019
@gmponos gmponos deleted the issue#2423 branch December 20, 2019 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants