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

Avoid constructor override #175

Merged
merged 99 commits into from
Dec 14, 2014
Merged

Conversation

Ocramius
Copy link
Owner

See #115

@Ocramius Ocramius added this to the 2.0.0 milestone Sep 28, 2014
@Ocramius
Copy link
Owner Author

Ping @malukenho for review :-)

@blanchonvincent
Copy link
Contributor

what a big change! interesting, but the php5.6 dependence should make problem for a lot of people to update PM to the 2.0.0 version

@Ocramius
Copy link
Owner Author

I really want to move to 1.0.0 and then start working on 2.0.0 by breaking BC mainly with minimum required versions.

This change should make ProxyManager 100% LSP-compliant

@Ocramius Ocramius self-assigned this Sep 28, 2014
. "\n\n";

foreach ($originalClass->getProperties() as $originalProperty) {
if ((PHP_VERSION_ID < 50400 || (defined('HHVM_VERSION'))) && $originalProperty->isPrivate()) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor performance optimization:
Render (PHP_VERSION_ID < 50400 || (defined('HHVM_VERSION')) into local variable before foreach loop.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHP_VERSION_ID < 50400 can be removed anyway since the composer.json file says that php 5.6 is a minimum version now.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go away with a rebase: I fixed it by using method_exists('Closure', 'bind') instead

@Ocramius
Copy link
Owner Author

Ocramius commented Oct 1, 2014

@blanchonvincent the 5.6 requirement is mainly because (new ReflectionClass('Foo'))->newInstanceWithoutConstructor() works well in 5.6, while it requires doctrine/instantiator for PHP <= 5.5 :-\

@Ocramius Ocramius force-pushed the hotfix/avoid-constructor-override branch from a302221 to 0a61ffc Compare October 1, 2014 11:56
@Pittiplatsch
Copy link

ReflectionClass::newInstanceWithoutConstructor() requires PHP 5.4+, doesn't it?
http://php.net/manual/en/reflectionclass.newinstancewithoutconstructor.php
Edit: Support for "all except final" classes with 5.6 - sorry, my bad :-(

@Ocramius
Copy link
Owner Author

Ocramius commented Oct 1, 2014

ReflectionClass::newInstanceWithoutConstructor() requires PHP 5.4+, doesn't it?

Yes, but in 5.4 and 5.5, you can't use it with anything either extending or being an internal php class. PHP 5.6 is much better on that front.

@Ocramius Ocramius force-pushed the hotfix/avoid-constructor-override branch 2 times, most recently from dee235c to b948637 Compare October 1, 2014 17:17
Ocramius added a commit that referenced this pull request Oct 1, 2014
Ocramius added a commit that referenced this pull request Oct 1, 2014
@Ocramius
Copy link
Owner Author

Ocramius commented Oct 1, 2014

This is ready for merge, but 1.0.0 bugs still need resolution first.

@Ocramius Ocramius changed the title [WIP] Avoid constructor override Avoid constructor override Oct 5, 2014
@Ocramius Ocramius force-pushed the hotfix/avoid-constructor-override branch from 04fec2e to cc610f3 Compare December 14, 2014 01:53
Ocramius added a commit that referenced this pull request Dec 14, 2014
@Ocramius Ocramius merged commit 99869b3 into master Dec 14, 2014
@Ocramius Ocramius deleted the hotfix/avoid-constructor-override branch December 14, 2014 02:04
Ocramius added a commit that referenced this pull request Dec 20, 2014
Cleanup: unused classes removal (constructors, not needed as per #115 and #175)
malukenho pushed a commit to malukenho/ProxyManager that referenced this pull request Jan 3, 2015
malukenho pushed a commit to malukenho/ProxyManager that referenced this pull request Jan 3, 2015
malukenho pushed a commit to malukenho/ProxyManager that referenced this pull request Jan 3, 2015
malukenho pushed a commit to malukenho/ProxyManager that referenced this pull request Jan 3, 2015
malukenho pushed a commit to malukenho/ProxyManager that referenced this pull request Jan 3, 2015
malukenho pushed a commit to malukenho/ProxyManager that referenced this pull request Jan 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants