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

> Am 24.11.2014 um 13:12 schrieb Dmitry Stogov <dmitry@zend.com>: > > On Mon, Nov 24, 2014 at 2:04 PM, Bob Weinand <bobwei9@hotmail.com > <mailto: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. It's a behavior change, but it shouldn't break code. But I agree, that's bad for a micro release. > 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. I somehow did something better when reimplementing for master, there class_constants_002.phpt is still the same and passes. I just don't understand _why_... I wonder now if we should leave that bug in 5.6 (and revert it there) or try to fix it there too? I will revert it today (because of 5.6.4rc1) if I don't get another reply from you today. Maybe you're able to quickly get what the difference between 5.6 and master is there? Bob > Thanks. Dmitry. > > > Thanks, > Bob > >> Am 24.11.2014 um 09:26 schrieb Dmitry Stogov <dmitry@zend.com >> <mailto: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 >> <mailto:bwoebi@php.net>> wrote: >> Commit: 5ef138b0c7c4e9532e205f45c18a72aa1d279c24 >> Author: Bob Weinand <bobwei9@hotmail.com >> <mailto: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 >> <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 >> <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/ >> <http://www.php.net/>) >> To unsubscribe, visit: http://www.php.net/unsub.php >> <http://www.php.net/unsub.php> >> > >