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, 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 >>>>> >>>> >>>> >>>> >>> >>> >> >> > >