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

Hey, Reverted in 5.6. I now didn't revert master, especially as that gets a completely merge mess to revert. Master is basically a separate re-implementation which seems to work without breaks. That way also, when we merge up, we need to anyway throw away the PHP-5.6 branch changes there, so the mess is unavoidable either way. Feel free to look at it, if you have a better idea :-) Thanks, Bob > Am 25.11.2014 um 12:16 schrieb Dmitry Stogov <dmitry@zend.com>: > > 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 > <mailto:bobwei9@hotmail.com>> wrote: >> Am 24.11.2014 um 13:12 schrieb Dmitry Stogov <dmitry@zend.com >> <mailto: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> >>> >> >> > >