Reflection: indicate final properties in string output #17827
Conversation
|
CC @iluuu1994 - my understanding is that final properties was added as a part of property hooks |
| @@ -931,6 +931,9 @@ static void _property_string(smart_str *str, zend_property_info *prop, const cha | |||
| if (!prop) { | |||
| smart_str_append_printf(str, "<dynamic> public $%s", prop_name); | |||
| } else { | |||
| if (prop->flags & ZEND_ACC_FINAL) { | |||
| smart_str_appends(str, "final "); | |||
| } | |||
There was a problem hiding this comment.
Can you move this down to ZEND_ACC_STATIC? Granted, this is inconsistent with the order for methods, but final public static $prop; is weird.
The order seems to be abstract/final/static for other structures.
There was a problem hiding this comment.
For me public final sounds odd, and public abstract would sound even odder - https://www.php.net/manual/en/language.oop5.abstract.php put abstract before visibility for properties, I didn't see any examples of final properties to compare with.
I tried to do a code search in github but the regex wasn't cooperating with me to match property declarations. Looking through the property hooks implementation PR, I see
Zend/tests/property_hooks/final_private_prop.phpt->final private ...Zend/tests/property_hooks/final_prop.phpt->public final ...Zend/tests/property_hooks/final_prop_2.phpt->public final ...Zend/tests/property_hooks/final_prop_final_hook.phpt->final public ...
in consecutive test files.
Without a standard practice one way or the other, I figured it was better to be consistent with methods
Add "final" to the result of `_property_string()` when outputting the string representation of a `ReflectionClass` or `ReflectionProperty` instance
Add "abstract" to the result of `_property_string()` when outputting the string representation of a `ReflectionClass` or `ReflectionProperty` instance
DanielEScherzer
force-pushed
the
reflection-prop-final
branch
from
438de46 to
1334776
Compare
Added I didn't send this to 8.4 originally since it isn't really a bug, but retargeted. I was planning to also work on some kind of indication of property hooks in a follow-up, should that also go to 8.4? |
|
I would classify this as a bug. Whether your other PR should target 8.4 depends on whether it's a bug. 😄 |
Add "final" to the result of
_property_string()when outputting the string representation of aReflectionClassorReflectionPropertyinstance