Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:56584
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.216.170 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <4ECEA547.9070605@garfieldtech.com>
References: <20111123015008.GA12933@panix.com>
	<CACQesk6UuGUgyJz3hsGVE0crJx3GCkjTTpLb4xNGTm1goLzO8Q@mail.gmail.com>
	<20111123023108.GA172@panix.com>
	<CACQesk5kj=4kzvTDiLL1oOLBXZrn4WdUcLXdbyMLTa=OdhGTcw@mail.gmail.com>
	<4ECCB549.904@lsces.co.uk>
	<4ECCBC56.3050602@sugarcrm.com>
	<20111123141408.GA11940@panix.com>
	<20111123153100.GB13420@panix.com>
	<CAKUjMCVkmd2+=K9n6XVMeKvMCaMwORQN-3phA2CqZVjXBChPSg@mail.gmail.com>
	<4ECD37D9.8090604@lsces.co.uk>
	<4ECEA547.9070605@garfieldtech.com>
Date: Thu, 24 Nov 2011 21:58:53 +0100
Message-ID: <CAH-PCH5OSpjo8x_Y3Burd2uDeOkb6Spf7BcQ37tAmDws-V++uQ@mail.gmail.com>
To: Larry Garfield <larry@garfieldtech.com>
Cc: internals@lists.php.net
Content-Type: multipart/alternative; boundary=0016364d2a6ddeafb204b2814d16
Subject: Re: [PHP-DEV] 5.4 regression: non-existent sub-sub keys now have values
From: tyra3l@gmail.com (Ferenc Kovacs)

--0016364d2a6ddeafb204b2814d16
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Thu, Nov 24, 2011 at 9:12 PM, Larry Garfield <larry@garfieldtech.com>wro=
te:

> On 11/23/2011 12:13 PM, Lester Caine 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 se=
e
> 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.

"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)

the least amount of change to fix that isset would be:
isset($arr["package"]["attribs"]) && is_array($arr["package"]["attribs"])
&& isset($arr["package"]["attribs"]["version"]);
a little bit bit longer, right.

I think that if you can't guarantee your data, then you have to
check/sanitize it.
isset($array['foo']['bar']['baz']) could seem as a good idea, but don't
forget that if $array or one of it's element will be, for example an
object(not implementing ArrayAccess) then your isset will still fail with a
fatal error:

$arr =3D new StdClass;
isset($arr["package"]["attribs"]["version"]);
PHP Fatal error:  Cannot use object of type stdClass as array


> This is not a step forward by any metric I can imagine.  It's changing
> long-standing behavior for no real benefit I can see except perhaps stric=
t
> adherence to a doc page.  However, PHP has always been an "implementation
> is the standard" language, which means this is a language API change.
>

Stas and Etienne explained in this thread what was the reason introducing
this change(removing the string offset pseudo-type).


>
> Please roll it back to avoid breaking a crapton of existing, legitimate,
> non-buggy code.


This change would only break buggy code, where a string is checked as it
would be an array, and I would also argue about the crapton part.
As I mentioned before, if your input validation is so fragile, that you can
be bitten by this bug, then most probably you can also be bitten by the
same problem which exists in 5.3( see "$arr['package']['attribs'] is not an
array but a string").
The only real-world example where this breaks something was a test in the
PEAR codebase, albeit Daniel didn't mentioned the test, I have a feeling
that if we check it, we will see that the test itself is buggy (else it
couldn't have triggered this thread).

--=20
Ferenc Kov=C3=A1cs
@Tyr43l - http://tyrael.hu

--0016364d2a6ddeafb204b2814d16--