Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: PHP-8.3
Choose a base branch
from

Conversation

DanielEScherzer
Copy link
Contributor

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 or __CLASS__.

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

Add test cases for both of the callable and __CLASS__ cases to confirm that they now properly show the type of the constant.

Closes GH-15902

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` or `__CLASS__`.

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

Add test cases for both of the `callable` and `__CLASS__` cases to confirm that
they now properly show the type of the constant.

Closes phpGH-15902
@DanielEScherzer
Copy link
Contributor Author

This replaces #15922


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

@DanielEScherzer
Copy link
Contributor Author

CC @iluuu1994 @staabm @nielsdos from the original patch

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change LGTM now, thank you for following up!

@@ -0,0 +1,20 @@
--TEST--
ReflectionProperty object default - used to say "callable"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this description, why would it say callable? Maybe that's just what it printed for you due to memory corruption?

Copy link
Contributor Author

@DanielEScherzer DanielEScherzer Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea why it would say "callable", but it does - I found that depending on when the reflection object was created it would either say callable or __CLASS__ (or in the case of lazy ghosts, it would say "...")

See my analysis at #15902 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IS_INDIRECT most likely

[skip ci]
@DanielEScherzer
Copy link
Contributor Author

iluuu1994 and nielsdos both approved this - since I don't have the ability to merge patches, can one of you also merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants