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

Fixed new instance creating for php 5.4+ #1053

Closed
wants to merge 1 commit into from
Closed

Fixed new instance creating for php 5.4+ #1053

wants to merge 1 commit into from

Conversation

nkt
Copy link

@nkt nkt commented Jun 7, 2014

No description provided.

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3158

We use Jira to track the state of pull requests and the versions they got
included in.

@tristanbes
Copy link

+1 The original error was :

__clone method called on non-object in ClassMetadataInfo.php line 872

@Ocramius
Copy link
Member

Ocramius commented Jun 7, 2014

We already had some bugs introduced by this hotfix (see #1051), so this needs more testing before it lands.

@nkt
Copy link
Author

nkt commented Jun 7, 2014

@Ocramius previous code hardcodes php versions, suggested changes just removes hardcoded versions.
newInstanceWithoutConstructor was added in 5.4.0 proof

@Ocramius
Copy link
Member

Ocramius commented Jun 7, 2014

@nkt yes, and I'm saying that using newInstanceWithoutConstructor already broke something.

@lavoiesl
Copy link
Member

Rather than testing for version, you should test for the method itself as mentioned here : #1056 (comment)

@Majkl578
Copy link
Contributor

OMG people, at least fifth PR with same change?

@stof
Copy link
Member

stof commented Jun 12, 2014

Please read the discussion on 530c01b and you will see why we want to change the Doctrine behavior only on affected PHP version rather than on all PHP 5.4+. this change is breaking other stuff

@nkt
Copy link
Author

nkt commented Jun 12, 2014

@stof I don't really understand how this code could broke something?
Previous code is dirty hack:

$this->_prototype = unserialize(sprintf('O:%d:"%s":0:{}', strlen($this->name), $this->name))

No one promised that this code will works fine in future, and now it's broken.

This method should return new instance without calling costructor, so it's doing it.

@Ocramius
Copy link
Member

No one promised that this code will works fine in future, and now it's broken.

It was a hack, and a well known one as well. It was used everywhere we needed to do this, and ReflectionClass#newInstanceWithoutConstructor was neglected because we had a good working solution for it. Turns out that ReflectionClass#newInstanceWithoutConstructor is not yet good enough, as it breaks anywhere you extend an internal class.

So yes, the hack is still more reliable than the official solution.

@nkt
Copy link
Author

nkt commented Jun 12, 2014

@Ocramius why it should break something, this is spited method, it's could working whatever it's want, but must return new instance of given class.
Can you show me any examples, where it broke something?

@Ocramius
Copy link
Member

Here: http://3v4l.org/gGf7X

@nkt
Copy link
Author

nkt commented Jun 12, 2014

@Ocramius thanks. Now I see.
Can you release version with current fix?

@Ocramius
Copy link
Member

@nkt it was already released

@nkt
Copy link
Author

nkt commented Jun 12, 2014

@Ocramius 👍
So I close it, maybe we should tell php-team fix this BC-break in next releases.

@nkt nkt closed this Jun 12, 2014
@stof
Copy link
Member

stof commented Jun 12, 2014

there is already a discussion about this on php-internals

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

Successfully merging this pull request may close these issues.

None yet

7 participants