Skip to content
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

Can't instantiate final class extending ArrayIterator #39

Closed
ciaranmcnulty opened this issue Oct 15, 2017 · 18 comments
Closed

Can't instantiate final class extending ArrayIterator #39

ciaranmcnulty opened this issue Oct 15, 2017 · 18 comments
Assignees
Milestone

Comments

@ciaranmcnulty
Copy link
Contributor

ciaranmcnulty commented Oct 15, 2017

final class Foo extends ArrayIterator
{
}

(new \Doctrine\Instantiator\Instantiator()->instantiate(Foo::class);
PHP Fatal error:  Uncaught Exception: unserialize(): Error at offset 13 of 14 bytes in /Users/cmcnulty/Documents/Code/phpspec/vendor/doctrine/instantiator/src/Doctrine/Instantiator/Exception/UnexpectedValueException.php:76

Reproduced on 1.0.5 and 1.1.0 and, not had a chance to debug. Was reported to PhpSpec as phpspec/phpspec#1124

@Ocramius
Copy link
Member

Ocramius commented Oct 15, 2017 via email

@ciaranmcnulty
Copy link
Contributor Author

non-final doesn't reproduce the error

@ciaranmcnulty
Copy link
Contributor Author

ciaranmcnulty commented Oct 15, 2017

Yep final makes it go back to the unserialization strategy, which seems to have an issue with the ArrayIterator

(that is, if I try to unserializing the generated string the final doesn't make a diff)

https://3v4l.org/1dYQD

@mikeSimonson
Copy link
Contributor

mikeSimonson commented Oct 16, 2017

@ciaranmcnulty This seems to be a hhvm serialization bug. https://3v4l.org/1dYQD

@ciaranmcnulty
Copy link
Contributor Author

Waaait, is it that it's working fine but the error handler is catching the notice?

@Ocramius
Copy link
Member

Ocramius commented Oct 17, 2017 via email

@mikeSimonson
Copy link
Contributor

mikeSimonson commented Oct 17, 2017

note that with that class that extends ArrayIterator
This works in PHP and thow a notice in hhvm

unserialize('C:3:"Foo":0:{}')

And This works in hhvm and breaks in PHP

unserialize('O:3:"Foo":0:{}')

@Ocramius
Copy link
Member

@mikeSimonson what is the resolution here?

@ciaranmcnulty
Copy link
Contributor Author

I was wondering too :-)

If it’s “won’t fix” can you say explicitly so we can plan how we handle this error being thrown?

@Ocramius Ocramius reopened this Oct 19, 2017
@Ocramius
Copy link
Member

Needs clarification. I'm fine with dropping HHVM support and having a workaround just for ArrayIterator if anybody has an idea of how to get there :P

@ciaranmcnulty
Copy link
Contributor Author

The fact it works differently in HHVM is a red herring IMO

the final is excluding the Reflection strategy for creating the object

The fact it extends ArrayObject means it somehow can't be deserialized from a string starting with c: but I don't know enough about serialization to suggest why (I tried to read http://www.phpinternalsbook.com/classes_objects/serialization.html#serializing-internal-objects and my eyes got tired)

@ciaranmcnulty
Copy link
Contributor Author

As a side note I very much doubt specific serialization formats are at all going to be thought about in terms of BC when PHP changes things :-/

@mikeSimonson
Copy link
Contributor

Sorry that it wasn't clear enough.

In my tests the only way to serialize the sample code in https://3v4l.org/1dYQD to 'O:3:"Foo":0:{}' is to use hhvm. And hhvm (HHVM-3.22.1) can unserialize that on my machine without any notice or error. https://3v4l.org/eX9TO

It just failed trying to unserialize hhvm serialization in php. Which seemed like a edge case to me.

That's the reason I closed it.

But maybe that's not was is going on in your case ?

@ciaranmcnulty
Copy link
Contributor Author

I'm not using HHVM, no

@mikeSimonson
Copy link
Contributor

Then, how did you ended up with the 'O:3:"Foo":0:{}' serialized string ?

@ciaranmcnulty
Copy link
Contributor Author

ciaranmcnulty commented Oct 19, 2017

I was debugging my failure case and it was generated by Doctrine/Instantiator here:

https://github.com/doctrine/instantiator/blob/master/src/Doctrine/Instantiator/Instantiator.php#L104-L105

nicolas-grekas added a commit to symfony/symfony that referenced this issue Sep 9, 2018
…ending internal ones (nicolas-grekas)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[VarExporter] fix exporting final serializable classes extending internal ones

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

Another edge case, discovered while reading doctrine/instantiator#39

Commits
-------

a5bf9b0 [VarExporter] fix exporting final serializable classes extending internal ones
@gquemener
Copy link
Contributor

This issue can now be closed 🎉

@Ocramius Ocramius added this to the 1.3.0 milestone Oct 21, 2019
@Ocramius Ocramius self-assigned this Oct 21, 2019
@Ocramius
Copy link
Member

Fixed in #61

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

No branches or pull requests

4 participants