Reflection: show the type of object constants used as default properties

[DanielEScherzer]

When a property default is based on a global constant, show the type of the default. Previously, format_default_value() assumed that non-scalar and non-array defaults were always going to be IS_CONSTANT_AST pointers, and when the AST expression had been evaluated and produced an object, depending on when the ReflectionClass or ReflectionProperty instance had been created, the default was shown as one of callable, __CLASS__, or ....

Instead, if the default value is an object (IS_OBJECT), show the type of that object.

Add test cases for each of the callable, __CLASS__, and ... cases to confirm that they all now properly show the name of the constant.

Closes gh-15902

[DanielEScherzer]

Somehow, the updated tests were run on an older version of the patch?

014-     Property [ public stdClass $a = object(stdClass) ]
014+     Property [ public stdClass $a = FOO ]

I'll rebase the patch so that the tests run again, I have no idea how this could even happen

[nielsdos]

Yeah the tests pass locally, don't know what went wrong in CI. The idea of the patch seems good to me.
Also, this bug is in PHP 8.2 too. So please retarget to 8.2 and make afterwards a follow-up PR to add the lazy ghost test.

[DanielEScherzer]

Yeah the tests pass locally, don't know what went wrong in CI. The idea of the patch seems good to me. Also, this bug is in PHP 8.2 too. So please retarget to 8.2 and make afterwards a follow-up PR to add the lazy ghost test.

Okay - I figured since this was kind of a new feature I should target master, but I'll switch (lesson learned from 16192 - force push before changing target branch)

[DanielEScherzer]

Well, this got ugly, since until 998bce1 there was already an IS_OBJECT handling, so the news entry is kind of misleading (it wasn't going to be a core dump, just an assertion failure, and I'm not sure the reproduction cases would have had the output I claimed) and the change was weird.

When this is upmerged to 8.4, please keep the IS_OBJECT check with

} else if (Z_TYPE_P(value) == IS_OBJECT) {
	// Show a consistent output with the type of the object, GH-15902
	smart_str_appends(str, "object(");
	smart_str_append(str, Z_OBJCE_P(value)->name);
	smart_str_appends(str, ")");
} else {

when rebasing past 998bce1. I sent #16490 for the lazy ghost tests

[DanielEScherzer]

@nielsdos (or someone else with rights) can you re-trigger the github actions? It seems that when I changed the target branch those didn't trigger jobs to run again, so only circleci is actually testing

[DanielEScherzer]

Thanks, but again CI is somehow reflecting an old version of the patch...???

[iluuu1994]

No, it's more likely to be related to the configuration, e.g. opcache/jit.

[DanielEScherzer]

No, it's more likely to be related to the configuration, e.g. opcache/jit.

Oh, I guess it isn't using the old version that is searching for the constant name, but rather it isn't an object (IS_OBJECT) when tested and so the IS_CONSTANT_AST is still reached - not sure how to fix this, I'm not very familiar with opcache/jit

[iluuu1994]

Try:

diff --git a/ext/reflection/php_reflection.c b/ext/reflection/php_reflection.c
index cbb071626a..88ca1cd634 100644
--- a/ext/reflection/php_reflection.c
+++ b/ext/reflection/php_reflection.c
@@ -884,7 +884,10 @@ static zval *property_get_default(zend_property_info *prop_info) {
 		ZVAL_DEINDIRECT(prop);
 		return prop;
 	} else {
-		return &ce->default_properties_table[OBJ_PROP_TO_NUM(prop_info->offset)];
+		if (UNEXPECTED(zend_update_class_constants(ce) != SUCCESS)) {
+			return &EG(uninitialized_zval);
+		}
+		return &CE_DEFAULT_PROPERTIES_TABLE(ce)[OBJ_PROP_TO_NUM(prop_info->offset)];
 	}
 }

[DanielEScherzer]

Try:

diff --git a/ext/reflection/php_reflection.c b/ext/reflection/php_reflection.c
index cbb071626a..88ca1cd634 100644
--- a/ext/reflection/php_reflection.c
+++ b/ext/reflection/php_reflection.c
@@ -884,7 +884,10 @@ static zval *property_get_default(zend_property_info *prop_info) {
 		ZVAL_DEINDIRECT(prop);
 		return prop;
 	} else {
-		return &ce->default_properties_table[OBJ_PROP_TO_NUM(prop_info->offset)];
+		if (UNEXPECTED(zend_update_class_constants(ce) != SUCCESS)) {
+			return &EG(uninitialized_zval);
+		}
+		return &CE_DEFAULT_PROPERTIES_TABLE(ce)[OBJ_PROP_TO_NUM(prop_info->offset)];
 	}
 }

Thanks - that works locally, lets see if GitHub CI agrees

[iluuu1994]

Please remove the spaces inside parens

[iluuu1994]

Also, this can just use echo $reflector;, the var_dump doesn't really have a purpose.

[iluuu1994]

Note though that this is actually kind of breaking. We're changing the default value to show the evaluated constant, rather than the constant AST. This was already the case when not using opcache, because we're evaluating the AST in-place. However, when using opcache, the AST is locked in shm and copied into "userspace" to be evaluated there. Adjusting this makes sense IMO to match the behavior of opcache and non-opcache, but I think that's better fixed on master.

Maybe you can propose this in a separate PR on master, and adjust tests to disable opcache (opcache.enable_cli=0 in --INI--).

[DanielEScherzer]

Not sure why I didn't see notifications for the replies, but since 8.2 is no longer supported, I recreated this at #17781 incorporating the feedback from @iluuu1994 above