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

ok. so if all tests passed, I commit it one into 5.6 and master. Thanks. Dmitry. On Tue, Nov 25, 2014 at 5:51 PM, Bob Weinand <bobwei9@hotmail.com> wrote: > Ah great idea how to remove the 2 in the zend_bool... > > Looks fine :-) > > Thanks, > Bob > > Am 25.11.2014 um 15:45 schrieb Dmitry Stogov <dmitry@zend.com>: > > Hi Bob, > > https://gist.github.com/dstogov/d82284a77a93cc3e2fe8 > > What do you think about the patch? > > I'm running tests, and then (if no problems found) I'm going to commit it > into 5.6 and master. > > Thanks. Dmitry. > > On Tue, Nov 25, 2014 at 3:23 PM, Bob Weinand <bobwei9@hotmail.com> wrote: > >> Wait. Master works just as expected with _no_ BC breaks. I just don't >> understand what's different in master than in 5.6, but somehow it works >> there just as expected. >> It's just 5.6 which needs special handling here. >> >> Bob >> >> Am 25.11.2014 um 13:17 schrieb Dmitry Stogov <dmitry@zend.com>: >> >> 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 >>>>>> >>>>> >>>>> >>>>> >>>> >>>> >>> >>> >> >> > >