I've had a look at making string offsets of strings a bit saner.
At present with the fix for array dereferencing : ?search=hello and a
test like isset($_GET['search']['name']) results in true, which is has
potential security problems and is very confusing for any programmer
finding and working out why something like that may be failing.
To solve this quite a few people agreed that not allowing non-numeric
string offsets on strings would be the smart way to go, the change is
going to break BC, so the idea is to at least not break it too badly...
This patch is a start.
https://bugs.php.net/patch-display.php?bug_id=60362&patch=first_effort_to_fix_this&revision=latest
It's been quite a while since I hacked on the engine, so the patch only
works reasonably well.. (see the FIXME on the tests at the bottom of the
patch.)
The patch changes the following:
- $s = "string"; $s['offset'] -- produces a warning (and returns an
empty string) - $s = "string"; $s['1'] -- works as before..
- $s = "string"; $s[true] $s[false] $s[0.1] -- give a notice (cast it
to an int if you want to get rid of the notice) - however work as before. - changes the warning on invalid indexes to say "Uninitialized or
invalid" rather than just "Uninitialized" - fixes most of the related tests
I would appreciate if someone with better engine knowledge would have a
look and work out what the "BAD" usage should return.
In theory.. the fetch_dim behavior should be return a empty string if an
invalid offset is used, or uninitialized zval if ISSET is calling it
Regards
Alan
This is ready for review now.
This resolves the worst behavior changes introduced by the dereferencing
of strings fix.
https://bugs.php.net/bug.php?id=60362
All tests (in Zend/tests) pass (after they have been modified to suit
the change)
Please review / comment...
Regards
Alan
I've had a look at making string offsets of strings a bit saner.
At present with the fix for array dereferencing : ?search=hello and a
test like isset($_GET['search']['name']) results in true, which is
has potential security problems and is very confusing for any
programmer finding and working out why something like that may be
failing.To solve this quite a few people agreed that not allowing non-numeric
string offsets on strings would be the smart way to go, the change is
going to break BC, so the idea is to at least not break it too badly...This patch is a start.
https://bugs.php.net/patch-display.php?bug_id=60362&patch=first_effort_to_fix_this&revision=latestIt's been quite a while since I hacked on the engine, so the patch
only works reasonably well.. (see the FIXME on the tests at the bottom
of the patch.)The patch changes the following:
- $s = "string"; $s['offset'] -- produces a warning (and returns an
empty string)- $s = "string"; $s['1'] -- works as before..
- $s = "string"; $s[true] $s[false] $s[0.1] -- give a notice (cast
it to an int if you want to get rid of the notice) - however work as
before.- changes the warning on invalid indexes to say "Uninitialized or
invalid" rather than just "Uninitialized"- fixes most of the related tests
I would appreciate if someone with better engine knowledge would have
a look and work out what the "BAD" usage should return.In theory.. the fetch_dim behavior should be return a empty string if
an invalid offset is used, or uninitialized zval if ISSET is calling itRegards
Alan
Seems like a good change +1.
This is ready for review now.
This resolves the worst behavior changes introduced by the dereferencing of
strings fix.
https://bugs.php.net/bug.php?id=60362All tests (in Zend/tests) pass (after they have been modified to suit the
change)Please review / comment...
Regards
AlanI've had a look at making string offsets of strings a bit saner.
At present with the fix for array dereferencing : ?search=hello and a
test like isset($_GET['search']['name']) results in true, which is has
potential security problems and is very confusing for any programmer finding
and working out why something like that may be failing.To solve this quite a few people agreed that not allowing non-numeric
string offsets on strings would be the smart way to go, the change is going
to break BC, so the idea is to at least not break it too badly...This patch is a start.
https://bugs.php.net/patch-display.php?bug_id=60362&patch=first_effort_to_fix_this&revision=latest
It's been quite a while since I hacked on the engine, so the patch only
works reasonably well.. (see the FIXME on the tests at the bottom of the
patch.)The patch changes the following:
* $s = "string"; $s['offset'] -- produces a warning (and returns an
empty string)
* $s = "string"; $s['1'] -- works as before..
* $s = "string"; $s[true] $s[false] $s[0.1] -- give a notice (cast it to
an int if you want to get rid of the notice) - however work as before.
* changes the warning on invalid indexes to say "Uninitialized or
invalid" rather than just "Uninitialized"
* fixes most of the related testsI would appreciate if someone with better engine knowledge would have a
look and work out what the "BAD" usage should return.In theory.. the fetch_dim behavior should be return a empty string if an
invalid offset is used, or uninitialized zval if ISSET is calling itRegards
Alan
hi!
same here.
Thanks for this patch!
Seems like a good change +1.
Cheers,
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
Hi,
2011/12/4 Alan Knowles alan@akbkhome.com:
This is ready for review now.
This resolves the worst behavior changes introduced by the dereferencing of
strings fix.
https://bugs.php.net/bug.php?id=60362All tests (in Zend/tests) pass (after they have been modified to suit the
change)Please review / comment...
Regards
AlanI've had a look at making string offsets of strings a bit saner.
At present with the fix for array dereferencing : ?search=hello and a
test like isset($_GET['search']['name']) results in true, which is has
potential security problems and is very confusing for any programmer finding
and working out why something like that may be failing.To solve this quite a few people agreed that not allowing non-numeric
string offsets on strings would be the smart way to go, the change is going
to break BC, so the idea is to at least not break it too badly...This patch is a start.
https://bugs.php.net/patch-display.php?bug_id=60362&patch=first_effort_to_fix_this&revision=latest
It's been quite a while since I hacked on the engine, so the patch only
works reasonably well.. (see the FIXME on the tests at the bottom of the
patch.)The patch changes the following:
* $s = "string"; $s['offset'] -- produces a warning (and returns an
empty string)
* $s = "string"; $s['1'] -- works as before..
* $s = "string"; $s[true] $s[false] $s[0.1] -- give a notice (cast it to
an int if you want to get rid of the notice) - however work as before.
* changes the warning on invalid indexes to say "Uninitialized or
invalid" rather than just "Uninitialized"
* fixes most of the related testsI would appreciate if someone with better engine knowledge would have a
look and work out what the "BAD" usage should return.In theory.. the fetch_dim behavior should be return a empty string if an
invalid offset is used, or uninitialized zval if ISSET is calling itRegards
Alan--
Take a look at Zend/tests/offset_assign.phpt, there is a path hardcoded.
--
Regards,
Felipe Pena
Hi Alan:
Thanks so much for the patch. Looking at the source I noticed some
small things (care to build a bike shed with me? :).
Many of the test EXPECT's have %d changed to hard coded line numbers and
others have %s changed to hard coded file names. In some of them,
that's all that changed. I assume that will be cleaned up before
committing.
There are some extraneous whitespace changes in the source.
I tried to apply the patch to 54 and trunk via "patch < bug.diff"
(which usually works fine) and all hunks failled. Not sure what I'm
doing wrong.
Thanks,
--Dan
--
T H E A N A L Y S I S A N D S O L U T I O N S C O M P A N Y
data intensive web and database programming
http://www.AnalysisAndSolutions.com/
4015 7th Ave #4, Brooklyn NY 11232 v: 718-854-0335 f: 718-854-0409
Hi Alan:
I tried to apply the patch to 54 and trunk via "patch < bug.diff"
(which usually works fine) and all hunks failled. Not sure what I'm
doing wrong.
Ah, I needed to do "patch -p0 < bug.diff" But now that I applied it to 54,
running make chokes...
[ snip ]
/bin/bash /php/php/php-src/branches/PHP_5_4/libtool --silent
--preserve-dup-deps --mode=compile cc -Isapi/cli/
-I/php/php/php-src/branches/PHP_5_4/sapi/cli/ -DPHP_ATOM_INC
-I/php/php/php-src/branches/PHP_5_4/include
-I/php/php/php-src/branches/PHP_5_4/main
-I/php/php/php-src/branches/PHP_5_4
-I/php/php/php-src/branches/PHP_5_4/ext/date/lib
-I/php/php/php-src/branches/PHP_5_4/ext/ereg/regex
-I/usr/include/libxml2
-I/php/php/php-src/branches/PHP_5_4/ext/mbstring/oniguruma
-I/php/php/php-src/branches/PHP_5_4/ext/mbstring/libmbfl
-I/php/php/php-src/branches/PHP_5_4/ext/mbstring/libmbfl/mbfl
-I/usr/include/postgresql
-I/php/php/php-src/branches/PHP_5_4/ext/sqlite3/libsqlite
-I/php/php/php-src/branches/PHP_5_4/TSRM
-I/php/php/php-src/branches/PHP_5_4/Zend -I/usr/include -g -O2
-fvisibility=hidden -DZEND_SIGNALS -c
/php/php/php-src/branches/PHP_5_4/sapi/cli/php_cli.c -o
sapi/cli/php_cli.lo
make: *** [Zend/zend_execute.lo] Error 1
make: *** Waiting for unfinished jobs....
That's after running make clean and vcsclean. And before that having
built 54 successfully with the exact same configure options.
Thanks,
--Dan
--
T H E A N A L Y S I S A N D S O L U T I O N S C O M P A N Y
data intensive web and database programming
http://www.AnalysisAndSolutions.com/
4015 7th Ave #4, Brooklyn NY 11232 v: 718-854-0335 f: 718-854-0409
I've had a look at making string offsets of strings a bit saner.
At present with the fix for array dereferencing : ?search=hello and a
test like isset($_GET['search']['name']) results in true, which is has
potential security problems and is very confusing for any programmer
finding and working out why something like that may be failing.To solve this quite a few people agreed that not allowing non-numeric
string offsets on strings would be the smart way to go, the change is going
to break BC, so the idea is to at least not break it too badly...This patch is a start.
https://bugs.php.net/patch-display.php?bug_id=60362&
patch=first_effort_to_fix_**this&revision=latesthttps://bugs.php.net/patch-display.php?bug_id=60362&patch=first_effort_to_fix_this&revision=latestIt's been quite a while since I hacked on the engine, so the patch only
works reasonably well.. (see the FIXME on the tests at the bottom of the
patch.)The patch changes the following:
- $s = "string"; $s['offset'] -- produces a warning (and returns an
empty string)- $s = "string"; $s['1'] -- works as before..
- $s = "string"; $s[true] $s[false] $s[0.1] -- give a notice (cast it to
an int if you want to get rid of the notice) - however work as before.- changes the warning on invalid indexes to say "Uninitialized or
invalid" rather than just "Uninitialized"- fixes most of the related tests
I think that those changes are pretty much in line with the discussion that
we had.
Thanks for fixing this!
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
+1.
thanks.
I've had a look at making string offsets of strings a bit saner.
At present with the fix for array dereferencing : ?search=hello and a
test like isset($_GET['search']['name']) results in true, which is has
potential security problems and is very confusing for any programmer
finding and working out why something like that may be failing.To solve this quite a few people agreed that not allowing non-numeric
string offsets on strings would be the smart way to go, the change is going
to break BC, so the idea is to at least not break it too badly...This patch is a start.
https://bugs.php.net/patch-display.php?bug_id=60362&
patch=first_effort_to_fix_**this&revision=latesthttps://bugs.php.net/patch-display.php?bug_id=60362&patch=first_effort_to_fix_this&revision=latestIt's been quite a while since I hacked on the engine, so the patch only
works reasonably well.. (see the FIXME on the tests at the bottom of the
patch.)The patch changes the following:
* $s = "string"; $s['offset'] -- produces a warning (and returns an
empty string)
* $s = "string"; $s['1'] -- works as before..
* $s = "string"; $s[true] $s[false] $s[0.1] -- give a notice (cast it to
an int if you want to get rid of the notice) - however work as before.
* changes the warning on invalid indexes to say "Uninitialized or
invalid" rather than just "Uninitialized"
* fixes most of the related testsI think that those changes are pretty much in line with the discussion that
we had.
Thanks for fixing this!--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
--
Laruence Xinchen Hui
http://www.laruence.com/
Hi,
+1.
thanks.
I've had a look at making string offsets of strings a bit saner.
At present with the fix for array dereferencing : ?search=hello and a
test like isset($_GET['search']['name']) results in true, which is has
potential security problems and is very confusing for any programmer
finding and working out why something like that may be failing.To solve this quite a few people agreed that not allowing non-numeric
string offsets on strings would be the smart way to go, the change is
going
to break BC, so the idea is to at least not break it too badly...This patch is a start.
https://bugs.php.net/patch-display.php?bug_id=60362&
patch=first_effort_to_fix_**this&revision=latest<
https://bugs.php.net/patch-display.php?bug_id=60362&patch=first_effort_to_fix_this&revision=latestIt's been quite a while since I hacked on the engine, so the patch only
works reasonably well.. (see the FIXME on the tests at the bottom of the
patch.)The patch changes the following:
- $s = "string"; $s['offset'] -- produces a warning (and returns an
empty string)- $s = "string"; $s['1'] -- works as before..
- $s = "string"; $s[true] $s[false] $s[0.1] -- give a notice (cast it
to
an int if you want to get rid of the notice) - however work as before.- changes the warning on invalid indexes to say "Uninitialized or
invalid" rather than just "Uninitialized"- fixes most of the related tests
What about other edge cases like $string[' 2 '], $string['2foo']?
I like the idea of the patch, I just find it a bit inconsistent for
$s['offset'] to return an empty string while other cases of implicit
conversions work as before. It doesn't bring much to return an empty string
instead of the first char. I believe every case should work as before,
throwing a notice is enough IMO.
Also, you don't mention whether your patch modifies the behavior of
isset(), is $str = "foo"; isset($str['bar']) true or false ?
Best,
I think that those changes are pretty much in line with the discussion
that
we had.
Thanks for fixing this!--
Ferenc Kovács
@Tyr43l - http://tyrael.hu--
Laruence Xinchen Hui
http://www.laruence.com/--
--
Etienne Kneuss
http://www.colder.ch
Hi,
+1.
thanks.
I've had a look at making string offsets of strings a bit saner.
At present with the fix for array dereferencing : ?search=hello and a
test like isset($_GET['search']['name']) results in true, which is has
potential security problems and is very confusing for any programmer
finding and working out why something like that may be failing.To solve this quite a few people agreed that not allowing non-numeric
string offsets on strings would be the smart way to go, the change is
going
to break BC, so the idea is to at least not break it too badly...This patch is a start.
https://bugs.php.net/patch-display.php?bug_id=60362&patch=first_effort_to_fix_**this&revision=latesthttps://bugs.php.net/patch-display.php?bug_id=60362&patch=first_effort_to_fix_this&revision=latest
It's been quite a while since I hacked on the engine, so the patch only
works reasonably well.. (see the FIXME on the tests at the bottom of
the
patch.)The patch changes the following:
* $s = "string"; $s['offset'] -- produces a warning (and returns an
empty string)
* $s = "string"; $s['1'] -- works as before..
* $s = "string"; $s[true] $s[false] $s[0.1] -- give a notice (cast it
to
an int if you want to get rid of the notice) - however work as before.
* changes the warning on invalid indexes to say "Uninitialized or
invalid" rather than just "Uninitialized"
* fixes most of the related testsWhat about other edge cases like $string[' 2 '], $string['2foo']?
I like the idea of the patch, I just find it a bit inconsistent for
$s['offset'] to return an empty string while other cases of implicit
conversions work as before. It doesn't bring much to return an empty string
instead of the first char. I believe every case should work as before,
throwing a notice is enough IMO.
I agree after a deep think, a notice is enough. thanksAlso, you don't mention whether your patch modifies the behavior of isset(),
is $str = "foo"; isset($str['bar']) true or false ?Best,
I think that those changes are pretty much in line with the discussion
that
we had.
Thanks for fixing this!--
Ferenc Kovács
@Tyr43l - http://tyrael.hu--
Laruence Xinchen Hui
http://www.laruence.com/--
--
Etienne Kneuss
http://www.colder.ch
--
Laruence Xinchen Hui
http://www.laruence.com/
Hi:
I have submit a new patch based on the origin patch, which only
trigger notice when string offset cast occurred.
thanks
+1.
thanks.
I've had a look at making string offsets of strings a bit saner.
At present with the fix for array dereferencing : ?search=hello and a
test like isset($_GET['search']['name']) results in true, which is has
potential security problems and is very confusing for any programmer
finding and working out why something like that may be failing.To solve this quite a few people agreed that not allowing non-numeric
string offsets on strings would be the smart way to go, the change is going
to break BC, so the idea is to at least not break it too badly...This patch is a start.
https://bugs.php.net/patch-display.php?bug_id=60362&
patch=first_effort_to_fix_**this&revision=latesthttps://bugs.php.net/patch-display.php?bug_id=60362&patch=first_effort_to_fix_this&revision=latestIt's been quite a while since I hacked on the engine, so the patch only
works reasonably well.. (see the FIXME on the tests at the bottom of the
patch.)The patch changes the following:
* $s = "string"; $s['offset'] -- produces a warning (and returns an
empty string)
* $s = "string"; $s['1'] -- works as before..
* $s = "string"; $s[true] $s[false] $s[0.1] -- give a notice (cast it to
an int if you want to get rid of the notice) - however work as before.
* changes the warning on invalid indexes to say "Uninitialized or
invalid" rather than just "Uninitialized"
* fixes most of the related testsI think that those changes are pretty much in line with the discussion that
we had.
Thanks for fixing this!--
Ferenc Kovács
@Tyr43l - http://tyrael.hu--
Laruence Xinchen Hui
http://www.laruence.com/
--
Laruence Xinchen Hui
http://www.laruence.com/
A few answers...
$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..
$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.
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
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.
Regards
Alan
Hi:
I have submit a new patch based on the origin patch, which only
trigger notice when string offset cast occurred.thanks
+1.
thanks.
I've had a look at making string offsets of strings a bit saner.
At present with the fix for array dereferencing : ?search=hello and a
test like isset($_GET['search']['name']) results in true, which is has
potential security problems and is very confusing for any programmer
finding and working out why something like that may be failing.To solve this quite a few people agreed that not allowing non-numeric
string offsets on strings would be the smart way to go, the change is going
to break BC, so the idea is to at least not break it too badly...This patch is a start.
https://bugs.php.net/patch-display.php?bug_id=60362&
patch=first_effort_to_fix_**this&revision=latesthttps://bugs.php.net/patch-display.php?bug_id=60362&patch=first_effort_to_fix_this&revision=latestIt's been quite a while since I hacked on the engine, so the patch only
works reasonably well.. (see the FIXME on the tests at the bottom of the
patch.)The patch changes the following:
- $s = "string"; $s['offset'] -- produces a warning (and returns an
empty string)- $s = "string"; $s['1'] -- works as before..
- $s = "string"; $s[true] $s[false] $s[0.1] -- give a notice (cast it to
an int if you want to get rid of the notice) - however work as before.- changes the warning on invalid indexes to say "Uninitialized or
invalid" rather than just "Uninitialized"- fixes most of the related tests
I think that those changes are pretty much in line with the discussion that
we had.
Thanks for fixing this!--
Ferenc Kovács
@Tyr43l - http://tyrael.hu--
Laruence Xinchen Hui
http://www.laruence.com/
Hi:
I think we can only trigger notice, then act the same behavior as
before. include isset.
this would introduce the fewest bc breaks,
what do you think?
thanks
A few answers...
$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..$s = "string"; $s[' 2 ']; and $s['2foo'] both emit errors, and return
false from isset() - the code pretty much usesis_numeric()internally. -
I'm pretty sure these would be un-intentional bugs, so behaving as such
seems consistent.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 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.Regards
AlanHi:
I have submit a new patch based on the origin patch, which only
trigger notice when string offset cast occurred.thanks
On Sun, Dec 4, 2011 at 10:25 PM, Laruencelaruence@php.net wrote:
+1.
thanks.
On Sun, Dec 4, 2011 at 10:05 PM, Ferenc Kovacstyra3l@gmail.com wrote:
On Sat, Dec 3, 2011 at 5:08 PM, Alan Knowlesalan@akbkhome.com wrote:
I've had a look at making string offsets of strings a bit saner.
At present with the fix for array dereferencing : ?search=hello and a
test like isset($_GET['search']['name']) results in true, which is has
potential security problems and is very confusing for any programmer
finding and working out why something like that may be failing.To solve this quite a few people agreed that not allowing non-numeric
string offsets on strings would be the smart way to go, the change is
going
to break BC, so the idea is to at least not break it too badly...This patch is a start.
https://bugs.php.net/patch-display.php?bug_id=60362&patch=first_effort_to_fix_**this&revision=latesthttps://bugs.php.net/patch-display.php?bug_id=60362&patch=first_effort_to_fix_this&revision=latest
It's been quite a while since I hacked on the engine, so the patch only
works reasonably well.. (see the FIXME on the tests at the bottom of
the
patch.)The patch changes the following:
* $s = "string"; $s['offset'] -- produces a warning (and returns an
empty string)
* $s = "string"; $s['1'] -- works as before..
* $s = "string"; $s[true] $s[false] $s[0.1] -- give a notice (cast it
to
an int if you want to get rid of the notice) - however work as before.
* changes the warning on invalid indexes to say "Uninitialized or
invalid" rather than just "Uninitialized"
* fixes most of the related testsI think that those changes are pretty much in line with the discussion
that
we had.
Thanks for fixing this!--
Ferenc Kovács
@Tyr43l - http://tyrael.hu--
Laruence Xinchen Hui
http://www.laruence.com/--
--
Laruence Xinchen Hui
http://www.laruence.com/
Hi:
I think we can only trigger notice, then act the same behavior as
before. include isset.this would introduce the fewest bc breaks,
I think the isset behavior should be fixed (as the BC will be broken
anyway with deferenced strings), for the other fix, If it was my project
(haha), I'd release the empty string change, and revert it quickly in
the next release if there were any serious bug reports saying working
code was affected by it.
Not sure if that's a great approach, but I'd be interested in hearing
opinions about it.
Regards
Alan
what do you think?thanks
A few answers...
$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..$s = "string"; $s[' 2 ']; and $s['2foo'] both emit errors, and return
false from isset() - the code pretty much usesis_numeric()internally. -
I'm pretty sure these would be un-intentional bugs, so behaving as such
seems consistent.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 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.Regards
AlanHi:
I have submit a new patch based on the origin patch, which only
trigger notice when string offset cast occurred.thanks
+1.
thanks.
I've had a look at making string offsets of strings a bit saner.
At present with the fix for array dereferencing : ?search=hello and a
test like isset($_GET['search']['name']) results in true, which is has
potential security problems and is very confusing for any programmer
finding and working out why something like that may be failing.To solve this quite a few people agreed that not allowing non-numeric
string offsets on strings would be the smart way to go, the change is
going
to break BC, so the idea is to at least not break it too badly...This patch is a start.
https://bugs.php.net/patch-display.php?bug_id=60362&patch=first_effort_to_fix_**this&revision=latesthttps://bugs.php.net/patch-display.php?bug_id=60362&patch=first_effort_to_fix_this&revision=latest
It's been quite a while since I hacked on the engine, so the patch only
works reasonably well.. (see the FIXME on the tests at the bottom of
the
patch.)The patch changes the following:
- $s = "string"; $s['offset'] -- produces a warning (and returns an
empty string)- $s = "string"; $s['1'] -- works as before..
- $s = "string"; $s[true] $s[false] $s[0.1] -- give a notice (cast it
to
an int if you want to get rid of the notice) - however work as before.- changes the warning on invalid indexes to say "Uninitialized or
invalid" rather than just "Uninitialized"- fixes most of the related tests
I think that those changes are pretty much in line with the discussion
that
we had.
Thanks for fixing this!--
Ferenc Kovács
@Tyr43l - http://tyrael.hu--
Laruence Xinchen Hui
http://www.laruence.com/
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 usesis_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 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 usesis_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 onHere 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
--
--
Laruence Xinchen Hui
http://www.laruence.com/
Hi!
I think only trigger notice when a convertion of string to numberindex is a good way (trivial bc break).
This however doesn't solve the problem with isset() (which still
produces true then). BTW, notices there may lead to interruption
problems (imagine some error handler that may change variables that
being involved in the expression).
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
This patch is a start.
https://bugs.php.net/patch-display.php?bug_id=60362&patch=first_effort_to_fix_this&revision=latestIt's been quite a while since I hacked on the engine, so the patch only
works reasonably well.. (see the FIXME on the tests at the bottom of the
patch.)The patch changes the following:
- $s = "string"; $s['offset'] -- produces a warning (and returns an
empty string)- $s = "string"; $s['1'] -- works as before..
- $s = "string"; $s[true] $s[false] $s[0.1] -- give a notice (cast it
to an int if you want to get rid of the notice) - however work as before.- changes the warning on invalid indexes to say "Uninitialized or
invalid" rather than just "Uninitialized"- fixes most of the related tests
I think it's bad to add another set of casting rules to the language.
I'd prefer splitting the string offset operator from array offset.
$a = [0,1,2,3];
$s = "string";
$a{0}; // wrong
$s{0}; // ok
$a[0]; // ok
$s[0]; // wrong
Yes, something like this has been discussed before, back and forth and
doing this is too late for 5.4, but let's look to 5.5.
johannes
2011/12/4 Johannes Schlüter johannes@schlueters.de
This patch is a start.
https://bugs.php.net/patch-display.php?bug_id=60362&patch=first_effort_to_fix_this&revision=latest
It's been quite a while since I hacked on the engine, so the patch only
works reasonably well.. (see the FIXME on the tests at the bottom of the
patch.)The patch changes the following:
- $s = "string"; $s['offset'] -- produces a warning (and returns an
empty string)- $s = "string"; $s['1'] -- works as before..
- $s = "string"; $s[true] $s[false] $s[0.1] -- give a notice (cast it
to an int if you want to get rid of the notice) - however work as before.- changes the warning on invalid indexes to say "Uninitialized or
invalid" rather than just "Uninitialized"- fixes most of the related tests
I think it's bad to add another set of casting rules to the language.
I'd prefer splitting the string offset operator from array offset.$a = [0,1,2,3];
$s = "string";$a{0}; // wrong
$s{0}; // ok
$a[0]; // ok
$s[0]; // wrongYes, something like this has been discussed before, back and forth and
doing this is too late for 5.4, but let's look to 5.5.
this would have a much bigger impact than the currently proposed change
(more people use [] than $foo['bar'] where $foo is a string.
and while I think this would make the language much more cleaner, and
explicit, this would be a major U-turn.
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu