On Thu, Nov 24, 2011 at 11:03 PM, Larry Garfield <la...@garfieldtech.com>wrote:
> On 11/24/2011 02:58 PM, Ferenc Kovacs wrote:
>
>> On Thu, Nov 24, 2011 at 9:12 PM, Larry
>> Garfield<larry@garfieldtech.**com<la...@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() 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
