On Monday, December 05, 2011 11:21 AM, Laruence wrote:
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
On Mon, Dec 5, 2011 at 7:25 AM, Alan Knowles<a...@akbkhome.com> wrote:
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
On Monday, December 05, 2011 01:28 AM, Laruence wrote:
Hi:
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, Laruence<larue...@php.net> wrote:
+1.
thanks.
On Sun, Dec 4, 2011 at 10:05 PM, Ferenc Kovacs<tyr...@gmail.com> wrote:
On Sat, Dec 3, 2011 at 5:08 PM, Alan Knowles<a...@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=latest<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 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/
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
