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

Jump to bottom

Better defaults for PHP installations with old ICU lib #2454

Merged
merged 2 commits into from Dec 18, 2019
Merged

Better defaults for PHP installations with old ICU lib #2454

merged 2 commits into from Dec 18, 2019

Conversation

alexeyshockov
Copy link
Collaborator

@alexeyshockov alexeyshockov commented Dec 12, 2019 •
edited

Fixes #2448

This was referenced Dec 12, 2019
Closed
Closed
@GrahamCampbell
Copy link
Member

But what if the function was implemented by a pollyfill?

@alexeyshockov
Copy link
Collaborator Author

@GrahamCampbell, don't see any issues with a polyfill. If it implements idn_to_ascii(), then all should be fine. symfony/polyfill implements both the function and INTL_IDNA_VARIANT_UTS46 constant, so idn_conversion option will be true by default.

@GrahamCampbell
Copy link
Member

Maybe guzzle should always load the polyfill?

@alexeyshockov
Copy link
Collaborator Author

I think it will not help here. The original issue with 6.5.0 is about old version of ICU library (intl extension is available, just "partially"). So symfony/polyfill will not work in this case, because it discriminates things by just checking for idn_to_ascii() existence.

Creating a polyfill for exactly this case seems to be very challenging, and IMO Guzzle should not force any polyfills usage to the end developer.

brandonkelly
brandonkelly reviewed Dec 12, 2019
View reviewed changes
src/Client.php Outdated Show resolved Hide resolved
@brandonkelly brandonkelly mentioned this pull request Dec 12, 2019
Closed
@alexeyshockov @brandonkelly
Faster PHP version comparison
6a8a624 
Co-Authored-By: Brandon Kelly <brandon@pixelandtonic.com>
@johncarlson21
Copy link

guys this needs to be merged like now.. after my update, now I have a non working script. I'm going to manually implement this fix.

reviewtypo3org pushed a commit to TYPO3/typo3 that referenced this pull request Dec 13, 2019
@bmack @ohader
[BUGFIX] Mark guzzlehttp/guzzle >= 6.5.0 as conflict
59d5159 
Due to the INTL/ICU bug, which we
have seen on various places, Guzzle, which
does not cover our edge cases yet, ran
in the same issue as our Core versions earlier
in 2019.

See
guzzle/guzzle#2448
guzzle/guzzle#2454

For the time being, lets mark guzzle as
incompatible until Guzzle has solved the issue
and released a new version, so we can loosen
the conflict constraint.

Related: #87953
Resolves: #89904
Releases: master, 9.5, 8.7
Change-Id: If64fb9472d046f020c850cd0551beeaf78796b60
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/62606
Tested-by: TYPO3com <noreply@typo3.com>
Tested-by: Oliver Hader <oliver.hader@typo3.org>
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
TYPO3IncTeam pushed a commit to TYPO3-CMS/core that referenced this pull request Dec 13, 2019
@bmack @ohader
[BUGFIX] Mark guzzlehttp/guzzle >= 6.5.0 as conflict
9eb93de 
Due to the INTL/ICU bug, which we
have seen on various places, Guzzle, which
does not cover our edge cases yet, ran
in the same issue as our Core versions earlier
in 2019.

See
guzzle/guzzle#2448
guzzle/guzzle#2454

For the time being, lets mark guzzle as
incompatible until Guzzle has solved the issue
and released a new version, so we can loosen
the conflict constraint.

Related: #87953
Resolves: #89904
Releases: master, 9.5, 8.7
Change-Id: If64fb9472d046f020c850cd0551beeaf78796b60
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/62606
Tested-by: TYPO3com <noreply@typo3.com>
Tested-by: Oliver Hader <oliver.hader@typo3.org>
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
reviewtypo3org pushed a commit to TYPO3/typo3 that referenced this pull request Dec 13, 2019
@bmack @ohader
[BUGFIX] Mark guzzlehttp/guzzle >= 6.5.0 as conflict
bb11a3e 
Due to the INTL/ICU bug, which we
have seen on various places, Guzzle, which
does not cover our edge cases yet, ran
in the same issue as our Core versions earlier
in 2019.

See
guzzle/guzzle#2448
guzzle/guzzle#2454

For the time being, lets mark guzzle as
incompatible until Guzzle has solved the issue
and released a new version, so we can loosen
the conflict constraint.

Related: #87953
Resolves: #89904
Releases: master, 9.5, 8.7
Change-Id: If64fb9472d046f020c850cd0551beeaf78796b60
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/62613
Tested-by: TYPO3com <noreply@typo3.com>
Tested-by: Oliver Hader <oliver.hader@typo3.org>
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
reviewtypo3org pushed a commit to TYPO3/typo3 that referenced this pull request Dec 13, 2019
@bmack @ohader
[BUGFIX] Mark guzzlehttp/guzzle >= 6.5.0 as conflict
4a8e800 
Due to the INTL/ICU bug, which we
have seen on various places, Guzzle, which
does not cover our edge cases yet, ran
in the same issue as our Core versions earlier
in 2019.

See
guzzle/guzzle#2448
guzzle/guzzle#2454

For the time being, lets mark guzzle as
incompatible until Guzzle has solved the issue
and released a new version, so we can loosen
the conflict constraint.

Related: #87953
Resolves: #89904
Releases: master, 9.5, 8.7
Change-Id: If64fb9472d046f020c850cd0551beeaf78796b60
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/62612
Tested-by: TYPO3com <noreply@typo3.com>
Tested-by: Oliver Hader <oliver.hader@typo3.org>
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
TYPO3IncTeam pushed a commit to TYPO3-CMS/core that referenced this pull request Dec 13, 2019
@bmack @ohader
[BUGFIX] Mark guzzlehttp/guzzle >= 6.5.0 as conflict
29c306e 
Due to the INTL/ICU bug, which we
have seen on various places, Guzzle, which
does not cover our edge cases yet, ran
in the same issue as our Core versions earlier
in 2019.

See
guzzle/guzzle#2448
guzzle/guzzle#2454

For the time being, lets mark guzzle as
incompatible until Guzzle has solved the issue
and released a new version, so we can loosen
the conflict constraint.

Related: #87953
Resolves: #89904
Releases: master, 9.5, 8.7
Change-Id: If64fb9472d046f020c850cd0551beeaf78796b60
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/62613
Tested-by: TYPO3com <noreply@typo3.com>
Tested-by: Oliver Hader <oliver.hader@typo3.org>
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
TYPO3IncTeam pushed a commit to TYPO3-CMS/core that referenced this pull request Dec 13, 2019
@bmack @ohader
[BUGFIX] Mark guzzlehttp/guzzle >= 6.5.0 as conflict
1cef457 
Due to the INTL/ICU bug, which we
have seen on various places, Guzzle, which
does not cover our edge cases yet, ran
in the same issue as our Core versions earlier
in 2019.

See
guzzle/guzzle#2448
guzzle/guzzle#2454

For the time being, lets mark guzzle as
incompatible until Guzzle has solved the issue
and released a new version, so we can loosen
the conflict constraint.

Related: #87953
Resolves: #89904
Releases: master, 9.5, 8.7
Change-Id: If64fb9472d046f020c850cd0551beeaf78796b60
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/62612
Tested-by: TYPO3com <noreply@typo3.com>
Tested-by: Oliver Hader <oliver.hader@typo3.org>
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
This was referenced Dec 13, 2019
Closed
Closed
@Mark-H
Copy link

Mark-H commented Dec 13, 2019

This PR indeed fixes the issue I reported in #2458.

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

I assume this has to be forward ported to master as well, right?

@alexeyshockov
Copy link
Collaborator Author

alexeyshockov commented Dec 14, 2019 •
edited

@sagikazarmark, correct.

In master we already depend on PHP 7.2+, so the check can be simplified to function_exists('idn_to_ascii') && defined('INTL_IDNA_VARIANT_UTS46').

@alexeyshockov alexeyshockov mentioned this pull request Dec 14, 2019
Closed
@jonnott
Copy link
Contributor

jonnott commented Dec 14, 2019

...so when is 6.5.1 shipping? ;)

ohader
ohader approved these changes Dec 14, 2019
View reviewed changes
Copy link

@ohader ohader left a comment

Choose a reason for hiding this comment

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

Looks good +1

@sagikazarmark sagikazarmark mentioned this pull request Dec 14, 2019
Merged
@jonnott
Copy link
Contributor

jonnott commented Dec 16, 2019

Can 6.5.1 be expedited ... ? Currently anyone running CentOS 6 has broken code if using Guzzle :(

@sagikazarmark
Copy link
Member

@jonnott you can downgrade to 6.4 for the time being. 6.5.1 will be released today or tomorrow.

@sagikazarmark
Copy link
Member

@alexeyshockov can you send this (or the appropriate version) patch to master as well?

sagikazarmark
sagikazarmark approved these changes Dec 18, 2019
View reviewed changes
Copy link
Member

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@sagikazarmark sagikazarmark merged commit 2eda2b9 into guzzle:6.5 Dec 18, 2019
@jonnott
Copy link
Contributor

jonnott commented Dec 18, 2019

@jonnott you can downgrade to 6.4 for the time being. 6.5.1 will be released today or tomorrow.

I temporarily added 'idn_conversion'=>false to all my Guzzle\Client instances instead!

@DaPoHou DaPoHou mentioned this pull request Feb 6, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brandonkelly brandonkelly brandonkelly left review comments

@ohader ohader ohader approved these changes

@sagikazarmark sagikazarmark sagikazarmark approved these changes

@gmponos gmponos gmponos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
6.5.1
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@alexeyshockov @GrahamCampbell @johncarlson21 @Mark-H @sagikazarmark @jonnott @brandonkelly @ohader @gmponos