php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #72174 ReflectionProperty#getValue() causes __isset call
Submitted: 2016-05-07 18:24 UTC Modified: 2016-05-12 15:40 UTC
From: ocramius at gmail dot com Assigned: nikic
Status: Closed Package: Reflection related
PHP Version: 7.0.6 OS:
Private report: No CVE-ID:
 [2016-05-07 18:24 UTC] ocramius at gmail dot com
Description:
------------
When using `ReflectionProperty#getValue($object)` on a `$object` with an `unset()` property and defined `__isset` and `__get` methods, the `__get` method is called, while it should only use `__get`.

Note that this is a bug that affects only PHP 7.0.6, and isn't part of 7.0.5.

Test script:
---------------
<?php

class Foo
{
    private $bar;

    public function __construct()
    {
        unset($this->bar);
    }

    public function __isset($name)
    {
        var_dump(__METHOD__);

        return true;
    }
    
    public function __get($name)
    {
        var_dump(__METHOD__);

        return $name;
    }
}

$instance = new Foo();

$reflectionBar = (new ReflectionProperty(Foo::class, 'bar'));
$reflectionBar->setAccessible(true);
$closureBar = (function (Foo $instance) {
    return $instance->bar;
})->bindTo(null, Foo::class);

var_dump($reflectionBar->getValue($instance));
var_dump($closureBar($instance));

Expected result:
----------------
string(10) "Foo::__get"
string(3) "bar"
string(10) "Foo::__get"
string(3) "bar"


Actual result:
--------------
string(12) "Foo::__isset"
string(10) "Foo::__get"
string(3) "bar"
string(10) "Foo::__get"
string(3) "bar"


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2016-05-07 18:25 UTC] ocramius at gmail dot com
See https://3v4l.org/9BipO
 [2016-05-07 18:27 UTC] ocramius at gmail dot com
Note: bug was discovered in builds of ProxyManager at https://travis-ci.org/Ocramius/ProxyManager/jobs/128480921#L384
 [2016-05-07 18:48 UTC] ocramius at gmail dot com
Bug seems to really only affect reflection access: https://3v4l.org/GY4NC
 [2016-05-07 18:59 UTC] nikic@php.net
To summarize OTR discussion:

The issue is that ReflectionProperty::getValue() currently performs a "silenced read", which is roughly equivalent to doing isset($obj->prop) ? $obj->prop : NULL. Prior to PHP 7.0.6 the isset($obj->prop) part did not result in a call to __isset() due to a bug in the magic method implementation.

However, it can be be argued that ReflectionProperty::getValue() should not be performing a silenced read in the first place. It should simply return the value of $obj->prop. If we make this change then the behavior described in the bug report will go back to what it was. On the other hand, this means that using ReflectionProperty::getValue() to read an undefined property will throw an undefined property notice (currently suppressed: https://3v4l.org/vdOjh).

The current plan is to change ReflectionProperty::getValue() to perform the read without silencing.
 [2016-05-07 19:00 UTC] ocramius at gmail dot com
As per discussion with NikiC and bwoebi in http://chat.stackoverflow.com/transcript/message/30402977#30402977

There has been an ever-living bug in `ReflectionProperty#getValue()` that causes the reflection property to be read (and return `NULL`) even when not set, and that without causing a warning. That can be seen at https://3v4l.org/vdOjh

Code below as reference:

```php
<?php

class Foo
{
    public $bar;
}

$instance = new Foo;

unset($instance->bar);

var_dump((new ReflectionProperty(Foo::class, 'bar'))->getValue($instance));
```

This code always returns `NULL`, but it should raise a NOTICE.

PHP 7.0.6 somehow mitigates the bug by doing an implicit `isset()` check internally, but that changes behavior of `ReflectionProperty#getValue()` in such a way that it breaks some userland existing code ( https://github.com/Ocramius/ProxyManager/issues/306 - possibly also other codebases) by causing `__isset` calls.

Making `ReflectionProperty#getValue()` trigger a notice when a property IS_UNDEF could probably be a good solution.
 [2016-05-10 10:18 UTC] nikic@php.net
Automatic comment on behalf of nikic
Revision: http://git.php.net/?p=php-src.git;a=commit;h=a1ed4ab3caf33b59742897b43462d033864bb490
Log: Fixed bug #72174
 [2016-05-10 10:18 UTC] nikic@php.net
-Status: Open +Status: Closed
 [2016-05-10 14:14 UTC] ocramius at gmail dot com
Looks good from here! Related tests seem to pass on 7.0.7-DEV

:beer:
 [2016-05-12 11:33 UTC] mbeccati@php.net
-Assigned To: +Assigned To: mbeccati
 [2016-05-12 11:33 UTC] mbeccati@php.net
it's actually a weird coincidence that it was you who submitted the bug. The fix is actually causing a regression in Doctrine now:

https://revive.beccati.com/bamboo/browse/PHP-DOCTR-PHP70-720/test/case/30048596

because:

object(ReflectionProperty)#201 (2) {
  ["name"]=>
  string(7) "country"
  ["class"]=>
  string(33) "Doctrine\Tests\Models\Cache\State"
}

and getValue() is called with a Doctrine\Tests\Models\Cache\City entity.

PHP 5.6 doesn't complain, whereas PHP-7.0 current does:

"Undefined property: Doctrine\Tests\Models\Cache\City::$country"

I'll leave it up to you guys to decide what to do ;)
 [2016-05-12 11:34 UTC] mbeccati@php.net
-Assigned To: mbeccati +Assigned To: nikic
 [2016-05-12 15:12 UTC] ocramius at gmail dot com
Saw that failure in my mailbox, thought it was unrelated.

As far as I can remember, doctrine only unsets `public` properties and uses a reflection workaround to access them (https://github.com/doctrine/common/blob/master/lib/Doctrine/Common/Reflection/RuntimePublicReflectionProperty.php), so something else is going on there.

I'll test against the latest PHP-7.0 ASAP
 [2016-05-12 15:25 UTC] ocramius at gmail dot com
@mbeccati this seems to be a failure on doctrine's side. I traced it down, and it seems like:

 * `Doctrine\Tests\Models\Cache\City#$state` is not defined (property doesn't exist)
 * `new ReflectionProperty(\Doctrine\Tests\Models\Cache\State::class, 'country')` is used as reflection property (wrong class)
 * PHP doesn't seem to check that the instance passed to `getValue()` respects the original interface ( see https://3v4l.org/0EAZe ), and that causes the code to fail with a notice rather than with something like a `ReflectionException`

I will open a new issue about this.
 [2016-05-12 15:39 UTC] ocramius at gmail dot com
Findings moved to  https://bugs.php.net/bug.php?id=72209&thanks=4
 [2016-05-12 15:40 UTC] ocramius at gmail dot com
Closing, moved remaining bits to https://bugs.php.net/bug.php?id=72209
 [2016-07-20 11:31 UTC] davey@php.net
Automatic comment on behalf of nikic
Revision: http://git.php.net/?p=php-src.git;a=commit;h=a1ed4ab3caf33b59742897b43462d033864bb490
Log: Fixed bug #72174
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Sun Apr 30 18:01:35 2017 UTC