Repository navigation
ReflectionClass::getStaticPropertyValue() returns UNDEF zval for uninitialized typed properties #12856
Description
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 useBP_VAR_Rinstead ofBP_VAR_IS(or just usezend_read_static_property_exwithsilent=false) - otherwise the conditional in
zend_std_get_static_propertyshould be fixed to return a null pointer regardless of theBP_VAR_*mode.
PHP Version
PHP 8.3-dev
Operating System
irrelevant
Activity
ndossche commentedon Dec 3, 2023
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 commentedon Dec 3, 2023
Why are there two APIs (
ReflectionClass::getStaticPropertyValue()vsReflectionProperty::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 commentedon Dec 3, 2023
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:
It should say:
EDIT: see php/doc-en#2987
kocsismate commentedon Dec 3, 2023
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 theReflectionPropertyone. 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 aReflectionClass::isStaticPropertyInitialized()method. But at this point it starts to look like a separate property API baked intoReflectionClass.... Which is not ideal.kocsismate commentedon Dec 3, 2023
IMO this should either throw or return the default value (
$defaultparameter). I lean towards the latter solution.ndossche commentedon Dec 3, 2023
Default makes sense too, but then you can't distinguish between a non-existent property and one that exists but is uninitialized.
kocsismate commentedon Dec 3, 2023
Yeah, I'm fine with throwing an exception. It would make the removal of the default parameter easier. :)
iluuu1994 commentedon Dec 5, 2023
Tbh it seems odd to throw when there's an explicit
$defaultparameter. I realize that's not the original purpose of the parameter.That's true, although there is more fitting API to do so (i.e.
ReflectionProperty::isInitialized()).ndossche commentedon Dec 8, 2023
The way to go forward with this (exception / use default value) should probably be discussed on the mailing list.
Fix phpGH-12856: ReflectionClass::getStaticPropertyValue() returns UN…
Fix phpGH-12856: ReflectionClass::getStaticPropertyValue() returns UN…