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

Alan Knowles Sat, 17 Dec 2011 21:14:34 -0800

This should implement the isset() return false, and accessing producinga warning (but 'less' BC by returning the first character)

https://bugs.php.net/patch-display.php?bug_id=60362&patch=isset_changed_warning_only_on_access.patch&revision=latest<https://bugs.php.net/patch-display.php?bug_id=60362&patch=isset_changed_warning_only_on_access.patch&revision=latest>

I did end up changing most of the error messages in the affected teststo "%s on line %d".... is that the prefered way of doing it?


Regards
Alan


On Sunday, December 18, 2011 12:16 PM, Alan Knowles wrote:

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

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

Reply via email to