Skip to content

Implemented FR #61602 Allow access to name of constant used as default v... #35

Merged
merged 2 commits into from May 15, 2012

3 participants

@reeze
reeze commented Apr 3, 2012

Feature request from: https://bugs.php.net/bug.php?id=61602

Hi , I've impled this FR, it differs from pierrick's :

  • rename defaultValueIsConstant to isDefaultValueConstant to match bool return values's is* method naming.
  • handle global constant.
  • refactor code to remove duplicate.
  • add 3 tests file for it.

and I've make test everything looks fine.

thanks.

@php-pulls
@reeze
reeze commented Apr 3, 2012

Hi, thanks for you reply

1, before I try to imply the FR request pierrick havn't attach the patch. Last night I havn't wrote tests file , today I made some improvement and sent the pull request, at the same time a wrote a comment on https://bugs.php.net/bug.php?id=61602 and ask pierrick to review it for me. Sure it's ok to credit pirrerrick, we all want php to be a better language. I will add him to NEWS.

Update: short for long : I did't make the patch based on pirrerrick's original patch.

  1. As for exception throwing I want it to consist with getDefaultValue() and etc in Reflection extension. http://cn.php.net/manual/en/reflectionparameter.getdefaultvalue.php.
@php-pulls
@reeze
reeze commented Apr 3, 2012

Your totally miss my point. I don't ever mention my patch is earlier submited than pierrick. I said it, bugs comment log tells either. I mean I did't make my patch based on pierrick. that's it.

if your look the original ext/reflection/php_reflection.c: ZEND_METHOD getDefaultValue() you will find that, yes it totally the same. I guess pierrick do the same as me: copy the getDefaultValue() implement and modify to our own suit.

I do the same, but find duplicated the same code over and over will make code smelly, then I refactor them.

For me: got the Request -> try to implement it -> find I can make use of getDefaultValue()->refactor->add Test file.

May be pierrick would say something.

Thanks.

@php-pulls
@nikic nikic and 1 other commented on an outdated diff Apr 3, 2012
ext/reflection/php_reflection.c
+ return NULL;
+ }
+
+ return param;
+}
+/* }}} */
+
+/* {{{ _reflection_param_get_default_precv */
+static zend_op *_reflection_param_get_default_precv(INTERNAL_FUNCTION_PARAMETERS)
+{
+ zend_op *precv;
+ parameter_reference *param = _reflection_param_get_default_param(INTERNAL_FUNCTION_PARAM_PASSTHRU);
+
+ if (zend_parse_parameters_none() == FAILURE) {
+ return NULL;
+ }
@nikic
nikic added a note Apr 3, 2012

You are doing this check already in _reflection_param_get_default_param. But in any case I'm not sure that this check should be in any of these functions. It's normally always done in the main function/method.

@reeze
reeze added a note Apr 4, 2012

Thank your for your review, I've updated the code, follow coding standard and move parameter parse to main function, and remove a duplicate function call. it should be better now. thanks ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nikic nikic commented on an outdated diff Apr 3, 2012
ext/reflection/php_reflection.c
+
+ return param;
+}
+/* }}} */
+
+/* {{{ _reflection_param_get_default_precv */
+static zend_op *_reflection_param_get_default_precv(INTERNAL_FUNCTION_PARAMETERS)
+{
+ zend_op *precv;
+ parameter_reference *param = _reflection_param_get_default_param(INTERNAL_FUNCTION_PARAM_PASSTHRU);
+
+ if (zend_parse_parameters_none() == FAILURE) {
+ return NULL;
+ }
+
+ if(!param) {
@nikic
nikic added a note Apr 3, 2012

*nit There should be a space after the if :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@php-pulls php-pulls merged commit 6712d0d into php:master May 15, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.