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

Yes, we may make BC breaks in master, but they should be done on purpose and not just because we can. I'll think how to fix it in PHP5.6, but in case I find a solution I'll need to revert your changes in master anyway. And I afraid, I can miss some peaces of your changes. Thanks. Dmitry. On Tue, Nov 25, 2014 at 3:08 PM, Bob Weinand <bobwei9@hotmail.com> wrote: > 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> 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 >>>> >>> >>> >>> >> >> > >