Re: Re: [PHP-DEV] Fixing string offsets of strings.

From: Date: Sun, 18 Dec 2011 04:16:17 +0000
Subject: Re: Re: [PHP-DEV] Fixing string offsets of strings.
References: 1  Groups: php.internals 
Request: Send a blank email to [email protected] to get a copy of this message
I think I got what you where after - not to clear on the 
$s['offset']  should result in empty or 'o' 

This is the latest relivant patch
https://bugs.php.net/patch-display.php?bug_id=60362&patch=fix_disabling_bad_string_offsets&revision=latest

In that patch $s['offset']  would return an empty string
to change that behaviour so it returns first character as before, just remove the lines in
zend_execute, that munge the offset to -1

if (invalid_string) {
      dim->value.lval = -1;
}

As I said before, isset() should not show a warning, and should return false (yes it's a BC,
but I do not think it's significant)

Once check I need to do if the -1 munge is removed, is to see if
isset($array['a_string']['test'])  return true or false.. (as I'm not sure
if the vm handles the array dereferencing on the secondary array or the execute calls do it.)

Regards
Alan


 --- On 18/Dec/2011, Stas Malyshev 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



Thread (29 messages)

« previous php.internals (#56945) next »