Add ReflectionProperty::isDynamic() as an alternative to isDefault()
#15758
Conversation
|
Thank you! Since this is a new feature and we're already in beta stage, this likely needs RM approval. |
ext/reflection/php_reflection.c
Outdated
| @@ -5921,8 +5921,7 @@ ZEND_METHOD(ReflectionProperty, isVirtual) | |||
| _property_check_flag(INTERNAL_FUNCTION_PARAM_PASSTHRU, ZEND_ACC_VIRTUAL); | |||
| } | |||
|
|
|||
| /* {{{ Returns whether this property is default (declared at compilation time). */ | |||
| ZEND_METHOD(ReflectionProperty, isDefault) | |||
| static void _property_check_dynamic(INTERNAL_FUNCTION_PARAMETERS, bool dynamicTrue) /* {{{ */ | |||
There was a problem hiding this comment.
nit: feel free to omit the {{{ and /* }}} */ tags
There was a problem hiding this comment.
This parameter name is more in line with the conventions:
| static void _property_check_dynamic(INTERNAL_FUNCTION_PARAMETERS, bool dynamicTrue) /* {{{ */ | |
| static void _property_check_dynamic(INTERNAL_FUNCTION_PARAMETERS, bool is_dynamic) /* {{{ */ |
There was a problem hiding this comment.
I think that might get confusing since I use isDynamic to hold the result of checking if the property is dynamic a few lines lower down - how about dynamic_true?
There was a problem hiding this comment.
Hm, ok, it's ok as long as you consistently use the camel_case style ^^ At least, most code in php-src uses this convention so the camelCase looks a bit weird for me here.
There was a problem hiding this comment.
Isn't camelCase with the capitals, and snake_case what is used?
There was a problem hiding this comment.
Isn't camelCase with the capitals, and snake_case what is used?
Sorry, I'm not in my sharpest form today
There was a problem hiding this comment.
And again, thank you, I support this feature very much, the original method name is very misleading. I'd say that the change is pretty much self contained, but RMs should approve it nevertheless. Please also write an upgrading note once you have the green light.
|
This feature is small and self-contained and should be approved by RMs @php/release-managers-84 |
|
@DanielEScherzer Now, only the UPGRADING note is missing before I can merge this :) |
Dynamic properties are generally referred to as "dynamic" properties, while non-dynamic properties are not commonly referred to as "default" properties. Thus, the existing method `ReflectionProperty::isDefault()` has a non obvious name; while an alias could be added for `isNotDynamic()`, a new `isDynamic()` method seems cleaner. The new method returns the opposite of `isDefault()`; dynamic properties are not present on the class by default, and properties present by default are not added dynamically. Closes phpGH-15754
DanielEScherzer
force-pushed
the
reflection-dynamic
branch
from
27d00be to
76b8b8f
Compare
Done, along with a rebase so that there wouldn't be merge conflicts |
|
Thank you! |
Dynamic properties are generally referred to as "dynamic" properties, while non-dynamic properties are not commonly referred to as "default" properties. Thus, the existing method
ReflectionProperty::isDefault()has a non obvious name; while an alias could be added forisNotDynamic(), a newisDynamic()method seems cleaner. The new method returns the opposite ofisDefault(); dynamic properties are not present on the class by default, and properties present by default are not added dynamically.Closes GH-15754