Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:73973
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.220.179 as permitted sender)
MIME-Version: 1.0
Sender: tjerk.meesters@gmail.com
In-Reply-To: <CA+9eiLu+W5tMqu+ugGs+BcW=C8-NMXuW8+vGfnett9JNyYqDug@mail.gmail.com>
References: <CA+9eiLtZjPo7CJ2eVJX_XJ3tM4KadRf7UK6=7XiVkACyfUyQCw@mail.gmail.com>
	<CAHMUw2G_W=5ejYLWsf3=G=+qUW_oTW-MaFdcqB3hd1_O1w+U+A@mail.gmail.com>
	<CA+9eiLu=1cd5Ts9vRYmUnPgphUk5kh=ozdGpY5Pe8AeX6BBEbg@mail.gmail.com>
	<CAHMUw2EMqgHk4ehtq8e4h8NZ02qEhpkRkZFzTMZNTbv=45zLJQ@mail.gmail.com>
	<CA+9eiLu+W5tMqu+ugGs+BcW=C8-NMXuW8+vGfnett9JNyYqDug@mail.gmail.com>
Date: Tue, 6 May 2014 20:36:33 +0800
Message-ID: <CAHMUw2EPwqcwRVSqDEoU2hu57v0O+XyXBD=+mQfmjs29dRyzAw@mail.gmail.com>
To: Dmitry Stogov <dmitry@zend.com>
Cc: Julien Pauli <jpauli@php.net>, PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=001a11c2b94e87a94a04f8ba7f0c
Subject: Re: 5dee3c11 break
From: datibbaw@php.net (Tjerk Anne Meesters)

--001a11c2b94e87a94a04f8ba7f0c
Content-Type: text/plain; charset=UTF-8

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

--001a11c2b94e87a94a04f8ba7f0c--