5.4 regression: non-existent sub-sub keys now have values

hi Rasmus,

I had a quick look through the Drupal code. I don't see a single place
that this is done where a deeply nested array index is applied to a
string. I think you are misunderstanding what has changed here.

We are leading to yet another underestimated impact to userland code,
for zero gain.

The risk is to high, let revert that please.

For all the people saying, "Revert". You guys realize that also means we
revert all the array dereferencing we added elsewhere, right? That
includes function array dereferencing which pretty most everyone has
been clamoring for for years.

So just to clarify, you think we should remove function array
dereferencing? Otherwise, please provide a proposal for how we separate
these various derefencing mechanisms in a way that doesn't completely
destroy the clean and consistent implementation we have right now.

-Rasmus

Generally $foo, bar, and baz will all be arrays, and if they're not it
means someone else had a bug somewhere. Of course, Drupal module
developers never have bugs in their code that accidentally puts a string
where they should have put an array, no, not at all. :-) (Generally
when that happens we already hit a "first argument to foreach() must be
an array" error.)

Currently we don't use ArrayAccess anywhere aside from inheriting it
from PDO.

If that doesn't change, then I rescind my previous panic attack.

Yes, no change in any of that. In your usage, the case that behaves
differently in 5.4 was actually a fatal error in 5.3, so chances are
pretty good you don't have too many of these.

-Rasmus

On Thu, Nov 24, 2011 at 11:03 PM, Larry Garfield larry@garfieldtech.comwrote:

On Thu, Nov 24, 2011 at 9:12 PM, Larry Garfield<larry@garfieldtech.**comlarry@garfieldtech.com

wrote:

Richard Quadling wrote:

I agree with Daniel on this.

Just looking for any test relating to isset() to see what tests will
now fail.

So it's not just me :)
I am seeing this break real world projects and can't see a easy way to
fix the break. There is nothing really wrong with the current code
except that the sub keys have yet to be populated.

This is going to be a huge problem for Drupal. Drupal uses deep
associative array structures a lot, by design. That means we isset() or
empty() on arrays a lot. I haven't had a chance to test it yet, but I
see
this change breaking, um, A LOT. And as Daniel noted, the fix is to turn
one line of very readable code into 8 lines of hard to read code.

Did you managed to read the whole thread?
I'm asking because there were lot of confusion about the actual impact of
this problem, and Lester particularly seemed confused.

To be fair, no, I hadn't read the whole thread by the time I sent that.
(One of the challenges of long fast-moving threads is they're hard to keep
up with.)

"There is nothing really wrong with the current code except that the sub

keys have yet to be populated."
that isn't true, if the array or some sub elements are empty("yet to be
populated"), you won't bump into this change. See my previous mail for the
exact details.

so the above mentioned one liner:

"if (isset($arr['package']['**attribs']['version'])) {"

what could go wrong:
$arr is not an array, but a string -> it would fatal on 5.3(Fatal: cannot
use string offset as an array), but it would work with 5.4
$arr['package'] is not an array but a string -> false on 5.3, true on 5.4
(this is the worst case)
$arr['package']['attribs'] is not an array but a string -> true on both
5.3
and 5.4 (so you are already screwed)

So to clarify, what Drupal does on a regular basis is something like:

if (isset($foo['bar']['baz']['**beep'])) {
// Assume that bar, baz, and beep all exist, and that beep has a value
}
// or sometimes
if (!empty($foo['bar']['baz']['**beep'])) {
// Assume that bar, baz, and beep all exist,
// and that beep has a meaningful value
}

Generally $foo, bar, and baz will all be arrays, and if they're not it
means someone else had a bug somewhere. Of course, Drupal module
developers never have bugs in their code that accidentally puts a string
where they should have put an array, no, not at all. :-) (Generally when
that happens we already hit a "first argument to foreach() must be an
array" error.)

if the foreach wouldn't catch this, you would have a chance to run into
this problem (both with 5.3 and 5.4)

Currently we don't use ArrayAccess anywhere aside from inheriting it from
PDO.

I only mentioned ArrayAccess because if an object implements that
interface, then $object['foo'] won't throw an error, but works as it would
be an array.

If that doesn't change, then I rescind my previous panic attack.

I think that bumping into this is less-less-less likely than what the
replies on this thread suggests, and this behavior is already there and
documented, the current change only add another edge-case where you can
trigger it.
The real gotcha is that the string offset index is substituted to the php
style type juggling($string['foo'] is valid and will be interpreted as
$string[0]), and that we use the same syntax for accessing array elements
and string offsets.
The two combined with a wrong/unexpected argument can screw you over, and
we didn't issue a notice on this yet.
It was just a coincidence that by historical reasons the string offset
chaining was disallowed, so there were one corner case, which was prevented
by this.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

-----Original Message-----
From: Larry Garfield [mailto:larry@garfieldtech.com]
Sent: 24 November 2011 22:04

[... BIG SNIP ...]

If that doesn't change, then I rescind my previous panic attack.

--Larry Garfield

I echo that sentiment. On fuller review, I find a very high FUD
factor in effect here, and on actually checking my codebase I find
only 1 place where an additional is_array() might be needed (in
several tens of kLOC!).

Cheers!

Mike

--
Mike Ford,
Electronic Information Developer, Libraries and Learning Innovation,
Portland PD507, City Campus, Leeds Metropolitan University,
Portland Way, LEEDS,  LS1 3HE,  United Kingdom
E: m.ford@leedsmet.ac.uk T: +44 113 812 4730

To view the terms under which this email is distributed, please go to http://disclaimer.leedsmet.ac.uk/email.htm

On November-23-11 5:31 PM Gustavo Lopes wrote:

On Wed, 23 Nov 2011 21:06:09 -0000, Pierre Joye pierre.php@gmail.com
wrote:

The fact that we have reports here showing code not working anymore
because of this change tells me that it is a BC break. We can call it
a bug fix but it still breaks code out there for no real benefit but
edge case usages. We had this situation before, that does not help
us.

I'd say for no benefit at all. Why would anyone ever want to take a
string offset from a string that certainly has length 1? Except for
taking satisfaction in this "improved consistency", I see absolutely no
benefit.

Up until now, it was deemed a useless but innocuous change. Now that we
found it has pernicious side effects, we ought to revert it.

Notwithstanding that this behaviour was possible because of a bug, it has
admittedly been relied on since time immemorial, making it a significant
BC break.

As distasteful as it seems, it absolutely should be reverted IMHO.

Best Regards,

Mike Robinson

-----Original Message-----
From: Gustavo Lopes [mailto:glopes@nebm.ist.utl.pt]
Sent: 23 November 2011 22:31

On Wed, 23 Nov 2011 21:06:09 -0000, Pierre Joye
pierre.php@gmail.com
wrote:

The fact that we have reports here showing code not working
anymore
because of this change tells me that it is a BC break. We can call
it
a bug fix but it still breaks code out there for no real benefit
but
edge case usages. We had this situation before, that does not help
us.

I'd say for no benefit at all. Why would anyone ever want to take a
string
offset from a string that certainly has length 1? Except for taking
satisfaction in this "improved consistency", I see absolutely no
benefit.

Up until now, it was deemed a useless but innocuous change. Now that
we
found it has pernicious side effects, we ought to revert it.

That's exactly my take on it. As far as I can see, if 5.4 is released
with this "fix" in, it will effectively be rendered a non-upgradable-to
version for most big projects.

For what it's worth (not much!), my two pennorth on this is that
whilst, like some others, I can see the marginal benefit of making the
behaviour consistent, I really don't think the change can be done in
one go. Given that this WILL break existing code (some of it mine :( ),
there needs to be a release where the usage generates an error message

• probably an E_WARNING, or maybe an E_STRICT - and an easy alternative
  is provided. Since the simplest alternative way of performing the
  necessary chained checks would seem to be:

  if (is_array($arr) && array_key_exists('key1', $arr)
  && is_array($arr['key1'] && array_key_exists('key2', $arr['key2'])
  ...
  )

which substitutes 2 function calls and 2 array accesses per dimension
for a single function call and 1 multi-dimensional array access, there has
to be scope for a function called something like array_offset_exists with
a signature like

array_offset_exists(array $array, mixed $offset, ...)

(Yes, I know, inconsistent argument order and all that, but....)

so that people can have a chance to transform

isset($arr['key1']['key2']['key3'])

into

array_offset_exists($arr, 'key1', 'key2, 'key3')

before the full fix is put into place in a subsequent version.

Cheers!

Mike

--
Mike Ford,
Electronic Information Developer, Libraries and Learning Innovation,
Portland PD507, City Campus, Leeds Metropolitan University,
Portland Way, LEEDS,  LS1 3HE,  United Kingdom
E: m.ford@leedsmet.ac.uk T: +44 113 812 4730

To view the terms under which this email is distributed, please go to http://disclaimer.leedsmet.ac.uk/email.htm

Hi!

But neither is introducing a potential bomb of the kind that the 'date' saga
created. The problem this change IS causing is likely to hit many live sites

The claim that many live sites actually regularly use string multiple
string offsets to distinguish strings from arrays sounds implausible to me.

without any real explanation as to why it is happening. And I still don't think
that the type of code that it is hitting is 'wrong', it IS just doing
a lookup

There's real explanation and it's in UPGRADING now. And the code is not
just plain "wrong" - it relies on a bug to distinguish between array and
string, when it should use proper functions instead. If you want to
check if something is an array, use is_array. Using side effect of a bug
and claiming that the engine now should stay broken forever because you
used it doesn't make any sense.

for an array element that does not exist to decide if it needs to create it.

No, it's not going for array element - it applies multiple array
operations to a string without checking and relies on a bug to pass it
off. That's broken, plain and simple.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

Hi!

And I believe you're missing the point. I'm not suggesting it for 5.4,
but illustrating that until something like it (or better) exists, the
"bug fix" should wait until such a solution exists.

Again, you seem to miss the major point. The bug fix was fixing the
result of string operation, which before that was using "string offset"
pseudo-type that never really worked properly, since it required special
branches throughout the engine and tons of special cases and still
produced bugs and segfaults. The fact that $a['foo']['bar'] works is
just a side effect of that bug fix - because the result of string offset
op is now string, as it always should have been.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

David Muir wrote:

Personally I think that the main issue is that we silently convert $foo['bar']
to $foo[0] for strings, which imo rarely the intended behavior and doesn't
really makes much sense (except that the offset expects the index to be int,
and this is how we type juggle between string and int), so I think it would be
a good idea to raise a notice here, so the developer can fix his code, or add
some input validation.

That would be a help.
That would help a lot ... this is not a problem of "so the developer can fix his
code" but rather so we can fix legacy code which other non-developers are
currently using happily ... If I had written the code I would not have done it
the way it is currently structured, but I'm not about to spend time
re-engineering it ... I just want to plug the holes quickly so it continues to work.

--
Lester Caine - G8HFL

Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk//
Firebird - http://www.firebirdsql.org/index.php

Just to clarify, the changes introduced in 5.4 will result in the
following:

<?php

$string = 'foo';
$array = array(
'foo' => array(
'bar' => 'baz',
//expected structure
//'bar' => array('baz' => array('values'))
));

var_dump(
isset($string['foo']), //true
isset($string[0][0]), //false, true in 5.4
isset($array['foo']['bar'][0]), //true
isset($array['foo']['bar']['baz']), //true
isset($array['foo']['bar']['baz']['0']) //false, true as of 5.4
isset($string['foo']['bar']['baz']['0']) //false, true as of 5.4
);

you are missing a comma from the end of the
isset($array['foo']['bar']['baz']['0']) //false, true as of 5.4
line

Yeah, I added that one at the last minute. That's what I get for a quick
copy/paste...

isset($string['foo']['bar']['baz']['0']) //false, true as of 5.4
gives me a fatal error on 5.3 ("PHP Fatal error: Cannot use string offset
as an array" as you can't "chain" string offsets before 5.4)

It gives me false in 5.3.6. Using it outside of isset() results in the
fatal error.

hm.

tyrael@thor:~$ php -r '$string =
"foo";isset($string["foo"]["bar"]["baz"]["0"]);';
PHP Fatal error: Cannot use string offset as an array in Command line code
on line 1
tyrael@thor:~$ php -v
PHP 5.3.8-1~dotdeb.2 with Suhosin-Patch (cli) (built: Aug 25 2011 13:30:46)
Copyright (c) 1997-2011 The PHP Group
Zend Engine v2.3.0, Copyright (c) 1998-2011 Zend Technologies
with Suhosin v0.9.32.1, Copyright (c) 2007-2010, by SektionEins GmbH

I will check this against a vanilla version, as there is a chance that
either dotdeb(less likely) or suhosin patched something.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu