Hi.
Following http://news.php.net/php.internals/73957
This change breaks at least doctrine and phpunit.
Source code:
https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php#L911
https://github.com/sebastianbergmann/phpunit-mock-objects/blob/master/src/Framework/MockObject/Generator.php#L274
Something like, (used to not call __construct) :
$object = unserialize(sprintf('O:%d:"%s":0:{}', strlen($className),
$className)
Big reproducer:
pear channel-discover pear.horde.org
pear install horde/Horde_Test
pear install horde/Horde_Imap_Client
pear download horde/Horde_Imap_Client
tar xf Horde_Imap_Client-2.20.0.tgz
cd Horde_Imap_Client-2.20.0/
cd test/Horde/Imap/Client/
wget https://phar.phpunit.de/phpunit.phar
php phpunit.phar .
PHPUnit 4.1.0 by Sebastian Bergmann.
Configuration read from /root/Horde_Imap_Client-2.20.0/test/Horde/Imap/Client/phpunit.xml
...............EEEEEEEEEEEEEEEEEEEEEEEEEEEE.................... 63 / 211 ( 29%)
.................................FF............................ 126 / 211 ( 59%)
SS............................................................. 189 / 211 ( 89%)
......................
Time: 1.07 seconds, Memory: 18.50Mb
There were 28 errors:
- Horde_Imap_Client_Cache_CacheTest::testGet
Erroneous data format for unserializing 'Mock_Horde_Imap_Client_Socket_1e0b6300'
/root/Horde_Imap_Client-2.20.0/test/Horde/Imap/Client/Cache/TestBase.php:36
phar:///root/Horde_Imap_Client-2.20.0/test/Horde/Imap/Client/phpunit.phar/phpunit/TextUI/Command.php:179
phar:///root/Horde_Imap_Client-2.20.0/test/Horde/Imap/Client/phpunit.phar/phpunit/TextUI/Command.php:132
- Horde_Imap_Client_Cache_CacheTest::testGetCachedUids
Erroneous data format for unserializing 'Mock_Horde_Imap_Client_Socket_1e0b6300'
....
Remi.
@Julien: So what is the strategy now? This should be reverted ASAP.
After this my question, besides the PHPT, aren't any tests against php
libraries run before releases?
Especially this one would have broken PHPUnit already in any test using
mocks.
Hi.
Following http://news.php.net/php.internals/73957
This change breaks at least doctrine and phpunit.
Source code:
Something like, (used to not call __construct) :
$object = unserialize(sprintf('O:%d:"%s":0:{}', strlen($className),
$className)Big reproducer:
pear channel-discover pear.horde.org
pear install horde/Horde_Test
pear install horde/Horde_Imap_Client
pear download horde/Horde_Imap_Client
tar xf Horde_Imap_Client-2.20.0.tgz
cd Horde_Imap_Client-2.20.0/
cd test/Horde/Imap/Client/
wget https://phar.phpunit.de/phpunit.phar
php phpunit.phar .
PHPUnit 4.1.0 by Sebastian Bergmann.
Configuration read from
/root/Horde_Imap_Client-2.20.0/test/Horde/Imap/Client/phpunit.xml...............EEEEEEEEEEEEEEEEEEEEEEEEEEEE.................... 63 / 211
( 29%)
.................................FF............................ 126 / 211
( 59%)
SS............................................................. 189 / 211
( 89%)
......................Time: 1.07 seconds, Memory: 18.50Mb
There were 28 errors:
- Horde_Imap_Client_Cache_CacheTest::testGet
Erroneous data format for unserializing
'Mock_Horde_Imap_Client_Socket_1e0b6300'/root/Horde_Imap_Client-2.20.0/test/Horde/Imap/Client/Cache/TestBase.php:36
phar:///root/Horde_Imap_Client-2.20.0/test/Horde/Imap/Client/phpunit.phar/phpunit/TextUI/Command.php:179
phar:///root/Horde_Imap_Client-2.20.0/test/Horde/Imap/Client/phpunit.phar/phpunit/TextUI/Command.php:132
- Horde_Imap_Client_Cache_CacheTest::testGetCachedUids
Erroneous data format for unserializing
'Mock_Horde_Imap_Client_Socket_1e0b6300'
....Remi.
On Tue, Jun 3, 2014 at 5:07 PM, Benjamin Eberlei kontakt@beberlei.de
wrote:
@Julien: So what is the strategy now? This should be reverted ASAP.
After this my question, besides the PHPT, aren't any tests against php
libraries run before releases?
Especially this one would have broken PHPUnit already in any test using
mocks.
We were aware of the BC break before release (see
http://marc.info/?l=php-internals&m=139932135130155&w=2), but it looks like
the issue was forgotten after some initial discussion.
Nikita
After this my question, besides the PHPT, aren't any tests against php
libraries run before releases?
We send out RCs two weeks before a release, but receive barely any
feedback. I'd love if external projects would verify those and raise a
red flag ...
johannes
We send out RCs two weeks before a release, but receive barely any
feedback. I'd love if external projects would verify those and raise a
red flag ...
We don't have the resources to test against RCs, and we don't have any CI
environment on our own except for travis-ci (which is what most projects
currently use).
Maybe push a php-nightly binary to the Travis folks instead? We'd gladly
test against that, and even if people won't report immediately, the travis
API is simple enough to allow mining on the builds to discover problems
eagerly.
Marco Pivetta
On 3 June 2014 17:55, Johannes Schlüter johannes@schlueters.de
wrote:We send out RCs two weeks before a release, but receive barely any feedback. I'd love if external projects would verify those and raise a red flag ...We don't have the resources to test against RCs, and we don't have any
CI environment on our own except for travis-ci (which is what most
projects currently use).Maybe push a
php-nightlybinary to the Travis folks instead? We'd
gladly test against that, and even if people won't report immediately,
the travis API is simple enough to allow mining on the builds to
discover problems eagerly.
Nightly testing (or push testing) I see on our side. The issue with
testing applications is that we need the time from the individual
project maintainers. Not knowing the application it often is quite hard
to dig down what in the underlying systems broke. Project maintainers
typically an pinpoint this faster.
Speaking about resources: The core PHP team is very small, way to small
to keep up with a relevant amount of projects all moving.
johannes
Nightly testing (or push testing) I see on our side. The issue with
testing applications is that we need the time from the individual
project maintainers. Not knowing the application it often is quite hard
to dig down what in the underlying systems broke. Project maintainers
typically an pinpoint this faster.
Pushing a "nightly" package to Travis would solve this issue almost
entirely.
Speaking about resources: The core PHP team is very small, way to small
to keep up with a relevant amount of projects all moving.
There's an already working test suite by the HHVM folks at
https://github.com/facebook/hhvm/blob/1ecb4443d95c2e2659028f148083dd5c85f2099b/hphp/test/frameworks/frameworks.yaml
In the eventuality in which PHP-SRC retains control over the CI environment
that runs the framework tests, this suite could be reused. The problem is
that it actually takes days to run all those tests, and I don't know if the
hardware is available.
Marco Pivetta
In the eventuality in which PHP-SRC retains control over the CI environment
that runs the framework tests, this suite could be reused. The problem is
that it actually takes days to run all those tests, and I don't know if the
hardware is available.
Perhaps just a small, frequently-used subset could be tested?
--
Andrea Faulds
http://ajf.me/
Nightly testing (or push testing) I see on our side. The issue with
testing applications is that we need the time from the individual
project maintainers. Not knowing the application it often is quite hard
to dig down what in the underlying systems broke. Project maintainers
typically an pinpoint this faster.Pushing a "nightly" package to Travis would solve this issue almost
entirely.Speaking about resources: The core PHP team is very small, way to small
to keep up with a relevant amount of projects all moving.There's an already working test suite by the HHVM folks at
https://github.com/facebook/hhvm/blob/1ecb4443d95c2e2659028f148083dd5c85f2099b/hphp/test/frameworks/frameworks.yamlIn the eventuality in which PHP-SRC retains control over the CI environment
that runs the framework tests, this suite could be reused. The problem is
that it actually takes days to run all those tests, and I don't know if the
hardware is available.
I agree we should run our tests against well known tested projects,
such as PHPUnit , Doctrine or other.
That's in a perfect world though.
Julien.P
I suggest we merge Anatol's patch to 5.5 so it's gonna be in 5.5.14 and up.
I'm not really fan to merge it to 5.6. The patch fixing the BC
introduces inconsistencies between internal and userland classes, and
I don't really like that.
Other thoughts perhaps ?
Julien
Hi Julien, Marco,
I agree we should run our tests against well known tested projects,
such as PHPUnit , Doctrine or other.That's in a perfect world though.
I finished setting up a similar CI environment for the project I work
on, so that the Revive Adserver test suite is also run with latest PHP
5.4, 5.5, 5.6, master, phpng (disabled atm) and int64 every night.
I've sent an email about it to php-qa just a few days ago.
I could set up some jobs for a few more open source projects if they are
not extremely resource hungry.
Cheers
Matteo Beccati
Development & Consulting - http://www.beccati.com/
I could set up some jobs for a few more open source projects if they are
not extremely resource hungry.
PHPUnit was fairly easy:
https://revive.beccati.com/bamboo/browse/PHPUN-LATE-2
It currently fails on 5.4, 5.5, phpng from last night.
Cheers
Matteo Beccati
Development & Consulting - http://www.beccati.com/
I could set up some jobs for a few more open source projects if they are
not extremely resource hungry.PHPUnit was fairly easy:
https://revive.beccati.com/bamboo/browse/PHPUN-LATE-2
It currently fails on 5.4, 5.5, phpng from last night.
Ow, I missed the mail to qa so.
That's a nice job, I'll use that frequently before releases to ehance
our quality.
Thank you for helping in this critical part :-)
Julien
Le 03/06/2014 17:55, Johannes Schlüter a écrit :
After this my question, besides the PHPT, aren't any tests against php
libraries run before releases?We send out RCs two weeks before a release, but receive barely any
feedback. I'd love if external projects would verify those and raise a
red flag ...
And the bug is known by upstream project.
It is fixed in Doctrine and in PHPUnit.
Both project doesn't seems to think it is important as they don't
publish any new version with the fix included.
My 0,02€
Remi.
And the bug is known by upstream project.
We didn't know of the bug/problem until it landed in the release
It is fixed in Doctrine and in PHPUnit.
Both project doesn't seems to think it is important as they don't
publish any new version with the fix included.
It is going to be released, but yes, we actually have the expectation a
patch release for 5.4 and 5.5 with a rollback of this breakage.
Even if we fix the bug ourselves, this is going to break apps that receive
a PHP upgrade and not an upgrade of the library dependencies.
Marco Pivetta
It is going to be released, but yes, we actually have the expectation a
patch release for 5.4 and 5.5 with a rollback of this breakage.
The original Doctrine PR that fixed this issue was
https://github.com/doctrine/doctrine2/pull/1045, and Benjamin made some
revisions in
https://github.com/doctrine/doctrine2/commit/e577e7786796e9393df381f02221046dea6253a5
(note: there is a typo there, as he actually should be checking for 5.4.29
instead of 5.4.28). Based on the conversation above, it sounds like the
revert will end up in 5.4.30 and 5.5.14, but we should expect the change
for 5.6+.
In that case, Benjamin's commit needs to (a) fix the 5.4.x typo and (b) add
another condition to utilize Reflection for 5.6+.
Has PHPUnit already incorporated a work-around for this? At a glance, I
didn't see anything in the main repository or
https://github.com/sebastianbergmann/phpunit-mock-objects.
--
jeremy mikola
Le 03/06/2014 21:04, Jeremy Mikola a écrit :
It is going to be released, but yes, we actually have the expectation a
patch release for 5.4 and 5.5 with a rollback of this breakage.The original Doctrine PR that fixed this issue was
https://github.com/doctrine/doctrine2/pull/1045, and Benjamin made some
revisions in
https://github.com/doctrine/doctrine2/commit/e577e7786796e9393df381f02221046dea6253a5
(note: there is a typo there, as he actually should be checking for 5.4.29
instead of 5.4.28). Based on the conversation above, it sounds like the
revert will end up in 5.4.30 and 5.5.14, but we should expect the change
for 5.6+.
Why don't you simply use newInstanceWithoutConstructor() as soon as
possible (5.4+) ?
I rather see unzerialize('O:%d:"%s":0:{}') as a workaround for old
version, instead of the opposite...
The fix is not really reverted, only a workaround for this regression,
and only for user class (so the segfault is fixed, for internal class).
FYI, a user of my repo have confirm (real app.) than
- doctrine + php 5.4.29 is broken
(the reason why I raised this issue here last Friday, I was aware of
PHPUnit problem for a long time, but was thinking it is 5.6 only) - doctrine + initial patch works with php 5.4.29
- doctrine works with php 5.4.29 + patch
In that case, Benjamin's commit needs to (a) fix the 5.4.x typo and (b) add
another condition to utilize Reflection for 5.6+.Has PHPUnit already incorporated a work-around for this? At a glance, I
didn't see anything in the main repository or
https://github.com/sebastianbergmann/phpunit-mock-objects.
Remi.
P.S. FYI, in Fedora:
- Doctrine package includes the initial patch, so works
- PHPUnit package includes the upstream patch, so works
- PHP 5.5.13 includes the patch, so even a manually installed Doctrine
or PHPUnit will work.
Remi,
I didn't have time to check the implications of
"newInstanceWithoutConstructor", i didn't want to merge that directly into
our stable branches. Plans to migrate to the function for php5.4++ exist
though.
greetings
Benjamin
Le 03/06/2014 21:04, Jeremy Mikola a écrit :
On Tue, Jun 3, 2014 at 1:11 PM, Marco Pivetta ocramius@gmail.com
wrote:It is going to be released, but yes, we actually have the expectation a
patch release for 5.4 and 5.5 with a rollback of this breakage.The original Doctrine PR that fixed this issue was
https://github.com/doctrine/doctrine2/pull/1045, and Benjamin made some
revisions inhttps://github.com/doctrine/doctrine2/commit/e577e7786796e9393df381f02221046dea6253a5
(note: there is a typo there, as he actually should be checking for
5.4.29
instead of 5.4.28). Based on the conversation above, it sounds like the
revert will end up in 5.4.30 and 5.5.14, but we should expect the change
for 5.6+.Why don't you simply use newInstanceWithoutConstructor() as soon as
possible (5.4+) ?I rather see unzerialize('O:%d:"%s":0:{}') as a workaround for old
version, instead of the opposite...The fix is not really reverted, only a workaround for this regression,
and only for user class (so the segfault is fixed, for internal class).FYI, a user of my repo have confirm (real app.) than
- doctrine + php 5.4.29 is broken
(the reason why I raised this issue here last Friday, I was aware of
PHPUnit problem for a long time, but was thinking it is 5.6 only)- doctrine + initial patch works with php 5.4.29
- doctrine works with php 5.4.29 + patch
In that case, Benjamin's commit needs to (a) fix the 5.4.x typo and (b)
add
another condition to utilize Reflection for 5.6+.Has PHPUnit already incorporated a work-around for this? At a glance, I
didn't see anything in the main repository or
https://github.com/sebastianbergmann/phpunit-mock-objects.Remi.
P.S. FYI, in Fedora:
- Doctrine package includes the initial patch, so works
- PHPUnit package includes the upstream patch, so works
- PHP 5.5.13 includes the patch, so even a manually installed Doctrine
or PHPUnit will work.
Remi,
I didn't have time to check the implications of
"newInstanceWithoutConstructor", i didn't want to merge that directly into
our stable branches. Plans to migrate to the function for php5.4++ exist
though.
newInstanceWithoutConstructor() has been introducted into PHP to
address exactly the
use-cases we are considering in this topic. The Doctrine one for example.
Julien
i know that, but its no reason for me to introduce this in a point bugfix
release, just to fix an upstream BC break, without testing it further.
On Wed, Jun 4, 2014 at 8:03 AM, Benjamin Eberlei kontakt@beberlei.de
wrote:Remi,
I didn't have time to check the implications of
"newInstanceWithoutConstructor", i didn't want to merge that directly
into
our stable branches. Plans to migrate to the function for php5.4++ exist
though.newInstanceWithoutConstructor() has been introducted into PHP to
address exactly the
use-cases we are considering in this topic. The Doctrine one for example.Julien
any news?
On Wed, Jun 4, 2014 at 6:56 PM, Benjamin Eberlei kontakt@beberlei.de
wrote:
i know that, but its no reason for me to introduce this in a point bugfix
release, just to fix an upstream BC break, without testing it further.On Wed, Jun 4, 2014 at 8:03 AM, Benjamin Eberlei kontakt@beberlei.de
wrote:Remi,
I didn't have time to check the implications of
"newInstanceWithoutConstructor", i didn't want to merge that directly
into
our stable branches. Plans to migrate to the function for php5.4++ exist
though.newInstanceWithoutConstructor() has been introducted into PHP to
address exactly the
use-cases we are considering in this topic. The Doctrine one for example.Julien
any news?
Patch will be included in next 5.5.14RCs and 5.5.14.
Julien
Patch will be included in next 5.5.14RCs and 5.5.14.
What about 5.6.x? I'm getting failure reports from other folks that have
the beta installed...
Marco Pivetta
Patch will be included in next 5.5.14RCs and 5.5.14.
What about 5.6.x? I'm getting failure reports from other folks that have the
beta installed...
I'm myself against merging it to 5.6
But for consistency, and if we follow our rules, the patch should also
be applied to 5.6
Ferenc, what do you think about it ?
Julien
Le 10/06/2014 14:32, Julien Pauli a écrit :
Patch will be included in next 5.5.14RCs and 5.5.14.
What about 5.6.x? I'm getting failure reports from other folks that have the
beta installed...
See related discussion on
https://github.com/sebastianbergmann/phpunit-mock-objects/commit/5f58fcb4f94a9802e0861c94760d0ce2190fb6d3#commitcomment-6591497
I'm myself against merging it to 5.6
But for consistency, and if we follow our rules, the patch should also
be applied to 5.6
Ferenc, what do you think about it ?Julien
Patch will be included in next 5.5.14RCs and 5.5.14.
What about 5.6.x? I'm getting failure reports from other folks that have
the
beta installed...I'm myself against merging it to 5.6
But for consistency, and if we follow our rules, the patch should also
be applied to 5.6
Ferenc, what do you think about it ?Julien
--
Hi,
I agree that 5.6 should have a clean behavior solving both issues(being
able to initialize any object without a calling the constructor but now
having crash issues with improperly initialized internal objects). but I
have yet to see one.
If we don't have any other alternatives(in a reasonable timeframe) I would
be ok having the same fix as in 5.4 and 5.5.
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
Hi there,
I wrote a small wrapper around this "hack" that we've been using for years
(as suggested by Sebastian) and I put it at
https://github.com/Ocramius/Instantiator.
It seems like PHP 5.6 is still failing though (5.6.0-beta1):
https://travis-ci.org/Ocramius/Instantiator/builds/27493715
Test fails also on my machine (5.6.0-beta3).
Is any eventual fix going to be merged before the next beta?
Marco Pivetta
On Tue, Jun 10, 2014 at 2:22 PM, Marco Pivetta ocramius@gmail.com
wrote:Patch will be included in next 5.5.14RCs and 5.5.14.
What about 5.6.x? I'm getting failure reports from other folks that
have the
beta installed...I'm myself against merging it to 5.6
But for consistency, and if we follow our rules, the patch should also
be applied to 5.6
Ferenc, what do you think about it ?Julien
--
Hi,
I agree that 5.6 should have a clean behavior solving both issues(being
able to initialize any object without a calling the constructor but now
having crash issues with improperly initialized internal objects). but I
have yet to see one.
If we don't have any other alternatives(in a reasonable timeframe) I would
be ok having the same fix as in 5.4 and 5.5.--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
Le 13/06/2014 15:57, Marco Pivetta a écrit :
Hi there,
I wrote a small wrapper around this "hack" that we've been using for
years (as suggested by Sebastian) and I put it at
https://github.com/Ocramius/Instantiator.It seems like PHP 5.6 is still failing though (5.6.0-beta1):
https://travis-ci.org/Ocramius/Instantiator/builds/27493715
Test fails also on my machine (5.6.0-beta3).Is any eventual fix going to be merged before the next beta?
See my proposal
http://marc.info/?l=php-internals&m=140257374715698&w=2
Not designed as a fix, but rather a small improvement to make this hack
possible for Internal and Serializable classes
Remi
See my proposal
http://marc.info/?l=php-internals&m=140257374715698&w=2Not designed as a fix, but rather a small improvement to make this hack
possible for Internal and Serializable classes
Thanks Remi, that looks indeed like what we'd want :-)
Marco Pivetta
See my proposal
http://marc.info/?l=php-internals&m=140257374715698&w=2Not designed as a fix, but rather a small improvement to make this hack
possible for Internal and Serializable classesThanks Remi, that looks indeed like what we'd want :-)
Marco Pivetta
Its a BC break people.
It does not matter if we think A or B are correct, a point release CANNOT
break BC.
It has been 18 days without a 5.5.14 release or a rollback of 5.5.13.
This is highly unacceptable and irresponsible of a core language.
I cannot understand why we are discussing this and leaving userland broken,
this affect a lot more the phpunit/doctrine.
Rollback the release, or release a 5.5.14 release that fixes it, THEN you
discuss what the proper thing should be and worry about it in 5.5.15.
--
Rafael Dohms
PHP Evangelist and Community Leader
http://doh.ms
http://www.amsterdamphp.nl <http://wwwamsterdamphp.nl
On Tue, Jun 17, 2014 at 11:01 AM, Rafael Dohms
listas@rafaeldohms.com.br wrote:
Its a BC break people.
It does not matter if we think A or B are correct, a point release CANNOT
break BC.It has been 18 days without a 5.5.14 release or a rollback of 5.5.13.
This is highly unacceptable and irresponsible of a core language.
I can only agree with you here.
I totally fail to understand why we did not fire a new release right
away after we knew a BC break was introduced. We fail here.
I cannot understand why we are discussing this and leaving userland broken,
this affect a lot more the phpunit/doctrine.
Rollback the release, or release a 5.5.14 release that fixes it, THEN you
discuss what the proper thing should be and worry about it in 5.5.15.
Not even that. We know it is a BC break. So unless there is a change
that does not break BC, we cannot do that in any 5.x, not even 5.6,
except if there is a real appealing reason to do so (security, crash
bugs, etc).
Cheers,
Pierre
@pierrejoye | http://www.libgd.org
Hi,
My replies are inline and the tl;dr is that I don't think that it is a good
idea to release the current "fix" to the problem as-is:
On Tue, Jun 17, 2014 at 11:01 AM, Rafael Dohms listas@rafaeldohms.com.br
wrote:
See my proposal
http://marc.info/?l=php-internals&m=140257374715698&w=2Not designed as a fix, but rather a small improvement to make this hack
possible for Internal and Serializable classesThanks Remi, that looks indeed like what we'd want :-)
Marco Pivetta
Its a BC break people.
It does not matter if we think A or B are correct, a point release CANNOT
break BC.
generally BC breaks are not allowed in micro versions, but this doesn't
mean that we can't change existing behavior in micro versions.
currently there are 3 cases where we have precedence changing behavior in
micro versions:
- bugfix
- changing (explicitly) undefined behavior
- introducing a new class/function/method/constant (I don't like this,
but it seems that it is the consensus seems to be that this is okay).
In this particular case the BC was caused by a bugfix (
https://bugs.php.net/bug.php?id=67072) and changed the unserialize behavior
regarding of the usage of the Serializable interface to prevent such issues
while keeping the implementation in-line with the docs (see
http://markmail.org/message/u6r6rzj6tzejlaaf for the details).
Even though that we noticed that it would affect some userland code and
Stas argued for not introducing the BC break, Anatol's argument seemed to
be reasonable enough to keep the change, and nobody else complained.
After the release, we get a some more complaints and Anatol decided to make
a compromise and change the fix to allow userland classes to be
instanitated via this trick, but still preventing the constructorless
instanitation of the internal classes via the crafted unserialize string
method.
While this will solve like 99.X% of the cases for the userland, this still
a BC break, as previously it was possible to instaniate internal classes
implementing the Serializable interface with the unserialize trick without
calling the constructor and we don't allow that anymore, and on a second
look, this also brings back the original problem this change was trying to
prevent but with a bit more effort:
[tyrael@ferencs-mbp-135 php-src.git (PHP-5.5 ✗)]$ ./sapi/cli/php -v
PHP 5.5.15-dev (cli) (built: Jun 17 2014 15:33:34) (DEBUG)
Copyright (c) 1997-2014 The PHP Group
Zend Engine v2.5.0, Copyright (c) 1998-2014 Zend Technologies
[tyrael@ferencs-mbp-135 php-src.git (PHP-5.5 ✗)]$ ./sapi/cli/php -r 'class
Foo extends SplFileObject{};$foo =
unserialize("O:".strlen("Foo").":"Foo":0:{}");echo $foo;'
[1] 72973 segmentation fault ./sapi/cli/php -r
So the current "fix" still breaks BC compared to the original buggy
behavior, but still not completely fixes the problem...
I think that the original change was warranted here and even though that we
introduced a BC break, we shouldn't allow instanitation of
incomplete/unstable objects because that opens up more serious problems for
the users, and I'm somehow glad that we didn't rushed with the release of
this "fix", as I'm not entirely convinced, that this is the proper way for
handling this issue.
It has been 18 days without a 5.5.14 release or a rollback of 5.5.13.
This is highly unacceptable and irresponsible of a core language.I cannot understand why we are discussing this and leaving userland broken,
this affect a lot more the phpunit/doctrine.
Rollback the release, or release a 5.5.14 release that fixes it, THEN you
discuss what the proper thing should be and worry about it in 5.5.15.
I think that this is a bit blown out of proportion, even thought that this
change affected some really popular tools, but this was still caused by a
warranted fix in the internal unserialization behavior which was documented
to work the way as Anatol changed to behave in the first place.
I think it is a pretty good thing that we haven't rushed out another
release without making sure we have a fix we are satisfied with.
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
generally BC breaks are not allowed in micro versions, but this doesn't
mean that we can't change existing behavior in micro versions.
currently there are 3 cases where we have precedence changing behavior in
micro versions:
- bugfix
- changing (explicitly) undefined behavior
- introducing a new class/function/method/constant (I don't like this,
but it seems that it is the consensus seems to be that this is okay).
It is definitively not OK. I cannot imagine any feature that cannot
x.y+1, which happens less than a year later.
Cheers,
Pierre
@pierrejoye | http://www.libgd.org
generally BC breaks are not allowed in micro versions, but this doesn't
mean that we can't change existing behavior in micro versions.
currently there are 3 cases where we have precedence changing behavior in
micro versions:
- bugfix
- changing (explicitly) undefined behavior
- introducing a new class/function/method/constant (I don't like this,
but it seems that it is the consensus seems to be that this is okay).It is definitively not OK. I cannot imagine any feature that cannot
x.y+1, which happens less than a year later.
I agree with you, however the facts shows differently:
http://php.net/ChangeLog-5.php#5.5.11
OPCache: Added function opcache_is_script_cached().
And even the releaseprocess rfc is written in a way, which implies that
introducing new features are not considered as BC breaks:
https://wiki.php.net/rfc/releaseprocess
x.y.z to x.y+1.z
Bugfixes
New featuresExtensions support can be ended (moved to pecl)
Backward compatibility must be keptAPI compatibility must be kept
(userland)
ABI and API can be broken (internals), src compatibility should be kept if
possible, while breakages are allowed
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
Hi!
It is definitively not OK. I cannot imagine any feature that cannot
x.y+1, which happens less than a year later.
I think we've been through this a number of times and most of the people
agree that waiting for the next release cycle (at least a year) for a
self-contained minor function or feature addition makes little sense.
People add functions because they have need for them now, not years
later when they're ready to make a major version upgrade. We're not
talking something huge like new syntax or language feature, we're
talking about this:
OPCache: Added function
opcache_is_script_cached().
Why one should wait for a year or more to be able to check if the script
is cached? It makes little sense to me. Adding it to active branch does
not hurt anything, so I see no problem with it.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi!
It is definitively not OK. I cannot imagine any feature that cannot
x.y+1, which happens less than a year later.I think we've been through this a number of times and most of the people
agree that waiting for the next release cycle (at least a year) for a
self-contained minor function or feature addition makes little sense.
People add functions because they have need for them now, not years
later when they're ready to make a major version upgrade. We're not
talking something huge like new syntax or language feature, we're
talking about this:OPCache: Added function
opcache_is_script_cached().Why one should wait for a year or more to be able to check if the script
is cached? It makes little sense to me. Adding it to active branch does
not hurt anything, so I see no problem with it.
And where is the limit? Add a full extension is fine too? No, it is not.
It reintroduces the pain to add dozen minors version checks for php
version dependencies.
--
Pierre
@pierrejoye | http://www.libgd.org
Hi!
And where is the limit?
We define the limit.
Add a full extension is fine too? No, it is not.
Depends on the extension. Adding new extension to core may be not
warranted, because you can just as well put it on PECL. So it's not a
good example - you can't just put a new function or parameter on PECL.
It reintroduces the pain to add dozen minors version checks for php
version dependencies.
You don't need version checks. You need functionality checks. You'd need
them anyway if you want to support multiple versions.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi!
And where is the limit?
We define the limit.
Add a full extension is fine too? No, it is not.
Depends on the extension. Adding new extension to core may be not
warranted, because you can just as well put it on PECL. So it's not a
good example - you can't just put a new function or parameter on PECL.It reintroduces the pain to add dozen minors version checks for php
version dependencies.You don't need version checks. You need functionality checks. You'd need
them anyway if you want to support multiple versions.
you need both, and it is painful.
--
Pierre
@pierrejoye | http://www.libgd.org
Hi!
you need both, and it is painful.
It may be, but denying functionality because it's hard to ensure forward
compatibility is not a good way to go. If you need these checks, you
need this functionality, so delaying it for a year won't do good.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
I agree with you, however the facts shows differently:
http://php.net/ChangeLog-5.php#5.5.11
OPCache: Added functionopcache_is_script_cached().
Fact that some does whatever they want at wishes is bad, I agree.
And even the releaseprocess rfc is written in a way, which implies that
introducing new features are not considered as BC breaks:https://wiki.php.net/rfc/releaseprocess
x.y.z to x.y+1.z
Bugfixes
New features
Extensions support can be ended (moved to pecl)
Backward compatibility must be kept
API compatibility must be kept (userland)
ABI and API can be broken (internals), src compatibility should be kept if
possible, while breakages are allowed
But no new features in released branches.
--
Pierre
@pierrejoye | http://www.libgd.org
Hi!
It has been 18 days without a 5.5.14 release or a rollback of 5.5.13.
This is highly unacceptable and irresponsible of a core language.
We have a release schedule. Which is widely known - in fact, we've just
announced 5.5.14 RC1 last Thursday. I agree BC break is bad but not bad
enough to break the release schedule. If you suffer from this particular
problem, just keep on 5.5.12 for 2 weeks. And please test the RCs, if
people would actually test the RCs we'd know about such problems
before the release, not after. That's what RCs are for.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi!
It has been 18 days without a 5.5.14 release or a rollback of 5.5.13.
This is highly unacceptable and irresponsible of a core language.We have a release schedule. Which is widely known - in fact, we've just
announced 5.5.14 RC1 last Thursday. I agree BC break is bad but not bad
enough to break the release schedule. If you suffer from this particular
problem, just keep on 5.5.12 for 2 weeks. And please test the RCs, if
people would actually test the RCs we'd know about such problems
before the release, not after. That's what RCs are for.
And here Stas you get my huge +1.
I cant understand the stress about this bug. It seems like the earth
is gonna blow up.
Suddenly, people are not "stuck with" 5.3.x anymore (thing we still
ear about, nearly daily), but they do run exactly 5.5.13. Common, are you
kidding ?
Just stay in 5.5.12, keep calm as there really is no need to lose
anyone's temper.
It is a BC break, yes, and we all apologize about it, believe it. But
it won't blow the earth up in the few next seconds, will it ?
The only moments I know we've been releasing a new version in rush,
was about huge security holes that got disclosed just after a release.
However, as I can see the patch seems to still have problems in our
RC1, we could go for an RC2 if no consensus is found about it before
next tuesday (the day we usually tag).
What do you think about this ?
Like Ferenc said, rushing a patch is a high risk at introducing
another bug, or just not fixing the bug properly. And then a bad
loop's gonna start from that.
Let's take time to test our patches / solutions, to keep our level of
quality. If RC2 is needed, there will be an RC2 (and more again if
needed). I'am all in favor of improving our quality , which usually is
the opposite of rushing.
Julien Pauli