[WIP] Fix instantiation for latest PHP versions.

[whatthejeff]

PHP 5.4.29 and 5.5.13 introduced a BC break that prevents using serialization as a means for instantiating internal objects without calling their constructor (unless they implement Serializable) [1]. This issue is still evolving and workarounds are currently being discussed in the PHP internals mailing list [2].

In the mean time, since many PHP libraries rely on this functionality which is becoming increasingly complicated to support, @Ocramius has created ocramius/instantiator [3]. This PR adds support for ocramius/instantiator so that phpspec can continue to support this feature and stay in sync with other PHP libraries which support this feature without having to duplicate logic/updates across multiple projects.

Please note that switching to ocramius/instantiator doesn't immediately solve all problems related to this BC break. Right now some of our tests are still failing. Ideally, as workarounds become available, they will be added to ocramius/instantiator.

[stof]

Could you just rephrase the commit message ? the important part is not that it uses ocramius/instantiator (this is clearly an implementation detail) but that it fixes the instantiation on latest PHP versions

[whatthejeff]

Sure thing.

[whatthejeff]

Test failures are related to https://bugs.php.net/bug.php?id=67453. Seems we have the same issue in PHPUnit (sebastianbergmann/phpunit-mock-objects#178). I'm unaware of any known workarounds at the moment. I'm going to close this until I have more information and more time.

[cordoval]

@stof do you think the dependencies here on the composer.json should be tighter to the 3rd digit so the ~ would not take us through the braks from 2.1 onwards? that i know the promise and BC compatibility applies from 2.3 onwards, so imo we should be doing this in all projects, unless is deliberate. But that is for symfony, for other libraries it should also be even more carefuly. But i could be wrong, if so please correct me @stof 🍰

[stof]

@cordoval the Symfony constraints are not changed in this PR, so I don't really see the reason of this comment here.

And given that Symfony follows semver, using the semver constraint is the best one. For the Instantiator library, I trust @Ocramius to follow semver properly as well instead of breaking BC in minor or patch releases.

[Ocramius]

FYI, the 5.6 situation is still not clear in internals. As for semver, I'm quite serious about stability of this component.

[whatthejeff]

@Ocramius Do you have any ideas to work around https://bugs.php.net/bug.php?id=67453 in 5.4/5.5?

[stof]

Reopening this as a WIP for the fix

[stof]

@Ocramius The existing code in PhpSpec has some more logic when building the serialized string. Maybe it could help for Instantiator as well ?

[Ocramius]

The php internals bug is exactly what is being discussed in internals
afaik. As for the code, is there anything specific that I should look at?
On 25 Jun 2014 11:40, "Christophe Coevoet" notifications@github.com wrote:

@Ocramius https://github.com/Ocramius The existing code in PhpSpec has
some more logic when building the serialized string. Maybe it could help
for Instantiator as well ?

—
Reply to this email directly or view it on GitHub
#377 (comment).

[stof]

@Ocramius the handling of default properties of the class, in the PhpSpec\Util\Instantiator class (the code removed in this PR)

[whatthejeff]

@stof Seems that was deliberately removed in Ocramius/Instantiator@dcba7c6.

[Ocramius]

@stof I actually removed all that logic in Ocramius/Instantiator@dcba7c6 - what is the specific use-case here?

[Ocramius]

@whatthejeff too fast :-)

[Ocramius]

Import this class, plus it should probably be cached locally (private property)

[whatthejeff]

Alright. It wasn't imported initially because it needs to be aliased and I wasn't feeling clever enough to think of a good alias :P

[Ocramius]

Ah, right. Then the FQCN is fine.

[whatthejeff]

@stof @Ocramius Should we be caching \PhpSpec\Util\Instantiator as well? Otherwise caching \Instantiator\Instantiator doesn't really add much value.

[Ocramius]

@whatthejeff if performance is not a problem here, then it's fine. It can also be left out and handled later instead.

[whatthejeff]

@Ocramius Yeah, unless we make the cache static or do a bit of refactoring, I don't think caching \Instantiator\Instantiator has any impact on the number of objects created. I'll leave this up to @stof.

[Ocramius]

The instantiator already caches internally, so it's pretty ok.

[cordoval]

@stof i meant to perhaps get feedback to open a separate ticket, it is not related.

" semver constraint" what you mean by that ~2.1 vs ~2.1.0 ? even when api is not broken, added features can break sometimes the userland layer, doing it ~2.1.0 makes it safer to just include bugfixes alone.

[stof]

@cordoval if you break something in userland when addign a new feature, it means your new feature is not implemented in a backward compatible way, and so the version introducing it MUST be a major version. Otherwise, you don't follow semver

[cordoval]

correct but i am referring to the cases where you add a method that is being overwritten already, that breaks stuff. This is explained at the beginning of the document

"For example, if we add a new method to a class, this will break an application which extended this class and added the same method, but with a different method signature." ref. http://symfony.com/doc/current/contributing/code/bc.html

of course it is an edge case but honestly we have little control on userland so that is why specifying 3 digits like in nodejs and elsewhere is a better practice imo. At least in userland, not in library maintainer land.

[stof]

but we don't extend the Instantiator library. We only use it, so we are not affected by this. We have control about what we do with the library. We are the userland for this case, as it is an internal dependency, not something exposed to phpspec users

[MarcelloDuarte]

@whatthejeff are you looking at why this is still blowing the tests for ExceptionFactory under php >= 5.4?

[whatthejeff]

@MarcelloDuarte It's mentioned earlier in this ticket. The test failures are due to https://bugs.php.net/bug.php?id=67453. According to @remicollet, the fix he committed is tentative for 5.6 and will not be ported to 5.4/5.5 [1].

Unfortunately, I don't think there's a workaround at the moment. We actually have the same issue in PHPUnit. As far as I know, the discussions on how to address this in 5.4/5.5 are still in progress.

[stof]

@MarcelloDuarte my suggestion is to merge this, which already fixes the library for some cases, even if it does not fix it everywhere. It is better than the current solution which is already broken in these cases anyway.
thus, fixing the remaining issues would hopefully involve changes only in the Instantiator library, giving them to us at the same time than to PHPUnit

[whatthejeff]

So PHP 5.4.30 has been released. From what I can tell, there will no longer be a way in PHP 5.4/5.5 to instantiate internal classes that don't implement custom serializers without invoking their constructors. This means that the tests that are failing on PHP 5.4/5.5 will continue to fail.

[whatthejeff]

For reference, here is the relevant excerpt from the upgrading guide:

4a. unserialize() change

• Starting PHP 5.4.29, the bug fix for bug #67072 introduces a change to
  unserialize() function, detailed below:

  If the string looking like 'O:..:"ClassName":...' is unserialized, and
  the class named is an internal class that declares custom unserializer
  function, or extends such class, unserialize would fail.

  Using O: for user classes not extending internal classes (including
  those implementing Serializable) is still supported in 5.4, though
  it is deprecated and may not be supported in 5.6. Same for using O: for
  internal classes implementing Serializable (like ArrayObject) and
  classes that extend them.

  The reason for that is that O: format is meant to be used with classes
  that do not define custom handlers, and was never intended for the use
  with classes that do. When used with the class that relies on custom
  unserializer, it can leave the object of such internal class in an
  inconsistent state which has been observed to result in crashes and may
  also lead to memory corruption and ultimately even arbitrary code
  execution. This was never the intended use of unserializer, and mere
  possibility of doing this constitutes a bug, since the data passed to
  unserialize() is not a valid serialization of any object. Since there
  are many applications applying unserialize() to untrusted data, this
  presents a potential security vulnerability. Thus, keeping such bug in
  the code base was considered too dangerous.

  We are aware that some applications use O: format as a way to
  instantiate classes. This was never the intended usage of serializer,
  and ReflectionClass methods such as newInstance or
  newInstanceWithoutConstructor can be used for that. We're working on
  providing more comprehensive solution for such use cases in PHP 5.6 and
  welcome the ideas in this area.

  We note also that using unserialize() on any data that is not the result
  of serialize() call continues to be an unsupported scenario and should
  not be relied on to produce any specific result.

[whatthejeff]

The HHVM failure is a prophecy bug. Prophecy is failing because it's grabbing scalar type hints from HNI extensions. For instance:

$prophet = new Prophecy\Prophet;
$prophet->prophesize('ReflectionMethod')->reveal();

will fail because it adds the scalar type hints to the export method:

public static function export(string $cls, string $name, bool $ret = false) {
   return $this->getProphecy()->makeProphecyMethodCall(__FUNCTION__, func_get_args());
}

[whatthejeff]

This appears to be fixed in 5.5.14, at least according to the release notes.

@ciaranmcnulty: It's as fixed as it's going to be. Due to security issues, certain internal objects can no longer be instantiated with the unserialize() hack. For instance: http://3v4l.org/DO5kW

After a bit of testing, the following seem to be the only core classes that generate the 'Erroneous data format for unserializing...' message now:

• Closure
• Generator
• SplFileInfo
• DirectoryIterator
• FilesystemIterator
• RecursiveDirectoryIterator
• GlobIterator
• SplFileObject
• SplTempFileObject
• PDORow
• SimpleXMLElement
• SimpleXMLIterator
• Phar
• PharData
• PharFileInfo

[sstok]

Any news on this?

[Ocramius]

There's still no clear resolution path from internals about PHP 5.6 :\

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

On Wed, Jul 16, 2014 at 2:13 PM, Sebastiaan Stok notifications@github.com
wrote:

Any news on this?

—
Reply to this email directly or view it on GitHub
#377 (comment).

[ciaranmcnulty]

@whatthejeff Is this just blocked on the prophecy PR? It looks like it fixes our test suite, which is a win...

[ciaranmcnulty]

@whatthejeff Ah, so it's merged and we're waiting on a new point release of Prophecy for this?

[Ocramius]

FYI, I provided some fixes that give us compatibility when working with 5.5.14 and 5.4.30.

5.6.0-rc* is still unclear and depends on php/php-src#733, but if that's merged, then instantiator/instantiator:1.1.* can be used here.

[ciaranmcnulty]

I guess we need a point release of Prophecy to point the new version to (to fix HHVM issue) and then we can merge this.

@everzet any chance of getting Prophecy 1.1.1?

[ciaranmcnulty]

Ah wait, this is already in the latest prophecy.

@whatthejeff can you bump the Prophecy dependency in your branch please, to see if that fixes the HHVM failures in this PR?

[whatthejeff]

@ciaranmcnulty We probably need to update travis to test against HHVM nightly since HHVM 3 has some parity regressions in the Reflection API.

[ciaranmcnulty]

HHVM being such a moving target seems to be problematic - I wonder at what point we have to start paying attention to testing multiple HHVM versions?

[whatthejeff]

Looks like the prophecy dependency doesn't need to be bumped. Seems there is still a remaining HHVM failure I need to look into. I should have time to investigate it in the next couple of days.

[whatthejeff]

Alright, I've confirmed that the remaining issue is indeed another Zend compatibility issue in HHVM.

Here is a quick reproduce case:

<?php

class TestArray extends ArrayObject 
{
    public function offsetExists($index) 
    {
        return true;
    }
}

$test = new TestArray();
var_dump(isset($test['abc']));

In Zend PHP you get the following:

bool(true)

And in HHVM you get the following:

Notice: Undefined index: abc in...
bool(false)

This is related to facebook/hhvm#2181 and facebook/hhvm#2871. Basically when you use isset() on an ArrayObject subclass, HHVM calls offsetGet on the base ArrayObject class if offsetExists returns true. I can look into fixing this in HHVM soonish, but maybe we should add a workaround in the meantime. Thoughts?

[whatthejeff]

HHVM issue is here: facebook/hhvm#3309

[whatthejeff]

@ciaranmcnulty The HHVM bug is fixed now. I just confirmed that all tests pass in the latest HHVM. Would you mind restarting the travis build?

[ciaranmcnulty]

I don't have permissions for that - @MarcelloDuarte ?

[whatthejeff]

@ciaranmcnulty Are you logged in? You should be able to do a restart on travis if you're a collaborator on github.

[ciaranmcnulty]

@whatthejeff Thanks ;-)

[whatthejeff]

@ciaranmcnulty Oh wait, I don't think you reran my test. Can you rerun this one: https://travis-ci.org/phpspec/phpspec/builds/31117450

[whatthejeff]

Thanks, @ciaranmcnulty. All is green now \o/

[ciaranmcnulty]

Thanks for all the hard work @whatthejeff