ReflectionClass::getStaticPropertyValue() returns UNDEF zval for uninitialized typed properties

[ju1ius]

Description

The following code:

<?php declare(strict_types=1);

class Bug {
    private static $foo;
    private static int $bar;
}

$rc = new ReflectionClass(Bug::class);
var_dump($rc->getStaticPropertyValue('foo'));
var_dump($v = $rc->getStaticPropertyValue('bar'));
echo "bar = {$v}\n";

Resulted in this output:

NULL
UNKNOWN:0

Warning: Undefined variable $v in ... on line 11
bar = 

But I expected this output instead:
Well TBH, I'm not sure... If the goal is to be consistent with ReflectionProperty::getValue(), then:

NULL
Fatal error: Uncaught Error: Typed static property Bug::$bar must not be accessed before initialization in ...

Otherwise, then:

NULL
NULL

Looking at the implementation, the code calls zend_std_get_static_property(ce, name, BP_VAR_IS); which silences exceptions, but also completely bypasses the typed property initialization check.

So it looks like:

• if the behaviour of ReflectionProperty::getValue() is desired, then the method should use BP_VAR_R instead of BP_VAR_IS (or just use zend_read_static_property_ex with silent=false)
• otherwise the conditional in zend_std_get_static_property should be fixed to return a null pointer regardless of the BP_VAR_* mode.

PHP Version

PHP 8.3-dev

Operating System

irrelevant

[nielsdos]

It should probably throw to be consistent with getValue(), i.e. use BP_VAR_R. This would make the most sense to me. Getting NULL would be strange in this case especially when the type is not nullable...
But before I do anything: what do others think?

[ju1ius]

Why are there two APIs (ReflectionClass::getStaticPropertyValue() vs ReflectionProperty::getValue()) in the first place? Probably legacy?

Then there is the fact thatReflectionProperty::setValue now raises a deprecation warning when used on static properties, and the docs officially recommend using an API that hasn't seen a commit in more than 3 years...

IMO this deprecation was a bad move. ReflectionClass::(get|set)StaticPropertyValue() should have been deprecated instead.

@kocsismate What do you think?

[ju1ius]

IMO this deprecation was a bad move. ReflectionClass::(get|set)StaticPropertyValue() should have been deprecated instead.

After reading the deprecation RFC, I see that the new way to do this is ReflectionProperty::setValue(null, $value).

So there is an issue in the docs. The signature should be updated to reflect the nullability of the $object argument, and
instead of this:

Note: As of PHP 8.3.0, calling this method with a single argument is deprecated,
use ReflectionClass::setStaticPropertyValue() instead.

It should say:

Note: As of PHP 8.3.0, calling this method with a single argument is deprecated,
use ReflectionProperty::setValue(null, $value) instead.

EDIT: see php/doc-en#2987

[kocsismate]

I agree that the recommendation in the manual should be changed or at least extended with ReflectionProperty::setValue(null, $value). I'm not really sure how it ended be like this.

Regarding deprecating ReflectionClass::(get|set)StaticPropertyValue(), I don't have a strong opinion. We could do that, but I can image that in some case it's easier to use this API than the ReflectionProperty one. If other people agree, we could add these methods to the PHP 8.4 deprecations RFC, but personally, I'm fine having the two APIs.

UPDATE: I've just (re-)realized that ReflectionClass::getStaticPropertyValue() doesn't have a proper default value either... So that serves as a motivation for me to either remove this API completely or just to remove the 2nd parameter + add a ReflectionClass::isStaticPropertyInitialized() method. But at this point it starts to look like a separate property API baked into ReflectionClass.... Which is not ideal.

[kocsismate]

It should probably throw to be consistent with getValue(), i.e. use BP_VAR_R. This would make the most sense to me. Getting NULL would be strange in this case especially when the type is not nullable...
But before I do anything: what do others think?

IMO this should either throw or return the default value ($default parameter). I lean towards the latter solution.

[nielsdos]

Default makes sense too, but then you can't distinguish between a non-existent property and one that exists but is uninitialized.

[kocsismate]

Default makes sense too, but then you can't distinguish between a non-existent property and one that exists but is uninitialized.

Yeah, I'm fine with throwing an exception. It would make the removal of the default parameter easier. :)

[iluuu1994]

Tbh it seems odd to throw when there's an explicit $default parameter. I realize that's not the original purpose of the parameter.

but then you can't distinguish between a non-existent property and one that exists but is uninitialized.

That's true, although there is more fitting API to do so (i.e. ReflectionProperty::isInitialized()).

[nielsdos]

The way to go forward with this (exception / use default value) should probably be discussed on the mailing list.