Re: 5dee3c11 break

Hi, On Tue, May 6, 2014 at 8:43 PM, Dmitry Stogov <dmitry@zend.com> wrote: > zend_std_has_dimension() doesn't know what (check_empty == 2) means. > > check_empty == 0 - ISSET => we don't need to call offsetGet() and relay on > offsetExists() return value. > check_empty == 1 - ISEMPTY => we should call offsetGet() after > offsetExists(). > > NULL values should be handled by offsetExists(). > Okay, I suppose it's fair to make its behaviour the same as for zend_std :) I'll make the necessary changes by tonight for 5.6 and master. > > Thanks. Dmitry. > > > On Tue, May 6, 2014 at 4:36 PM, Tjerk Anne Meesters <datibbaw@php.net>wrote: > >> >> >> On Tue, May 6, 2014 at 7:50 PM, Dmitry Stogov <dmitry@zend.com> wrote: >> >>> Ah, I didn't get what did you mean by (check_empty == 2), but they >>> probably should be changed into (check_empty != 1) >>> >> >> When I said mostly, perhaps I should have mentioned that the isset() >> behaviour was deliberately updated as well; in other words, the following >> works as expected: >> >> $x['foo'] = null; >> isset($x['foo']); // false >> >> After taking a closer look at zend_std_has_dimension() it seems that both >> isset() and empty() behave as if empty() was always used; of course, if >> this behaviour was translated into spl_array.c, the infinite recursion >> would still be an issue. So, at this point I'm unsure what the proper >> action point should be. >> >> >>> >>> >>> It fixes the problem. >>> Could you please verify and commit into all relevant branches. >>> >>> Thanks. Dmitry. >>> >>> --- a/ext/spl/spl_array.c >>> +++ b/ext/spl/spl_array.c >>> @@ -603,7 +603,7 @@ static int spl_array_has_dimension_ex(int >>> check_inherited, zval *object, zval *o >>> >>> if (rv && zend_is_true(rv TSRMLS_CC)) { >>> zval_ptr_dtor(&rv); >>> - if (check_empty == 2) { >>> + if (check_empty != 1) { >>> return 1; >>> } else if (intern->fptr_offset_get) { >>> value = spl_array_read_dimension_ex(1, >>> object, offset, BP_VAR_R TSRMLS_CC); >>> @@ -622,7 +622,7 @@ static int spl_array_has_dimension_ex(int >>> check_inherited, zval *object, zval *o >>> switch(Z_TYPE_P(offset)) { >>> case IS_STRING: >>> if (zend_symtable_find(ht, >>> Z_STRVAL_P(offset), Z_STRLEN_P(offset)+1, (void **) &tmp) != FAILURE) { >>> - if (check_empty == 2) { >>> + if (check_empty != 1) { >>> return 1; >>> } >>> } else { >>> @@ -639,7 +639,7 @@ static int spl_array_has_dimension_ex(int >>> check_inherited, zval *object, zval *o >>> index = Z_LVAL_P(offset); >>> } >>> if (zend_hash_index_find(ht, index, >>> (void **)&tmp) != FAILURE) { >>> - if (check_empty == 2) { >>> + if (check_empty != 1) { >>> return 1; >>> } >>> } else { >>> >>> >>> >>> On Tue, May 6, 2014 at 3:29 PM, Tjerk Anne Meesters <datibbaw@php.net>wrote: >>> >>>> Hi, >>>> >>>> >>>> On Tue, May 6, 2014 at 7:25 PM, Dmitry Stogov <dmitry@zend.com> wrote: >>>> >>>>> I didn't review you patch careful, but why do you need to call >>>>> offsetGet() when offsetExists() must be enough? >>>>> is it for empty()? >>>>> >>>> >>>> Mostly for empty(), yes. It fixes the expected empty() behaviour for >>>> $a['foo'] == 0 when using ArrayObject. >>>> This is further explained in this >>>> bug report: https://bugs.php.net/bug.php?id=66834 >>>> >>>> >>>>> Then you probably should implement it a way similar to >>>>> zend_std_has_dimension() in Zend/zend_object_handlers.c. >>>>> call offsetExists() and then offsetGet() if necessary. >>>>> >>>> >>>> That's already the case :) >>>> >>>> >>>> http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_object_handlers.c#719 >>>> >>>> >>>>> >>>>> Thanks. Dmitry. >>>>> >>>>> >>>>> On Tue, May 6, 2014 at 2:31 PM, Tjerk Anne Meesters >>>>> <datibbaw@php.net>wrote: >>>>> >>>>>> Hi Dmitry, >>>>>> >>>>>> On Tue, May 6, 2014 at 4:11 PM, Dmitry Stogov <dmitry@zend.com>wrote: >>>>>> >>>>>>> Hi Tjerk, >>>>>>> >>>>>>> your commit broke the code that worked fine before (still works in >>>>>>> 5.5 but broken in 5.6 and above). >>>>>>> It leads into infinity recursion until stack overflow. >>>>>>> >>>>>>> It must be fixed or reverted. >>>>>>> >>>>>> >>>>>> This has been mentioned by Jakub before and a fix to ZF2 has already >>>>>> been merged: >>>>>> >>>>>> >>>>>> https://github.com/zendframework/zf2/pull/6096 >>>>>> >>>>>> The previous code and my patch basically cannot coexist; it used to >>>>>> work in 5.6 before, but only by the "virtue" of an unfortunate >>>>>> implementation. >>>>>> >>>>>> I believe this is not the only 5.6 issue that ZF2 is dealing with, >>>>>> but if you feel that this breaks too many things for a 5.x release I >>>>>> suppose we can revert it in PHP-5.6 and keep it for PHP-6? >>>>>> >>>>>> Let me know. >>>>>> >>>>>> >>>>>>> Thanks. Dmitry. >>>>>>> >>>>>>> <?php >>>>>>> class Parameters extends ArrayObject { >>>>>>> public function __construct(array $values = null) { >>>>>>> if (null === $values) { >>>>>>> $values = array(); >>>>>>> } >>>>>>> parent::__construct($values, ArrayObject::ARRAY_AS_PROPS); >>>>>>> } >>>>>>> public function offsetGet($name) { >>>>>>> if (isset($this[$name])) { >>>>>>> return parent::offsetGet($name); >>>>>>> } >>>>>>> return null; >>>>>>> } >>>>>>> } >>>>>>> $x = new Parameters(); >>>>>>> var_dump($x['foo']); >>>>>>> $x['foo'] = 'bar'; >>>>>>> var_dump($x['foo']); >>>>>>> ?> >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> -- >>>>>> Tjerk >>>>>> >>>>> >>>>> >>>> >>>> >>>> -- >>>> -- >>>> Tjerk >>>> >>> >>> >> >> >> -- >> -- >> Tjerk >> > > -- -- Tjerk