Hi:
I have to say, -1 on this pull.

1. I didn't see much different between your patch with pierrick's
   patch, PHP is a open-source project, but you also should respect
   others work, that means, if you do some thing based on others work,
   you should ask for his agree or note his name on somewhere, in this
   case, it should be in the NEWS.
2. I really don't like throw exception anywhere, people have to
   try catch anywhere, although they only what a true or false return.

thanks

On Tue, Apr 3, 2012 at 1:55 PM, reeze
[email protected]
wrote:

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.

You can merge this Pull Request by running:

 git pull https://github.com/reeze/php-src add-const-name

Or you can view, comment on it, or merge it online at:

 #35

-- Commit Summary --

• Implemented FR #61602 Allow access to name of constant used as default value

-- File Changes --

M NEWS (2)
M ext/reflection/php_reflection.c (104)
A ext/reflection/tests/ReflectionParameter_DefaultValueConstant_basic1.phpt (52)
A ext/reflection/tests/ReflectionParameter_DefaultValueConstant_basic2.phpt (30)
A ext/reflection/tests/ReflectionParameter_DefaultValueConstant_error.phpt (25)

-- Patch Links --

 https://github.com/php/php-src/pull/35.patch
 https://github.com/php/php-src/pull/35.diff

---

Reply to this email directly or view it on GitHub:
#35

Git Pull Requests Mailing List (https://github.com/php)
To unsubscribe, visit: http://www.php.net/unsub.php

Laruence  Xinchen Hui
http://www.laruence.com/

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.

On Tue, Apr 3, 2012 at 4:02 PM, reeze
[email protected]
wrote:

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.

so you are saying , you patch is earlier than pirrerrick's? you must
be kidding me...

let's see your patch with pirrerrick's :

yours https://github.com/reeze/php-src/blob/054f3e3ce5af13c2c3a6ccd54f7dc3e2f6cd4f74/ext/reflection/php_reflection.c
line 1470:

GET_REFLECTION_OBJECT_PTR(param);

if (param->fptr->type != ZEND_USER_FUNCTION) {
zend_throw_exception_ex(reflection_exception_ptr, 0 TSRMLS_CC,
"Cannot determine default value for internal functions");
return NULL;
}

if (param->offset < param->required) {      
zend_throw_exception_ex(reflection_exception_ptr, 0 TSRMLS_CC,

"Parameter is not optional");
return NULL;
}

pierrick's https://bugs.php.net/patch-display.php?bug_id=61602&patch=getDefaultValueConstantName.diff&revision=latest:

• if (param->fptr->type != ZEND_USER_FUNCTION)
• {

•   zend_throw_exception_ex(reflection_exception_ptr, 0 TSRMLS_CC,
  

  "Cannot determine default value for internal functions");

•   return;
  

• }
• if (param->offset < param->required) {

•   zend_throw_exception_ex(reflection_exception_ptr, 0 TSRMLS_CC,
  

  "Parameter is not optional");

•   return;
  

• }

and another, your patch, line 1501:
precv = _get_recv_op((zend_op_array*)param->fptr, param->offset);
if (!precv || precv->opcode != ZEND_RECV_INIT || precv->op2_type ==
IS_UNUSED) {
zend_throw_exception_ex(reflection_exception_ptr, 0 TSRMLS_CC,
"Internal error");
return NULL;
}

pierrick 's :

• precv = _get_recv_op((zend_op_array*)param->fptr, param->offset);
• if (!precv || precv->opcode != ZEND_RECV_INIT || precv->op2_type ==
  IS_UNUSED) {

•   zend_throw_exception_ex(reflection_exception_ptr, 0 TSRMLS_CC,
  

  "Internal error");

•   return;
  

• }

you know what ? the are totally the same!

thanks

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.

---

Reply to this email directly or view it on GitHub:
#35 (comment)

Git Pull Requests Mailing List (https://github.com/php)
To unsubscribe, visit: http://www.php.net/unsub.php

Laruence  Xinchen Hui
http://www.laruence.com/

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.

hi：
You words now make sense to me, i maybe misunderstanded you,
sorry for that.

 Now I think leave it to somebody else to review.

Thanks

Sent from my iPad

在 2012-4-3，18:00，reeze
[email protected]
写道：

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.

---

Reply to this email directly or view it on GitHub:
#35 (comment)

Git Pull Requests Mailing List (https://github.com/php)
To unsubscribe, visit: http://www.php.net/unsub.php