Re: [PATCH] Closures and reflection improvements

Hello Christian, Johannes, Friday, August 8, 2008, 11:48:37 PM, you wrote: > Hello Christian, > I updated your patch for 5.3 due to recent changes. It works pretty fine > for me. Care to test again, especially with your new tests? Note that test > ext/reflection/tests/closures_001.phpt does not work for me and I have no > clue why. Well, besides that obviously the closure class no longer has an > entry for __closure, so it is more a question of how to know when to add > this. Also, it appears you do not have a cvs account, or did I overlook > something? > Modified patch attached, including the updated reflection tests. Ok, it is actually easy to fix, we just check whether the thing is a closure and add the method manually, done. New patch attached. Johannes, this also adresses the switch from E_ERROR to E_RECOVERABLE_ERROR for the closure handlers. best regards marcus > Monday, August 4, 2008, 12:59:13 PM, you wrote: >> Christian, >> On Mon, 2008-08-04 at 11:33 +0400, Dmitry Stogov wrote: >>> >> >>> >> http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-5.3.patch >>> >> >>> >> >>> >> >>> >> http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-6.patch >>> >> >>> > >> without applying and testing the patch: >> - zend_hash_apply_with_arguments(&ce->function_table TSRMLS_CC, >> (apply_func_args_t) _addmethod, 3, &ce, return_value, filter); >> + zend_hash_apply_with_arguments(&ce->function_table TSRMLS_CC, >> (apply_func_args_t) _addmethod, 3, &ce, return_value, filter, intern->obj); >> that looks a bit strange, probably you want 4 instead of 3 now? >> johannes > Best regards, > Marcus Best regards, Marcus

Index: Zend/zend_closures.c =================================================================== RCS file: /repository/ZendEngine2/zend_closures.c,v retrieving revision 1.3.2.10 diff -u -p -d -r1.3.2.10 zend_closures.c --- Zend/zend_closures.c 7 Aug 2008 13:35:51 -0000 1.3.2.10 +++ Zend/zend_closures.c 8 Aug 2008 22:31:51 -0000 @@ -26,11 +26,10 @@ #include "zend_objects_API.h" #include "zend_globals.h" -#define ZEND_INVOKE_FUNC_NAME "__invoke" #define ZEND_CLOSURE_PRINT_NAME "Closure object" #define ZEND_CLOSURE_PROPERTY_ERROR() \ - zend_error(E_ERROR, "Closure object cannot have properties") + zend_error(E_RECOVERABLE_ERROR, "Closure object cannot have properties") typedef struct _zend_closure { zend_object std; @@ -38,7 +37,8 @@ typedef struct _zend_closure { zval *this_ptr; } zend_closure; -static zend_class_entry *zend_ce_closure; +/* non-static since it needs to be referenced */ +ZEND_API zend_class_entry *zend_ce_closure; static zend_object_handlers closure_handlers; ZEND_METHOD(Closure, __invoke) /* {{{ */ @@ -50,7 +50,7 @@ ZEND_METHOD(Closure, __invoke) /* {{{ */ arguments = emalloc(sizeof(zval**) * ZEND_NUM_ARGS()); if (zend_get_parameters_array_ex(ZEND_NUM_ARGS(), arguments) == FAILURE) { efree(arguments); - zend_error(E_ERROR, "Cannot get arguments for calling closure"); + zend_error(E_RECOVERABLE_ERROR, "Cannot get arguments for calling closure"); RETVAL_FALSE; } else if (call_user_function_ex(CG(function_table), NULL, this_ptr, &closure_result_ptr, ZEND_NUM_ARGS(), arguments, 1, NULL TSRMLS_CC) == FAILURE) { RETVAL_FALSE; @@ -74,21 +74,21 @@ ZEND_METHOD(Closure, __invoke) /* {{{ */ static zend_function *zend_closure_get_constructor(zval *object TSRMLS_DC) /* {{{ */ { - zend_error(E_ERROR, "Instantiation of 'Closure' is not allowed"); + zend_error(E_RECOVERABLE_ERROR, "Instantiation of 'Closure' is not allowed"); return NULL; } /* }}} */ static int zend_closure_serialize(zval *object, unsigned char **buffer, zend_uint *buf_len, zend_serialize_data *data TSRMLS_DC) /* {{{ */ { - zend_error(E_ERROR, "Serialization of 'Closure' is not allowed"); + zend_error(E_RECOVERABLE_ERROR, "Serialization of 'Closure' is not allowed"); return FAILURE; } /* }}} */ static int zend_closure_unserialize(zval **object, zend_class_entry *ce, const unsigned char *buf, zend_uint buf_len, zend_unserialize_data *data TSRMLS_DC) /* {{{ */ { - zend_error(E_ERROR, "Unserialization of 'Closure' is not allowed"); + zend_error(E_RECOVERABLE_ERROR, "Unserialization of 'Closure' is not allowed"); return FAILURE; } /* }}} */ @@ -99,6 +99,24 @@ static int zend_closure_compare_objects( } /* }}} */ +ZEND_API zend_function *zend_get_closure_invoke_method(zval *obj TSRMLS_DC) /* {{{ */ +{ + zend_closure *closure = (zend_closure *)zend_object_store_get_object(obj TSRMLS_CC); + zend_function *invoke = (zend_function*)emalloc(sizeof(zend_function)); + + invoke = (zend_function*)emalloc(sizeof(zend_function)); + invoke->common = closure->func.common; + invoke->type = ZEND_INTERNAL_FUNCTION; + invoke->internal_function.handler = ZEND_MN(Closure___invoke); + invoke->internal_function.module = 0; + invoke->internal_function.scope = zend_ce_closure; + invoke->internal_function.function_name = estrndup(ZEND_INVOKE_FUNC_NAME, sizeof(ZEND_INVOKE_FUNC_NAME)-1); + invoke->internal_function.fn_flags = ZEND_ACC_PUBLIC | ZEND_ACC_CALL_VIA_HANDLER; + return invoke; + +} +/* }}} */ + static zend_function *zend_closure_get_method(zval **object_ptr, char *method_name, int method_len TSRMLS_DC) /* {{{ */ { char *lc_name; @@ -109,18 +127,8 @@ static zend_function *zend_closure_get_m if ((method_len == sizeof(ZEND_INVOKE_FUNC_NAME)-1) && memcmp(lc_name, ZEND_INVOKE_FUNC_NAME, sizeof(ZEND_INVOKE_FUNC_NAME)-1) == 0 ) { - zend_closure *closure = (zend_closure *)zend_object_store_get_object(*object_ptr TSRMLS_CC); - zend_function *invoke = (zend_function*)emalloc(sizeof(zend_function)); - - invoke->common = closure->func.common; - invoke->type = ZEND_INTERNAL_FUNCTION; - invoke->internal_function.fn_flags = ZEND_ACC_CALL_VIA_HANDLER; - invoke->internal_function.handler = ZEND_MN(Closure___invoke); - invoke->internal_function.module = 0; - invoke->internal_function.scope = zend_ce_closure; - invoke->internal_function.function_name = estrndup(ZEND_INVOKE_FUNC_NAME, sizeof(ZEND_INVOKE_FUNC_NAME)-1); free_alloca(lc_name, use_heap); - return invoke; + return zend_get_closure_invoke_method(*object_ptr TSRMLS_CC); } free_alloca(lc_name, use_heap); return NULL; @@ -170,7 +178,7 @@ static void zend_closure_free_storage(vo zend_execute_data *ex = EG(current_execute_data); while (ex) { if (ex->op_array == &closure->func.op_array) { - zend_error(E_ERROR, "Cannot destroy active lambda function"); + zend_error(E_RECOVERABLE_ERROR, "Cannot destroy active lambda function"); } ex = ex->prev_execute_data; } Index: Zend/zend_closures.h =================================================================== RCS file: /repository/ZendEngine2/zend_closures.h,v retrieving revision 1.1.2.2 diff -u -p -d -r1.1.2.2 zend_closures.h --- Zend/zend_closures.h 14 Jul 2008 09:48:59 -0000 1.1.2.2 +++ Zend/zend_closures.h 8 Aug 2008 22:31:51 -0000 @@ -24,10 +24,15 @@ BEGIN_EXTERN_C() +#define ZEND_INVOKE_FUNC_NAME "__invoke" + void zend_register_closure_ce(TSRMLS_D); +extern ZEND_API zend_class_entry *zend_ce_closure; + ZEND_API void zend_create_closure(zval *res, zend_function *op_array, zend_class_entry *scope, zval *this_ptr TSRMLS_DC); ZEND_API int zend_get_closure(zval *obj, zend_class_entry **ce_ptr, zend_function **fptr_ptr, zval **zobj_ptr, zval ***zobj_ptr_ptr TSRMLS_DC); +ZEND_API zend_function *zend_get_closure_invoke_method(zval *obj TSRMLS_DC); END_EXTERN_C() Index: ext/reflection/php_reflection.c =================================================================== RCS file: /repository/php-src/ext/reflection/php_reflection.c,v retrieving revision 1.164.2.33.2.45.2.27 diff -u -p -d -r1.164.2.33.2.45.2.27 php_reflection.c --- ext/reflection/php_reflection.c 8 Aug 2008 10:52:48 -0000 1.164.2.33.2.45.2.27 +++ ext/reflection/php_reflection.c 8 Aug 2008 22:31:53 -0000 @@ -536,6 +536,14 @@ static void _class_string(string *str, z zend_hash_get_current_key_ex(&ce->function_table, &key, &key_len, &num_index, 0, &pos) != HASH_KEY_IS_STRING || zend_binary_strcasecmp(key, key_len-1, mptr->common.function_name, len) == 0) { + zend_function *closure_mptr; + /* see if this is a closure */ + if (ce == zend_ce_closure && obj && (len == sizeof(ZEND_INVOKE_FUNC_NAME)-1) && + memcmp(mptr->common.function_name, ZEND_INVOKE_FUNC_NAME, sizeof(ZEND_INVOKE_FUNC_NAME)-1) == 0 && + (closure_mptr = zend_get_closure_invoke_method(obj TSRMLS_CC)) != NULL + ) { + mptr = closure_mptr; + } string_printf(&dyn, "\n"); _function_string(&dyn, mptr, ce, sub_indent.string TSRMLS_CC); count++; @@ -1890,15 +1898,34 @@ ZEND_METHOD(reflection_parameter, __cons convert_to_string_ex(method); lcname_len = Z_STRLEN_PP(method); lcname = zend_str_tolower_dup(Z_STRVAL_PP(method), lcname_len); - if (zend_hash_find(&ce->function_table, lcname, lcname_len + 1, (void **) &fptr) == FAILURE) { + if (ce == zend_ce_closure && Z_TYPE_PP(classref) == IS_OBJECT && + (lcname_len == sizeof(ZEND_INVOKE_FUNC_NAME)-1) && + memcmp(lcname, ZEND_INVOKE_FUNC_NAME, sizeof(ZEND_INVOKE_FUNC_NAME)-1) == 0 && + (fptr = zend_get_closure_invoke_method(*classref TSRMLS_CC)) != NULL + ) { + /* do nothing, fptr is already set */ + } else if (zend_hash_find(&ce->function_table, lcname, lcname_len + 1, (void **) &fptr) == FAILURE) { efree(lcname); zend_throw_exception_ex(reflection_exception_ptr, 0 TSRMLS_CC, - "Method %s::%s() does not exist", Z_STRVAL_PP(classref), Z_TYPE_PP(method), Z_STRVAL_PP(method)); + "Method %s::%s() does not exist", ce->name, Z_STRVAL_PP(method)); return; } efree(lcname); } break; + + case IS_OBJECT: { + ce = Z_OBJCE_P(reference); + + if (ce == zend_ce_closure && (fptr = zend_get_closure_invoke_method(reference TSRMLS_CC)) != NULL) { + /* do nothing, fptr is already set */ + } else if (zend_hash_find(&ce->function_table, ZEND_INVOKE_FUNC_NAME, sizeof(ZEND_INVOKE_FUNC_NAME), (void **)&fptr) == FAILURE) { + zend_throw_exception_ex(reflection_exception_ptr, 0 TSRMLS_CC, + "Method %s::%s() does not exist", ce->name, ZEND_INVOKE_FUNC_NAME); + return; + } + } + break; default: _DO_THROW("The parameter class is expected to be either a string or an array(class, method)"); @@ -2207,7 +2234,7 @@ ZEND_METHOD(reflection_method, export) ZEND_METHOD(reflection_method, __construct) { zval *name, *classname; - zval *object; + zval *object, *orig_obj; reflection_object *intern; char *lcname; zend_class_entry **pce; @@ -2217,7 +2244,11 @@ ZEND_METHOD(reflection_method, __constru int name_len, tmp_len; zval ztmp; - if (zend_parse_parameters_ex(ZEND_PARSE_PARAMS_QUIET, ZEND_NUM_ARGS() TSRMLS_CC, "zs", &classname, &name_str, &name_len) == FAILURE) { + if (zend_parse_parameters_ex(ZEND_PARSE_PARAMS_QUIET, ZEND_NUM_ARGS() TSRMLS_CC, "o", &classname) == SUCCESS) { + name_str = ZEND_INVOKE_FUNC_NAME; + name_len = sizeof(ZEND_INVOKE_FUNC_NAME)-1; + orig_obj = classname; + } else if (zend_parse_parameters_ex(ZEND_PARSE_PARAMS_QUIET, ZEND_NUM_ARGS() TSRMLS_CC, "zs", &classname, &name_str, &name_len) == FAILURE) { if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &name_str, &name_len) == FAILURE) { return; } @@ -2230,6 +2261,11 @@ ZEND_METHOD(reflection_method, __constru ZVAL_STRINGL(classname, name_str, tmp_len, 1); name_len = name_len - (tmp_len + 2); name_str = tmp + 2; + orig_obj = NULL; + } else if (Z_TYPE_P(classname) == IS_OBJECT) { + orig_obj = classname; + } else { + orig_obj = NULL; } object = getThis(); @@ -2275,7 +2311,12 @@ ZEND_METHOD(reflection_method, __constru lcname = zend_str_tolower_dup(name_str, name_len); - if (zend_hash_find(&ce->function_table, lcname, name_len + 1, (void **) &mptr) == FAILURE) { + if (ce == zend_ce_closure && orig_obj && (name_len == sizeof(ZEND_INVOKE_FUNC_NAME)-1) && + memcmp(lcname, ZEND_INVOKE_FUNC_NAME, sizeof(ZEND_INVOKE_FUNC_NAME)-1) == 0 && + (mptr = zend_get_closure_invoke_method(orig_obj TSRMLS_CC)) != NULL + ) { + /* do nothing, mptr already set */ + } else if (zend_hash_find(&ce->function_table, lcname, name_len + 1, (void **) &mptr) == FAILURE) { efree(lcname); zend_throw_exception_ex(reflection_exception_ptr, 0 TSRMLS_CC, "Method %s::%s() does not exist", ce->name, name_str); @@ -3139,7 +3180,13 @@ ZEND_METHOD(reflection_class, getMethod) GET_REFLECTION_OBJECT_PTR(ce); lc_name = zend_str_tolower_dup(name, name_len); - if (zend_hash_find(&ce->function_table, lc_name, name_len + 1, (void**) &mptr) == SUCCESS) { + if (ce == zend_ce_closure && intern->obj && (name_len == sizeof(ZEND_INVOKE_FUNC_NAME)-1) && + memcmp(lc_name, ZEND_INVOKE_FUNC_NAME, sizeof(ZEND_INVOKE_FUNC_NAME)-1) == 0 && + (mptr = zend_get_closure_invoke_method(intern->obj TSRMLS_CC)) != NULL + ) { + reflection_method_factory(ce, mptr, return_value TSRMLS_CC); + efree(lc_name); + } else if (zend_hash_find(&ce->function_table, lc_name, name_len + 1, (void**) &mptr) == SUCCESS) { reflection_method_factory(ce, mptr, return_value TSRMLS_CC); efree(lc_name); } else { @@ -3152,19 +3199,36 @@ ZEND_METHOD(reflection_class, getMethod) /* }}} */ /* {{{ _addmethod */ -static int _addmethod(zend_function *mptr TSRMLS_DC, int num_args, va_list args, zend_hash_key *hash_key) +static void _addmethod(zend_function *mptr, zend_class_entry *ce, zval *retval, long filter, zval *obj TSRMLS_DC) { zval *method; - zend_class_entry *ce = *va_arg(args, zend_class_entry**); - zval *retval = va_arg(args, zval*); - long filter = va_arg(args, long); + uint len = strlen(mptr->common.function_name); + zend_function *closure_mptr; if (mptr->common.fn_flags & filter) { ALLOC_ZVAL(method); + if (ce == zend_ce_closure && obj && (len == sizeof(ZEND_INVOKE_FUNC_NAME)-1) && + memcmp(mptr->common.function_name, ZEND_INVOKE_FUNC_NAME, sizeof(ZEND_INVOKE_FUNC_NAME)-1) == 0 && + (closure_mptr = zend_get_closure_invoke_method(obj TSRMLS_CC)) != NULL + ) { + mptr = closure_mptr; + } reflection_method_factory(ce, mptr, method TSRMLS_CC); add_next_index_zval(retval, method); } - return 0; +} +/* }}} */ + +/* {{{ _addmethod */ +static int _addmethod_va(zend_function *mptr TSRMLS_DC, int num_args, va_list args, zend_hash_key *hash_key) +{ + zend_class_entry *ce = *va_arg(args, zend_class_entry**); + zval *retval = va_arg(args, zval*); + long filter = va_arg(args, long); + zval *obj = va_arg(args, zval *); + + _addmethod(mptr, ce, retval, filter, obj TSRMLS_CC); + return ZEND_HASH_APPLY_KEEP; } /* }}} */ @@ -3190,7 +3254,16 @@ ZEND_METHOD(reflection_class, getMethods GET_REFLECTION_OBJECT_PTR(ce); array_init(return_value); - zend_hash_apply_with_arguments(&ce->function_table TSRMLS_CC, (apply_func_args_t) _addmethod, 3, &ce, return_value, filter); + zend_hash_apply_with_arguments(&ce->function_table TSRMLS_CC, (apply_func_args_t) _addmethod_va, 4, &ce, return_value, filter, intern->obj); + if (intern->obj && instanceof_function(ce, zend_ce_closure)) { + zend_function *closure = zend_get_closure_invoke_method(intern->obj TSRMLS_CC); + if (closure) { + _addmethod(closure, ce, return_value, filter, intern->obj TSRMLS_CC); + if (closure->type != ZEND_USER_FUNCTION && (closure->internal_function.fn_flags & ZEND_ACC_CALL_VIA_HANDLER) != 0) { + efree(closure); + } + } + } } /* }}} */ Index: ext/reflection/tests/closures_001.phpt =================================================================== RCS file: ext/reflection/tests/closures_001.phpt diff -N ext/reflection/tests/closures_001.phpt --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ ext/reflection/tests/closures_001.phpt 8 Aug 2008 22:31:53 -0000 @@ -0,0 +1,70 @@ +--TEST-- +Reflection on closures +--FILE-- +<?php + +$closure = function ($a, $b = 0) { }; + +$ro = new ReflectionObject ($closure); +$rm = $ro->getMethod ('__invoke'); +var_dump ($rm->getNumberOfParameters()); +var_dump ($rm->getNumberOfRequiredParameters()); +$rms = $ro->getMethods(); +foreach ($rms as $rm) { + if ($rm->getName () == '__invoke') { + var_dump ($rm->getNumberOfParameters()); + var_dump ($rm->getNumberOfRequiredParameters()); + } +} + +echo "---\n"; + +$rm = new ReflectionMethod ($closure); +var_dump ($rm->getName ()); +var_dump ($rm->getNumberOfParameters()); +var_dump ($rm->getNumberOfRequiredParameters()); + +echo "---\n"; + +$rp = new ReflectionParameter (array ($closure, '__invoke'), 0); +var_dump ($rp->isOptional ()); +$rp = new ReflectionParameter (array ($closure, '__invoke'), 1); +var_dump ($rp->isOptional ()); +$rp = new ReflectionParameter (array ($closure, '__invoke'), 'a'); +var_dump ($rp->isOptional ()); +$rp = new ReflectionParameter (array ($closure, '__invoke'), 'b'); +var_dump ($rp->isOptional ()); + +echo "---\n"; + +$rp = new ReflectionParameter ($closure, 0); +var_dump ($rp->isOptional ()); +$rp = new ReflectionParameter ($closure, 1); +var_dump ($rp->isOptional ()); +$rp = new ReflectionParameter ($closure, 'a'); +var_dump ($rp->isOptional ()); +$rp = new ReflectionParameter ($closure, 'b'); +var_dump ($rp->isOptional ()); + +?> +===DONE=== +--EXPECTF-- +int(2) +int(1) +int(2) +int(1) +--- +string(8) "__invoke" +int(2) +int(1) +--- +bool(false) +bool(true) +bool(false) +bool(true) +--- +bool(false) +bool(true) +bool(false) +bool(true) +===DONE=== Index: ext/reflection/tests/closures_002.phpt =================================================================== RCS file: ext/reflection/tests/closures_002.phpt diff -N ext/reflection/tests/closures_002.phpt --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ ext/reflection/tests/closures_002.phpt 8 Aug 2008 22:31:53 -0000 @@ -0,0 +1,25 @@ +--TEST-- +Reflection on invokable objects +--FILE-- +<?php + +$rm = new ReflectionMethod (new C); +var_dump ($rm->getName ()); +var_dump ($rm->getNumberOfParameters()); +var_dump ($rm->getNumberOfRequiredParameters()); + +$rp = new ReflectionParameter (new C, 0); +var_dump ($rp->isOptional ()); + +class C { + function __invoke ($a) { } +} + +?> +===DONE=== +--EXPECTF-- +string(8) "__invoke" +int(1) +int(1) +bool(false) +===DONE=== Index: Zend/tests/closure_017.phpt =================================================================== RCS file: /repository/ZendEngine2/tests/closure_017.phpt,v retrieving revision 1.1.2.1 diff -u -p -d -r1.1.2.1 closure_017.phpt --- Zend/tests/closure_017.phpt 14 Jul 2008 13:36:40 -0000 1.1.2.1 +++ Zend/tests/closure_017.phpt 8 Aug 2008 22:31:53 -0000 @@ -9,4 +9,4 @@ $a($a); ?> --EXPECTF-- -Fatal error: Cannot destroy active lambda function in %s on line %d +Catchable fatal error: Cannot destroy active lambda function in %s on line %d Index: Zend/tests/closure_022.phpt =================================================================== RCS file: /repository/ZendEngine2/tests/closure_022.phpt,v retrieving revision 1.1.2.2 diff -u -p -d -r1.1.2.2 closure_022.phpt --- Zend/tests/closure_022.phpt 22 Jul 2008 07:29:31 -0000 1.1.2.2 +++ Zend/tests/closure_022.phpt 8 Aug 2008 22:31:53 -0000 @@ -8,5 +8,5 @@ $foo = function() use ($a) { $foo->a = 1; ?> --EXPECTF-- -Fatal error: Closure object cannot have properties in %sclosure_022.php on line 5 +Catchable fatal error: Closure object cannot have properties in %sclosure_022.php on line 5