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

Hi Bob, Thanks. Please revert it for master as well. Once you find a good fix you may commit it into 5.6 and master again. Otherwise we will get a mess. If you like, I may try to look into the problem and try to fix it as well. Thanks. Dmitry. On Tue, Nov 25, 2014 at 2:03 PM, Bob Weinand <bobwei9@hotmail.com> wrote: > 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> 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>: >> >> 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 >>> >> >> >> > >