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

[Polyfill] A new component for portability across PHP versions and extensions #16240

Closed
wants to merge 1 commit into
base: 2.8
from

Conversation

Projects
None yet
@nicolas-grekas
Member

nicolas-grekas commented Oct 14, 2015

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

I know I come a bit late in the 2.8 releasing process, but I really hope we could merge this new component into 2.8, especially since merging it in 3.1 would remove a great deal of its value since the lowest PHP version there is 5.5, whereas this code has shims for pre-5.5 features.

So here we are:

  • polyfills for the mbstring
  • polyfills for the Normalizer class and grapheme_* functions
  • polyfills for some constants and functions introduced in 5.3, 5.4, 5.5, etc. borrowed from https://github.com/nicolas-grekas/Patchwork/tree/master/core/compat (yet an other code of mine)
  • polyfills for hash_equals, hash_pbkdf2 and ldap_escape moved out of existing private methods
  • utility class Binary to be used when mbstring.func_overload is enabled

This allows simplifying the full code base by removing many defined and function_exists checks.

For reference, here is the lists of e.g. functions introduced in 5.4:
http://www.php.net/migration54.functions.php

Show outdated Hide outdated src/Symfony/Component/Polyfill/bootstrap.php
define('JSON_BIGINT_AS_STRING', 2);
define('JSON_UNESCAPED_SLASHES', 64);
define('JSON_PRETTY_PRINT', 128);
define('JSON_UNESCAPED_UNICODE', 256);

This comment has been minimized.

@stof

stof Oct 14, 2015

Member

-1 for this. This breaks libraries detecting whether the feature is detected (old versions of Symfony were defining the constant at the beginning of the file, and it was changed precisely for this reason).

constants should be defined only if their behavior is also polyfilled.

@stof

stof Oct 14, 2015

Member

-1 for this. This breaks libraries detecting whether the feature is detected (old versions of Symfony were defining the constant at the beginning of the file, and it was changed precisely for this reason).

constants should be defined only if their behavior is also polyfilled.

This comment has been minimized.

@stof

stof Oct 14, 2015

Member

the same is true for the JsonSerializable polyfill btw

@stof

stof Oct 14, 2015

Member

the same is true for the JsonSerializable polyfill btw

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Oct 14, 2015

Member

I don't think creating a component with many polyfills in it is a good idea. Someone wanting to have a random_bytes should not have to depend on the whole UTF-8 polyfills (especially given that such polyfills cannot be loaded on demand as they are for functions and constants, not for autoloadable classes)

So my vote is a -1 here.

If the random_bytes polyfill is buggy on Windows, send a PR to the existing library to improve the Windows support, and it will benefit everyone.

Member

stof commented Oct 14, 2015

I don't think creating a component with many polyfills in it is a good idea. Someone wanting to have a random_bytes should not have to depend on the whole UTF-8 polyfills (especially given that such polyfills cannot be loaded on demand as they are for functions and constants, not for autoloadable classes)

So my vote is a -1 here.

If the random_bytes polyfill is buggy on Windows, send a PR to the existing library to improve the Windows support, and it will benefit everyone.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 14, 2015

Member

This needs some explanations: polyfill are autoloaded, they don't pollute anyone’s app.
This is possible thanks to the static classes proxies for polyfills: the bootstrap file is all that is required, with very minimal code to parse/load. Loading the implementation happens only when a polyfill is actually used.

I do think that one single component for all polyfills is a greater idea: polyfills have a finite upper limit (100% feature backporting). By collecting all efforts in one code base, we can concentrate efforts for everyone, instead of scattering one polyfill here, an other there, with varying implementation quality.

For random_bytes, my implementation is radically different so patching would mean removing most of the code. I'm not that rude.

Please reconsider your vote, or at least allow some discussion.

Member

nicolas-grekas commented Oct 14, 2015

This needs some explanations: polyfill are autoloaded, they don't pollute anyone’s app.
This is possible thanks to the static classes proxies for polyfills: the bootstrap file is all that is required, with very minimal code to parse/load. Loading the implementation happens only when a polyfill is actually used.

I do think that one single component for all polyfills is a greater idea: polyfills have a finite upper limit (100% feature backporting). By collecting all efforts in one code base, we can concentrate efforts for everyone, instead of scattering one polyfill here, an other there, with varying implementation quality.

For random_bytes, my implementation is radically different so patching would mean removing most of the code. I'm not that rude.

Please reconsider your vote, or at least allow some discussion.

Show outdated Hide outdated src/Symfony/Component/HttpFoundation/Response.php
@@ -1147,7 +1147,7 @@ public static function closeOutputBuffers($targetLevel, $flush)
{
$status = ob_get_status(true);
$level = count($status);
$flags = defined('PHP_OUTPUT_HANDLER_REMOVABLE') ? PHP_OUTPUT_HANDLER_REMOVABLE | ($flush ? PHP_OUTPUT_HANDLER_FLUSHABLE : PHP_OUTPUT_HANDLER_CLEANABLE) : -1;
$flags = PHP_OUTPUT_HANDLER_REMOVABLE | ($flush ? PHP_OUTPUT_HANDLER_FLUSHABLE : PHP_OUTPUT_HANDLER_CLEANABLE);

This comment has been minimized.

@stof

stof Oct 14, 2015

Member

this is actually changing the behavior on PHP 5.3

@stof

stof Oct 14, 2015

Member

this is actually changing the behavior on PHP 5.3

Show outdated Hide outdated src/Symfony/Component/Polyfill/Php55.php
return self::opcache_reset();
}
public static function opcache_reset()

This comment has been minimized.

@stof

stof Oct 14, 2015

Member

polyfilling opcache methods by applying changes in other accelerators looks really weird to me.

@stof

stof Oct 14, 2015

Member

polyfilling opcache methods by applying changes in other accelerators looks really weird to me.

Show outdated Hide outdated src/Symfony/Component/Polyfill/Php70.php
}
$allBytes .= $bytes;
}
if ($h = @fopen('/dev/urandom', 'rb')) {

This comment has been minimized.

@stof

stof Oct 14, 2015

Member

isn't your implementation affected by paragonie/random_compat#49 ?

@stof

stof Oct 14, 2015

Member

isn't your implementation affected by paragonie/random_compat#49 ?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 14, 2015

Member

true, fixed by excluding windows and checking for char device

@nicolas-grekas

nicolas-grekas Oct 14, 2015

Member

true, fixed by excluding windows and checking for char device

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 14, 2015

Member

I removed constants definitions that did not match any shim implementation. I also added function_exists checks to the bootstrap file to enhance compat with existing shims.

Member

nicolas-grekas commented Oct 14, 2015

I removed constants definitions that did not match any shim implementation. I also added function_exists checks to the bootstrap file to enhance compat with existing shims.

Show outdated Hide outdated src/Symfony/Component/Polyfill/bootstrap.php
if (!ini_get('session.entropy_file') && !ini_get('session.entropy_length')) {
ini_set('session.entropy_length', 32);
if (false !== @file_get_contents('/dev/urandom', false, null, -1, 1) ) {

This comment has been minimized.

@stof

stof Oct 14, 2015

Member

windows here too

@stof

stof Oct 14, 2015

Member

windows here too

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 14, 2015

Member

in fact I'm going to remove these ini_setting: it's reducing the entropy for auto conf, not a good

@nicolas-grekas

nicolas-grekas Oct 14, 2015

Member

in fact I'm going to remove these ini_setting: it's reducing the entropy for auto conf, not a good

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Oct 14, 2015

Contributor

@nicolas-grekas why do we need this in Symfony 3 ? the requirements are php > 5.5

Contributor

mickaelandrieu commented Oct 14, 2015

@nicolas-grekas why do we need this in Symfony 3 ? the requirements are php > 5.5

Show outdated Hide outdated src/Symfony/Component/Polyfill/bootstrap.php
function mb_http_input($type = '') {return p\Mbstring::mb_http_input($type);}
}
if (!function_exists('utf8_encode')) {

This comment has been minimized.

@stof

stof Oct 14, 2015

Member

when is it not defined ?

@stof

stof Oct 14, 2015

Member

when is it not defined ?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 14, 2015

Member

this function is provided by the xml extension, which is not always loaded

@nicolas-grekas

nicolas-grekas Oct 14, 2015

Member

this function is provided by the xml extension, which is not always loaded

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Oct 14, 2015

Member

@nicolas-grekas discussion is allowed (this is even required to make a decider reconsider their vote btw).

Member

stof commented Oct 14, 2015

@nicolas-grekas discussion is allowed (this is even required to make a decider reconsider their vote btw).

Show outdated Hide outdated src/Symfony/Component/Polyfill/Resources/stubs/Throwable.php
@@ -0,0 +1,13 @@
<?php
interface Throwable

This comment has been minimized.

@stof

stof Oct 14, 2015

Member

Defining this interface while Exception does not implement it does not seem like a good idea to me

@stof

stof Oct 14, 2015

Member

Defining this interface while Exception does not implement it does not seem like a good idea to me

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 14, 2015

Member

fine by me, I'm removing the PHP7 exceptions related shims

@nicolas-grekas

nicolas-grekas Oct 14, 2015

Member

fine by me, I'm removing the PHP7 exceptions related shims

Show outdated Hide outdated src/Symfony/Component/Polyfill/Resources/stubs/Error.php
@@ -0,0 +1,8 @@
<?php
class Error extends Exception implements Throwable

This comment has been minimized.

@stof

stof Oct 14, 2015

Member

this stub is misleading. Error in PHP 7 is not catched by a catch(\Exception $e), while it will catch your stub. this makes the stub unusable

@stof

stof Oct 14, 2015

Member

this stub is misleading. Error in PHP 7 is not catched by a catch(\Exception $e), while it will catch your stub. this makes the stub unusable

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Oct 14, 2015

Contributor

Humm this component is a good idea for Symfony 2.

My only problem is that all components concerned have a new dependency on it, and this component will be - in most cases - useless because PHP 5.5 will become the standard for Symfony 3.

So, if the pull request is only for Symfony 2.3+ && < 3.0 it's 👍 else 👎

Contributor

mickaelandrieu commented Oct 14, 2015

Humm this component is a good idea for Symfony 2.

My only problem is that all components concerned have a new dependency on it, and this component will be - in most cases - useless because PHP 5.5 will become the standard for Symfony 3.

So, if the pull request is only for Symfony 2.3+ && < 3.0 it's 👍 else 👎

Show outdated Hide outdated src/Symfony/Component/Polyfill/Tests/IconvTest.php
/**
* @covers Symfony\Component\Polyfill\Iconv::iconv_strlen
* @covers Symfony\Component\Polyfill\Iconv::strlen1
* @covers Symfony\Component\Polyfill\Iconv::strlen2

This comment has been minimized.

@stof

stof Oct 14, 2015

Member

we don't use @covers in Symfony

@stof

stof Oct 14, 2015

Member

we don't use @covers in Symfony

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 14, 2015

Member

yes I know but this PR imports a code that existed previously, and removing the @covers annotations would only reduce the quality of the code (when running coverage tests). Which means I'd like we keep them.

@nicolas-grekas

nicolas-grekas Oct 14, 2015

Member

yes I know but this PR imports a code that existed previously, and removing the @covers annotations would only reduce the quality of the code (when running coverage tests). Which means I'd like we keep them.

Show outdated Hide outdated src/Symfony/Component/Polyfill/Tests/IntlTest.php
$this->assertTrue(true, 'Regular PHP throws a warning');
} catch (\PHPUnit_Framework_Error_Notice $e) {
$this->assertTrue(true, 'HHVM throws a notice');
}

This comment has been minimized.

@stof

stof Oct 14, 2015

Member

shouldn't it actually use a different expectation based on HHVM vs PHP ? There is nothing ensuring that the stub produces a notice on HHVM here

@stof

stof Oct 14, 2015

Member

shouldn't it actually use a different expectation based on HHVM vs PHP ? There is nothing ensuring that the stub produces a notice on HHVM here

Show outdated Hide outdated src/Symfony/Component/Polyfill/Tests/MbstringTest.php
* @author Nicolas Grekas <p@tchwork.com>
*
* @covers Symfony\Component\Polyfill\Mbstring::<!public>
* @requires extension mbstring

This comment has been minimized.

@stof

stof Oct 14, 2015

Member

several tests don't require the extension actually

@stof

stof Oct 14, 2015

Member

several tests don't require the extension actually

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 14, 2015

Member

well, I thought about that and I think it's better for tests to run against reference implementations (because we don't test the shim). So IMO it's better to keep the annotations.

@nicolas-grekas

nicolas-grekas Oct 14, 2015

Member

well, I thought about that and I think it's better for tests to run against reference implementations (because we don't test the shim). So IMO it's better to keep the annotations.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 14, 2015

Member

@mickaelandrieu yes I'd like this component to land in 3.0. But of course, all shims for pre-5.5 features should be removed when merging.

Member

nicolas-grekas commented Oct 14, 2015

@mickaelandrieu yes I'd like this component to land in 3.0. But of course, all shims for pre-5.5 features should be removed when merging.

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Oct 14, 2015

Contributor

@nicolas-grekas ok then 👍 btw great job as always :)

Contributor

mickaelandrieu commented Oct 14, 2015

@nicolas-grekas ok then 👍 btw great job as always :)

@csarrazi

This comment has been minimized.

Show comment
Hide comment
@csarrazi

csarrazi Oct 14, 2015

Contributor

@nicolas-grekas Nice one. However, should this PR limit itself to functions, or also define constants if they are not available?

I'm talking about constants like LDAP_ESCAPE_DN or LDAP_ESCAPE_FILTER, for example.

Contributor

csarrazi commented Oct 14, 2015

@nicolas-grekas Nice one. However, should this PR limit itself to functions, or also define constants if they are not available?

I'm talking about constants like LDAP_ESCAPE_DN or LDAP_ESCAPE_FILTER, for example.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 14, 2015

Member

@stof comments addressed, tests should be green on travis and appveyor
@csarrazi these constants are required since they are provided as arguments to the ldap_escape() shim.

Member

nicolas-grekas commented Oct 14, 2015

@stof comments addressed, tests should be green on travis and appveyor
@csarrazi these constants are required since they are provided as arguments to the ldap_escape() shim.

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Oct 14, 2015

Member

Shouldn't the new polyfill component be only in the suggest section of other components but included with the standard edition? It will avoid to download this extra set of files for projects using a recent PHP version.

Member

dunglas commented Oct 14, 2015

Shouldn't the new polyfill component be only in the suggest section of other components but included with the standard edition? It will avoid to download this extra set of files for projects using a recent PHP version.

*
* @internal
*/
class Normalizer

This comment has been minimized.

@webmozart

webmozart Oct 14, 2015

Contributor

Doesn't this rather belong into the Intl extension? Or should the relevant Intl code be moved here at some point?

@webmozart

webmozart Oct 14, 2015

Contributor

Doesn't this rather belong into the Intl extension? Or should the relevant Intl code be moved here at some point?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 14, 2015

Member

Merging Intl's functions.php with bootstrap.php would be great as it will remove one include yes.

@nicolas-grekas

nicolas-grekas Oct 14, 2015

Member

Merging Intl's functions.php with bootstrap.php would be great as it will remove one include yes.

@webmozart

This comment has been minimized.

Show comment
Hide comment
@webmozart

webmozart Oct 14, 2015

Contributor

I like the idea behind this! I agree that one single code base for all polyfills is probably better than several repositories where it's hard to know what is where.

Contributor

webmozart commented Oct 14, 2015

I like the idea behind this! I agree that one single code base for all polyfills is probably better than several repositories where it's hard to know what is where.

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Oct 14, 2015

Contributor

@webmozart agree to your last comment: this is a dependency I can accept because make theses features available for each component is a mess to maintain and will result to a lot of duplications.

I'd like to see a pull request opened soon to remove all unused stuff for Symfony 3, I can help you @nicolas-grekas if you don't have time for it now :)

Contributor

mickaelandrieu commented Oct 14, 2015

@webmozart agree to your last comment: this is a dependency I can accept because make theses features available for each component is a mess to maintain and will result to a lot of duplications.

I'd like to see a pull request opened soon to remove all unused stuff for Symfony 3, I can help you @nicolas-grekas if you don't have time for it now :)

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Oct 14, 2015

Contributor

@dunglas what did you suggest? Afaik we can't suggest packages in composer accordingly to the PHP version (could be a great feature indeed)

Contributor

mickaelandrieu commented Oct 14, 2015

@dunglas what did you suggest? Afaik we can't suggest packages in composer accordingly to the PHP version (could be a great feature indeed)

Show outdated Hide outdated src/Symfony/Component/Polyfill/bootstrap.php
if (!function_exists('hash_equals')) {
function hash_equals($knownString, $userInput) {return p\Php56::hash_equals($knownString, $userInput);}
}
if (!function_exists('ldap_escape')) {

This comment has been minimized.

@csarrazi

csarrazi Oct 14, 2015

Contributor

We should not only check whether the ldap_escape function exists or not, but also check whether the extension is enabled or not.

@csarrazi

csarrazi Oct 14, 2015

Contributor

We should not only check whether the ldap_escape function exists or not, but also check whether the extension is enabled or not.

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Oct 15, 2015

Member

@mickaelandrieu if someone needs the Polyfill because he uses an old PHP version it should install the Polyfill Component manually. That way people running on the last PHP version are not forced to download it.

Member

dunglas commented Oct 15, 2015

@mickaelandrieu if someone needs the Polyfill because he uses an old PHP version it should install the Polyfill Component manually. That way people running on the last PHP version are not forced to download it.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Oct 15, 2015

Member

@dunglas the polyfill is not only about older versions, but also about some extensions you don't have.

If we don't require it, we cannot safely use mb_* functions in components, as it would force users of the component to install either mbstring or the polyfill component, while the code currently works for them all the time. So it would be a BC break.

Member

stof commented Oct 15, 2015

@dunglas the polyfill is not only about older versions, but also about some extensions you don't have.

If we don't require it, we cannot safely use mb_* functions in components, as it would force users of the component to install either mbstring or the polyfill component, while the code currently works for them all the time. So it would be a BC break.

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Oct 15, 2015

Member

@stof you're right I've not thought about the BC break it introduces.
Maybe can it can be kept in require for 2.8 (BC) but removed in 3.0?

Member

dunglas commented Oct 15, 2015

@stof you're right I've not thought about the BC break it introduces.
Maybe can it can be kept in require for 2.8 (BC) but removed in 3.0?

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Oct 15, 2015

Member

@dunglas the experience would still be bad for all these component, as they would be broken by default (and many projects are depending on them, so the end user might not know about the need to choose between installing mbstring or the polyfill component)

Member

stof commented Oct 15, 2015

@dunglas the experience would still be bad for all these component, as they would be broken by default (and many projects are depending on them, so the end user might not know about the need to choose between installing mbstring or the polyfill component)

@csarrazi

This comment has been minimized.

Show comment
Hide comment
@csarrazi

csarrazi Oct 15, 2015

Contributor

@dunglas @stof @nicolas-grekas What about also adding a suggest for the different PHP extensions, in all bundles which require them?

Contributor

csarrazi commented Oct 15, 2015

@dunglas @stof @nicolas-grekas What about also adding a suggest for the different PHP extensions, in all bundles which require them?

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Oct 15, 2015

Member

As you want but if the polyfill is always included it would be nice to have an optin notice explaning that upgrading to a newer PHP version or installing an extension would enhance performance and sometimes security (hash_equals is less reliable in PHP than in C).

Member

dunglas commented Oct 15, 2015

As you want but if the polyfill is always included it would be nice to have an optin notice explaning that upgrading to a newer PHP version or installing an extension would enhance performance and sometimes security (hash_equals is less reliable in PHP than in C).

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu
Contributor

mickaelandrieu commented Oct 15, 2015

@csarrazi 👍

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 16, 2015

Member

Upgrade pushed with a major enhancement: tests are now run twice automatically. The first time against the internal implementation if available, the second time on the shim implementation. This makes this component a high quality framework for creating shims in PHP (and allowed me to fix some differences already).

I also merged intl's functions.php into bootstrap.php, which means one less global require for composer.
And added the extensions to the composer.json's suggestions.
@webmozart I'd like to keep Normalizer out of Intl for at least one historical reason: I plan to build tchwork/utf8 2.0 on top of this polyfill, but don't want to import 11Mb of data when doing so...

ping @symfony/deciders, all comments addressed, no more work to be done here on my side.

Member

nicolas-grekas commented Oct 16, 2015

Upgrade pushed with a major enhancement: tests are now run twice automatically. The first time against the internal implementation if available, the second time on the shim implementation. This makes this component a high quality framework for creating shims in PHP (and allowed me to fix some differences already).

I also merged intl's functions.php into bootstrap.php, which means one less global require for composer.
And added the extensions to the composer.json's suggestions.
@webmozart I'd like to keep Normalizer out of Intl for at least one historical reason: I plan to build tchwork/utf8 2.0 on top of this polyfill, but don't want to import 11Mb of data when doing so...

ping @symfony/deciders, all comments addressed, no more work to be done here on my side.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 16, 2015

Member

I removed all @requires extension mbstring|iconv, tests are still green (see appveyor with php.ini-min).

Member

nicolas-grekas commented Oct 16, 2015

I removed all @requires extension mbstring|iconv, tests are still green (see appveyor with php.ini-min).

@sstok

This comment has been minimized.

Show comment
Hide comment
@sstok

sstok Oct 18, 2015

Contributor

Is there a special reason for changing the order in random_bytes?

The paragonie/random_compat checks sodium first and then mcrypt while in the Polyfill you changed the this order. And openssl_random_pseudo_bytes() should be considered last as it's actually not fully cryptographically safe!

/cc @paragonie-scott

Contributor

sstok commented Oct 18, 2015

Is there a special reason for changing the order in random_bytes?

The paragonie/random_compat checks sodium first and then mcrypt while in the Polyfill you changed the this order. And openssl_random_pseudo_bytes() should be considered last as it's actually not fully cryptographically safe!

/cc @paragonie-scott

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 18, 2015

Member

@sstok the reason is that it is documented that mcrypt "does the right thing" on Windows an on *nixes.
Since this is a shim, we shouldn't try to be better than the internal implementation in PHP7, which is considered good enough.
For openssl, it's better than others when $cryptoSafe is true, which is checked here.

Member

nicolas-grekas commented Oct 18, 2015

@sstok the reason is that it is documented that mcrypt "does the right thing" on Windows an on *nixes.
Since this is a shim, we shouldn't try to be better than the internal implementation in PHP7, which is considered good enough.
For openssl, it's better than others when $cryptoSafe is true, which is checked here.

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Oct 21, 2015

Member

I'm still annoyed by this new component being a dependency of almost all other components even with Symfony 3.0 (as I already explained, it doesn't make sense to download some extra MB for nothing in the case where extensions are already installed and the PHP version is recent) but the idea of having a centralized polyfill for missing features is great so I'm 👍 to merge it in 2.8.

Member

dunglas commented Oct 21, 2015

I'm still annoyed by this new component being a dependency of almost all other components even with Symfony 3.0 (as I already explained, it doesn't make sense to download some extra MB for nothing in the case where extensions are already installed and the PHP version is recent) but the idea of having a centralized polyfill for missing features is great so I'm 👍 to merge it in 2.8.

@derrabus

This comment has been minimized.

Show comment
Hide comment
@derrabus

derrabus Oct 21, 2015

Contributor

The dependency we're about to add affects the global namespace. No other Symfony Component did this to such an extent. I fear side effects in projects that only use some few Symfony Components and that might already have other/own polyfills in place. See my concerns regarding the self-contained components HttpFoundation and Serializer above.

Contributor

derrabus commented Oct 21, 2015

The dependency we're about to add affects the global namespace. No other Symfony Component did this to such an extent. I fear side effects in projects that only use some few Symfony Components and that might already have other/own polyfills in place. See my concerns regarding the self-contained components HttpFoundation and Serializer above.

@teohhanhui

This comment has been minimized.

Show comment
Hide comment
@teohhanhui

teohhanhui Oct 21, 2015

Contributor

If Security was split from one monolithic component into smaller single-purpose components, why can't the same be done here?

Contributor

teohhanhui commented Oct 21, 2015

If Security was split from one monolithic component into smaller single-purpose components, why can't the same be done here?

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 21, 2015

Member

@derrabus the intl component already does inject a few functions in the global namespace. In this PR, the only file that injects things into the global namespace is bootstrap.php. We should carefully review it so that we ensure that your concern does not apply. I just did so: all declarations are enclosed in if blocks that IMO are granular enough to prevent any collision with any existing shim (that also must be enabled conditionally). What can happen is that an implementation from this PR can take over an other existing shim. But since it's a shim, either they behave the same and it's not an issue at all, or they behave differently and we will able to decide the right behavior by comparing to the native implem behavior. And fixing the shim here if needed. This is theoretical pov: all shims provided here were already written prior to being proposed for this component. Which means they are quite proven. In practice, we're talking about a few simple functions that can't diverge from the native behavior, thus are "correct". And the most complex ones (e.g. hash_equals) have already been reviewed carefully in the past.

@teohhanhui it's not possible to conditionally load packages at the composer level, which means it's not possible to split into several packages without requiring everyone to take care of this subject by understanding why then adding explicit requirements to the composer.json. Which is btw not possible for OSS where some portability is a basic requirement.

@dunglas just to tell real facts "some extra MB" is wrong: 500kb is the right number (80kb compressed)

Member

nicolas-grekas commented Oct 21, 2015

@derrabus the intl component already does inject a few functions in the global namespace. In this PR, the only file that injects things into the global namespace is bootstrap.php. We should carefully review it so that we ensure that your concern does not apply. I just did so: all declarations are enclosed in if blocks that IMO are granular enough to prevent any collision with any existing shim (that also must be enabled conditionally). What can happen is that an implementation from this PR can take over an other existing shim. But since it's a shim, either they behave the same and it's not an issue at all, or they behave differently and we will able to decide the right behavior by comparing to the native implem behavior. And fixing the shim here if needed. This is theoretical pov: all shims provided here were already written prior to being proposed for this component. Which means they are quite proven. In practice, we're talking about a few simple functions that can't diverge from the native behavior, thus are "correct". And the most complex ones (e.g. hash_equals) have already been reviewed carefully in the past.

@teohhanhui it's not possible to conditionally load packages at the composer level, which means it's not possible to split into several packages without requiring everyone to take care of this subject by understanding why then adding explicit requirements to the composer.json. Which is btw not possible for OSS where some portability is a basic requirement.

@dunglas just to tell real facts "some extra MB" is wrong: 500kb is the right number (80kb compressed)

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 21, 2015

Member

By leveraging the fact that iconv is now a requirement of sf 2.8, I removed the polyfill dependency for the Debug and Templating components and for the Doctrine bridge also. The others are legitimate.

Member

nicolas-grekas commented Oct 21, 2015

By leveraging the fact that iconv is now a requirement of sf 2.8, I removed the polyfill dependency for the Debug and Templating components and for the Doctrine bridge also. The others are legitimate.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Oct 21, 2015

Member

@nicolas-grekas intl was not split separately. And what caused us issues in the past with symfony/icu was not that it was in a separate package. It was that different versions of the package needed to be installed depending on whether ext-intl was installed or no (meaning it was not written as a polyfill).

To make things easier for packages needing a polyfill for only a few functions and not wanting to depend on a huge polyfill, I agree with people saying previously that they should probably be separate packages, allowing composer packages to depend only on the polyfill they really need.

Instead of creating a single Symfony component aimed at containing all high-quality polyfills, what do you think about creating a github organization (and a composer vendor namespace) dedicated at high-quality polyfills instead ? This way, it would be possible to have many separate packages. And Symfony could depend on the polyfills it needs (allowing HttpFoundation and Serializer to depend only on a json_last_error_msg polyfill, and dropping the dependency in 3.0 too)

Member

stof commented Oct 21, 2015

@nicolas-grekas intl was not split separately. And what caused us issues in the past with symfony/icu was not that it was in a separate package. It was that different versions of the package needed to be installed depending on whether ext-intl was installed or no (meaning it was not written as a polyfill).

To make things easier for packages needing a polyfill for only a few functions and not wanting to depend on a huge polyfill, I agree with people saying previously that they should probably be separate packages, allowing composer packages to depend only on the polyfill they really need.

Instead of creating a single Symfony component aimed at containing all high-quality polyfills, what do you think about creating a github organization (and a composer vendor namespace) dedicated at high-quality polyfills instead ? This way, it would be possible to have many separate packages. And Symfony could depend on the polyfills it needs (allowing HttpFoundation and Serializer to depend only on a json_last_error_msg polyfill, and dropping the dependency in 3.0 too)

@teohhanhui

This comment has been minimized.

Show comment
Hide comment
@teohhanhui

teohhanhui Oct 21, 2015

Contributor

@nicolas-grekas

it's not possible to conditionally load packages at the composer level, which means it's not possible to split into several packages without requiring everyone to take care of this subject by understanding why then adding explicit requirements to the composer.json. Which is btw not possible for OSS where some portability is a basic requirement.

What I mean is that each Symfony component should only depend on the polyfill packages that it needs. (And symfony/symfony would probably require all of the polyfill packages.) It's not conditional requirements.

Contributor

teohhanhui commented Oct 21, 2015

@nicolas-grekas

it's not possible to conditionally load packages at the composer level, which means it's not possible to split into several packages without requiring everyone to take care of this subject by understanding why then adding explicit requirements to the composer.json. Which is btw not possible for OSS where some portability is a basic requirement.

What I mean is that each Symfony component should only depend on the polyfill packages that it needs. (And symfony/symfony would probably require all of the polyfill packages.) It's not conditional requirements.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 21, 2015

Member

What I mean is that each Symfony component should only depend on the
polyfill packages that it needs. It's not conditional requirements.

OK. Then what benefits do you expect?

Member

nicolas-grekas commented Oct 21, 2015

What I mean is that each Symfony component should only depend on the
polyfill packages that it needs. It's not conditional requirements.

OK. Then what benefits do you expect?

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 21, 2015

Member

@stof I'm sorry but I really don't see what's "huge" in this polyfill. Splitting it into several packages would only add more require to the bootstrapping process without any concrete benefit. Or put an other way: what would be the benefits of splitting?

Member

nicolas-grekas commented Oct 21, 2015

@stof I'm sorry but I really don't see what's "huge" in this polyfill. Splitting it into several packages would only add more require to the bootstrapping process without any concrete benefit. Or put an other way: what would be the benefits of splitting?

$map = $lower;
}
static $ulenMask = array("\xC0" => 2, "\xD0" => 2, "\xE0" => 3, "\xF0" => 4);

This comment has been minimized.

@rparsi

rparsi Oct 21, 2015

@nicolas-grekas Why isn't this a class property?

@rparsi

rparsi Oct 21, 2015

@nicolas-grekas Why isn't this a class property?

@rparsi

This comment has been minimized.

Show comment
Hide comment
@rparsi

rparsi Oct 21, 2015

@nicolas-grekas @stof @mickaelandrieu @csarrazi @dunglas @webmozart @sstok @xabbuh @paragonie-scott @ircmaxell @derrabus @teohhanhui @fabpot
DISCLAIMER: I'm just a random php dev so feel free to ignore this, and my apologies in advance for anything newb-ish.

I've read all the comments and looked at the code changes, and here's my attempt to summarize the concerns so you the maintainers/collaborators of Symfony can decide what to do.

  1. The Polyfill component doesn't behave like a component in that other components become dependent on it. I'm using the current definition of components as defined here http://symfony.com/components
    which is "Symfony Components are a set of decoupled and reusable PHP libraries"
  2. As others have pointed out, this Polyfill "super" component (as in super class, sorry can't think of a better description atm) introduces compatibility problems for mixing/matching components, as developers may need to for their given use case.

Personally I like stof's idea:
"Instead of creating a single Symfony component aimed at containing all high-quality polyfills, what do you think about creating a github organization (and a composer vendor namespace) dedicated at high-quality polyfills instead ? This way, it would be possible to have many separate packages. And Symfony could depend on the polyfills it needs (allowing HttpFoundation and Serializer to depend only on a json_last_error_msg polyfill, and dropping the dependency in 3.0 too)"

You get the benefits of the polyfills with less headaches.

Just my opinion:
The Polyfill component as is in this PR solves one problem (cleaner code) but breaks the meaning of what a Symonfy Component is supposed to be, and introduces potential compatibility headaches for the rest of us common php developer folks. As someone who really likes the Symfony framework and it's decoupled nature this is alarming; Polyfill feels like a hack almost.

If you guys decide to go down this road then the official documentation should be updated, so newbs like me aren't mislead.

rparsi commented Oct 21, 2015

@nicolas-grekas @stof @mickaelandrieu @csarrazi @dunglas @webmozart @sstok @xabbuh @paragonie-scott @ircmaxell @derrabus @teohhanhui @fabpot
DISCLAIMER: I'm just a random php dev so feel free to ignore this, and my apologies in advance for anything newb-ish.

I've read all the comments and looked at the code changes, and here's my attempt to summarize the concerns so you the maintainers/collaborators of Symfony can decide what to do.

  1. The Polyfill component doesn't behave like a component in that other components become dependent on it. I'm using the current definition of components as defined here http://symfony.com/components
    which is "Symfony Components are a set of decoupled and reusable PHP libraries"
  2. As others have pointed out, this Polyfill "super" component (as in super class, sorry can't think of a better description atm) introduces compatibility problems for mixing/matching components, as developers may need to for their given use case.

Personally I like stof's idea:
"Instead of creating a single Symfony component aimed at containing all high-quality polyfills, what do you think about creating a github organization (and a composer vendor namespace) dedicated at high-quality polyfills instead ? This way, it would be possible to have many separate packages. And Symfony could depend on the polyfills it needs (allowing HttpFoundation and Serializer to depend only on a json_last_error_msg polyfill, and dropping the dependency in 3.0 too)"

You get the benefits of the polyfills with less headaches.

Just my opinion:
The Polyfill component as is in this PR solves one problem (cleaner code) but breaks the meaning of what a Symonfy Component is supposed to be, and introduces potential compatibility headaches for the rest of us common php developer folks. As someone who really likes the Symfony framework and it's decoupled nature this is alarming; Polyfill feels like a hack almost.

If you guys decide to go down this road then the official documentation should be updated, so newbs like me aren't mislead.

@derrabus

This comment has been minimized.

Show comment
Hide comment
@derrabus

derrabus Oct 21, 2015

Contributor

@rparsi 👏

Contributor

derrabus commented Oct 21, 2015

@rparsi 👏

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 21, 2015

Member

@rparsi thanks for your input but you misread the previous comments and the definition:

  • some raised compat concern, but none is proven, at least nobody refuted my arguments saying there is no compat issue.
  • the definition of a component does not prevent components being dependent on each others, which is of course already the case for many components. There is no difference.

...

Member

nicolas-grekas commented Oct 21, 2015

@rparsi thanks for your input but you misread the previous comments and the definition:

  • some raised compat concern, but none is proven, at least nobody refuted my arguments saying there is no compat issue.
  • the definition of a component does not prevent components being dependent on each others, which is of course already the case for many components. There is no difference.

...

@rparsi

This comment has been minimized.

Show comment
Hide comment
@rparsi

rparsi Oct 21, 2015

@nicolas-grekas @derrabus
That's why I put the disclaimer first :)

I think I can summarize it better:
The added dependency between Polyfill and other components itself is not the root concern, it's that the scope/depth of the dependency is considerable or extreme compared to the current Symfony framework/ecosystem.

So I think the real vote should be on whether or not to do something as @stof suggests.

By the way derrabus, I can't tell if your clap is sarcastic or earnest. :)

rparsi commented Oct 21, 2015

@nicolas-grekas @derrabus
That's why I put the disclaimer first :)

I think I can summarize it better:
The added dependency between Polyfill and other components itself is not the root concern, it's that the scope/depth of the dependency is considerable or extreme compared to the current Symfony framework/ecosystem.

So I think the real vote should be on whether or not to do something as @stof suggests.

By the way derrabus, I can't tell if your clap is sarcastic or earnest. :)

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Oct 21, 2015

Contributor

the definition of a component does not prevent components being dependent on each others, which is of course already the case for many components. There is no difference.

this argument sounds bad to me: this is not because we don't respect the promises we have done before that we have to continue this way.
Since 2.3, more and more components are coupled to anothers - and of course - this is bad for reuse. Symfony project is too much "Symfony Standard Edition" centric. I stopped fighting against it, doesn't means - like a lot of developers - that I think this is the good way to go.

Contributor

mickaelandrieu commented Oct 21, 2015

the definition of a component does not prevent components being dependent on each others, which is of course already the case for many components. There is no difference.

this argument sounds bad to me: this is not because we don't respect the promises we have done before that we have to continue this way.
Since 2.3, more and more components are coupled to anothers - and of course - this is bad for reuse. Symfony project is too much "Symfony Standard Edition" centric. I stopped fighting against it, doesn't means - like a lot of developers - that I think this is the good way to go.

@rparsi

This comment has been minimized.

Show comment
Hide comment
@rparsi

rparsi Oct 21, 2015

@mickaelandrieu Thanks, you captured my sentiments exactly.

rparsi commented Oct 21, 2015

@mickaelandrieu Thanks, you captured my sentiments exactly.

Show outdated Hide outdated src/Symfony/Component/Polyfill/README.md
compatibility PHP implementations for some extensions and functions. You should
use it when portability across PHP versions and extensions is desired.
Polyfills are provided are for:

This comment has been minimized.

@lsmith77

lsmith77 Oct 21, 2015

Contributor

Polyfills are provided for:

@lsmith77

lsmith77 Oct 21, 2015

Contributor

Polyfills are provided for:

@derrabus

This comment has been minimized.

Show comment
Hide comment
@derrabus

derrabus Oct 21, 2015

Contributor

@rparsi earnest

Contributor

derrabus commented Oct 21, 2015

@rparsi earnest

@ircmaxell

This comment has been minimized.

Show comment
Hide comment
@ircmaxell

ircmaxell Oct 21, 2015

Contributor

One question: What does this have to do with Symfony at all? Why isn't this a generic PHP package (why does it have to be a Symfony component, or shipped by Symfony)?

Contributor

ircmaxell commented Oct 21, 2015

One question: What does this have to do with Symfony at all? Why isn't this a generic PHP package (why does it have to be a Symfony component, or shipped by Symfony)?

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Oct 21, 2015

Member

@ircmaxell I'm not sure I understand your concern. Symfony2 has been a set of components from day one, so why not having one on polyfills? We already have similar components like the Intl one. Moreover, Symfony is not just this monolith repository as each component has its own separate repository (which is automatically synced in real-time with the main Symfony one) and Composer package. So, in that sense, that's no different from any other "generic PHP package" (not sure I understand what a generic PHP package is to be honest though). And of course, being under the Symfony umbrella brings a lot of "benefits" like LTS releases, the BC promise, documentation, 2 releases a year, and much more. Last, but not the least, it just happens that we need that "abstraction" in Symfony, that much of the code was already scattered in the different Symfony components (sometimes duplicated) and Nicolas, who is working on this topic is a member of the Symfony core team. Seems very natural to me that we are doing that component in the Symfony project itself and not elsewhere. It looks like I'm missing something here. Or does it has a negative connotation when something is part of Symfony? Is it a bad thing?

Member

fabpot commented Oct 21, 2015

@ircmaxell I'm not sure I understand your concern. Symfony2 has been a set of components from day one, so why not having one on polyfills? We already have similar components like the Intl one. Moreover, Symfony is not just this monolith repository as each component has its own separate repository (which is automatically synced in real-time with the main Symfony one) and Composer package. So, in that sense, that's no different from any other "generic PHP package" (not sure I understand what a generic PHP package is to be honest though). And of course, being under the Symfony umbrella brings a lot of "benefits" like LTS releases, the BC promise, documentation, 2 releases a year, and much more. Last, but not the least, it just happens that we need that "abstraction" in Symfony, that much of the code was already scattered in the different Symfony components (sometimes duplicated) and Nicolas, who is working on this topic is a member of the Symfony core team. Seems very natural to me that we are doing that component in the Symfony project itself and not elsewhere. It looks like I'm missing something here. Or does it has a negative connotation when something is part of Symfony? Is it a bad thing?

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Oct 21, 2015

I think @ircmaxell was trying to suggest making one canonical framework-agnostic polyfill for new PHP features rather than having it be something that Symfony provides and maintains for Symfony exclusively, because if every framework does this, we end up with needless code duplication and bugs fixed in one place might not percolate to the rest of the community.

I don't think he meant to imply any negative connotation just because it's a part of Symfony.

paragonie-scott commented Oct 21, 2015

I think @ircmaxell was trying to suggest making one canonical framework-agnostic polyfill for new PHP features rather than having it be something that Symfony provides and maintains for Symfony exclusively, because if every framework does this, we end up with needless code duplication and bugs fixed in one place might not percolate to the rest of the community.

I don't think he meant to imply any negative connotation just because it's a part of Symfony.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Oct 21, 2015

Member

@paragonie-scott I did understand that. But why the Symfony one couldn't be the canonical one? It would be an standalone Github repository with its own Packagist entry (no other Symfony deps). So, the only reason I can think of is the "Symfony" name. I thought that the PHP community stopped framework wars years ago and that nowadays people were more open to use components/libraries coming from many different frameworks. Or am I wrong?

Member

fabpot commented Oct 21, 2015

@paragonie-scott I did understand that. But why the Symfony one couldn't be the canonical one? It would be an standalone Github repository with its own Packagist entry (no other Symfony deps). So, the only reason I can think of is the "Symfony" name. I thought that the PHP community stopped framework wars years ago and that nowadays people were more open to use components/libraries coming from many different frameworks. Or am I wrong?

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Oct 21, 2015

But why the Symfony one couldn't be the canonical one?

I don't see why it couldn't.

I started an initiative to make a polyfill a while ago, but it's nowhere near as comprehensive. If Symfony\Polyfill is accepted by the community, I have no problem retiring mine in favor of this one.

It would be an standalone Github repository with its own Packagist entry (no other Symfony deps).

That's a good point in favor of this one.

I thought that the PHP community stopped framework wars years ago and that nowadays people were more open to use components/libraries coming from many different frameworks. Or am I wrong?

Well, this might be off-topic, but I still see pointless arguments about which framework is better almost every day, on Reddit, LinkedIn, Facebook, various forums, and IRC. Very rarely among professionals. It's hard to say if they're really over or not.

paragonie-scott commented Oct 21, 2015

But why the Symfony one couldn't be the canonical one?

I don't see why it couldn't.

I started an initiative to make a polyfill a while ago, but it's nowhere near as comprehensive. If Symfony\Polyfill is accepted by the community, I have no problem retiring mine in favor of this one.

It would be an standalone Github repository with its own Packagist entry (no other Symfony deps).

That's a good point in favor of this one.

I thought that the PHP community stopped framework wars years ago and that nowadays people were more open to use components/libraries coming from many different frameworks. Or am I wrong?

Well, this might be off-topic, but I still see pointless arguments about which framework is better almost every day, on Reddit, LinkedIn, Facebook, various forums, and IRC. Very rarely among professionals. It's hard to say if they're really over or not.

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Oct 21, 2015

Member

In general I think the Polyfill component is a good thing. It bundles all those fallback implementations in one place instead of having it spread across different components and each having to deal with workarounds.

I can think of one major argument why should live outside of symfony/symfony though. We increase minimum PHP requirements in symfony with major versions. But for the Polyfill it would not make sense to have the same release timeline and minimum PHP requirement as all the other symfony components. The Polyfill will probably have to provide the Polyfill for older PHP versions longer than symfony components because that is one of the major points of Polyfill. So it makes no sense to tie Polyfill and Components together and increase minimum PHP requirements at the same time. Otherwise it would mean we have to create a version 3 of Polyfill right now that removes all Polyfills for PHP < 5.5 which makes no sense already...

Member

Tobion commented Oct 21, 2015

In general I think the Polyfill component is a good thing. It bundles all those fallback implementations in one place instead of having it spread across different components and each having to deal with workarounds.

I can think of one major argument why should live outside of symfony/symfony though. We increase minimum PHP requirements in symfony with major versions. But for the Polyfill it would not make sense to have the same release timeline and minimum PHP requirement as all the other symfony components. The Polyfill will probably have to provide the Polyfill for older PHP versions longer than symfony components because that is one of the major points of Polyfill. So it makes no sense to tie Polyfill and Components together and increase minimum PHP requirements at the same time. Otherwise it would mean we have to create a version 3 of Polyfill right now that removes all Polyfills for PHP < 5.5 which makes no sense already...

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Oct 21, 2015

Member

The release timeline was also one argument why we extracted several bundles/components from symfony/symfony in the past. And for Polyfill this is even more relevant IMO.

Member

Tobion commented Oct 21, 2015

The release timeline was also one argument why we extracted several bundles/components from symfony/symfony in the past. And for Polyfill this is even more relevant IMO.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 21, 2015

Member

The 2.8 polyfill is for 5.3.9 minimum, and it will be supported in a LTS release cycle. 5.3 and 5.4 won't get any new feature. Which means the polyfill is feature full. Closed. Over. It won't receive anything more but bug fixes if any. So there is no issue at all. Target achieved, package published, job done, we can all move on and let the legacy there.
So I really don't see your point...

Member

nicolas-grekas commented Oct 21, 2015

The 2.8 polyfill is for 5.3.9 minimum, and it will be supported in a LTS release cycle. 5.3 and 5.4 won't get any new feature. Which means the polyfill is feature full. Closed. Over. It won't receive anything more but bug fixes if any. So there is no issue at all. Target achieved, package published, job done, we can all move on and let the legacy there.
So I really don't see your point...

@ircmaxell

This comment has been minimized.

Show comment
Hide comment
@ircmaxell

ircmaxell Oct 21, 2015

Contributor

And of course, being under the Symfony umbrella brings a lot of "benefits" like LTS releases, the BC promise, documentation, 2 releases a year, and much more.

None of which benefit a polyfill. Polyfill versioning needs to be tied to the release cycle of the thing being polyfilled (as 7.1 is coming out, the polyfill would need to be updated for new patches), as well as for new extensions (which come at sporatic times). It also means that people using the LTS version of the framework (and hence the polyfill) will be coupled to the old version and not get any of the new functionality released since then...

As far as documentation, if it differs from php.net, it's not a polyfill (or that difference is a bug).

Last, but not the least, it just happens that we need that "abstraction" in Symfony

The difference though that I see is that every other component in Symfony is a relatively opinionated component. It's not aiming to be a canonical implementation, but instead an option. This is different and doesn't fit that paradigm. What we're talking about here is an unopinionated implementation of something that's directly tied to PHP itself.

It's not an abstraction at all. It's a polyfill. If you wanted to maintain an abstraction that included polyfill behavior, then I'd agree 100%. But that's not what's being discussed here. We're discussing a direct polyfill.

Ultimately do what you want, I'm not going to point fingers or anything. I'm just pointing out that this is unlike any of the other Symfony components, and by definition has different requirements (as I point out above in the versioning issue). It just feels "odd" to me. Call me crazy (I know, many do), but I'm just trying to point out that this is unlike anything else that you call a component (and calling it a component actually may work against you).

Contributor

ircmaxell commented Oct 21, 2015

And of course, being under the Symfony umbrella brings a lot of "benefits" like LTS releases, the BC promise, documentation, 2 releases a year, and much more.

None of which benefit a polyfill. Polyfill versioning needs to be tied to the release cycle of the thing being polyfilled (as 7.1 is coming out, the polyfill would need to be updated for new patches), as well as for new extensions (which come at sporatic times). It also means that people using the LTS version of the framework (and hence the polyfill) will be coupled to the old version and not get any of the new functionality released since then...

As far as documentation, if it differs from php.net, it's not a polyfill (or that difference is a bug).

Last, but not the least, it just happens that we need that "abstraction" in Symfony

The difference though that I see is that every other component in Symfony is a relatively opinionated component. It's not aiming to be a canonical implementation, but instead an option. This is different and doesn't fit that paradigm. What we're talking about here is an unopinionated implementation of something that's directly tied to PHP itself.

It's not an abstraction at all. It's a polyfill. If you wanted to maintain an abstraction that included polyfill behavior, then I'd agree 100%. But that's not what's being discussed here. We're discussing a direct polyfill.

Ultimately do what you want, I'm not going to point fingers or anything. I'm just pointing out that this is unlike any of the other Symfony components, and by definition has different requirements (as I point out above in the versioning issue). It just feels "odd" to me. Call me crazy (I know, many do), but I'm just trying to point out that this is unlike anything else that you call a component (and calling it a component actually may work against you).

@rparsi

This comment has been minimized.

Show comment
Hide comment
@rparsi

rparsi Oct 21, 2015

@nicolas-grekas @fabpot From your comments you guys really want to merge this. I have a request that if implemented should help others detect/debug conflicts with other components (custom or otherwise).

Nicolas how much work would it be to add a console command that outputs the following information:

  • your environments php version
  • the list of functions that the polyfill thinks it should implement for your application (ie if you triggered a controller action), and for each one if it already exists before the Polyfill would implement it

@ircmaxell Thanks you explain it a lot better then I ever could. This Polyfill works like a component but as it's tied to PHP itself feels out of place as a Symfony component. Not sure if I'm making sense here so my apologies.

rparsi commented Oct 21, 2015

@nicolas-grekas @fabpot From your comments you guys really want to merge this. I have a request that if implemented should help others detect/debug conflicts with other components (custom or otherwise).

Nicolas how much work would it be to add a console command that outputs the following information:

  • your environments php version
  • the list of functions that the polyfill thinks it should implement for your application (ie if you triggered a controller action), and for each one if it already exists before the Polyfill would implement it

@ircmaxell Thanks you explain it a lot better then I ever could. This Polyfill works like a component but as it's tied to PHP itself feels out of place as a Symfony component. Not sure if I'm making sense here so my apologies.

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Oct 21, 2015

Member

-1 to include it in symfony/symfony
+0 to create a separate repo like symfony/polyfill that bundles all polyfills
+1 to create a vendor/organisation just for polyfills where each polyfilled extension has it's own repo as stof suggested

Member

Tobion commented Oct 21, 2015

-1 to include it in symfony/symfony
+0 to create a separate repo like symfony/polyfill that bundles all polyfills
+1 to create a vendor/organisation just for polyfills where each polyfilled extension has it's own repo as stof suggested

@rparsi

This comment has been minimized.

Show comment
Hide comment
@rparsi

rparsi Oct 21, 2015

I know my vote doesn't count, so this just for myself :)
👍 to @Tobion @stof

rparsi commented Oct 21, 2015

I know my vote doesn't count, so this just for myself :)
👍 to @Tobion @stof

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 22, 2015

Member

Sleeping helping, I now understand what @Tobion says about minimum PHP versions and why that doesn't fit for a polyfill component.

Member

nicolas-grekas commented Oct 22, 2015

Sleeping helping, I now understand what @Tobion says about minimum PHP versions and why that doesn't fit for a polyfill component.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Oct 22, 2015

Member

Thank for the constructive discussion everyone, I really appreciate it. We heard your arguments, and we've decided to create a new repo for polyfills, symfony/polyfill which is going to have a different version numbering than Symfony itself and a different release cycle.

Go over at https://github.com/symfony/polyfill for the new implementation which should be ready soon.

Member

fabpot commented Oct 22, 2015

Thank for the constructive discussion everyone, I really appreciate it. We heard your arguments, and we've decided to create a new repo for polyfills, symfony/polyfill which is going to have a different version numbering than Symfony itself and a different release cycle.

Go over at https://github.com/symfony/polyfill for the new implementation which should be ready soon.

fabpot added a commit that referenced this pull request Oct 28, 2015

feature #16317 Rely on iconv and symfony/polyfill-* (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-*
@edlington

This comment has been minimized.

Show comment
Hide comment
@edlington

edlington Dec 14, 2015

I don't see how polyfills result in loosely coupled code. What I'm seeing is that code rewritten to remove function_exists checks for backwards compatibility now will fail unless polyfill components are used. Above it says polyfills are autoloaded. They don't seem to be autoloading in my install. One definition of clean code is to minimize the number of conditions and loops in a function. That's great. Creating new components moves complexity from the file level to the directory level. Cleaner code, more confusing directory structure, more difficult to track down errors. For Symfony wizards, no problem. Solutions are always political and in this case I think the polyfills make the framework less friendly for those of us who are learners rather than experts.

edlington commented Dec 14, 2015

I don't see how polyfills result in loosely coupled code. What I'm seeing is that code rewritten to remove function_exists checks for backwards compatibility now will fail unless polyfill components are used. Above it says polyfills are autoloaded. They don't seem to be autoloading in my install. One definition of clean code is to minimize the number of conditions and loops in a function. That's great. Creating new components moves complexity from the file level to the directory level. Cleaner code, more confusing directory structure, more difficult to track down errors. For Symfony wizards, no problem. Solutions are always political and in this case I think the polyfills make the framework less friendly for those of us who are learners rather than experts.

@derrabus

This comment has been minimized.

Show comment
Hide comment
@derrabus

derrabus Dec 14, 2015

Contributor

@edlington If the polyfills don't work for you as expected, please open an issue explaining your problem. This gives others a chance to help you.

Contributor

derrabus commented Dec 14, 2015

@edlington If the polyfills don't work for you as expected, please open an issue explaining your problem. This gives others a chance to help you.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Dec 14, 2015

Member

@edlington are you using the Composer autoloader, or are you using your own autoloading system ?

Member

stof commented Dec 14, 2015

@edlington are you using the Composer autoloader, or are you using your own autoloading system ?

@edlington

This comment has been minimized.

Show comment
Hide comment
@edlington

edlington Dec 14, 2015

@stof thank you for that question. I think I see the problem is that I need to add an autoload field to my composer file.

edlington commented Dec 14, 2015

@stof thank you for that question. I think I see the problem is that I need to add an autoload field to my composer file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment