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

On Thu, Nov 24, 2011 at 11:03 PM, Larry Garfield <larry@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<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() 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