Re: Fixing string offsets of strings.

On Sun, Dec 18, 2011 at 7:54 AM, Stas Malyshev <smalyshev@sugarcrm.com> wrote: > Hi! > > I think the idea behind this patch is good. I'll do final checks and apply > it tomorrow if no objections heard until then. > > Some notes: > > >> $s = "string";  isset($s['offset']) returns false >> This is pretty critical, as it's the only way to detect this situation, >> and ensure that array tests do not return positive results for strings. >> It also catches an obvious, but previously hidden and probably serious >> bugs in the PHP code.. > > > This change concerns me a bit since it's a behavior change and provided that > most of the people aren't actually aware that $s['offset'] means $s[0] it > may lead to subtle code breakage. Then again, doing $s['offset'] leads to > code breakage right now - I just encountered another bug caused by this in > production code mere days ago, code was checking that $s['offset'] is set > without actually checking that $s is an array, and it happened to be not.... > > >> $s = "string";  $s['  2  ']; and $s['2foo'] both emit >> errors, and return >> false from isset() - the code pretty much uses is_numeric() internally. >> - I'm pretty sure these would be un-intentional bugs, so behaving as >> such seems consistent. > > > Agreed, I think is_numeric is the right way. > > >> I do not mind either way if $s['offset'] returns the first char or an >> empty string, however it seemed a little inconstant to return false on > > > Here I am conflicted. The right thing would be to return empty but it may > cause some breakage. Then again, doing what we do now causes breakage right > now too (not new in 5.4, same breakage happening in 5.3 too) so I'm inclined > to go with false here unless somebody gives me a use case where it's useful > for anything. > > >> isset(), yet actually return a string. (although technically an empty >> string does kind of indicate it is 'isset') - it at least leaves the >> engine in a consistent state. Perhaps that change can wait's until 5.5.. >> thoughts... >> >> As for dropping the syntax for Strings eventually.. It would be nice, >> but I'm not sure it is feasible anymore unfortunately. > > > We tried to move to {} syntax but that didn't work, and probably isn't > possible anymore without massive breakage. > -- > Stanislav Malyshev, Software Architect > SugarCRM: http://www.sugarcrm.com/ > (408)454-6900 ext. 227 > Hi: I think only trigger notice when a convertion of string to number index is a good way (trivial bc break). :) thanks > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > -- Laruence  Xinchen Hui http://www.laruence.com/