Re: [PHP-DEV] [VOTE] Fix list() behavior inconsistency

[Nikita Popov]

On Thu, Sep 25, 2014 at 10:32 PM, Nikita Popov <nikita....@gmail.com> wrote:

> On Thu, Sep 25, 2014 at 1:15 PM, Dmitry Stogov <dmi...@zend.com> wrote:
>
>> disabling string handling would allow make operation simpler and would
>> improve regular access to array elements.
>> We won't need to check for (opline->extended_value & ZEND_FETCH_ADD_LOCK)
>> in FETCH_DIM_R handler.
>> However, it's going to be very small improvement, and I don't care a lot.
>> :)
>>
>> enabling string handling would require complication of
>> ZEND_FETCH_DIM_TMP_VAR handler (for strings support).
>> It's going to make list() handling a bit slower, but not significantly.
>>
>> my choice +1 for disabling.
>>
>
> Could you please clarify why removing string support would make the
> operation simpler? I voted for always supporting strings because I thought
> that is the option that simplifies things - in particular it would allow
> use to drop the FETCH_DIM_TMP_VAR opcode and always go through FETCH_DIM_R
> instead. Sample patch here:
> https://github.com/nikic/php-src/compare/stringOffsetsInList Or did I
> miss something and we can't do that?
>
> Nikita
>

I'd like to add that the FETCH_DIM_TMP_VAR opcode also provides incorrect
results if objects are used. list() generally accepts objects implementing
ArrayAccess, but if the object happens to be a TMP_VAR and
FETCH_DIM_TMP_VAR is used instead of FETCH_DIM_R, you'll just get a NULL
result:

<?php

class Arr implements ArrayAccess {
    private $arr;
    public function offsetGet($k) { return $this->arr[$k]; }
    public function offsetSet($k, $v) { $this->arr[$k] = $v; }
    public function offsetExists($k) { return isset($this->arr[$k]); }
    public function offsetUnset($k) { unset($this->arr[$k]); }
}

$arr = new Arr;
$arr[0] = 'foo';
$arr[1] = 'bar';

list($a, $b) = $arr;
var_dump($a, $b); // foo, bar

// (object) forces TMP_VAR
list($a, $b) = (object) $arr;
var_dump($a, $b); // NULL, NULL

So to handle that case we'd already have to extend the FETCH_DIM_TMP_VAR
handler to support objects in addition to arrays. At which point I don't
really see the point of making this a special case and would always use
FETCH_DIM_R instead (which already has all the necessary code to support
arrays, objects and strings).

Nikita