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

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 > <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 > <mailto: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 >> <mailto: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 < >>mailto: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 >>> <mailto: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 <���OyBP >>>> �}Ý!qmailto: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> >>>>> >>>> >>>> >>> >>> >> >> > >