Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

loosen the restrictions on ReflectionClass::newInstanceWithoutConstructor() #733

Merged
merged 2 commits into from Jul 30, 2014

Conversation

Projects
None yet
7 participants
Owner

Tyrael commented Jul 23, 2014

allow instantiating any class except internal classes with a final __construct().
see http://www.serverphorums.com/read.php?7,959450,987654#msg-987654 (and the whole thread) for the full discussion, but basically with the removal of the unserialize O: trick which allowed the instantiation of internal classes with a custom serializer there is no way to do that from the userland, and that breaks a bunch of libs/apps out there.
the original restriction was in place, because there are internal classes with custom object storage, where the object initialization happens in the __construct instead of the create_object hook, but for non final constructors it is already a problem which can occur (a class which has it's own __construct method which doesn't call parent::__construct() extending an internal class), so it is ok to allow the same problems surfacing here also.

Contributor

malukenho commented Jul 23, 2014

+1

Contributor

Ocramius commented Jul 23, 2014

@Tyrael could tests for:

  • classes extending internal classes
  • ArrayObject

be added to this suite?

Contributor

Ocramius commented Jul 23, 2014

Also, I'll need to compile this and try it against Instantiator - maybe over the weekend...

Contributor

indeyets commented Jul 23, 2014

👍

Owner

jpauli commented Jul 23, 2014

Shouldn't we disallow as well classes with a non public constructor() ? Should they be intern or user. Someone putting a non public constructor want to take hand over its objects creation (singletons for example), so I guess it would be better also to disallow newInstanceWithoutConstructor() in such a case

Owner

Tyrael commented Jul 23, 2014

@jpauli I don't think that we should:
1, we didn't checked this previously, so even if we decide to do this, it should only happen for internal classes otherwise we just made the newInstanceWithoutConstructor more strict.
2, we have ReflectionMethod::setAccessible() so we already promote that Reflection can override visibility modifiers.
3, there is nothing preventing you from extending the class with the private or protected __construct() and override the __construct method with a public one from the subclass so this restriction would be moot anyways(except for final classes/methods but we already check that).

Owner

jpauli commented Jul 23, 2014

Yup last argument I forgot about is enough to me :-)
Thx.

@Ocramius Ocramius referenced this pull request in Ocramius/Instantiator Jul 27, 2014

Merged

PHP 5.4.30 5.5.14 5.6.0-rc* compat #8

5 of 5 tasks complete
Contributor

Ocramius commented Jul 27, 2014

This patch seems to work for me (as of Ocramius/Instantiator#8).

The only thing missing now is finding out when it doesn't or shouldn't work.

If I get this correctly, I this should give me the classes that are not yet supported by ReflectionClass#newInstanceWithoutConstructor() (considering subclasses for abstract types):

<?php

$instantiable = function($className) use (& $instantiable) {
    if (! $className) {
        return true;
    }

    $reflection = new ReflectionClass($className);
    $hasFinalConstruct = false;

    if ($reflection->hasMethod('__construct')) {
        $hasFinalConstruct = $reflection->getMethod('__construct')->isFinal();
    }

    $parentClass = $reflection->getParentClass();

    return $instantiable($parentClass ? $parentClass->getName() : null)
        && ! ($reflection->isInternal() && $hasFinalConstruct);
};

$negate = function ($function) {
    return function (...$args) use ($function) {
        return ! $function(...$args);
    };
};

var_dump([
    'instantiable' => array_filter(get_declared_classes(), $instantiable),
    'not_instantiable' => array_filter(get_declared_classes(), $negate($instantiable)),
]);

Here's an example output: http://3v4l.org/rAOrK

@Tyrael does this make any sense?

EDIT: was excluding abstract classes, my bad...
EDIT2: nope, it's actually "final classes". @Tyrael the exception message should probably be changed...

@Ocramius Ocramius added a commit to Ocramius/Instantiator that referenced this pull request Jul 27, 2014

@Ocramius Ocramius HHVM-nightly will fail until in sync with php/php-src#733 2853ccb
Owner

nikic commented Jul 27, 2014

@Ocramius Final classes are disallowed, not final constructors. The error message in the patch should also be updated to reflect that.

Contributor

Ocramius commented Jul 27, 2014

Yeah, figured that, probably edited that comment of mine a dozen times by now :-)

Owner

Tyrael commented Jul 28, 2014

@nikic @Ocramius hm, you just made me realize that we have classes with final final constructors where the class itself isn't final.
I guess the patch should be modified to check for checking the constructor instead of checking the class, what do you think?

@Ocramius is there a reason to check so many things via Reflection in Instantiator instead of "always" trying to use Reflection to instantiate the class first, and catch the ReflectionException and do the dirty hacks there? In 5.6 most classes will be instantiated through Reflection, so the checks there just wasted cpu and memory cycles.

Owner

nikic commented Jul 28, 2014

@Tyrael Is there any class apart from SimpleXMLElement that does that?

Contributor

Ocramius commented Jul 28, 2014

@Ocramius is there a reason to check so many things via Reflection in Instantiator instead of "always" trying to use Reflection to instantiate the class first, and catch the ReflectionException and do the dirty hacks there?

Yeah, fatals fatals fatals everywhere, plus previous php versions, plus the bogous 5.4.29 and 5.5.13. Checks are cached anyway.

Owner

Tyrael commented Jul 28, 2014

@nikic from a quick look only SimpleXMLElement and Transliterator does this.
Maybe we should keep the current check, turn those two into final classes and update the error message in the pull request to refer to internal final classes?

Owner

Tyrael commented Jul 30, 2014

I've updated the exception message to better match the actual check.
I've also created a feature request on bugs.php.net to be referenced in NEWS:
https://bugs.php.net/bug.php?id=67713

@php-pulls php-pulls merged commit d18b162 into php:master Jul 30, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details
Contributor

Ocramius commented Jul 30, 2014

That was quick!

Owner

Tyrael commented Jul 30, 2014

yeah, I've really want to have an RC3 out this week, we can still tweak on the change if something comes up.

@Tyrael Tyrael deleted the Tyrael:newInstanceWithoutConstructor branch Aug 12, 2014

@Tyrael Tyrael added a commit that referenced this pull request Aug 18, 2014

@Tyrael Tyrael mention the changes regarding ReflectionClass::newInstanceWithoutCons…
…tructor in UPGRADING

see the discussion on the mailing list and in the pull request for details:
#733
003c2c1

@Tyrael Tyrael added a commit that referenced this pull request Aug 18, 2014

@Tyrael Tyrael Merge branch 'PHP-5.6'
* PHP-5.6:
  mention the changes regarding ReflectionClass::newInstanceWithoutConstructor in UPGRADING see the discussion on the mailing list and in the pull request for details: #733
eda5d30

@Tyrael Tyrael added a commit that referenced this pull request Aug 27, 2014

@Tyrael Tyrael mention the changes regarding ReflectionClass::newInstanceWithoutCons…
…tructor in UPGRADING

see the discussion on the mailing list and in the pull request for details:
#733
4b3de4d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment