Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Rely on iconv and symfony/polyfill-*#16317
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
xabbuh commentedOct 22, 2015
The DoctrineBridge needs an update of the |
nicolas-grekas commentedOct 22, 2015
@xabbuh not sure: iconv is an implicit requirement now (same as ext-json) |
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.
This must be kept (random_bytes() is used in theSecureRandom class).
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.
yep, I'm going to add random_compat to polyfill-php70 that's why it's removed from here
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.
okay
xabbuh commentedOct 22, 2015
@nicolas-grekas Is iconv even available on Windows by default? |
nicolas-grekas commentedOct 22, 2015
@xabbuh yes, it's bundled on Windows without adding any dll, at least in the binaries provided on windows.php.net |
Tobion commentedOct 22, 2015
Is the idea also to move the Intl component to Polyfill? Isn't the Intl component not basically a Polyfill or does it do more? |
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.
isn't this constant polyfilled?
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.
shouldn't we check forextension_loaded('mbstring') here ? Only the extension itself can overload functions anyway
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.
the extension must be enabled for ini_get to return2 so this is equivalent
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.
1c39e08 toe8c5ffcComparenicolas-grekas commentedOct 23, 2015
@Tobion there where a few missing require for symfony/polyfill-intl-icu that I fixed. I looked at symfony/intl and it's both a shim and a utility component. I added |
1baf102 to11e866eComparenicolas-grekas commentedOct 25, 2015
Tests are green! (appveyor failure is a transient test) |
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.
why do we need that? Shouldn't it be deprecated if it is for bc? Looks like a hack.
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.
it's because iconv on windows doesn't accept utf8 as an encoding name: only utf-8 is supported
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.
would be good to add this as comment
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.
comment added
Tobion commentedOct 25, 2015
👍 Status: Reviewed |
0c9a2e6 to0e052daComparefabpot commentedOct 28, 2015
Thank you@nicolas-grekas. |
This PR was merged into the 2.8 branch.Discussion----------Rely on iconv and symfony/polyfill-*| Q | A| ------------- | ---| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets |#16240| License | MIT| Doc PR | -Status: needs worksymfony/polyfill-* packages are not ready yet. Still, review welcomed.Commits-------303f05b Rely on iconv and symfony/polyfill-*
ghost commentedOct 28, 2015
Tobion commentedOct 29, 2015
I merged 2.8 in master and resolved all the conflicts. |
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.
This is wrong:#16382
…5 (Tobion)This PR was merged into the 2.8 branch.Discussion----------[Serializer] polyfill for json_last_error_msg is in php 5.5| Q | A| ------------- | ---| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#16317| License | MIT| Doc PR | -We need the polyfill for `json_last_error_msg` which is part of php 5.5, not 5.4.Commits-------fb031f1 [Serializer] polyfill for json_last_error_msg is in php 5.5
This PR was merged into the 3.0-dev branch.Discussion----------remove polyfills for unsupported php versions| Q | A| ------------- | ---| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Remove obsolete polyfills in master as introduced in#16317Commits-------78512cc remove polyfills for unsupported php versions
…ency (xabbuh)This PR was merged into the 2.8 branch.Discussion----------[HttpFoundation] add missing symfony/polyfill-php55 dependency| Q | A| ------------- | ---| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#16317| License | MIT| Doc PR |The `json_last_error_msg()` function used in the `JsonResponse` classrequires PHP 5.5.Commits-------3cc4e4d add missing symfony/polyfill-php55 dependency
Status: needs work
symfony/polyfill-* packages are not ready yet. Still, review welcomed.