Rely on iconv and symfony/polyfill-* #16317

nicolas-grekas · 2015-10-22T20:22:36Z

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 work
symfony/polyfill-* packages are not ready yet. Still, review welcomed.

xabbuh · 2015-10-22T20:30:30Z

The DoctrineBridge needs an update of the composer.json file.

nicolas-grekas · 2015-10-22T20:42:12Z

@xabbuh not sure: iconv is an implicit requirement now (same as ext-json)

xabbuh · 2015-10-22T20:43:11Z

src/Symfony/Component/Security/Core/composer.json

@@ -17,25 +17,25 @@
    ],
    "require": {
        "php": ">=5.3.9",
-        "paragonie/random_compat": "~1.0"


This must be kept (random_bytes() is used in the SecureRandom class).

yep, I'm going to add random_compat to polyfill-php70 that's why it's removed from here

xabbuh · 2015-10-22T20:45:14Z

@nicolas-grekas Is iconv even available on Windows by default?

nicolas-grekas · 2015-10-22T20:48:22Z

@xabbuh yes, it's bundled on Windows without adding any dll, at least in the binaries provided on windows.php.net

Tobion · 2015-10-22T21:36:51Z

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?

Tobion · 2015-10-22T21:56:28Z

src/Symfony/Component/Yaml/Inline.php

@@ -52,7 +52,7 @@ public static function parse($value, $exceptionOnInvalidType = false, $objectSup
            return '';
        }

-        if (function_exists('mb_internal_encoding') && ((int) ini_get('mbstring.func_overload')) & 2) {
+        if (2 /* MB_OVERLOAD_STRING */ & (int) ini_get('mbstring.func_overload')) {


isn't this constant polyfilled?

shouldn't we check for extension_loaded('mbstring') here ? Only the extension itself can overload functions anyway

the extension must be enabled for ini_get to return 2 so this is equivalent

See https://3v4l.org/aBCev

nicolas-grekas · 2015-10-23T08:23:48Z

@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 @internal tags to the shim-related classes. Moving the code to polyfill-intl-icu could be useful as a cleanup but it's a bigger work and it won't provide much practical value so I'd like to postpone it for later if we want it.

nicolas-grekas · 2015-10-25T17:57:28Z

Tests are green! (appveyor failure is a transient test)
Status: needs review

Tobion · 2015-10-25T18:15:14Z

src/Symfony/Component/Templating/PhpEngine.php

@@ -330,6 +330,9 @@ public function escape($value, $context = 'html')
     */
    public function setCharset($charset)
    {
+        if ('UTF8' === $charset = strtoupper($charset)) {


why do we need that? Shouldn't it be deprecated if it is for bc? Looks like a hack.

it's because iconv on windows doesn't accept utf8 as an encoding name: only utf-8 is supported

would be good to add this as comment

comment added

Tobion · 2015-10-25T21:08:27Z

👍

Status: Reviewed

fabpot · 2015-10-28T02:34:07Z

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 work symfony/polyfill-* packages are not ready yet. Still, review welcomed. Commits ------- 303f05b Rely on iconv and symfony/polyfill-*

ghost · 2015-10-28T10:01:52Z

@fabpot #16363

Tobion · 2015-10-29T12:20:16Z

I merged 2.8 in master and resolved all the conflicts.

Tobion · 2015-10-29T12:49:01Z

src/Symfony/Component/Serializer/composer.json

@@ -16,7 +16,8 @@
        }
    ],
    "require": {
-        "php": ">=5.3.9"
+        "php": ">=5.3.9",
+        "symfony/polyfill-php54": "~1.0"


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 #16317 Commits ------- 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` class requires PHP 5.5. Commits ------- 3cc4e4d add missing symfony/polyfill-php55 dependency

carsonbot added the Status: Needs Review label Oct 22, 2015

nicolas-grekas added Status: Needs Work and removed Status: Needs Review labels Oct 22, 2015

xabbuh reviewed Oct 22, 2015
View reviewed changes

Tobion reviewed Oct 22, 2015
View reviewed changes

nicolas-grekas force-pushed the polyfill branch 2 times, most recently from 1c39e08 to e8c5ffc Compare October 23, 2015 08:21

nicolas-grekas force-pushed the polyfill branch 4 times, most recently from 1baf102 to 11e866e Compare October 25, 2015 17:46

carsonbot added Status: Needs Review and removed Status: Needs Work labels Oct 25, 2015

Tobion reviewed Oct 25, 2015
View reviewed changes

nicolas-grekas force-pushed the polyfill branch from 11e866e to 062fa04 Compare October 25, 2015 19:08

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 25, 2015

nicolas-grekas force-pushed the polyfill branch 2 times, most recently from 0c9a2e6 to 0e052da Compare October 27, 2015 23:49

Rely on iconv and symfony/polyfill-*

303f05b

nicolas-grekas force-pushed the polyfill branch from 0e052da to 303f05b Compare October 28, 2015 02:15

fabpot merged commit 303f05b into symfony:2.8 Oct 28, 2015

nicolas-grekas deleted the polyfill branch October 28, 2015 02:40

This was referenced Oct 29, 2015

remove polyfills for unsupported php versions #16381

Merged

[Serializer] polyfill for json_last_error_msg is in php 5.5 #16382

Merged

Tobion reviewed Oct 29, 2015
View reviewed changes

fabpot mentioned this pull request Nov 16, 2015

Release v2.8.0-BETA1 #16550

Merged

xabbuh mentioned this pull request Dec 28, 2015

[HttpFoundation] add missing symfony/polyfill-php55 dependency #17156

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rely on iconv and symfony/polyfill-* #16317

Rely on iconv and symfony/polyfill-* #16317

nicolas-grekas commented Oct 22, 2015

xabbuh commented Oct 22, 2015

nicolas-grekas commented Oct 22, 2015

xabbuh Oct 22, 2015

nicolas-grekas Oct 22, 2015

xabbuh Oct 22, 2015

xabbuh commented Oct 22, 2015

nicolas-grekas commented Oct 22, 2015

Tobion commented Oct 22, 2015

Tobion Oct 22, 2015

stof Oct 23, 2015

nicolas-grekas Oct 23, 2015

nicolas-grekas Oct 23, 2015

nicolas-grekas commented Oct 23, 2015

nicolas-grekas commented Oct 25, 2015

Tobion Oct 25, 2015

nicolas-grekas Oct 25, 2015

Tobion Oct 25, 2015

nicolas-grekas Oct 25, 2015

Tobion commented Oct 25, 2015

fabpot commented Oct 28, 2015

ghost commented Oct 28, 2015

Tobion commented Oct 29, 2015

Tobion Oct 29, 2015

Rely on iconv and symfony/polyfill-* #16317

Rely on iconv and symfony/polyfill-* #16317

Conversation

nicolas-grekas commented Oct 22, 2015

xabbuh commented Oct 22, 2015

nicolas-grekas commented Oct 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Oct 22, 2015

nicolas-grekas commented Oct 22, 2015

Tobion commented Oct 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Oct 23, 2015

nicolas-grekas commented Oct 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion commented Oct 25, 2015

fabpot commented Oct 28, 2015

ghost commented Oct 28, 2015

Tobion commented Oct 29, 2015

Choose a reason for hiding this comment