Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [VOTE] Fix list() behavior inconsistency »

[VOTE] Fix list() behavior inconsistency

php.internals

Dmitry Stogov

11 years ago
Hi, The vote is opened at https://wiki.php.net/rfc/fix_list_behavior_inconsistency Thanks. Dmitry.

Leigh

11 years ago
On 25 September 2014 08:42, Dmitry Stogov <dmitry@zend.com> wrote:
> Hi, > > The vote is opened at > https://wiki.php.net/rfc/fix_list_behavior_inconsistency > > Thanks. Dmitry.
Since in the "should people be allowed to vote" thread, I said I think people should explain their votes, here is my explanation :) I am voting disable. +1 for consistency, but I think we already have a pretty rich suite of functions for working with strings that do not work with arrays, and vice versa. I think enabling list() to work on strings sets a precedent that more/all array handling mechanisms should be made to work with strings, and I'm not sure that's something we want. We've defined list as not working with strings in the manual, lets stick to our own definition.

Dmitry Stogov

11 years ago
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. Thank. Dmitry. On Thu, Sep 25, 2014 at 12:00 PM, Leigh <leight@gmail.com> wrote:

Nikita Popov

11 years ago
On Thu, Sep 25, 2014 at 1:15 PM, Dmitry Stogov <dmitry@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

Nikita Popov

11 years ago
On Thu, Sep 25, 2014 at 10:32 PM, Nikita Popov <nikita.ppv@gmail.com> wrote:
> On Thu, Sep 25, 2014 at 1:15 PM, Dmitry Stogov <dmitry@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

Dmitry Stogov

11 years ago
It was on design. list() was intended to support plain arrays only. Thanks. Dmitry. On Fri, Sep 26, 2014 at 12:45 AM, Nikita Popov <nikita.ppv@gmail.com> wrote:

Stas Malyshev

11 years ago
Hi!
> It was on design. list() was intended to support plain arrays only.
I'm not sure I'm getting this point - why list($a, $b) = $foo is not just translated as $a = $foo[0], $b = $foo[1], etc.? Is it hard to make it work that way?
-- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/

Leigh

11 years ago
On 25 September 2014 22:50, Stas Malyshev <smalyshev@sugarcrm.com> wrote:
> Hi! > >> It was on design. list() was intended to support plain arrays only. > > I'm not sure I'm getting this point - why list($a, $b) = $foo is not > just translated as $a = $foo[0], $b = $foo[1], etc.? Is it hard to make > it work that way? >
Why do array_* functions not treat strings as arrays of bytes? If we we can/want to make strings byte arrays, then I am 100% in favour of all array operations working on strings. If this is something we want to work towards for the future, that's great, lets get busy :) - If we want to say "yea list() should work with strings", but no other array functions should work with strings, it seems very odd to me.

Stas Malyshev

11 years ago
Hi!
> Why do array_* functions not treat strings as arrays of bytes?
How that's related? We're not talking about array_* functions, we're talking about list() operator.
> get busy :) - If we want to say "yea list() should work with strings", > but no other array functions should work with strings, it seems very > odd to me.
It's as odd as [] working with strings but -> not. Those are different things, so they work differently.
-- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/

Leigh

11 years ago
On 26 September 2014 08:01, Stas Malyshev <smalyshev@sugarcrm.com> wrote:
> > It's as odd as [] working with strings but -> not. Those are different > things, so they work differently.
Sorry, this was kind of my point, I probably just phrased it badly. The array_* "question" was meant to be rhetorical. My points are: * Strings are _not_ treated as arrays of bytes everywhere. * If we intend to give strings more array-like support after this RFC (like foreach($string as $char), making array_* work with strings), then I support the list() change. * Otherwise not

Nikita Popov

11 years ago
On Thu, Sep 25, 2014 at 11:50 PM, Stas Malyshev <smalyshev@sugarcrm.com> wrote:
> Hi! > > > It was on design. list() was intended to support plain arrays only. > > I'm not sure I'm getting this point - why list($a, $b) = $foo is not > just translated as $a = $foo[0], $b = $foo[1], etc.? Is it hard to make > it work that way? >
That's exactly what list() does. The only catch is that $foo here is reused multiple times and mustn't be freed in the meantime (for the cases where $foo is some complex expression resulting in an VAR or TMP_VAR operand). That's what the ZEND_FETCH_ADD_LOCK flags for FETCH_DIM_R does - it does $foo[0] without freeing $foo. However back when list() was introduced FETCH_DIM_R didn't support CONST or TMP_VAR operands, so instead these two used a separate FETCH_DIM_TMP_VAR opcode, which supports only arrays and not strings or objects. Support for CONST|TMP in FETCH_DIM_R was only added in PHP 5.5 as part of constant string/array dereferencing. Long story short, because FETCH_DIM_R now supports CONST and TMP_VAR operands, we can always use it and FETCH_DIM_TMP_VAR can be dropped - that's all that has to be done in order to always support strings and objects in list(). (I've linked a patch for this previously, see https://github.com/nikic/php-src/compare/stringOffsetsInList). If I understood it correctly, then Dmitry's alternative is to add support for CV and VAR operands to FETCH_DIM_TMP_VAR and always use that for list(). This avoids having to check the ZEND_FETCH_ADD_LOCK flag in FETCH_DIM_R. However I don't think that this optimization is related to whether or not we support strings and objects. We can have a separate opcode only for list() in either case, no matter which choice is made here. Nikita

Stas Malyshev

11 years ago
Hi!
> Long story short, because FETCH_DIM_R now supports CONST and TMP_VAR > operands, we can always use it and FETCH_DIM_TMP_VAR can be dropped - > that's all that has to be done in order to always support strings and > objects in list(). (I've linked a patch for this previously, see > https://github.com/nikic/php-src/compare/stringOffsetsInList).
Excellent, IMO this is the most logical way to proceed - and I'm not sure why one check for one flag is something worth worrying about. Is it really that slow to check for one flag?
> If I understood it correctly, then Dmitry's alternative is to add > support for CV and VAR operands to FETCH_DIM_TMP_VAR and always use that > for list(). This avoids having to check the ZEND_FETCH_ADD_LOCK flag in > FETCH_DIM_R. However I don't think that this optimization is related to > whether or not we support strings and objects. We can have a separate > opcode only for list() in either case, no matter which choice is made here.
We can, of course, but with your explanation I don't really see why we even should...
-- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/

Dmitry Stogov

11 years ago
I told it doesn't support strings and objects because it was designed this way. I don't know who and when did it. It's not complicated to change it in any way. The question which way is better, and it's the reason of voting. I would prefer not to extend list() to support strings, but in case "enabling" would win, I'll make it. Thanks. Dmitry. On Fri, Sep 26, 2014 at 1:50 AM, Stas Malyshev <smalyshev@sugarcrm.com> wrote:

Laruence

11 years ago
Hey: On Fri, Sep 26, 2014 at 6:10 AM, Dmitry Stogov <dmitry@zend.com> wrote:
> I told it doesn't support strings and objects because it was designed this > way. > I don't know who and when did it. > > It's not complicated to change it in any way. > The question which way is better, and it's the reason of voting. > > I would prefer not to extend list() to support strings, but in case > "enabling" would win, I'll make it. > > Thanks. Dmitry. >
I am working what should we do if none side get 2/3 votes? it seems it is going there :< thanks
> > > > On Fri, Sep 26, 2014 at 1:50 AM, Stas Malyshev <smalyshev@sugarcrm.com> > wrote: > >> Hi! >> >> > It was on design. list() was intended to support plain arrays only. >> >> I'm not sure I'm getting this point - why list($a, $b) = $foo is not >> just translated as $a = $foo[0], $b = $foo[1], etc.? Is it hard to make >> it work that way? >> >> -- >> Stanislav Malyshev, Software Architect >> SugarCRM: http://www.sugarcrm.com/ >>
-- Xinchen Hui @Laruence http://www.laruence.com/

Laruence

11 years ago
On Fri, Sep 26, 2014 at 11:54 AM, Xinchen Hui <laruence@php.net> wrote:
> Hey: > > > > On Fri, Sep 26, 2014 at 6:10 AM, Dmitry Stogov <dmitry@zend.com> wrote: >> I told it doesn't support strings and objects because it was designed this >> way. >> I don't know who and when did it. >> >> It's not complicated to change it in any way. >> The question which way is better, and it's the reason of voting. >> >> I would prefer not to extend list() to support strings, but in case >> "enabling" would win, I'll make it. >> >> Thanks. Dmitry. >> > I am working what should we do if none side get 2/3 votes?
worrying
> it seems it is going there :< > > thanks >> >> >> >> On Fri, Sep 26, 2014 at 1:50 AM, Stas Malyshev <smalyshev@sugarcrm.com> >> wrote: >> >>> Hi! >>> >>> > It was on design. list() was intended to support plain arrays only. >>> >>> I'm not sure I'm getting this point - why list($a, $b) = $foo is not >>> just translated as $a = $foo[0], $b = $foo[1], etc.? Is it hard to make >>> it work that way? >>> >>> -- >>> Stanislav Malyshev, Software Architect >>> SugarCRM: http://www.sugarcrm.com/ >>> > > > > -- > Xinchen Hui > @Laruence > http://www.laruence.com/
-- Xinchen Hui @Laruence http://www.laruence.com/

Nikita Popov

11 years ago
On Thu, Sep 25, 2014 at 11:47 PM, Dmitry Stogov <dmitry@zend.com> wrote:
> It was on design. list() was intended to support plain arrays only. > > Thanks. Dmitry. >
So, just to clarify: If we vote to "remove string handling in all cases" does that also mean that we "remove ArrayAccess support in all cases"? If so, could the RFC please explicitly mention that? Nikita

Dmitry Stogov

11 years ago
When I started this RFC I didn't thought about objects. Actually, they are handled with the same inconsistency problem. Nikita, feel free to add this note to RFC. May be it'll change mind of some voters :) also add a link to your patch. Thanks. Dmitry. On Fri, Sep 26, 2014 at 2:11 PM, Nikita Popov <nikita.ppv@gmail.com> wrote:

Pierre Joye

11 years ago
On Fri, Sep 26, 2014 at 2:30 PM, Dmitry Stogov <dmitry@zend.com> wrote:
> When I started this RFC I didn't thought about objects. > Actually, they are handled with the same inconsistency problem. > > Nikita, feel free to add this note to RFC. > May be it'll change mind of some voters :) > > also add a link to your patch.
Please do not :) Enough mess with RFC changed while being voting on. Cheers,
-- Pierre @pierrejoye | http://www.libgd.org

Andrew Faulds

11 years ago
On 26 Sep 2014, at 14:25, Pierre Joye <pierre.php@gmail.com> wrote:
> On Fri, Sep 26, 2014 at 2:30 PM, Dmitry Stogov <dmitry@zend.com> wrote: >> When I started this RFC I didn't thought about objects. >> Actually, they are handled with the same inconsistency problem. >> >> Nikita, feel free to add this note to RFC. >> May be it'll change mind of some voters :) >> >> also add a link to your patch. > > Please do not :) Enough mess with RFC changed while being voting on.
Yeah, that’s a pretty big change. I wouldn’t vote how I did if it meant affecting ArrayAccess. Please restart the vote.
-- Andrea Faulds http://ajf.me/

Andrew Faulds

11 years ago
On 26 Sep 2014, at 11:11, Nikita Popov <nikita.ppv@gmail.com> wrote:
> So, just to clarify: If we vote to "remove string handling in all cases" > does that also mean that we "remove ArrayAccess support in all cases"? If > so, could the RFC please explicitly mention that?
I myself would be in favour of removing string support, but I don’t want to remove ArrayAccess. There’s no good reason to get rid of it.
-- Andrea Faulds http://ajf.me/

Dmitry Stogov

11 years ago
just change your vote. I just did it. :) Even if ArrayAccess worked not by design, it's going to be a big compatibility issue, removing it. Strings support would work for free. Thanks. Dmitry, On Fri, Sep 26, 2014 at 5:03 PM, Andrea Faulds <ajf@ajf.me> wrote:

Andrew Faulds

11 years ago
On 26 Sep 2014, at 14:11, Dmitry Stogov <dmitry@zend.com> wrote:
> just change your vote. > I just did it. :) > > Even if ArrayAccess worked not by design, it's going to be a big compatibility issue, removing it. > Strings support would work for free.
What should I vote then? I want to vote against string support, but in favour ArrayAccess support.
-- Andrea Faulds http://ajf.me/

Dmitry Stogov

11 years ago
FETCH_DIM_TMP_VAR is used especiualy for list(). It expects array, don't check for objects and strings. It doesn't remove the operand and allow it's reuse in next opcode. FETCH_DIM_R is used for list() only in some cases (when operand IS_VAR). To work in list() context it has to keep operand not destroyed, and to do it FETCH_DIM_R has to check for (opline->extended_value & ZEND_FETCH_ADD_LOCK). Note that this check has to be executed for each array access operation unrelated to list() handling. Thanks. Dmitry. On Fri, Sep 26, 2014 at 12:32 AM, Nikita Popov <nikita.ppv@gmail.com> wrote:

Patrick ALLAERT

11 years ago
2014-09-25 9:42 GMT+02:00 Dmitry Stogov <dmitry@zend.com>:
> Hi, > > The vote is opened at > https://wiki.php.net/rfc/fix_list_behavior_inconsistency > > Thanks. Dmitry. >
Hi, I'm in favor of disabling for consistency as well, however, I wish a warning would be emitted. Not only it would tell me that I have a potential error while upgrading to PHP 7 but also if I am using it incorrectly, which is always the case when using a scalar value. I therefor propose that: list($a, $b) = 42; list($a, $b) = "42"; list($a, $b) = null; ... generates a warning like: PHP Warning: list() expects right operand to be array, <type> given in <path> on line <line> Currently, all of the above lines wouldn't generate any notice / warning / error, however, using: list($a, $b) = []; generates the following notices: PHP Notice: Undefined offset: 1 in ... on line ... PHP Notice: Undefined offset: 0 in ... on line ... This is confusing since (to me) using an array is at least better than using scalars. Patrick

Lars Strojny

11 years ago
Hi everyone, On 25 Sep 2014, at 17:27, Patrick ALLAERT <patrickallaert@php.net> wrote: [...]
> > I'm in favor of disabling for consistency as well, however, I wish a > warning would be emitted.
Voted in favour of disabling as well but could easily live with the other option as everything is better then leaving the inconsistency there. cu, Lars

Derick Rethans

11 years ago
On Thu, 25 Sep 2014, Lars Strojny wrote:
> On 25 Sep 2014, at 17:27, Patrick ALLAERT <patrickallaert@php.net> wrote: > [...] > > > > I'm in favor of disabling for consistency as well, however, I wish a > > warning would be emitted. > > Voted in favour of disabling as well but could easily live with the > other option as everything is better then leaving the inconsistency > there.
So you'd rather have that already working code now stops working, instead of new bits of code *starting* to work. That's incredibly backwards. Certainly it would be less of a pain for our *users* to allow both options‽ cheers, Derick

Patrick ALLAERT

11 years ago
2014-09-25 17:27 GMT+02:00 Patrick ALLAERT <patrickallaert@php.net>:
> 2014-09-25 9:42 GMT+02:00 Dmitry Stogov <dmitry@zend.com>: > >> Hi, >> >> The vote is opened at >> https://wiki.php.net/rfc/fix_list_behavior_inconsistency >> >> Thanks. Dmitry. >> > > Hi, > > I'm in favor of disabling for consistency as well, however, I wish a > warning would be emitted. > Not only it would tell me that I have a potential error while upgrading to > PHP 7 but also if I am using it incorrectly, which is always the case when > using a scalar value. > > I therefor propose that: > list($a, $b) = 42; > list($a, $b) = "42"; > list($a, $b) = null; > ... > generates a warning like: PHP Warning: list() expects right operand to be > array, <type> given in <path> on line <line> > > Currently, all of the above lines wouldn't generate any notice / warning / > error, however, using: > list($a, $b) = []; > > generates the following notices: > PHP Notice: Undefined offset: 1 in ... on line ... > PHP Notice: Undefined offset: 0 in ... on line ... > > This is confusing since (to me) using an array is at least better than > using scalars. > > Patrick >
bump ? (It is ok to say it's a stupid idea or that I just don't know what I am talking about) Patrick

Markus Fischer

11 years ago
On 25.09.14 09:42, Dmitry Stogov wrote:
> The vote is opened at > https://wiki.php.net/rfc/fix_list_behavior_inconsistency
Voted +1 for disabling. I think string handling needs more thorough designing and planning for edge case and such; i.e. the string handling alternative seems to rushed to me thus I err on the cautious side. thank you, - Markus

Gwynne Raskind

11 years ago
On Sep 25, 2014, at 2:42, Dmitry Stogov <dmitry@zend.com> wrote:
> Hi, > > The vote is opened at > https://wiki.php.net/rfc/fix_list_behavior_inconsistency > > Thanks. Dmitry.
Voting for always disabling string handling. This behavior is arcane and weird to me, and can be quickly emulated with list($a,$b) = str_split([“ab”][0]); if someone was actually using it. -- Gwynne Raskind

Stas Malyshev

11 years ago
Hi!
> and weird to me, and can be quickly emulated with list($a,$b) = > str_split([“ab”][0]); if someone was actually using it.
BC breaks don't work this way. When somebody's code would break on PHP 7, their first move would not be "oh, great, let's refactor it, it was too arcane anyway". It would be "OK, let's postpone the upgrade to $current_year + 5".
-- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/

Gwynne Raskind

11 years ago
On Sep 25, 2014, at 2:42, Dmitry Stogov <dmitry@zend.com> wrote:
> Hi, > > The vote is opened at > https://wiki.php.net/rfc/fix_list_behavior_inconsistency > > Thanks. Dmitry.
Voting for always disabling string handling. This behavior is arcane and weird to me, and can be quickly emulated with list($a,$b) = str_split([“ab”][0]); if someone was actually using it. -- Gwynne Raskind

Pascal MARTIN

11 years ago
On 25/09/2014 09:42, Dmitry Stogov wrote:
> Hi, > > The vote is opened at > https://wiki.php.net/rfc/fix_list_behavior_inconsistency > > Thanks. Dmitry. >
Hi, After discussing this RFC with a few other members of AFUP (French UG), we agree *something* should be done, to get to a consistent behavior: either all, or nothing, but not half. Most of us seem to go towards "disabling in all cases" (which indeed means BC-break for those who were using this -- probably not that many), but it seems like no-one will be sad if things end up going the other way around.
-- Pascal MARTIN http://blog.pascal-martin.fr/ @pascal_martin