Re: com php-src: Fix bug #68446 (bug with constant defaults and type hints): Zend/tests/class_constants_002.phpt Zend/zend_compile.c
Zend/zend_execute.c Zend/zend_vm_def.h Zend/zend_vm_execute.h

On Mon, Nov 24, 2014 at 2:04 PM, Bob Weinand <bobwei9@hotmail.com> wrote: > Hey Dmitry, > > My changes shouldn't break code which isn't already fragile. > class_constants_002.phpt is such an example. > This is definitely a break php-5.5 -n -r 'function foo($x=Foo::Bar){var_dump($x);} foo(10);' int(10) php-5.6 -n -r 'function foo($x=Foo::Bar){var_dump($x);} foo(10);' Fatal error: Class 'Foo' not found in Command line code on line 1 In some framework code it'll trigger Foo autoloading where it didn't do it before. Though, I could restore old behavior and just evaluate in case a constant > is passed _and_ there is an array/callable typehint. Would that be fine? > In this case there wouldn't be any BC break (only already fatal-ing code > working again). > This BC break is not the single problem in the patch, I actually noticed it because of slight performance degradation. Also, could you explain this change: allow_null is defined as zend_bool, but now it can be initialized with 1 or 2? @@ -1931,8 +1931,8 @@ void zend_do_receive_param(zend_uchar op, znode *varname, const znode *initializ if (class_type->u.constant.type == IS_ARRAY) { cur_arg_info->type_hint = IS_ARRAY; if (op == ZEND_RECV_INIT) { - if (Z_TYPE(initialization->u.constant) == IS_NULL || (Z_TYPE(initialization->u.constant) == IS_CONSTANT && !strcasecmp(Z_STRVAL(initialization->u.constant), "NULL")) || Z_TYPE(initialization->u.constant) == IS_CONSTANT_AST) { - cur_arg_info->allow_null = 1; + if (Z_TYPE(initialization->u.constant) == IS_NULL || (Z_TYPE(initialization->u.constant) & IS_CONSTANT_TYPE_MASK) == IS_CONSTANT || (Z_TYPE(initialization->u.constant) & IS_CONSTANT_TYPE_MASK) == IS_CONSTANT_AST) { + cur_arg_info->allow_null = (Z_TYPE(initialization->u.constant) != IS_NULL) + 1; } else if (Z_TYPE(initialization->u.constant) != IS_ARRAY) { zend_error_noreturn(E_COMPILE_ERROR, "Default value for parameters with array type hint can only be an array or NULL"); } I would prefer, if you revert your patch, and then propose better solution (without BC break). you patch is not simple, and analyzing one fix on top of another is not going to be simple. Thanks. Dmitry. > > Thanks, > Bob > > Am 24.11.2014 um 09:26 schrieb Dmitry Stogov <dmitry@zend.com>: > > Hi Bob, > > Please, send such changes for review before committing. > > It is not a trivial change and it breaks existing behavior > (Zend/tests/class_constants_002.phpt). > Default values don't have to be evaluated, if actual parameter was sent. > Your patch may affect existing code in unpredictable way (e.g. start fail > because of error or trigger __autoload()). > Please, revert it. Such changes can't be done in minor releases. > Probably, the bug may be fixed in another way. > > Thanks. Dmitry. > > On Sun, Nov 23, 2014 at 11:10 PM, Bob Weinand <bwoebi@php.net> wrote: > >> Commit: 5ef138b0c7c4e9532e205f45c18a72aa1d279c24 >> Author: Bob Weinand <bobwei9@hotmail.com> Sun, 23 Nov 2014 >> 21:09:31 +0100 >> Parents: c8dd41554387e100a09811cad7f7032a291a79c2 >> Branches: PHP-5.6 >> >> Link: >> >> http://git.php.net/?p=php-src.git;a=commitdiff;h=5ef138b0c7c4e9532e205f45c18a72aa1d279c24 >> >> Log: >> Fix bug #68446 (bug with constant defaults and type hints) >> >> Bugs: >> https://bugs.php.net/68446 >> >> Changed paths: >> M Zend/tests/class_constants_002.phpt >> M Zend/zend_compile.c >> M Zend/zend_execute.c >> M Zend/zend_vm_def.h >> M Zend/zend_vm_execute.h >> >> >> -- >> PHP CVS Mailing List (http://www.php.net/) >> To unsubscribe, visit: http://www.php.net/unsub.php >> > > >