Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:56601
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: <4ECEBF46.3090206@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>
	<CAH-PCH5OSpjo8x_Y3Burd2uDeOkb6Spf7BcQ37tAmDws-V++uQ@mail.gmail.com>
	<4ECEBF46.3090206@garfieldtech.com>
Date: Thu, 24 Nov 2011 23:19:05 +0100
Message-ID: <CAH-PCH42h7uJOGMdewcFPW-fBU8ptgfU=EnqFAtMpzLM9yqmig@mail.gmail.com>
To: Larry Garfield <larry@garfieldtech.com>
Cc: internals@lists.php.net
Content-Type: multipart/alternative; boundary=001636499237b03ee804b2826c99
Subject: Re: [PHP-DEV] 5.4 regression: non-existent sub-sub keys now have values
From: tyra3l@gmail.com (Ferenc Kovacs)

--001636499237b03ee804b2826c99
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Thu, Nov 24, 2011 at 11:03 PM, Larry Garfield <larry@garfieldtech.com>wr=
ote:

> On 11/24/2011 02:58 PM, Ferenc Kovacs wrote:
>
>> On Thu, Nov 24, 2011 at 9:12 PM, Larry Garfield<larry@garfieldtech.**com=
<larry@garfieldtech.com>
>> >wrote:
>>
>>  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() o=
r
>>> 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 tu=
rn
>>> 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 o=
f
>> 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 ke=
ep
> 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 t=
he
>> 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: cann=
ot
>> 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.

--=20
Ferenc Kov=C3=A1cs
@Tyr43l - http://tyrael.hu

--001636499237b03ee804b2826c99--