php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Thank you for your help! If the status of the bug report you submitted changes, you will be notified. You may return here and check the status or update your report at any time.
The URL for your bug report is: https://bugs.php.net/bug.php?id=72174.
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 (profile)
Status: Closed Package: Reflection related
PHP Version: 7.0.6 OS:
Private report: No CVE-ID: None
 [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-2024 The PHP Group
All rights reserved.
Last updated: Wed May 08 20:01:36 2024 UTC