Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[6.5] Fix various intl icu issues #2626
[6.5] Fix various intl icu issues #2626
Changes from 2 commits
8d3709f
44de375
18d89d7
6606165
05965c0
0758da5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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 callidn_to_ascii
(from the polyfill) rather than the internal API.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're not :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example CentOS 6, which is still supported until 2020-11-30, has ICU 4.2.1.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) .. ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 byidn_to_ascii
. It does not change the fact thatidn_to_ascii
is defined by the extension when defined (and so the polyfill could not define it).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
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 theidn_conversion
configuration.