# BC break in 5.4.29 and 5.5.13 - Thread: https://externals.io/message/74654 - Original archive: https://news-web.php.net/php.internals/74654 - Started by Remi Collet on 2014-05-30T11:45:55+00:00 - Messages: 47 - Page 1 of 2: messages 1 to 25 (25 per page, add `?page=N` to the URL) - Next page: https://externals.io/message/74654.md?page=2 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #74654 — Remi Collet — 2014-05-30T11:45:55+00:00 URL: https://externals.io/message/74654#74654 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: 1) 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 2) Horde_Imap_Client_Cache_CacheTest::testGetCachedUids Erroneous data format for unserializing 'Mock_Horde_Imap_Client_Socket_1e0b6300' .... Remi. --- ## #74656 — Anatol Belski — 2014-05-30T14:25:59+00:00 URL: https://externals.io/message/74654#74656 Hi Remi, > Le 30/05/2014 13:45, Remi Collet a écrit : > > > […] > https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Mappin > g /ClassMetadataInfo.php#L911 > > > Fixed in doctrine > > > https://github.com/doctrine/doctrine2/commit/93c276d059b40b0783ba9a24549a > 8 > b135e257693#diff-093aaad4da679d0374260757bf7bafca > > > […] > https://github.com/sebastianbergmann/phpunit-mock-objects/blob/master/src > / > Framework/MockObject/Generator.php#L274 > > > Fix in PHPUnit > > > https://github.com/sebastianbergmann/phpunit-mock-objects/commit/1c68f133 > 8 > f1940deb8265428bb2a7cbc5bc074b5#diff-64dbbc1c21f9be2d92e2b715617ffe34 > > https://github.com/sebastianbergmann/phpunit-mock-objects/commit/1c68f133 > 8 > f1940deb8265428bb2a7cbc5bc074b5#diff-64dbbc1c21f9be2d92e2b715617ffe34 > > thanks for the reproduce case. As discussed on the IRC, I'll be working on a solution to minimize the BC break in 5.4 and 5.5. IMHO 5.6 is fine with that "as is", as it's correct with the current specifications of the Serializable and fixes wide range crash vulnerability. Regards Anatol --- ## #74707 — Anatol Belski — 2014-06-03T12:41:23+00:00 URL: https://externals.io/message/74654#74707 · In reply to: #74656 > Hi Remi, > > > > > > […] > thanks for the reproduce case. As discussed on the IRC, I'll be working > on a solution to minimize the BC break in 5.4 and 5.5. IMHO 5.6 is fine > with that "as is", as it's correct with the current specifications of the > Serializable and fixes wide range crash vulnerability. > > > Regards > > > Anatol > > > -- > > > > > I've invented a patch fixing BC regarding this issue. It passes the userspace classes but blocks the internal classes. While it's a must for the stable 5.4 and 5.5, I still doubt we should put it into 5.6++. Even it can work with userspace classes, it's highly inconsistent. For example - a userclass implementing Serializable but throwing an exception in `unserialize()` will behave wrong when instantiated that way. Basically before I apply this fix - IMHO, it should go into 5.4 and 5.5 only, 5.6 should have clean behaviour. From the userspace perspective - doctrine and phpunit have already fixed this using ReflectionClass::newInstanceWithoutConstructor(), so that is fine with the future. The fix should only go for legacy. So, what you say? Regards Anatol --- ## #74710 — Jakub Zelenka — 2014-06-03T13:23:39+00:00 URL: https://externals.io/message/74654#74710 · In reply to: #74707 > Basically before I apply this fix - IMHO, it should go into 5.4 and 5.5 > only, 5.6 should have clean behaviour. From the userspace perspective - > doctrine and phpunit have already fixed this using > ReflectionClass::newInstanceWithoutConstructor(), so that is fine with the > future. The fix should only go for legacy. So, what you say? > > Would it be possible to do a quick releases of 5.4 and 5.5 that fixes the BC break (it means 5.4.30 and 5.5.14)? I had to spent some time this morning updating one project just to update Doctrine. The thing is that not everyone knows about the issue and not everyone is using "dev-master" in the composer... Also updating composer is for some project with lots of depencies really painful (especially if you don't have time to do it often... :) ). I don't think that requiring it for php bug release is a good idea. It would be great to do a new release as soon as possible and prevent any such issue. Thanks Jakub --- ## #74711 — Remi Collet — 2014-06-03T13:28:28+00:00 URL: https://externals.io/message/74654#74711 · In reply to: #74707 Le 03/06/2014 14:41, Anatol Belski a écrit : > I've invented a patch fixing BC regarding this issue. Tested. Work fine. Thanks. > It passes the > userspace classes but blocks the internal classes. While it's a must for > the stable 5.4 and 5.5, Strongly agree. > I still doubt we should put it into 5.6++. Even it > can work with userspace classes, it's highly inconsistent. For example - a > userclass implementing Serializable but throwing an exception in > `unserialize()` will behave wrong when instantiated that way. > > Basically before I apply this fix - IMHO, it should go into 5.4 and 5.5 > only, 5.6 should have clean behaviour. Agree. Remi. > From the userspace perspective - > doctrine and phpunit have already fixed this using > ReflectionClass::newInstanceWithoutConstructor(), so that is fine with the > future. The fix should only go for legacy. So, what you say? > > Regards > > Anatol --- ## #74713 — johannes@schlueters.de — 2014-06-03T14:27:37+00:00 URL: https://externals.io/message/74654#74713 · In reply to: #74710 > Would it be possible to do a quick releases of 5.4 and 5.5 that fixes > the > BC break (it means 5.4.30 and 5.5.14)? such an issue btw. is a case where I can imagine a php.net frontpage news entry by the RMs :) johannes --- ## #74717 — Benjamin Eberlei — 2014-06-03T15:07:30+00:00 URL: https://externals.io/message/74654#74717 · In reply to: #74654 @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: > > 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: > > 1) 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 > > 2) Horde_Imap_Client_Cache_CacheTest::testGetCachedUids > Erroneous data format for unserializing > 'Mock_Horde_Imap_Client_Socket_1e0b6300' > .... > > > > > Remi. --- ## #74719 — Nikita Popov — 2014-06-03T15:22:26+00:00 URL: https://externals.io/message/74654#74719 · In reply to: #74717 On Tue, Jun 3, 2014 at 5:07 PM, Benjamin Eberlei 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 --- ## #74720 — johannes@schlueters.de — 2014-06-03T15:55:13+00:00 URL: https://externals.io/message/74654#74720 · In reply to: #74717 > > 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 --- ## #74721 — Marco Pivetta — 2014-06-03T15:59:26+00:00 URL: https://externals.io/message/74654#74721 · In reply to: #74720 > 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 http://twitter.com/Ocramius http://ocramius.github.com/ --- ## #74722 — johannes@schlueters.de — 2014-06-03T16:07:04+00:00 URL: https://externals.io/message/74654#74722 · In reply to: #74721 > On 3 June 2014 17:55, Johannes Schlüter > 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-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. > 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 --- ## #74723 — Marco Pivetta — 2014-06-03T16:17:20+00:00 URL: https://externals.io/message/74654#74723 · In reply to: #74722 > 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 http://twitter.com/Ocramius http://ocramius.github.com/ --- ## #74724 — Andrea Faulds — 2014-06-03T16:25:33+00:00 URL: https://externals.io/message/74654#74724 · In reply to: #74723 > 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/ --- ## #74725 — Julien Pauli — 2014-06-03T16:26:49+00:00 URL: https://externals.io/message/74654#74725 · In reply to: #74723 > > > […] > > > Pushing a "nightly" package to Travis would solve this issue almost > entirely. > > > […] > > > 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. 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 --- ## #74726 — Julien Pauli — 2014-06-03T16:40:03+00:00 URL: https://externals.io/message/74654#74726 · In reply to: #74725 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 --- ## #74727 — Remi Collet — 2014-06-03T16:58:25+00:00 URL: https://externals.io/message/74654#74727 · In reply to: #74720 Le 03/06/2014 17:55, Johannes Schlüter a écrit : > > > […] > > 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. --- ## #74728 — Marco Pivetta — 2014-06-03T17:11:02+00:00 URL: https://externals.io/message/74654#74728 · In reply to: #74727 > 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 http://twitter.com/Ocramius http://ocramius.github.com/ --- ## #74729 — Jeremy Mikola — 2014-06-03T19:04:47+00:00 URL: https://externals.io/message/74654#74729 · In reply to: #74728 > > 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 --- ## #74736 — Remi Collet — 2014-06-04T05:24:05+00:00 URL: https://externals.io/message/74654#74736 · In reply to: #74729 Le 03/06/2014 21:04, Jeremy Mikola a écrit : > > > > […] > > > 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. https://github.com/sebastianbergmann/phpunit-mock-objects/commit/1c68f1338f1940deb8265428bb2a7cbc5bc074b5 https://github.com/sebastianbergmann/phpunit-mock-objects/commit/ba8784d88ff4cc52cf9424d7f7c631ccd867019c 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. --- ## #74737 — Benjamin Eberlei — 2014-06-04T06:03:42+00:00 URL: https://externals.io/message/74654#74737 · In reply to: #74736 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 : > > […] > wrote: > > […] > https://github.com/doctrine/doctrine2/commit/e577e7786796e9393df381f02221046dea6253a5 > > […] > 5.4.29 > > […] > > 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 > > > […] > add > > […] > > > https://github.com/sebastianbergmann/phpunit-mock-objects/commit/1c68f1338f1940deb8265428bb2a7cbc5bc074b5 > > > https://github.com/sebastianbergmann/phpunit-mock-objects/commit/ba8784d88ff4cc52cf9424d7f7c631ccd867019c > > > 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. --- ## #74738 — Julien Pauli — 2014-06-04T07:53:44+00:00 URL: https://externals.io/message/74654#74738 · In reply to: #74737 > 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 --- ## #74739 — Matteo Beccati — 2014-06-04T11:45:58+00:00 URL: https://externals.io/message/74654#74739 · In reply to: #74725 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/ --- ## #74741 — Matteo Beccati — 2014-06-04T13:53:55+00:00 URL: https://externals.io/message/74654#74741 · In reply to: #74739 > 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/ --- ## #74742 — Julien Pauli — 2014-06-04T16:45:56+00:00 URL: https://externals.io/message/74654#74742 · In reply to: #74741 > > > […] > > 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 --- ## #74743 — Benjamin Eberlei — 2014-06-04T16:56:52+00:00 URL: https://externals.io/message/74654#74743 · In reply to: #74738 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 > wrote: > > […] > into > > […] > > newInstanceWithoutConstructor() has been introducted into PHP to > address exactly the > use-cases we are considering in this topic. The Doctrine one for example. > > Julien --- Next page: https://externals.io/message/74654.md?page=2