5.4 regression: non-existent sub-sub keys now have values

13 years ago by Daniel Convissor — view source

unread

Hi Folks:

I just stumbled upon a regression in 5.4. In an array, a sub-sub-key of
an existing key is now returning a letter of the value indexed by the
main key. I'm raising it here so it doesn't get lost.

https://bugs.php.net/bug.php?id=60362

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

13 years ago by Etienne Kneuss — view source

unread

Hi,

AFAIK this is one of the change that was explicitly put in to make it
more consistent.

Before, you had:

$string = "asd";
$string[0] == "a"
but:
$string[0][0] == Fatal: cannot use string offset as an array

while $string[0] == "a", it makes no sense for $string[0][0] not to
return "a" again, since

$str = "a";

$str[0] == "a";

Best Regards,

On Wed, Nov 23, 2011 at 02:50, Daniel Convissor
danielc@analysisandsolutions.com wrote:

Hi Folks:

I just stumbled upon a regression in 5.4. In an array, a sub-sub-key of
an existing key is now returning a letter of the value indexed by the
main key. I'm raising it here so it doesn't get lost.

https://bugs.php.net/bug.php?id=60362

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

--

--
Etienne Kneuss
http://www.colder.ch

13 years ago by Daniel Convissor — view source

unread

Hi Etienne:

AFAIK this is one of the change that was explicitly put in to make it
more consistent.

But now it breaks code in the wild. I came across this due to an
isset() in PEAR now passing when it didn't before.

Thanks,

--Dan

13 years ago by Etienne Kneuss — view source

unread

On Wed, Nov 23, 2011 at 03:31, Daniel Convissor
danielc@analysisandsolutions.com wrote:

Hi Etienne:

AFAIK this is one of the change that was explicitly put in to make it
more consistent.

But now it breaks code in the wild. I came across this due to an
isset() in PEAR now passing when it didn't before.

PEAR is definitely doing some strange things :) Is it checking indices
of a variables without checking first if it's an array?
Sounds like the comeback of "Please don't break bad code!" seen with is_a :)

IMO it still makes sense to make that change for 5_4, makes everything
less magic and more consistent.

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

--
Etienne Kneuss
http://www.colder.ch

13 years ago by Stas Malyshev — view source

unread

Hi!

PEAR is definitely doing some strange things :) Is it checking indices
of a variables without checking first if it's an array?
Sounds like the comeback of "Please don't break bad code!" seen with is_a :)

I think somebody should start a code cleanup effort :) We can't build
all PHP development around preserving every line of bad code doing bad
things around. We care about BC but there should be some limits, and bug
compatibility is beyond those limits.

We long had array access syntax on strings. Expression $a[0]["foo"]
where $a[0] is a string has long been treated as $a[0][0] and returned
first letter of $a[0] (btw code doing this is already a bug, even though
PHP lets you get away with it - but I'd put some notice there). Since
this letter is a string, it should behave the same as other strings.
Otherwise this code:
echo $a[0]["foo"]["bar"]
and this one:
$b = $a[0]["foo"];
echo $b["bar"];

behave differently, which was a huge WTF and can only be considered a
bug. Any code relying on this should be considered a bug too and fixed
immediately. I can not think of any legitimate reason for the code to
rely on such things.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by Lester Caine — view source

unread

Etienne Kneuss wrote:

AFAIK this is one of the change that was explicitly put in to make it

more consistent.

But now it breaks code in the wild. I came across this due to an
isset() in PEAR now passing when it didn't before.
PEAR is definitely doing some strange things:) Is it checking indices
of a variables without checking first if it's an array?
Sounds like the comeback of "Please don't break bad code!" seen with is_a:)

IMO it still makes sense to make that change for 5_4, makes everything
less magic and more consistent.

I'm having a lot of trouble with working out WHAT is going on here at all.

https://bugs.php.net/bug.php?id=60362 as far as I can tell is using array and
sub array elements? Strings don't come into the equation? So I check if I've
populated the sub-array be seeing if a sub array element exists, such as would
happen if I've populated the array in a previous scan through, but now I'm
getting sub array elements caused by some conversion of the string making up the
element above?

Now that this activity has been highlighted it may well explain why I'm getting
problems with a section of code that is building a complex tree structure array
from GEDCOM data strings ... since the code is not my own working out where
things were breaking down was a problem I simply do not have time to
investigate. It currently fails on PHP5.4 and it took some time to tidy the code
on 5.3 ... and that still only runs with warnings switched off :(

So what is the current preferred method of checking if sub array elements have
been created manually, rather than as 'less magic' string elements?

--
Lester Caine - G8HFL

Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk//
Firebird - http://www.firebirdsql.org/index.php

13 years ago by Laruence — view source

unread

Hi:

I think it is really need make some difference between {} and []

thanks

Etienne Kneuss wrote:

AFAIK this is one of the change that was explicitly put in to make it

more consistent.

But now it breaks code in the wild. I came across this due to an
isset() in PEAR now passing when it didn't before.

PEAR is definitely doing some strange things:) Is it checking indices
of a variables without checking first if it's an array?
Sounds like the comeback of "Please don't break bad code!" seen with
is_a:)

IMO it still makes sense to make that change for 5_4, makes everything
less magic and more consistent.

I'm having a lot of trouble with working out WHAT is going on here at all.

https://bugs.php.net/bug.php?id=60362 as far as I can tell is using array
and sub array elements? Strings don't come into the equation? So I check if
I've populated the sub-array be seeing if a sub array element exists, such
as would happen if I've populated the array in a previous scan through, but
now I'm getting sub array elements caused by some conversion of the string
making up the element above?

Now that this activity has been highlighted it may well explain why I'm
getting problems with a section of code that is building a complex tree
structure array from GEDCOM data strings ... since the code is not my own
working out where things were breaking down was a problem I simply do not
have time to investigate. It currently fails on PHP5.4 and it took some time
to tidy the code on 5.3 ... and that still only runs with warnings switched
off :(

So what is the current preferred method of checking if sub array elements
have been created manually, rather than as 'less magic' string elements?

--
Lester Caine - G8HFL

Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk//
Firebird - http://www.firebirdsql.org/index.php

--

--
Laruence Xinchen Hui
http://www.laruence.com/

13 years ago by Ferenc Kovacs — view source

unread

Etienne Kneuss wrote:

AFAIK this is one of the change that was explicitly put in to make it

more consistent.

But now it breaks code in the wild. I came across this due to an
isset() in PEAR now passing when it didn't before.

PEAR is definitely doing some strange things:) Is it checking indices
of a variables without checking first if it's an array?
Sounds like the comeback of "Please don't break bad code!" seen with
is_a:)

IMO it still makes sense to make that change for 5_4, makes everything
less magic and more consistent.

I'm having a lot of trouble with working out WHAT is going on here at all.

https://bugs.php.net/bug.php?**id=60362 https://bugs.php.net/bug.php?id=60362as far as I can tell is using array and sub array elements? Strings don't
come into the equation? So I check if I've populated the sub-array be
seeing if a sub array element exists, such as would happen if I've
populated the array in a previous scan through, but now I'm getting sub
array elements caused by some conversion of the string making up the
element above?

Now that this activity has been highlighted it may well explain why I'm
getting problems with a section of code that is building a complex tree
structure array from GEDCOM data strings ... since the code is not my own
working out where things were breaking down was a problem I simply do not
have time to investigate. It currently fails on PHP5.4 and it took some
time to tidy the code on 5.3 ... and that still only runs with warnings
switched off :(

So what is the current preferred method of checking if sub array elements
have been created manually, rather than as 'less magic' string elements?

some clarification:
in the test script, Daniel declared $arr as:
$arr = array('exists' => 'foo');
so when he tests
$arr['exists']['non_existent']
PHP will see that $arr['exists'] is a string, and it will convert the
'non_existent' index to int(0) and that will return the same
as $arr['exists'][0]: 'f'
If he would have defined $arr['exists'] as an array, then the code would
work as he expected, empty would returned true
for $arr['exists']['non_existent']

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Lester Caine — view source

unread

Ferenc Kovacs wrote:

some clarification:
in the test script, Daniel declared $arr as:
$arr = array('exists' => 'foo');
so when he tests
$arr['exists']['non_existent']
PHP will see that $arr['exists'] is a string, and it will convert the
'non_existent' index to int(0) and that will return the same
as $arr['exists'][0]: 'f'
If he would have defined $arr['exists'] as an array, then the code would work as
he expected, empty would returned true for $arr['exists']['non_existent']

The bit I am missing here is the statement that $arr = array ... has NOT created
an array with an element ['exists'] ... I suspect that this is perhaps where the
code I am looking at is breaking down ... how SHOULD you define the array so
that it is an array? So that $family = array ( 'fam1' => 'JONES' ); is the base
for the JONES family rather than simply a string. All of this USED to be simple,
but it seems to be getting so cryptic that this is what is causing the trouble :(

--
Lester Caine - G8HFL

13 years ago by Ferenc Kovacs — view source

unread

Ferenc Kovacs wrote:

some clarification:
in the test script, Daniel declared $arr as:
$arr = array('exists' => 'foo');
so when he tests
$arr['exists']['non_existent']
PHP will see that $arr['exists'] is a string, and it will convert the
'non_existent' index to int(0) and that will return the same
as $arr['exists'][0]: 'f'
If he would have defined $arr['exists'] as an array, then the code would
work as
he expected, empty would returned true for $arr['exists']['non_existent']

The bit I am missing here is the statement that $arr = array ... has NOT
created an array with an element ['exists']

yes it did, but ['exists'] wasn't defined as an array, but as a string.

... I suspect that this is perhaps where the code I am looking at is
breaking down ... how SHOULD you define the array so that it is an array?
So that $family = array ( 'fam1' => 'JONES' ); is the base for the JONES
family rather than simply a string. All of this USED to be simple, but it
seems to be getting so cryptic that this is what is causing the trouble :(

I think you just need a coffee.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Lester Caine — view source

unread

Ferenc Kovacs wrote:

On Wed, Nov 23, 2011 at 10:51 AM, Lester Caine <lester@lsces.co.uk
mailto:lester@lsces.co.uk> wrote:

Ferenc Kovacs wrote:

    some clarification:
    in the test script, Daniel declared $arr as:
    $arr = array('exists' => 'foo');
    so when he tests
    $arr['exists']['non_existent']
    PHP will see that $arr['exists'] is a string, and it will convert the
    'non_existent' index to int(0) and that will return the same
    as $arr['exists'][0]: 'f'
    If he would have defined $arr['exists'] as an array, then the code would
    work as
    he expected, empty would returned true for $arr['exists']['non_existent']


The bit I am missing here is the statement that $arr = array ... has NOT
created an array with an element ['exists']

yes it did, but ['exists'] wasn't defined as an array, but as a string.

THAT was what was confusing things ... but I wonder what code was failing in
PEAR? The code here was looking for an array element to check that the ARRAY had
been populated, but the holding element was just a simple string rather than an
empty array.

... I suspect that this is perhaps where the code *I* am looking at is
breaking down ... how SHOULD you define the array so that it is an array? So
that $family = array ( 'fam1' => 'JONES' ); is the base for the JONES family
rather than simply a string. All of this USED to be simple, but it seems to
be getting so cryptic that this is what is causing the trouble :(

The legacy code methods may be wrong, but a lot of this code originates in PHP4
days, so when things do break what is needed is a clean upgrade guide which may
help to isolate the bugs when they appear?

I think you just need a coffee.
No just a version of PHP where I'm not firefighting changes but getting back to
actually producing new code :( I can either test 5.4 or work ... I don't have
time for both ...

--
Lester Caine - G8HFL

13 years ago by Stas Malyshev — view source

unread

Hi!

I'm having a lot of trouble with working out WHAT is going on here at all.

It's actually very simple. Take variable $a which is a string ("foo").
Now it you do $a[0] that would produce first letter - "f". Now here's a
tricky part - if you do $a['blah'] it would convert 'blah' to number,
get 0 and return the same letter "f".
Now what happens if you do $a[0][0]? Since $a[0] is "f", $a[0][0] should
be first letter of "f" which is "f" again, right? Only in 5.3 it did not
work because of some deficiencies in implementation of string offsets.
In 5.4 it was fixed.
Now the situation in the bug - what happens if we do
$a['blah']['foobar']? As we noted before, it's the same as $a[0][0] and
thus produces "f" in 5.4. In 5.3 it didn't work because of the above
deficiency. That's it.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by Daniel Convissor — view source

unread

Hi Stas:

It's actually very simple. Take variable $a which is a string
("foo"). Now it you do $a[0] that would produce first letter - "f".
Now here's a tricky part - if you do $a['blah'] it would convert
'blah' to number, get 0 and return the same letter "f".

To me, this is the bug. $a['blah'] does not exist. An undefined index
notice should be raised. The key "blah" should not be converted to 0.
The following two things should behave the same:

$b = array('exists' => 'foo');
echo $b['blah'] . "\n";

$a = 'foo';
echo $a['blah'] . "\n";

But that second one echos out "f". This is a huge WTF.

Thanks,

--Dan

13 years ago by Ferenc Kovacs — view source

unread

On Wed, Nov 23, 2011 at 3:14 PM, Daniel Convissor <
danielc@analysisandsolutions.com> wrote:

Hi Stas:

It's actually very simple. Take variable $a which is a string
("foo"). Now it you do $a[0] that would produce first letter - "f".
Now here's a tricky part - if you do $a['blah'] it would convert
'blah' to number, get 0 and return the same letter "f".

To me, this is the bug. $a['blah'] does not exist. An undefined index
notice should be raised. The key "blah" should not be converted to 0.
The following two things should behave the same:

$b = array('exists' => 'foo');
echo $b['blah'] . "\n";

$a = 'foo';
echo $a['blah'] . "\n";

But that second one echos out "f". This is a huge WTF.

and the following should also behave the same:
$a = 'foo';

echo $a[2];
echo $a['2'];
echo $a['2 cats'];

because this is how the type juggling works in php:
http://php.net/manual/en/language.types.type-juggling.php

if we allow strings to be used as indexes for strings, we have to be
consistent with what we have already.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Gustavo Lopes — view source

unread

Em Wed, 23 Nov 2011 14:19:42 -0000, Ferenc Kovacs tyra3l@gmail.com
escreveu:

On Wed, Nov 23, 2011 at 3:14 PM, Daniel Convissor <
danielc@analysisandsolutions.com> wrote:

It's actually very simple. Take variable $a which is a string
("foo"). Now it you do $a[0] that would produce first letter - "f".
Now here's a tricky part - if you do $a['blah'] it would convert
'blah' to number, get 0 and return the same letter "f".

To me, this is the bug. $a['blah'] does not exist. An undefined index
notice should be raised. The key "blah" should not be converted to 0.
The following two things should behave the same:

$b = array('exists' => 'foo');
echo $b['blah'] . "\n";

$a = 'foo';
echo $a['blah'] . "\n";

But that second one echos out "f". This is a huge WTF.

and the following should also behave the same:
$a = 'foo';

echo $a[2];
echo $a['2'];
echo $a['2 cats'];

because this is how the type juggling works in php:
http://php.net/manual/en/language.types.type-juggling.php

But this is not (and afaik has never been) how indexes in arrays work.
The conversion of array keys has always different rules:

<?php
$a['2 cats'] = null;
var_dump($a);
^D
array(1) {
["2 cats"]=>
NULL
}

The mapping between strings and integers used for arrays is 1 to 1, and
strings that can be mapped to integers are always mapped (so you cannot
have a '1' (string) key and a 1 (int) key). These different rules are what
makes possible to store any kind of string in an array, and changing this
would be a huge error.

I see, however, that it's not the case for indexing strings, where the lax
rules are followed:

<?php
$a = 'foo';
var_dump($a['2 cats']);
^D
string(1) "o"

If there's any change to be made, it would be to change the way string
indexing works, not the other way around.

--
Gustavo Lopes

13 years ago by Ferenc Kovacs — view source

unread

On Wed, Nov 23, 2011 at 3:42 PM, Gustavo Lopes glopes@nebm.ist.utl.ptwrote:

Em Wed, 23 Nov 2011 14:19:42 -0000, Ferenc Kovacs tyra3l@gmail.com
escreveu:

On Wed, Nov 23, 2011 at 3:14 PM, Daniel Convissor <

danielc@analysisandsolutions.**com danielc@analysisandsolutions.com>
wrote:

It's actually very simple. Take variable $a which is a string

("foo"). Now it you do $a[0] that would produce first letter - "f".
Now here's a tricky part - if you do $a['blah'] it would convert
'blah' to number, get 0 and return the same letter "f".

To me, this is the bug. $a['blah'] does not exist. An undefined index
notice should be raised. The key "blah" should not be converted to 0.
The following two things should behave the same:

$b = array('exists' => 'foo');
echo $b['blah'] . "\n";

$a = 'foo';
echo $a['blah'] . "\n";

But that second one echos out "f". This is a huge WTF.

and the following should also behave the same:
$a = 'foo';

echo $a[2];
echo $a['2'];
echo $a['2 cats'];

because this is how the type juggling works in php:
http://php.net/manual/en/**language.types.type-juggling.**php http://php.net/manual/en/language.types.type-juggling.php

But this is not (and afaik has never been) how indexes in arrays work.
The conversion of array keys has always different rules:

<?php
$a['2 cats'] = null;
var_dump($a);
^D
array(1) {
["2 cats"]=>
NULL
}

yeah, because using associative arrays (hence strings as indexes) is a sane
operation.
but we are talking about strings here.

The mapping between strings and integers used for arrays is 1 to 1, and
strings that can be mapped to integers are always mapped (so you cannot
have a '1' (string) key and a 1 (int) key). These different rules are what
makes possible to store any kind of string in an array, and changing this
would be a huge error.

I see, however, that it's not the case for indexing strings, where the lax
rules are followed:

<?php
$a = 'foo';
var_dump($a['2 cats']);
^D
string(1) "o"

If there's any change to be made, it would be to change the way string
indexing works, not the other way around.

php.net/manual/en/language.types.string.php#language.types.string.substr http://hu.php.net/manual/en/language.types.string.php#language.types.string.substr
"Non-integer types are converted to integer."
So it is a documented behavior, so the current change is just a bugfix imo.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Daniel Convissor — view source

unread

Hi Ferenc:

[And the manual saaaays...]

"Non-integer types are converted to integer."
So it is a documented behavior, so the current change is just a bugfix imo.

Can someone please lend me their time machine so I can go back and tell
folks to just use {} for string offsets and [] for arrays? I'll
only need it for a few minutes.

Thanks,

--Dan

13 years ago by Ferenc Kovacs — view source

unread

On Wed, Nov 23, 2011 at 4:04 PM, Daniel Convissor <
danielc@analysisandsolutions.com> wrote:

Hi Ferenc:

[And the manual saaaays...]

"Non-integer types are converted to integer."
So it is a documented behavior, so the current change is just a bugfix
imo.

Can someone please lend me their time machine so I can go back and tell
folks to just use {} for string offsets and [] for arrays? I'll
only need it for a few minutes.

offtopic:

I used to preach about using {} for string offsets and that [] is
deprecated then we had that reversed in the documentation/5.3 migration
guide ("The use of {} to access string offsets is deprecated. Use []
instead.")...
Fortunately it turned out to be a mistake, and got fixed in the docs, since
then we document both behavior (albeit the [] seems to be preferred way in
the docs), and none of them triggers E_DEPRECATED.
So I think that it is the reason why people still use [] for string offsets.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Pierre Joye — view source

unread

if we allow strings to be used as indexes for strings, we have to be
consistent with what we have already.

Short notice to avoid confusion:

It has nothing to do with allowing string, or double, or whatever else
but loosely typing. Loosely typing is what is happening here, the op
fetching a byte gets an integer, as usual.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Daniel Convissor — view source

unread

Hi Again Folks:

$a = 'foo';
echo $a['blah'] . "\n";

But that second one echos out "f". This is a huge WTF.

Two things for the record on this front. First, i've been actively
using PHP for, what, ten years or so, and have never run into this
behavior before. Second, this behavior turns the following one liner:

if (isset($arr['package']['attribs']['version'])) {

Into this:

if (is_array($arr)
	&& array_key_exists('package', $arr)
	&& is_array($arr['package'])
	&& array_key_exists('attribs', $arr['package'])
	&& is_array($arr['package']['attribs'])
	&& array_key_exists('version', $arr['package']['attribs'])
	&& !empty($arr['package']['attribs']['version']))
{

Thanks,

--Dan

13 years ago by Ferenc Kovacs — view source

unread

On Wed, Nov 23, 2011 at 4:31 PM, Daniel Convissor <
danielc@analysisandsolutions.com> wrote:

> Hi Again Folks:
>
>
> > $a = 'foo';
> > echo $a['blah'] . "\n";
> >
> > But that second one echos out "f". This is a huge WTF.
>
> Two things for the record on this front. First, i've been actively
> using PHP for, what, ten years or so, and have never run into this
> behavior before. Second, this behavior turns the following one liner:
>
> if (isset($arr['package']['attribs']['version'])) {
>
> Into this:
>
> if (is_array($arr)
> && array_key_exists('package', $arr)
> && is_array($arr['package'])
> && array_key_exists('attribs', $arr['package'])
> && is_array($arr['package']['attribs'])
> && array_key_exists('version', $arr['package']['attribs'])
> && !empty($arr['package']['attribs']['version']))
> {
>

it is only necessary if you can't guarantee your data structure (so that
you can get a string where you expect an array)

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Richard Quadling — view source

unread

On 23 November 2011 15:31, Daniel Convissor
danielc@analysisandsolutions.com wrote:

Hi Again Folks:

$a = 'foo';
echo $a['blah'] . "\n";

But that second one echos out "f". This is a huge WTF.

Two things for the record on this front. First, i've been actively
using PHP for, what, ten years or so, and have never run into this
behavior before. Second, this behavior turns the following one liner:

if (isset($arr['package']['attribs']['version'])) {

Into this:

if (is_array($arr)
&& array_key_exists('package', $arr)
&& is_array($arr['package'])
&& array_key_exists('attribs', $arr['package'])
&& is_array($arr['package']['attribs'])
&& array_key_exists('version', $arr['package']['attribs'])
&& !empty($arr['package']['attribs']['version']))
{

I agree with Daniel on this.

Just looking for any test relating to isset() to see what tests will now fail.

Neither of the isset() tests ext/standard/tests/general_functions look
at arrays with associative indices.

The behaviour with regard to unavailable associative indices has no tests.

But using loose typing as a fallback for non existent keys seems
really really wrong.

Especially if the key is a constant. On what planet should ...

isset($arr['exists']['test_existance']) should become isset($arr['exists'][0])

That just seems really wrong and the work around is awful.

Richard Quadling
Twitter : EE : Zend : PHPDoc : Fantasy Shopper
@RQuadling : e-e.com/M_248814.html : bit.ly/9O8vFY : bit.ly/lFnVea :
fan.sh/6/370

13 years ago by Lester Caine — view source

unread

Richard Quadling wrote:

I agree with Daniel on this.

Just looking for any test relating to isset() to see what tests will now fail.

So it's not just me :)
I am seeing this break real world projects and can't see a easy way to fix the
break. There is nothing really wrong with the current code except that the sub
keys have yet to be populated.

--
Lester Caine - G8HFL

13 years ago by Larry Garfield — view source

unread

Richard Quadling wrote:

I agree with Daniel on this.

Just looking for any test relating to isset() to see what tests will
now fail.

So it's not just me :)
I am seeing this break real world projects and can't see a easy way to
fix the break. There is nothing really wrong with the current code
except that the sub keys have yet to be populated.

This is going to be a huge problem for Drupal. Drupal uses deep
associative array structures a lot, by design. That means we isset() or
empty() on arrays a lot. I haven't had a chance to test it yet, but I
see this change breaking, um, A LOT. And as Daniel noted, the fix is to
turn one line of very readable code into 8 lines of hard to read code.

This is not a step forward by any metric I can imagine. It's changing
long-standing behavior for no real benefit I can see except perhaps
strict adherence to a doc page. However, PHP has always been an
"implementation is the standard" language, which means this is a
language API change.

Please roll it back to avoid breaking a crapton of existing, legitimate,
non-buggy code.

--Larry Garfield

13 years ago by Rasmus Lerdorf — view source

unread

Richard Quadling wrote:

I agree with Daniel on this.

Just looking for any test relating to isset() to see what tests will
now fail.

So it's not just me :)
I am seeing this break real world projects and can't see a easy way to
fix the break. There is nothing really wrong with the current code
except that the sub keys have yet to be populated.

This is going to be a huge problem for Drupal. Drupal uses deep
associative array structures a lot, by design. That means we isset() or
empty() on arrays a lot. I haven't had a chance to test it yet, but I
see this change breaking, um, A LOT. And as Daniel noted, the fix is to
turn one line of very readable code into 8 lines of hard to read code.

This is not a step forward by any metric I can imagine. It's changing
long-standing behavior for no real benefit I can see except perhaps
strict adherence to a doc page. However, PHP has always been an
"implementation is the standard" language, which means this is a
language API change.

Please roll it back to avoid breaking a crapton of existing, legitimate,
non-buggy code.

I had a quick look through the Drupal code. I don't see a single place
that this is done where a deeply nested array index is applied to a
string. I think you are misunderstanding what has changed here.

-Rasmus

13 years ago by Pierre Joye — view source

unread

hi Rasmus,

I had a quick look through the Drupal code. I don't see a single place
that this is done where a deeply nested array index is applied to a
string. I think you are misunderstanding what has changed here.

We are leading to yet another underestimated impact to userland code,
for zero gain.

The risk is to high, let revert that please.

--
Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Rasmus Lerdorf — view source

unread

hi Rasmus,

I had a quick look through the Drupal code. I don't see a single place
that this is done where a deeply nested array index is applied to a
string. I think you are misunderstanding what has changed here.

We are leading to yet another underestimated impact to userland code,
for zero gain.

The risk is to high, let revert that please.

For all the people saying, "Revert". You guys realize that also means we
revert all the array dereferencing we added elsewhere, right? That
includes function array dereferencing which pretty most everyone has
been clamoring for for years.

So just to clarify, you think we should remove function array
dereferencing? Otherwise, please provide a proposal for how we separate
these various derefencing mechanisms in a way that doesn't completely
destroy the clean and consistent implementation we have right now.

-Rasmus

13 years ago by Stas Malyshev — view source

unread

Hi!

For all the people saying, "Revert". You guys realize that also means we
revert all the array dereferencing we added elsewhere, right? That
includes function array dereferencing which pretty most everyone has
been clamoring for for years.

I think you're underestimating it. We'd have to revert many of changes
done to the engine in 5.4, and I strongly suspect str_offset is
incompatible with literals improvement, which means all performance
gains in 5.4 would be ruined too. Pretty much we'd have to roll back
whole Zend engine to 5.3 state as far as I can see it. I don't think
this even worth discussing.
That, of course, does not preclude discussing how we can improve current
situation - but by now I would ask anybody who wants to comment about
"reverting" please not to do so because it's going nowhere. If you have
a proposal on how to make it better with regard to existing code that
relied on sting offsets bugs - you are more than welcome, especially if
you propose a patch :)

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by Felipe Pena — view source

unread

Hi,

2011/11/24 Rasmus Lerdorf rasmus@lerdorf.com:

hi Rasmus,

I had a quick look through the Drupal code. I don't see a single place
that this is done where a deeply nested array index is applied to a
string. I think you are misunderstanding what has changed here.

We are leading to yet another underestimated impact to userland code,
for zero gain.

The risk is to high, let revert that please.

For all the people saying, "Revert". You guys realize that also means we
revert all the array dereferencing we added elsewhere, right? That
includes function array dereferencing which pretty most everyone has
been clamoring for for years.

So just to clarify, you think we should remove function array
dereferencing? Otherwise, please provide a proposal for how we separate
these various derefencing mechanisms in a way that doesn't completely
destroy the clean and consistent implementation we have right now.

-Rasmus

The function array dereferencing is unrelated to these changes (it
just touched the parser), i.e. it uses the same code used to access
the array directly.

--
Regards,
Felipe Pena

13 years ago by Ferenc Kovacs — view source

unread

On Thu, Nov 24, 2011 at 9:12 PM, Larry Garfield larry@garfieldtech.comwrote:

Richard Quadling wrote:

I agree with Daniel on this.

Just looking for any test relating to isset() to see what tests will
now fail.

So it's not just me :)
I am seeing this break real world projects and can't see a easy way to
fix the break. There is nothing really wrong with the current code
except that the sub keys have yet to be populated.

This is going to be a huge problem for Drupal. Drupal uses deep
associative array structures a lot, by design. That means we isset() or
empty() on arrays a lot. I haven't had a chance to test it yet, but I see
this change breaking, um, A LOT. And as Daniel noted, the fix is to turn
one line of very readable code into 8 lines of hard to read code.

Did you managed to read the whole thread?
I'm asking because there were lot of confusion about the actual impact of
this problem, and Lester particularly seemed confused.

"There is nothing really wrong with the current code except that the sub
keys have yet to be populated."
that isn't true, if the array or some sub elements are empty("yet to be
populated"), you won't bump into this change. See my previous mail for the
exact details.

so the above mentioned one liner:

"if (isset($arr['package']['attribs']['version'])) {"

what could go wrong:
$arr is not an array, but a string -> it would fatal on 5.3(Fatal: cannot
use string offset as an array), but it would work with 5.4
$arr['package'] is not an array but a string -> false on 5.3, true on 5.4
(this is the worst case)
$arr['package']['attribs'] is not an array but a string -> true on both 5.3
and 5.4 (so you are already screwed)

the least amount of change to fix that isset would be:
isset($arr["package"]["attribs"]) && is_array($arr["package"]["attribs"])
&& isset($arr["package"]["attribs"]["version"]);
a little bit bit longer, right.

I think that if you can't guarantee your data, then you have to
check/sanitize it.
isset($array['foo']['bar']['baz']) could seem as a good idea, but don't
forget that if $array or one of it's element will be, for example an
object(not implementing ArrayAccess) then your isset will still fail with a
fatal error:

$arr = new StdClass;
isset($arr["package"]["attribs"]["version"]);
PHP Fatal error: Cannot use object of type stdClass as array

This is not a step forward by any metric I can imagine. It's changing
long-standing behavior for no real benefit I can see except perhaps strict
adherence to a doc page. However, PHP has always been an "implementation
is the standard" language, which means this is a language API change.

Stas and Etienne explained in this thread what was the reason introducing
this change(removing the string offset pseudo-type).

Please roll it back to avoid breaking a crapton of existing, legitimate,
non-buggy code.

This change would only break buggy code, where a string is checked as it
would be an array, and I would also argue about the crapton part.
As I mentioned before, if your input validation is so fragile, that you can
be bitten by this bug, then most probably you can also be bitten by the
same problem which exists in 5.3( see "$arr['package']['attribs'] is not an
array but a string").
The only real-world example where this breaks something was a test in the
PEAR codebase, albeit Daniel didn't mentioned the test, I have a feeling
that if we check it, we will see that the test itself is buggy (else it
couldn't have triggered this thread).

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Lester Caine — view source

unread

Ferenc Kovacs wrote:

Did you managed to read the whole thread?
I'm asking because there were lot of confusion about the actual impact of
this problem, and Lester particularly seemed confused.

"There is nothing really wrong with the current code except that the sub
keys have yet to be populated."
that isn't true, if the array or some sub elements are empty("yet to be
populated"), you won't bump into this change. See my previous mail for the
exact details.

My genealogical data site is not working on PHP5.4 ... the arrays of information
are not being populated ... WHEN I get some time I will go through things with a
fine tooth comb, but for the time being 5.4 is unusable for that site. The same
phpgedview code is used by many sites, and while it may simply be my port to
Firebird that has the problem, I suspect other sites will be affected. I've
tested PHP5.4 ... and it does not work for me ... in much the same way 5.3
didn't and needed a lot of work to get things ported over. Looks to me like
EXACTLY the same agro for many users this time as well :( All right the code may
be at fault, but much of it originated in PHP4 days and tracking these sorts of
black holes is not simple ... otherwise I would know why my site does not work now!

Other people have flagged a problem, although it would seem that showing where
the problems are is not being followed up, so can people sign off on this change
that it will NOT affect production sites? More important if it does affect a
site how does one find the problem code? If it worked but gave warnings that
would at least be a little better ...

--
Lester Caine - G8HFL

13 years ago by Larry Garfield — view source

unread

On Thu, Nov 24, 2011 at 9:12 PM, Larry Garfieldlarry@garfieldtech.comwrote:

Richard Quadling wrote:

I agree with Daniel on this.

Just looking for any test relating to isset() to see what tests will
now fail.

So it's not just me :)
I am seeing this break real world projects and can't see a easy way to
fix the break. There is nothing really wrong with the current code
except that the sub keys have yet to be populated.

This is going to be a huge problem for Drupal. Drupal uses deep
associative array structures a lot, by design. That means we isset() or
empty() on arrays a lot. I haven't had a chance to test it yet, but I see
this change breaking, um, A LOT. And as Daniel noted, the fix is to turn
one line of very readable code into 8 lines of hard to read code.

Did you managed to read the whole thread?
I'm asking because there were lot of confusion about the actual impact of
this problem, and Lester particularly seemed confused.

To be fair, no, I hadn't read the whole thread by the time I sent that.
(One of the challenges of long fast-moving threads is they're hard to
keep up with.)

"There is nothing really wrong with the current code except that the sub
keys have yet to be populated."
that isn't true, if the array or some sub elements are empty("yet to be
populated"), you won't bump into this change. See my previous mail for the
exact details.

so the above mentioned one liner:

"if (isset($arr['package']['attribs']['version'])) {"

what could go wrong:
$arr is not an array, but a string -> it would fatal on 5.3(Fatal: cannot
use string offset as an array), but it would work with 5.4
$arr['package'] is not an array but a string -> false on 5.3, true on 5.4
(this is the worst case)
$arr['package']['attribs'] is not an array but a string -> true on both 5.3
and 5.4 (so you are already screwed)

So to clarify, what Drupal does on a regular basis is something like:

if (isset($foo['bar']['baz']['beep'])) {
// Assume that bar, baz, and beep all exist, and that beep has a value
}
// or sometimes
if (!empty($foo['bar']['baz']['beep'])) {
// Assume that bar, baz, and beep all exist,
// and that beep has a meaningful value
}

Generally $foo, bar, and baz will all be arrays, and if they're not it
means someone else had a bug somewhere. Of course, Drupal module
developers never have bugs in their code that accidentally puts a string
where they should have put an array, no, not at all. :-) (Generally
when that happens we already hit a "first argument to foreach() must be
an array" error.)

Currently we don't use ArrayAccess anywhere aside from inheriting it
from PDO.

If that doesn't change, then I rescind my previous panic attack.

--Larry Garfield

13 years ago by Rasmus Lerdorf — view source

unread

Generally $foo, bar, and baz will all be arrays, and if they're not it
means someone else had a bug somewhere. Of course, Drupal module
developers never have bugs in their code that accidentally puts a string
where they should have put an array, no, not at all. :-) (Generally
when that happens we already hit a "first argument to foreach() must be
an array" error.)

Currently we don't use ArrayAccess anywhere aside from inheriting it
from PDO.

If that doesn't change, then I rescind my previous panic attack.

Yes, no change in any of that. In your usage, the case that behaves
differently in 5.4 was actually a fatal error in 5.3, so chances are
pretty good you don't have too many of these.

-Rasmus

13 years ago by Ferenc Kovacs — view source

unread

On Thu, Nov 24, 2011 at 11:03 PM, Larry Garfield larry@garfieldtech.comwrote:

On Thu, Nov 24, 2011 at 9:12 PM, Larry Garfield<larry@garfieldtech.**comlarry@garfieldtech.com

wrote:

Richard Quadling wrote:

I agree with Daniel on this.

Just looking for any test relating to isset() to see what tests will
now fail.

So it's not just me :)
I am seeing this break real world projects and can't see a easy way to
fix the break. There is nothing really wrong with the current code
except that the sub keys have yet to be populated.

This is going to be a huge problem for Drupal. Drupal uses deep
associative array structures a lot, by design. That means we isset() or
empty() on arrays a lot. I haven't had a chance to test it yet, but I
see
this change breaking, um, A LOT. And as Daniel noted, the fix is to turn
one line of very readable code into 8 lines of hard to read code.

Did you managed to read the whole thread?
I'm asking because there were lot of confusion about the actual impact of
this problem, and Lester particularly seemed confused.

To be fair, no, I hadn't read the whole thread by the time I sent that.
(One of the challenges of long fast-moving threads is they're hard to keep
up with.)

"There is nothing really wrong with the current code except that the sub

keys have yet to be populated."
that isn't true, if the array or some sub elements are empty("yet to be
populated"), you won't bump into this change. See my previous mail for the
exact details.

so the above mentioned one liner:

"if (isset($arr['package']['**attribs']['version'])) {"

what could go wrong:
$arr is not an array, but a string -> it would fatal on 5.3(Fatal: cannot
use string offset as an array), but it would work with 5.4
$arr['package'] is not an array but a string -> false on 5.3, true on 5.4
(this is the worst case)
$arr['package']['attribs'] is not an array but a string -> true on both
5.3
and 5.4 (so you are already screwed)

So to clarify, what Drupal does on a regular basis is something like:

if (isset($foo['bar']['baz']['**beep'])) {
// Assume that bar, baz, and beep all exist, and that beep has a value
}
// or sometimes
if (!empty($foo['bar']['baz']['**beep'])) {
// Assume that bar, baz, and beep all exist,
// and that beep has a meaningful value
}

Generally $foo, bar, and baz will all be arrays, and if they're not it
means someone else had a bug somewhere. Of course, Drupal module
developers never have bugs in their code that accidentally puts a string
where they should have put an array, no, not at all. :-) (Generally when
that happens we already hit a "first argument to foreach() must be an
array" error.)

if the foreach wouldn't catch this, you would have a chance to run into
this problem (both with 5.3 and 5.4)

Currently we don't use ArrayAccess anywhere aside from inheriting it from
PDO.

I only mentioned ArrayAccess because if an object implements that
interface, then $object['foo'] won't throw an error, but works as it would
be an array.

If that doesn't change, then I rescind my previous panic attack.

I think that bumping into this is less-less-less likely than what the
replies on this thread suggests, and this behavior is already there and
documented, the current change only add another edge-case where you can
trigger it.
The real gotcha is that the string offset index is substituted to the php
style type juggling($string['foo'] is valid and will be interpreted as
$string[0]), and that we use the same syntax for accessing array elements
and string offsets.
The two combined with a wrong/unexpected argument can screw you over, and
we didn't issue a notice on this yet.
It was just a coincidence that by historical reasons the string offset
chaining was disallowed, so there were one corner case, which was prevented
by this.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Yasuo Ohgaki — view source

unread

Hi all,

Just FYI

find ../php-src-5.3/Zend/ -name "*.[ch]" | xargs grep -n "Cannot use
string offset as an array" | wc -l111
I thought there are much less lines to raise notice for string offset
access, but it isn't. It almost everywhere in vm. Now I understand
the reason why Stats and Rusmus cares about decreased performance with
notices.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Gustavo Lopes — view source

unread

On Thu, 24 Nov 2011 22:44:46 -0000, Yasuo Ohgaki yohgaki@ohgaki.net
wrote:

find ../php-src-5.3/Zend/ -name "*.[ch]" | xargs grep -n "Cannot use
string offset as an array" | wc -l111
I thought there are much less lines to raise notice for string offset
access, but it isn't. It almost everywhere in vm. Now I understand
the reason why Stats and Rusmus cares about decreased performance with
notices.

This is just due to opcode handler specialization. In the source code
there are 7 instances in 5.4 and 10 in 5.3.

--
Gustavo Lopes

13 years ago by Ford — view source

unread

-----Original Message-----
From: Larry Garfield [mailto:larry@garfieldtech.com]
Sent: 24 November 2011 22:04

[... BIG SNIP ...]

If that doesn't change, then I rescind my previous panic attack.

--Larry Garfield

I echo that sentiment. On fuller review, I find a very high FUD
factor in effect here, and on actually checking my codebase I find
only 1 place where an additional is_array() might be needed (in
several tens of kLOC!).

Cheers!

Mike

--
Mike Ford,
Electronic Information Developer, Libraries and Learning Innovation,
Portland PD507, City Campus, Leeds Metropolitan University,
Portland Way, LEEDS, LS1 3HE, United Kingdom
E: m.ford@leedsmet.ac.uk T: +44 113 812 4730

To view the terms under which this email is distributed, please go to http://disclaimer.leedsmet.ac.uk/email.htm

13 years ago by Pierre Joye — view source

unread

I echo that sentiment. On fuller review, I find a very high FUD
factor in effect here, and on actually checking my codebase I find
only 1 place where an additional is_array() might be needed (in
several tens of kLOC!).

That's exactly the situation where it is a pain to find in the 1st
place, preventing smooth migrations. That's the whole point, nothing
to do with FUD but tech discussions about a possible annoying BC.

--
Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Stas Malyshev — view source

unread

Hi!

This is going to be a huge problem for Drupal. Drupal uses deep
associative array structures a lot, by design. That means we isset() or
empty() on arrays a lot. I haven't had a chance to test it yet, but I
see this change breaking, um, A LOT. And as Daniel noted, the fix is to
turn one line of very readable code into 8 lines of hard to read code.

Well, I do not think we're going to break the engine in a way that will
act as it acted in 5.3 - to introduce new pseudo-type that works with
$a[0] but not with $a[0][0] and bring in all the old bugs, even if
Drupal relies on some side effect of them. I am not a Drupal developer
so I have no idea why Drupal would use side effects of string offset
bugs to distinguish between arrays and strings, but this is a wrong
decision and needs to be changed, since among other things it means that
code like isset($foo[0][0]) and $bar=$foo[0]; isset($bar[0]); works
differently, which makes no sense.

So the question is - what can be done that works with 5.4 engine and is
consistent, but will be better for Drupal and others that made similar
mistakes of relying on a bug in pre-5.4 implementation of string offsets?

Please roll it back to avoid breaking a crapton of existing, legitimate,
non-buggy code.

Again, there's no "rolling back" this change, unless you want to throw
out all 5.4 improvements in the engine, on which stage we might as well
throw out whole 5.4 thing and forget about improving PHP, since it may
disrupt somebody's code that relies precisely on the bugs being fixed.
Sorry for being dramatic, but I've repeated it three times already so
I'd really want for it to sink in this time - this change is an integral
part of 5.4 engine cleanup and it can not be changed without seriously
breaking the engine - because the way string offsets work now is the
right way for them to work (or at least light years more right that what
happened in 5.3).
So unless somebody comes with a patch that proves me wrong and shows me
how old broken way of doing string offsets can be preserved without
messing up a lot of things in the engine - there's not "rolling back"
and no "reverting", there's nothing to roll back and revert.
If, however, anybody has any practical solution to it that improves
things - he's more than welcome to come up with the patch and discuss
it. I'll think about it too, but since I have no idea what Drupal does
I'm not sure if the direction I choose would be best.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by Pierre Joye — view source

unread

Just looking for any test relating to isset() to see what tests will now fail.

Neither of the isset() tests ext/standard/tests/general_functions look
at arrays with associative indices.

The behaviour with regard to unavailable associative indices has no tests.

But using loose typing as a fallback for non existent keys seems
really really wrong.

Especially if the key is a constant. On what planet should ...

isset($arr['exists']['test_existance']) should become isset($arr['exists'][0])

That just seems really wrong and the work around is awful.

That's exactly why I stoped to use this syntax a long time ago. And I
was in favor to deprecate it and actually remove it the next major
release (6 that is). But that's another discussion.

Unless I'm mistaken it seems that we have a clear and vicious BC break
here, the kind of changes that are annoying to catch and does not
bring a lot of benefits to PHP (given the actual usage of this
syntax). I would go with a revert, add tests and never change this
behavior again. It is a confusing enough topic.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Etienne Kneuss — view source

unread

Hi,

What people seem to have trouble with here is not that $string[0][0]
== $string[0], but rather that $string['index'] === $string[0]...

But as many people said, it is just how type juggling want: string
index? Get me an int => 0.

I believe the change introduced in 5.4 makes sense, the previous way
it worked made no sense and was simply an anomaly.

It is clear from this thread that the confusion comes from strings as
string offsets, something "totally" different, but changing that would
be a much bigger BC break I guess.

Best,

Just looking for any test relating to isset() to see what tests will now fail.

Neither of the isset() tests ext/standard/tests/general_functions look
at arrays with associative indices.

The behaviour with regard to unavailable associative indices has no tests.

But using loose typing as a fallback for non existent keys seems
really really wrong.

Especially if the key is a constant. On what planet should ...

isset($arr['exists']['test_existance']) should become isset($arr['exists'][0])

That just seems really wrong and the work around is awful.

That's exactly why I stoped to use this syntax a long time ago. And I
was in favor to deprecate it and actually remove it the next major
release (6 that is). But that's another discussion.

Unless I'm mistaken it seems that we have a clear and vicious BC break
here, the kind of changes that are annoying to catch and does not
bring a lot of benefits to PHP (given the actual usage of this
syntax). I would go with a revert, add tests and never change this
behavior again. It is a confusing enough topic.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

--

--
Etienne Kneuss
http://www.colder.ch

13 years ago by Stas Malyshev — view source

unread

Hi!

Unless I'm mistaken it seems that we have a clear and vicious BC break
here, the kind of changes that are annoying to catch and does not

No, we do not have a BC break here, we have a bugfix here that makes
string ops work consistently and only has a problem with completely
broken code. I am 100% opposed to changing anything there and
re-breaking string offsets because somebody uses string indexes to
operate on strings and expect them to work in weird ways that makes
$a[0][0] work differently from $b = $a[0]; $b[0]. If you use string
offsets on strings, expect them to be converted to numbers, it has been
there since forever and only didn't work on chained offsets because of a
bug.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by Ferenc Kovacs — view source

unread

On Wed, Nov 23, 2011 at 7:57 PM, Stas Malyshev smalyshev@sugarcrm.comwrote:

Hi!

Unless I'm mistaken it seems that we have a clear and vicious BC break

here, the kind of changes that are annoying to catch and does not

No, we do not have a BC break here, we have a bugfix here that makes
string ops work consistently and only has a problem with completely broken
code. I am 100% opposed to changing anything there and re-breaking string
offsets because somebody uses string indexes to operate on strings and
expect them to work in weird ways that makes $a[0][0] work differently from
$b = $a[0]; $b[0]. If you use string offsets on strings, expect them to be
converted to numbers, it has been there since forever and only didn't work
on chained offsets because of a bug.

duh, you are right.
my simplified example:
$foo = 'string';
echo isset($foo['bar']);

wouldn't trigger the new behavior, it is required to be a chained
reference.
sorry for the noise.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Pierre Joye — view source

unread

Hi!

Unless I'm mistaken it seems that we have a clear and vicious BC break
here, the kind of changes that are annoying to catch and does not

No, we do not have a BC break here, we have a bugfix here that makes string
ops work consistently and only has a problem with completely broken code

The fact that we have reports here showing code not working anymore
because of this change tells me that it is a BC break. We can call it
a bug fix but it still breaks code out there for no real benefit but
edge case usages. We had this situation before, that does not help us.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Gustavo Lopes — view source

unread

On Wed, 23 Nov 2011 21:06:09 -0000, Pierre Joye pierre.php@gmail.com
wrote:

The fact that we have reports here showing code not working anymore
because of this change tells me that it is a BC break. We can call it
a bug fix but it still breaks code out there for no real benefit but
edge case usages. We had this situation before, that does not help us.

I'd say for no benefit at all. Why would anyone ever want to take a string
offset from a string that certainly has length 1? Except for taking
satisfaction in this "improved consistency", I see absolutely no benefit.

Up until now, it was deemed a useless but innocuous change. Now that we
found it has pernicious side effects, we ought to revert it.

--
Gustavo Lopes

13 years ago by Mike Robinson — view source

unread

On November-23-11 5:31 PM Gustavo Lopes wrote:

On Wed, 23 Nov 2011 21:06:09 -0000, Pierre Joye pierre.php@gmail.com
wrote:

The fact that we have reports here showing code not working anymore
because of this change tells me that it is a BC break. We can call it
a bug fix but it still breaks code out there for no real benefit but
edge case usages. We had this situation before, that does not help
us.

I'd say for no benefit at all. Why would anyone ever want to take a
string offset from a string that certainly has length 1? Except for
taking satisfaction in this "improved consistency", I see absolutely no
benefit.

Up until now, it was deemed a useless but innocuous change. Now that we
found it has pernicious side effects, we ought to revert it.

Notwithstanding that this behaviour was possible because of a bug, it has
admittedly been relied on since time immemorial, making it a significant
BC break.

As distasteful as it seems, it absolutely should be reverted IMHO.

Best Regards,

Mike Robinson

13 years ago by Stas Malyshev — view source

unread

Hi!

As distasteful as it seems, it absolutely should be reverted IMHO.

You seem to misunderstand something here. This behavior is a direct
consequence of string offset result being a string. To revert it means
to reintroduce broken string-offset pseudo-var-type that led to huge
amount of bugs, never worked reliably and may as well be incompatible
with current engine after all changes that were made after that.
Reintroducing old broken way of handling string offsets is a MAJOR
engine refactoring, and doing it in RC stage is a worst idea ever.

I do not see any way to treat result of the offset operation not as a
string without engine refactoring, if you do - please explain how. If
the offset is handled as a string, the direct consequence is that is
behaves as a string, which means every operation working on string works
on it. It's not a question of taste, it's how the engine worked. In 5.3,
the engine had this weird pseudo-type called string offset, which led to
a lot of problems, one of them being that $a[0][0] didn't work because
DIM opcodes didn't know how to handle this pseudo-type, and had to be
handled specially on each corner (and produced tons of bugs because of
it). I do not consider going back to that as an option.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by Ford — view source

unread

-----Original Message-----
From: Gustavo Lopes [mailto:glopes@nebm.ist.utl.pt]
Sent: 23 November 2011 22:31

On Wed, 23 Nov 2011 21:06:09 -0000, Pierre Joye
pierre.php@gmail.com
wrote:

The fact that we have reports here showing code not working
anymore
because of this change tells me that it is a BC break. We can call
it
a bug fix but it still breaks code out there for no real benefit
but
edge case usages. We had this situation before, that does not help
us.

I'd say for no benefit at all. Why would anyone ever want to take a
string
offset from a string that certainly has length 1? Except for taking
satisfaction in this "improved consistency", I see absolutely no
benefit.

Up until now, it was deemed a useless but innocuous change. Now that
we
found it has pernicious side effects, we ought to revert it.

That's exactly my take on it. As far as I can see, if 5.4 is released
with this "fix" in, it will effectively be rendered a non-upgradable-to
version for most big projects.

For what it's worth (not much!), my two pennorth on this is that
whilst, like some others, I can see the marginal benefit of making the
behaviour consistent, I really don't think the change can be done in
one go. Given that this WILL break existing code (some of it mine :( ),
there needs to be a release where the usage generates an error message

probably an E_WARNING, or maybe an E_STRICT - and an easy alternative
is provided. Since the simplest alternative way of performing the
necessary chained checks would seem to be:

if (is_array($arr) && array_key_exists('key1', $arr)
&& is_array($arr['key1'] && array_key_exists('key2', $arr['key2'])
...
)

which substitutes 2 function calls and 2 array accesses per dimension
for a single function call and 1 multi-dimensional array access, there has
to be scope for a function called something like array_offset_exists with
a signature like

array_offset_exists(array $array, mixed $offset, ...)

(Yes, I know, inconsistent argument order and all that, but....)

so that people can have a chance to transform

isset($arr['key1']['key2']['key3'])

into

array_offset_exists($arr, 'key1', 'key2, 'key3')

before the full fix is put into place in a subsequent version.

Cheers!

Mike

To view the terms under which this email is distributed, please go to http://disclaimer.leedsmet.ac.uk/email.htm

13 years ago by Ferenc Kovacs — view source

unread

-----Original Message-----
From: Gustavo Lopes [mailto:glopes@nebm.ist.utl.pt]
Sent: 23 November 2011 22:31

On Wed, 23 Nov 2011 21:06:09 -0000, Pierre Joye
pierre.php@gmail.com
wrote:

The fact that we have reports here showing code not working
anymore
because of this change tells me that it is a BC break. We can call
it
a bug fix but it still breaks code out there for no real benefit
but
edge case usages. We had this situation before, that does not help
us.

I'd say for no benefit at all. Why would anyone ever want to take a
string
offset from a string that certainly has length 1? Except for taking
satisfaction in this "improved consistency", I see absolutely no
benefit.

Up until now, it was deemed a useless but innocuous change. Now that
we
found it has pernicious side effects, we ought to revert it.

That's exactly my take on it. As far as I can see, if 5.4 is released
with this "fix" in, it will effectively be rendered a non-upgradable-to
version for most big projects.

we yet to see such a project.
for example the ZF testsuite was ran against the RC1, and didn't bumped
into this.
we also run the symfony2 testsuite on ci.qa.php.net and the test results
seems to be consistent between 5.3, 5.4 and trunk
http://ci.qa.php.net/view/php-userland/job/php-symfony2/362/testReport/?

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Stas Malyshev — view source

unread

Hi!

To me, this is the bug. $a['blah'] does not exist. An undefined index

If it's a bug, this bug was in PHP since forever, nothing new here.

notice should be raised. The key "blah" should not be converted to 0.
The following two things should behave the same:

$b = array('exists' => 'foo');
echo $b['blah'] . "\n";

$a = 'foo';
echo $a['blah'] . "\n";

No they should not. First is array access, second is string offset
access, totally different ops. That's like saying + on arrays should
calculate the sum of all array elements because + on numbers calculates
the sum. Operators have different meanings for different types, it has
always been so in all languages.
One could argue that string access with non-string should produce a
notice, but probably if we added that a lot of people would come out to
complain we broke their perfectly working code ;)

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by Ferenc Kovacs — view source

unread

On Wed, Nov 23, 2011 at 7:52 PM, Stas Malyshev smalyshev@sugarcrm.comwrote:

Hi!

To me, this is the bug. $a['blah'] does not exist. An undefined index

If it's a bug, this bug was in PHP since forever, nothing new here.

notice should be raised. The key "blah" should not be converted to 0.

The following two things should behave the same:

$b = array('exists' => 'foo');
echo $b['blah'] . "\n";

$a = 'foo';
echo $a['blah'] . "\n";

No they should not. First is array access, second is string offset access,
totally different ops. That's like saying + on arrays should calculate the
sum of all array elements because + on numbers calculates the sum.
Operators have different meanings for different types, it has always been
so in all languages.
One could argue that string access with non-string should produce a
notice, but probably if we added that a lot of people would come out to
complain we broke their perfectly working code ;)

I think that it would make sense.
I mean using $foo['asd'] where $foo is a string is as likely to be a hidden
application bug as the array() => 'Array' conversion that we patched
recently to trigger a warning.

More clarification for the others who seems to be miss how specific this
change is:
The only case where the 5.4 branch works differently as before if you
reference a string type(int, float, etc. won't trigger this) variable using
an associative index and you expect it that to be undefined variable even
though that the documentation explicitly states that the index will be
converted to an integer in this case.
for example:

$foo = 'string';
echo isset($foo['bar']);

will return true (as $foo['bar'] will converted to $foo[0] which returns
's' in this case) in 5.4 while it returned false previously.

ps:
on a sidenote: the same change also introduces that using the following:
$foo[0][0][0]... on a non-empty string will be always valid, before this
change it produced a "PHP Fatal error: Cannot use string offset as an
array"

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Stas Malyshev — view source

unread

Hi!

The only case where the 5.4 branch works differently as before if you
reference a string type(int, float, etc. won't trigger this) variable
using an associative index and you expect it that to be undefined
variable even though that the documentation explicitly states that the

Actually, the only change 5.4 did was to make $a['foo']['bar'] work like
($a['foo'])['bar'] - i.e. chained offsets work the same way as if they
were applied separately. That's it. All the rest has been there since
forever. I can't see how one could argue it should stay this way.

Now I'm sorry somebody used the fact that chained offsets didn't work to
do a check "do we have any strings there" but that's not how PHP is
supposed to work and it's clearly a side-effect of a bug.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by David Muir — view source

unread

Hi!

The only case where the 5.4 branch works differently as before if you
reference a string type(int, float, etc. won't trigger this) variable
using an associative index and you expect it that to be undefined
variable even though that the documentation explicitly states that the

Actually, the only change 5.4 did was to make $a['foo']['bar'] work
like ($a['foo'])['bar'] - i.e. chained offsets work the same way as if
they were applied separately. That's it. All the rest has been there
since forever. I can't see how one could argue it should stay this way.

Now I'm sorry somebody used the fact that chained offsets didn't work
to do a check "do we have any strings there" but that's not how PHP is
supposed to work and it's clearly a side-effect of a bug.

One thing that I wasn't aware of was the string to int conversion for
string offsets. IMO, that should trigger a notice or something. Good to
be reminded of though.

Just to clarify, the changes introduced in 5.4 will result in the following:

<?php

$string = 'foo';
$array = array(
'foo' => array(
'bar' => 'baz',
//expected structure
//'bar' => array('baz' => array('values'))
));

var_dump(
isset($string['foo']), //true
isset($string[0][0]), //false, true in 5.4
isset($array['foo']['bar'][0]), //true
isset($array['foo']['bar']['baz']), //true
isset($array['foo']['bar']['baz']['0']) //false, true as of 5.4
isset($string['foo']['bar']['baz']['0']) //false, true as of 5.4
);

If so, then it's a major BC break.
Maybe it's a bugfix, but it's a bugfix that allows for behaviour that

doesn't really make sense
has little to no practical benefit (when are you ever going to be
chaining offsets on strings?)
introduces new, hard-to-detect bugs in live code.

What used to be a one-liner, now effectively needs a crapload of
boilerplate code.
If you want this fix to be palatable, we'll need a new language
construct that effectively runs a series of is_array && array_key_exists
with a final !== null check.

array_isset($string['foo']['bar']) //false (not an array)
array_isset($array['foo']['bar']) //true, 'foo' is an array, and 'bar'
is not null
array_isset($array['foo']['bar'][0]) //false, $array['foo']['bar'] is
not an array
array_isset($array['foo']['bar']['baz']) //false, $array['foo']['bar']
is not an array

Behaviour is different from 5.3's isset, but is more in-line with what I
and I believe most PHP programmers would expect isset to behave when
checking multi-dimensional arrays, and has the features, that
array_key_exists is missing.

Anybody got a better idea?

Cheers,
David

13 years ago by Stas Malyshev — view source

unread

Hi!

If you want this fix to be palatable, we'll need a new language
construct that effectively runs a series of is_array&& array_key_exists
with a final !== null check.

I'm not sure RC time is a good place for introducing new language
constructs. Actually, I'm pretty sure it's not.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by Lester Caine — view source

unread

Stas Malyshev wrote:

If you want this fix to be palatable, we'll need a new language
construct that effectively runs a series of is_array&& array_key_exists
with a final !== null check.

I'm not sure RC time is a good place for introducing new language constructs.
Actually, I'm pretty sure it's not.

But neither is introducing a potential bomb of the kind that the 'date' saga
created. The problem this change IS causing is likely to hit many live sites
without any real explanation as to why it is happening. And I still don't think
that the type of code that it is hitting is 'wrong', it IS just doing a lookup
for an array element that does not exist to decide if it needs to create it.
( Although I STILL have not tracked down where my own code is actually falling
over ... it just comes back with blank array branches ... )

--
Lester Caine - G8HFL

13 years ago by Stas Malyshev — view source

unread

Hi!

But neither is introducing a potential bomb of the kind that the 'date' saga
created. The problem this change IS causing is likely to hit many live sites

The claim that many live sites actually regularly use string multiple
string offsets to distinguish strings from arrays sounds implausible to me.

without any real explanation as to why it is happening. And I still don't think
that the type of code that it is hitting is 'wrong', it IS just doing
a lookup

There's real explanation and it's in UPGRADING now. And the code is not
just plain "wrong" - it relies on a bug to distinguish between array and
string, when it should use proper functions instead. If you want to
check if something is an array, use is_array. Using side effect of a bug
and claiming that the engine now should stay broken forever because you
used it doesn't make any sense.

for an array element that does not exist to decide if it needs to create it.

No, it's not going for array element - it applies multiple array
operations to a string without checking and relies on a bug to pass it
off. That's broken, plain and simple.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by devis@lucato.it — view source

unread

Hi!

But neither is introducing a potential bomb of the kind that the 'date'
saga
created. The problem this change IS causing is likely to hit many live
sites

The claim that many live sites actually regularly use string multiple string
offsets to distinguish strings from arrays sounds implausible to me.

Hi,

I hadn't the opportunity to install php 5.4 to test this, so I was
wondering if someone could test how would this code behave ?

###########
if (
!isset( $widget_options['dashboard_incoming_links'] )
|| !isset( $widget_options['dashboard_incoming_links']['home'] )
|| $widget_options['dashboard_incoming_links']['home'] != get_option('home') )
###########

Is that correct saying that if the first condition is false the second
will always be true ?

Note: that's a piece of Wordpress

Thank you
Devis

13 years ago by Alexey Shein — view source

unread

2011/11/24 devis@lucato.it:

Hi!

But neither is introducing a potential bomb of the kind that the 'date'
saga
created. The problem this change IS causing is likely to hit many live
sites

The claim that many live sites actually regularly use string multiple string
offsets to distinguish strings from arrays sounds implausible to me.

Hi,

I hadn't the opportunity to install php 5.4 to test this, so I was
wondering if someone could test how would this code behave ?

Hi, Davis, first of all you can use this service:
http://codepad.viper-7.com/ to test your code against 5.2, 5.3, 5.4
and latest trunk.

###########
if (
!isset( $widget_options['dashboard_incoming_links'] )
|| !isset( $widget_options['dashboard_incoming_links']['home'] )
|| $widget_options['dashboard_incoming_links']['home'] != get_option('home') )
###########

Is that correct saying that if the first condition is false the second
will always be true ?

Note: that's a piece of Wordpress

About your question if I understand it correctly: if
$widget_options['dashboard_incoming_links'] is not set, then
$widget_options['dashboard_incoming_links']['home'] can't be set in
any conditions.

Thank you
Devis

--

--
Regards,
Shein Alexey

13 years ago by Yasuo Ohgaki — view source

unread

2011/11/24 devis@lucato.it:

Hi,

I hadn't the opportunity to install php 5.4 to test this, so I was
wondering if someone could test how would this code behave ?

###########
if (
!isset( $widget_options['dashboard_incoming_links'] )
|| !isset( $widget_options['dashboard_incoming_links']['home'] )
|| $widget_options['dashboard_incoming_links']['home'] != get_option('home') )
###########

Is that correct saying that if the first condition is false the second
will always be true ?

Note: that's a piece of Wordpress

[yohgaki@dev php-src-5.4]$ ./php -r 'var_dump(!isset($a["a"]["a"]));'
bool(true)
[yohgaki@dev php-src-5.4]$ ./php -r '$a =
["a"=>["a"=>1]];var_dump(!isset($a["a"]), $a);'
bool(false)
array(1) {
["a"]=>
array(1) {
["a"]=>
int(1)
}
}
[yohgaki@dev php-src-5.4]$ ./php -r '$a =
["a"=>["a"=>1]];var_dump(!isset($a["a"]["a"]), $a);'
bool(false)
array(1) {
["a"]=>
array(1) {
["a"]=>
int(1)
}
}

So it's not always true.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Ferenc Kovacs — view source

unread

Hi!

But neither is introducing a potential bomb of the kind that the 'date'
saga
created. The problem this change IS causing is likely to hit many live
sites

The claim that many live sites actually regularly use string multiple
string
offsets to distinguish strings from arrays sounds implausible to me.

Hi,

I hadn't the opportunity to install php 5.4 to test this, so I was
wondering if someone could test how would this code behave ?

###########
if (
!isset( $widget_options['dashboard_incoming_links'] )
|| !isset( $widget_options['dashboard_incoming_links']['home'] )
|| $widget_options['dashboard_incoming_links']['home'] !=
get_option('home') )
###########

Is that correct saying that if the first condition is false the second
will always be true ?

Note: that's a piece of Wordpress

hi.

you can also test it http://codepad.viper-7.com/ if you don't want/can't
have a build from source.
What does get_option('home') return?
From a quick glance, I think that you are safe, except if you get a string
instead of an array(either for $widget_options
or $widget_options['dashboard_incoming_links']), and the first character of
that string is a slash and your get_option('home') returns a slash also.
in which case your code will behave as it would get a correct array
having $widget_options['dashboard_incoming_links']['home'] = '/'

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by David Muir — view source

unread

Hi!

If you want this fix to be palatable, we'll need a new language
construct that effectively runs a series of is_array&& array_key_exists
with a final !== null check.

I'm not sure RC time is a good place for introducing new language
constructs. Actually, I'm pretty sure it's not.

And I believe you're missing the point. I'm not suggesting it for 5.4,
but illustrating that until something like it (or better) exists, the
"bug fix" should wait until such a solution exists.

David

13 years ago by Stas Malyshev — view source

unread

Hi!

And I believe you're missing the point. I'm not suggesting it for 5.4,
but illustrating that until something like it (or better) exists, the
"bug fix" should wait until such a solution exists.

Again, you seem to miss the major point. The bug fix was fixing the
result of string operation, which before that was using "string offset"
pseudo-type that never really worked properly, since it required special
branches throughout the engine and tons of special cases and still
produced bugs and segfaults. The fact that $a['foo']['bar'] works is
just a side effect of that bug fix - because the result of string offset
op is now string, as it always should have been.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by Ferenc Kovacs — view source

unread

Hi!

The only case where the 5.4 branch works differently as before if you
reference a string type(int, float, etc. won't trigger this) variable
using an associative index and you expect it that to be undefined
variable even though that the documentation explicitly states that the

Actually, the only change 5.4 did was to make $a['foo']['bar'] work
like ($a['foo'])['bar'] - i.e. chained offsets work the same way as if
they were applied separately. That's it. All the rest has been there
since forever. I can't see how one could argue it should stay this way.

Now I'm sorry somebody used the fact that chained offsets didn't work
to do a check "do we have any strings there" but that's not how PHP is
supposed to work and it's clearly a side-effect of a bug.

One thing that I wasn't aware of was the string to int conversion for
string offsets. IMO, that should trigger a notice or something. Good to
be reminded of though.

Just to clarify, the changes introduced in 5.4 will result in the
following:

<?php

$string = 'foo';
$array = array(
'foo' => array(
'bar' => 'baz',
//expected structure
//'bar' => array('baz' => array('values'))
));

var_dump(
isset($string['foo']), //true
isset($string[0][0]), //false, true in 5.4
isset($array['foo']['bar'][0]), //true
isset($array['foo']['bar']['baz']), //true
isset($array['foo']['bar']['baz']['0']) //false, true as of 5.4
isset($string['foo']['bar']['baz']['0']) //false, true as of 5.4
);

you are missing a comma from the end of the
isset($array['foo']['bar']['baz']['0']) //false, true as of 5.4
line

isset($string['foo']['bar']['baz']['0']) //false, true as of 5.4
gives me a fatal error on 5.3 ("PHP Fatal error: Cannot use string offset
as an array" as you can't "chain" string offsets before 5.4)

If so, then it's a major BC break.
Maybe it's a bugfix, but it's a bugfix that allows for behaviour that

doesn't really make sense

I think Stas and Etienne summed up the benefits and the reason of this
change pretty much(mostly internals related).

has little to no practical benefit (when are you ever going to be
chaining offsets on strings?)

I agree that there are not (m)any usecase for the ability for method
chaining in it's current form.

introduces new, hard-to-detect bugs in live code.

the bugs are there already(you expect an array where you get a string), it
will just fail differently.

What used to be a one-liner, now effectively needs a crapload of
boilerplate code.

yeah, if you can't trust the data structures in your code, then you have to
validate it.
don't forget that this bug could also bite you with 5.3:

// you expect $foo to be an array
$foo = 'string';

//this will echo 's'
echo $foo['bar'];

Personally I think that the main issue is that we silently convert
$foo['bar'] to $foo[0] for strings, which imo rarely the intended behavior
and doesn't really makes much sense (except that the offset expects the
index to be int, and this is how we type juggle between string and int), so
I think it would be a good idea to raise a notice here, so the developer
can fix his code, or add some input validation.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Anthony Ferrara — view source

unread

One thing to note. The good test for what's being talked about would be simple:

isset($foo['bar'][1]['baz']) && is_array($foo['bar'][1])

You don't need to check each level. Only the one above the key you're
looking at.

I think it would be a good idea to raise a notice on string index
conversion ($string['foo']). I understand it is legacy behavior, but
given this bug fix, it would be a good way to identify these types of
"issues" people keep talking about.

Anthony

Hi!

The only case where the 5.4 branch works differently as before if you
reference a string type(int, float, etc. won't trigger this) variable
using an associative index and you expect it that to be undefined
variable even though that the documentation explicitly states that the

Actually, the only change 5.4 did was to make $a['foo']['bar'] work
like ($a['foo'])['bar'] - i.e. chained offsets work the same way as if
they were applied separately. That's it. All the rest has been there
since forever. I can't see how one could argue it should stay this way.

Now I'm sorry somebody used the fact that chained offsets didn't work
to do a check "do we have any strings there" but that's not how PHP is
supposed to work and it's clearly a side-effect of a bug.

One thing that I wasn't aware of was the string to int conversion for
string offsets. IMO, that should trigger a notice or something. Good to
be reminded of though.

Just to clarify, the changes introduced in 5.4 will result in the
following:

<?php

$string = 'foo';
$array = array(
'foo' => array(
'bar' => 'baz',
//expected structure
//'bar' => array('baz' => array('values'))
));

var_dump(
isset($string['foo']), //true
isset($string[0][0]), //false, true in 5.4
isset($array['foo']['bar'][0]), //true
isset($array['foo']['bar']['baz']), //true
isset($array['foo']['bar']['baz']['0']) //false, true as of 5.4
isset($string['foo']['bar']['baz']['0']) //false, true as of 5.4
);

you are missing a comma from the end of the
isset($array['foo']['bar']['baz']['0']) //false, true as of 5.4
line

isset($string['foo']['bar']['baz']['0']) //false, true as of 5.4
gives me a fatal error on 5.3 ("PHP Fatal error: Cannot use string offset
as an array" as you can't "chain" string offsets before 5.4)

If so, then it's a major BC break.
Maybe it's a bugfix, but it's a bugfix that allows for behaviour that

doesn't really make sense

I think Stas and Etienne summed up the benefits and the reason of this
change pretty much(mostly internals related).

has little to no practical benefit (when are you ever going to be
chaining offsets on strings?)

I agree that there are not (m)any usecase for the ability for method
chaining in it's current form.

introduces new, hard-to-detect bugs in live code.

the bugs are there already(you expect an array where you get a string), it
will just fail differently.

What used to be a one-liner, now effectively needs a crapload of
boilerplate code.

yeah, if you can't trust the data structures in your code, then you have to
validate it.
don't forget that this bug could also bite you with 5.3:

// you expect $foo to be an array
$foo = 'string';

//this will echo 's'
echo $foo['bar'];

Personally I think that the main issue is that we silently convert
$foo['bar'] to $foo[0] for strings, which imo rarely the intended behavior
and doesn't really makes much sense (except that the offset expects the
index to be int, and this is how we type juggle between string and int), so
I think it would be a good idea to raise a notice here, so the developer
can fix his code, or add some input validation.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by David Muir — view source

unread

On Thu, Nov 24, 2011 at 1:38 AM, David Muir <davidkmuir@gmail.com
mailto:davidkmuir@gmail.com> wrote:

Just to clarify, the changes introduced in 5.4 will result in the
following:

&lt;?php

$string = 'foo';
$array = array(
   'foo' => array(
       'bar' => 'baz',
       //expected structure
       //'bar' => array('baz' => array('values'))
));

var_dump(
   isset($string['foo']), //true
   isset($string[0][0]), //false, true in 5.4
   isset($array['foo']['bar'][0]), //true
   isset($array['foo']['bar']['baz']), //true
   isset($array['foo']['bar']['baz']['0']) //false, true as of 5.4
   isset($string['foo']['bar']['baz']['0']) //false, true as of 5.4
);

you are missing a comma from the end of the
isset($array['foo']['bar']['baz']['0']) //false, true as of 5.4
line

Yeah, I added that one at the last minute. That's what I get for a quick
copy/paste...

isset($string['foo']['bar']['baz']['0']) //false, true as of 5.4
gives me a fatal error on 5.3 ("PHP Fatal error: Cannot use string
offset as an array" as you can't "chain" string offsets before 5.4)

It gives me false in 5.3.6. Using it outside of isset() results in the
fatal error.

What used to be a one-liner, now effectively needs a crapload of
boilerplate code.
yeah, if you can't trust the data structures in your code, then you
have to validate it.
don't forget that this bug could also bite you with 5.3:

True, it does fail in 5.3 in some places, but it now fails in more
places with 5.4. Maybe what's triggering the hostile reaction to this
change is that people incorrectly assumed that they were validating it
in a valid manner. The docs for isset states that it works for array
keys, but there's no mention about the string offset gotcha, and it's a
much more convenient option that using array_key_exists + is_array for
each level.

Can we add the lines:

var_dump(isset($a['pie']['a'][0])); // TRUE
var_dump(isset($a['pie']['a']['b'])); // TRUE

to the isset docs? At least then the current behaviour would be
documented. And maybe add Anthony Ferrera's tip that only the last
element needs an array check?

Personally I think that the main issue is that we silently convert
$foo['bar'] to $foo[0] for strings, which imo rarely the intended
behavior and doesn't really makes much sense (except that the offset
expects the index to be int, and this is how we type juggle between
string and int), so I think it would be a good idea to raise a notice
here, so the developer can fix his code, or add some input validation.

That would be a help.

Cheers,
David

13 years ago by Lester Caine — view source

unread

David Muir wrote:

Personally I think that the main issue is that we silently convert $foo['bar']
to $foo[0] for strings, which imo rarely the intended behavior and doesn't
really makes much sense (except that the offset expects the index to be int,
and this is how we type juggle between string and int), so I think it would be
a good idea to raise a notice here, so the developer can fix his code, or add
some input validation.

That would be a help.
That would help a lot ... this is not a problem of "so the developer can fix his
code" but rather so we can fix legacy code which other non-developers are
currently using happily ... If I had written the code I would not have done it
the way it is currently structured, but I'm not about to spend time
re-engineering it ... I just want to plug the holes quickly so it continues to work.

--
Lester Caine - G8HFL

13 years ago by Stas Malyshev — view source

unread

Hi!

That would help a lot ... this is not a problem of "so the developer can fix his
code" but rather so we can fix legacy code which other non-developers are
currently using happily ... If I had written the code I would not have done it
the way it is currently structured, but I'm not about to spend time
re-engineering it ... I just want to plug the holes quickly so it continues to work.

OK, notice is probably technically feasible, though I still not like the
idea too much. When you want to produce this notice? Producing it on any
string would probably break code like $a['1'] which has it's legitimate
uses and I'm sure can be seen around. Should be produce notice if the
string has non-numeric chars? That's slow down this operation a little,
though probably not critically as conversion is going to scan the string
anyway.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

13 years ago by Lester Caine — view source

unread

Stas Malyshev wrote:

Hi!

That would help a lot ... this is not a problem of "so the developer can fix his
code" but rather so we can fix legacy code which other non-developers are
currently using happily ... If I had written the code I would not have done it
the way it is currently structured, but I'm not about to spend time
re-engineering it ... I just want to plug the holes quickly so it continues to
work.

OK, notice is probably technically feasible, though I still not like the idea
too much. When you want to produce this notice? Producing it on any string would
probably break code like $a['1'] which has it's legitimate uses and I'm sure can
be seen around. Should be produce notice if the string has non-numeric chars?
That's slow down this operation a little, though probably not critically as
conversion is going to scan the string anyway.

The 'index not found' error usually pops up where I'm messing up the existing
'logic' so something that flags that a 'phantom' index has been created when a
'real' ['sub-element'] was being looked for. In my own case I'm sure once I dig
out the problem the answer will be obvious as well, but I simply don't have the
time to spend. So currently a production 5.4 upgrade is not possible.

It would be useful if some of the other people flagging this problem could
provide some info on what is failing ... the original PEAR problem for example?
I'm not sure that any of us know just what edge case is causing a hickup?

--
Lester Caine - G8HFL

13 years ago by Ferenc Kovacs — view source

unread

Stas Malyshev wrote:

Hi!

That would help a lot ... this is not a problem of "so the developer can

fix his
code" but rather so we can fix legacy code which other non-developers are
currently using happily ... If I had written the code I would not have
done it
the way it is currently structured, but I'm not about to spend time
re-engineering it ... I just want to plug the holes quickly so it
continues to
work.

OK, notice is probably technically feasible, though I still not like the
idea
too much. When you want to produce this notice? Producing it on any
string would
probably break code like $a['1'] which has it's legitimate uses and I'm
sure can
be seen around. Should be produce notice if the string has non-numeric
chars?
That's slow down this operation a little, though probably not critically
as
conversion is going to scan the string anyway.

The 'index not found' error usually pops up where I'm messing up the
existing 'logic' so something that flags that a 'phantom' index has been
created when a 'real' ['sub-element'] was being looked for. In my own case
I'm sure once I dig out the problem the answer will be obvious as well, but
I simply don't have the time to spend. So currently a production 5.4
upgrade is not possible.

It would be useful if some of the other people flagging this problem could
provide some info on what is failing ... the original PEAR problem for
example? I'm not sure that any of us know just what edge case is causing a
hickup?

Without debugging the exact problem or having a script replicating the
different behavior between 5.3. and 5.4, your feedback doesn't really
provide anything valuable, as it can be caused by this change, or something
else.
We only aware of two behavior difference between 5.3 and 5.4 related to
this particular change:

offset chaining works now, while it was throwing a fatal error in 5.3
($string[0][0][0] returns the first character of $string)
which means that the type juggling behavior (unrelated to this change,
also exists in 5.3) can occur not just for $string['foo'] but for
$array['string_exists_where_array_expected']['foo'] also (this is what
Daniel bumped into in the PEAR tests).

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Yasuo Ohgaki — view source

unread

Just a quick note for this issue.
If I remember correctly, PHP is working this way at least from PHP 3.0.x.
$a[123] // integer index
$a['123'] // integer index
$a['123 abc'] // string index
$a['123.123'] // string index

Automatic string to integer conversion only happened if index string
has numeric chars.

It seems some people are confused.
isset($a[0][0]) became true if and only if $a is string.

If there is a code broken by this behavior, it would be enough for add
is_array()

if (is_array($a) && isset($a[0][0]))

I suppose.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Ferenc Kovacs — view source

unread

Just to clarify, the changes introduced in 5.4 will result in the
following:

<?php

$string = 'foo';
$array = array(
'foo' => array(
'bar' => 'baz',
//expected structure
//'bar' => array('baz' => array('values'))
));

var_dump(
isset($string['foo']), //true
isset($string[0][0]), //false, true in 5.4
isset($array['foo']['bar'][0]), //true
isset($array['foo']['bar']['baz']), //true
isset($array['foo']['bar']['baz']['0']) //false, true as of 5.4
isset($string['foo']['bar']['baz']['0']) //false, true as of 5.4
);

you are missing a comma from the end of the
isset($array['foo']['bar']['baz']['0']) //false, true as of 5.4
line

Yeah, I added that one at the last minute. That's what I get for a quick
copy/paste...

isset($string['foo']['bar']['baz']['0']) //false, true as of 5.4
gives me a fatal error on 5.3 ("PHP Fatal error: Cannot use string offset
as an array" as you can't "chain" string offsets before 5.4)

It gives me false in 5.3.6. Using it outside of isset() results in the
fatal error.

hm.

tyrael@thor:~$ php -r '$string =
"foo";isset($string["foo"]["bar"]["baz"]["0"]);';
PHP Fatal error: Cannot use string offset as an array in Command line code
on line 1
tyrael@thor:~$ php -v
PHP 5.3.8-1~dotdeb.2 with Suhosin-Patch (cli) (built: Aug 25 2011 13:30:46)
Copyright (c) 1997-2011 The PHP Group
Zend Engine v2.3.0, Copyright (c) 1998-2011 Zend Technologies
with Suhosin v0.9.32.1, Copyright (c) 2007-2010, by SektionEins GmbH

I will check this against a vanilla version, as there is a chance that
either dotdeb(less likely) or suhosin patched something.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Lester Caine — view source

unread

Ferenc Kovacs wrote:

I will check this against a vanilla version, as there is a chance that either
dotdeb(less likely) or suhosin patched something.

I had a look, my SUSE boxes have suhosin on still, but the 5.3 test machine is
running windows without it ...

--
Lester Caine - G8HFL

13 years ago by Lester Caine — view source

unread

Ferenc Kovacs wrote:

I will check this against a vanilla version, as there is a chance that either
dotdeb(less likely) or suhosin patched something.

I had a look, my SUSE boxes have suhosin on still, but the 5.3 test machine is running windows without it ...

5.4 machine even ... the SUSE boxes are 5.3 ...

--
Lester Caine - G8HFL

13 years ago by Ferenc Kovacs — view source

unread

It gives me false in 5.3.6. Using it outside of isset() results in the
fatal error.

hm.

tyrael@thor:~$ php -r '$string =
"foo";isset($string["foo"]["bar"]["baz"]["0"]);';
PHP Fatal error: Cannot use string offset as an array in Command line
code on line 1
tyrael@thor:~$ php -v
PHP 5.3.8-1~dotdeb.2 with Suhosin-Patch (cli) (built: Aug 25 2011 13:30:46)
Copyright (c) 1997-2011 The PHP Group
Zend Engine v2.3.0, Copyright (c) 1998-2011 Zend Technologies
with Suhosin v0.9.32.1, Copyright (c) 2007-2010, by SektionEins GmbH

I will check this against a vanilla version, as there is a chance that
either dotdeb(less likely) or suhosin patched something.

I just remembered that there is http://codepad.viper-7.com/
so:
5.3 - fatal error: http://codepad.viper-7.com/GGhoSn
5.4 - works: http://codepad.viper-7.com/WRdyni

Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by devis@lucato.it — view source

unread

Thanks Ferenc

Everything looks ok http://codepad.viper-7.com/JTXsGK

Devis

It gives me false in 5.3.6. Using it outside of isset() results in the
fatal error.

hm.

tyrael@thor:~$ php -r '$string =
"foo";isset($string["foo"]["bar"]["baz"]["0"]);';
PHP Fatal error: Cannot use string offset as an array in Command line
code on line 1
tyrael@thor:~$ php -v
PHP 5.3.8-1~dotdeb.2 with Suhosin-Patch (cli) (built: Aug 25 2011 13:30:46)
Copyright (c) 1997-2011 The PHP Group
Zend Engine v2.3.0, Copyright (c) 1998-2011 Zend Technologies
with Suhosin v0.9.32.1, Copyright (c) 2007-2010, by SektionEins GmbH

I will check this against a vanilla version, as there is a chance that
either dotdeb(less likely) or suhosin patched something.

I just remembered that there is http://codepad.viper-7.com/
so:
5.3 - fatal error: http://codepad.viper-7.com/GGhoSn
5.4 - works: http://codepad.viper-7.com/WRdyni

Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Ferenc Kovacs — view source

unread

Thanks Ferenc

Everything looks ok http://codepad.viper-7.com/JTXsGK

Devis

here are the corner cases:
5.3: http://codepad.viper-7.com/nPLorU
5.4: http://codepad.viper-7.com/MUdAlc

as you can see from the 4th example, your test will have an unexpected
result, if your $widget_options['dashboard_incoming_links'] is a string,
instead of an array, and it's first character is the same as
your get_option('home') return value.
this is because for string offsets the engine does a type juggling to int
for the index.

which is new in 5.4, that string offset chaining is supported, so as you
can see in the 5th example, your check will also fail if $widget_options is
a string instead of an array, and it's first character is the same as
your get_option('home') return value.

(this won't happen if your expected to be array variable is an empty
string, because isset($empty_string[0]) is false.)

another thing that I noticed while playing around:
in 5.3 the following code works:
$foobar = 'foobar';
isset($foobar['foo']);
isset($foobar['foo']['bar']);
echo $foobar['foo'];

but the following will trigger the fatal error:
isset($foobar['foo']['bar']['baz']);
echo $foobar['foo']['bar'];

So isset in 5.3 isn't immune to the fatal error, just it needs one more
chain to trigger it.
Which means that it is less likely to bump into this bug than I thought.

$string = 'foobar';
var_dump(isset($string['foo'])); // this behaves the same, both 5.3 and 5.4
returns true
var_dump($string['foo']); // this behaves the same, both 5.3 and 5.4
returns $string[0] -> 'f'
var_dump(isset($string['foo']['bar'])); // this behaves differently, 5.3
returns false but 5.4 returns true
var_dump($string['foo']['bar']); // this behaves differently, 5.3 fatals
but 5.4 returns $string[0] -> 'f'

5.3:
http://codepad.viper-7.com/yLajg6

5.4:
http://codepad.viper-7.com/S8Q7DG

Additional chaining will produce fatal errors in 5.3 (both for accessing
and calling isset) and will work in 5.4.
So there are only 2 cases, where 5.4 is different from 5.3, and the one of
them was throwing fatal error in 5.3, and I think that making it work
wouldn't count as a BC break.
Which means that there is only one single case which changes existing
behavior(breaks BC) and that is isset($string['foo']['bar']) the exact same
case that Daniel reported.

Another thing:
In 5.4 accessing arrays and strings through indexes/offsets have some
inconsistencies:

isset($array[0][0][0][0]..[0]) will always return true for strings, but
that's not true for arrays.
$array[0][0][0][0]..[0] will produce a notice for arrays(undefined
index), but that's not true for strings.

This is of course because they are different things, I'm just stating there
for the sake of completeness.

So I think if we could lessen/negate the impact of that single case, this
change wouldn't have BC issues.

ps:
I think that we can all agree that $foo[1][0] makes sense if $foo is an
array, but it isn't really useful if $foo is a string (as it will return
the same character as $foo[1][0]).

The other improvement (related but not introduced in this change) that I
suggested was that we could also trigger a notice when a "non-applicable"
string offset is passed(defining non-applicable is a little bit hard,
because of the current type-juggling rules, we have to allow $string["1"],
because '1" can come from a database, or get/post, where it would be a
string, not an int, but if we go with the current type juggling,
$string["2_foo_3"] would also be converted to $string[2], which isn't
really intended imo. .

Sorry for the long mail.

Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Christopher Jones — view source

unread

Hi Etienne:

AFAIK this is one of the change that was explicitly put in to make it
more consistent.

But now it breaks code in the wild. I came across this due to an
isset() in PEAR now passing when it didn't before.

Thanks,

--Dan

Since we have cases in the wild of this, I think we should make the
change more visible in UPGRADING or NEWS - once this current thread
has a conclusion. This will help forestall future repetitions of the
discussion. Daniel, can you "own" this doc task?

Chris

--
Email: christopher.jones@oracle.com
Tel: +1 650 506 8630
Blog: http://blogs.oracle.com/opal/

13 years ago by Richard Quadling — view source

unread

On 23 November 2011 01:50, Daniel Convissor
danielc@analysisandsolutions.com wrote:

Hi Folks:

I just stumbled upon a regression in 5.4. In an array, a sub-sub-key of
an existing key is now returning a letter of the value indexed by the
main key. I'm raising it here so it doesn't get lost.

https://bugs.php.net/bug.php?id=60362

Thanks,

--Dan

I've just ran the code for the bug through all the V5 releases/betas/alphas/RCs.

V5.0.0 to V5.0.2

BEHAVIOR CHANGED: sub-key 'non_existent' is not set.
expected: sub-key 1 is set: string(1) "o"

good: sub-sub-key 'sub_sub' is not set.
good: sub-sub-key 0 is not set.

BEHAVIOR CHANGED: sub-key 'non_existent' is empty.
expected: sub-key 1 is NOT empty: string(1) "o"

good: sub-sub-key 'sub_sub' is empty.
good: sub-sub-key 0 is empty.

V5.0.3 (don't have)

V5.0.4 to V5.3.9RC2-dev (as at around 10am GMT on the 23rd Nov 2011).
expected: sub-key 'non_existent' is set: string(1) "f"
expected: sub-key 1 is set: string(1) "o"

good: sub-sub-key 'sub_sub' is not set.
good: sub-sub-key 0 is not set.

expected: sub-key 'non_existent' is not empty: string(1) "f"
expected: sub-key 1 is NOT empty: string(1) "o"

good: sub-sub-key 'sub_sub' is empty.
good: sub-sub-key 0 is empty.

V5.4+ (betas and RCs)
expected: sub-key 'non_existent' is set: string(1) "f"
expected: sub-key 1 is set: string(1) "o"

BEHAVIOR CHANGED: sub-key 'sub_sub' is set: string(1) "f"
BEHAVIOR CHANGED: sub-sub-key 0 is set: string(1) "o"

expected: sub-key 'non_existent' is not empty: string(1) "f"
expected: sub-key 1 is NOT empty: string(1) "o"

BEHAVIOR CHANGED: sub-sub-key 'sub_sub' is not empty: string(1) "f"
BEHAVIOR CHANGED: sub-sub-key 0 is not empty: string(1) "o"

So there was a previous bug fix in V5.0.2 (maybe 5.0.3).

So, the whole type juggling point here is mute. It has been that way
for a very long time. I just didn't realise it.

And if that is a valid result
(var_dump($arr['exists']['non_existent']) === 'f') - which it now is
obviously is, then the change in V5.4 is certainly a bug fix.

But, it is a significant enough issue that warrants a decent amount of
documentation.

--
Richard Quadling
Twitter : EE : Zend : PHPDoc : Fantasy Shopper
@RQuadling : e-e.com/M_248814.html : bit.ly/9O8vFY : bit.ly/lFnVea :
fan.sh/6/370

13 years ago by Yasuo Ohgaki — view source

unread

Hi,

2011/11/24 Richard Quadling rquadling@gmail.com:

On 23 November 2011 01:50, Daniel Convissor
danielc@analysisandsolutions.com wrote:

Hi Folks:

I just stumbled upon a regression in 5.4. In an array, a sub-sub-key of
an existing key is now returning a letter of the value indexed by the
main key. I'm raising it here so it doesn't get lost.

https://bugs.php.net/bug.php?id=60362

Thanks,

--Dan

I've just ran the code for the bug through all the V5 releases/betas/alphas/RCs.

V5.0.0 to V5.0.2

BEHAVIOR CHANGED: sub-key 'non_existent' is not set.
expected: sub-key 1 is set: string(1) "o"

good: sub-sub-key 'sub_sub' is not set.
good: sub-sub-key 0 is not set.

BEHAVIOR CHANGED: sub-key 'non_existent' is empty.
expected: sub-key 1 is NOT empty: string(1) "o"

good: sub-sub-key 'sub_sub' is empty.
good: sub-sub-key 0 is empty.

V5.0.3 (don't have)

V5.0.4 to V5.3.9RC2-dev (as at around 10am GMT on the 23rd Nov 2011).
expected: sub-key 'non_existent' is set: string(1) "f"
expected: sub-key 1 is set: string(1) "o"

good: sub-sub-key 'sub_sub' is not set.
good: sub-sub-key 0 is not set.

expected: sub-key 'non_existent' is not empty: string(1) "f"
expected: sub-key 1 is NOT empty: string(1) "o"

good: sub-sub-key 'sub_sub' is empty.
good: sub-sub-key 0 is empty.

V5.4+ (betas and RCs)
expected: sub-key 'non_existent' is set: string(1) "f"
expected: sub-key 1 is set: string(1) "o"

BEHAVIOR CHANGED: sub-key 'sub_sub' is set: string(1) "f"
BEHAVIOR CHANGED: sub-sub-key 0 is set: string(1) "o"

expected: sub-key 'non_existent' is not empty: string(1) "f"
expected: sub-key 1 is NOT empty: string(1) "o"

BEHAVIOR CHANGED: sub-sub-key 'sub_sub' is not empty: string(1) "f"
BEHAVIOR CHANGED: sub-sub-key 0 is not empty: string(1) "o"

So there was a previous bug fix in V5.0.2 (maybe 5.0.3).

So, the whole type juggling point here is mute. It has been that way
for a very long time. I just didn't realise it.

And if that is a valid result
(var_dump($arr['exists']['non_existent']) === 'f') - which it now is
obviously is, then the change in V5.4 is certainly a bug fix.

But, it is a significant enough issue that warrants a decent amount of
documentation.

Nice effort for checking PHP 5.0.

I guess this difference comes from string offset access method change
introduced some where in 5.x.
Wasn't it supposed to access string offset like

<?php
$str = 'abc';
$c = $str{1};
var_dump($c);

Output from PHP 5.3
string(1) "b"

Accessing via [] is more consistent, so I prefer current method.

Howerver, following code executes with PHP 5.4

$ ./php -r '$s = "abc"; var_dump($s[0]["bar"]);
string(1) "a"

This is not right behavior. As far as I know, PHP only treat "string"
index as "integer" index if and only if "string was consisted only by
numbers".

I think current code is using convert_to_long() for sub array. It
should use string index to integer conversion code rather than
convert_to_long() to make consistent behavior.

With PHP 5.3, it rases fatal error
$ php -r '$s = "abc"; var_dump($s[0]["bar"]);'
PHP Fatal error: Cannot use string offset as an array in Command line
code on line 1

Raising notice error for accessing "string" by string offset seems to
be the feasible solution.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Yasuo Ohgaki — view source

unread

Hi all,

I should think twice before seding mail. "abc" as array index is
converted to 0 since it's not a integer. So with current code is
behave consistently with regards to string to long conversion.

However,

PHP 5.3
php -r '$s = "abc"; var_dump($s[0]["bar"]);'
PHP Fatal error: Cannot use string offset as an array in Command line
code on line 1

PHP 5.4
./php -r '$s = "abc"; var_dump($s[0]["bar"]);'
string(1) "a"

Isn't it better to raise notice for accessing string by string index?
There is no use to allowing string index access to strings. I think
raising notice is feasible. Isn't it?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by Rasmus Lerdorf — view source

unread

Hi all,

I should think twice before seding mail. "abc" as array index is
converted to 0 since it's not a integer. So with current code is
behave consistently with regards to string to long conversion.

However,

PHP 5.3
php -r '$s = "abc"; var_dump($s[0]["bar"]);'
PHP Fatal error: Cannot use string offset as an array in Command line
code on line 1

PHP 5.4
./php -r '$s = "abc"; var_dump($s[0]["bar"]);'
string(1) "a"

Isn't it better to raise notice for accessing string by string index?
There is no use to allowing string index access to strings. I think
raising notice is feasible. Isn't it?

String index access is still required since they are often numeric
strings. We could add a notice for non-numeric strings, but the check
would slow things down a bit.

-Rasmus

13 years ago by Ferenc Kovacs — view source

unread

Hi all,

I should think twice before seding mail. "abc" as array index is
converted to 0 since it's not a integer. So with current code is
behave consistently with regards to string to long conversion.

However,

PHP 5.3
php -r '$s = "abc"; var_dump($s[0]["bar"]);'
PHP Fatal error: Cannot use string offset as an array in Command line
code on line 1

PHP 5.4
./php -r '$s = "abc"; var_dump($s[0]["bar"]);'
string(1) "a"

Isn't it better to raise notice for accessing string by string index?
There is no use to allowing string index access to strings. I think
raising notice is feasible. Isn't it?

String index access is still required since they are often numeric
strings. We could add a notice for non-numeric strings, but the check
would slow things down a bit.

yeah, as I mentioned before:
"The other improvement (related but not introduced in this change) that I
suggested was that we could also trigger a notice when a "non-applicable"
string offset is passed(defining non-applicable is a little bit hard,
because of the current type-juggling rules, we have to allow $string["1"],
because '1" can come from a database, or get/post, where it would be a
string, not an int, but if we go with the current type juggling,
$string["2_foo_3"] would also be converted to $string[2], which isn't
really intended imo. ."

I would vote for allowing only numbers in the string index/offset: [0-9]+

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Ferenc Kovacs — view source

unread

Isn't it better to raise notice for accessing string by string index?
There is no use to allowing string index access to strings. I think
raising notice is feasible. Isn't it?

String index access is still required since they are often numeric
strings. We could add a notice for non-numeric strings, but the check
would slow things down a bit.

yeah, as I mentioned before:
"The other improvement (related but not introduced in this change) that I
suggested was that we could also trigger a notice when a "non-applicable"
string offset is passed(defining non-applicable is a little bit hard,
because of the current type-juggling rules, we have to allow $string["1"],
because '1" can come from a database, or get/post, where it would be a
string, not an int, but if we go with the current type juggling,
$string["2_foo_3"] would also be converted to $string[2], which isn't
really intended imo. ."

I would vote for allowing only numbers in the string index/offset: [0-9]+

By "allowing" I meant that we should trigger the notice if the string index
contains anything else than numbers.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

13 years ago by Daniel Convissor — view source

unread

Hi Ferenc:

I would vote for allowing only numbers in the string index/offset: [0-9]+

By "allowing" I meant that we should trigger the notice if the string index
contains anything else than numbers.

I agree with this approach as well.

Thanks,

--Dan

13 years ago by Yasuo Ohgaki — view source

unread

2011/11/25 Rasmus Lerdorf rasmus@lerdorf.com:

Hi all,

I should think twice before seding mail. "abc" as array index is
converted to 0 since it's not a integer. So with current code is
behave consistently with regards to string to long conversion.

However,

PHP 5.3
php -r '$s = "abc"; var_dump($s[0]["bar"]);'
PHP Fatal error: Cannot use string offset as an array in Command line
code on line 1

PHP 5.4
./php -r '$s = "abc"; var_dump($s[0]["bar"]);'
string(1) "a"

Isn't it better to raise notice for accessing string by string index?
There is no use to allowing string index access to strings. I think
raising notice is feasible. Isn't it?

String index access is still required since they are often numeric
strings. We could add a notice for non-numeric strings, but the check
would slow things down a bit.

How about enable notice only for PHP 5.4?

Programmer should check vars to see if it's an array, but finding the
cause of misbehavior without errors is pain for users.

I don't think we have to remove this feature, but it would be nice to
raise notice error for a while.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

13 years ago by devis@lucato.it — view source

unread

Hi all,

I should think twice before seding mail. "abc" as array index is
converted to 0 since it's not a integer. So with current code is
behave consistently with regards to string to long conversion.

However,

PHP 5.3
php -r '$s = "abc"; var_dump($s[0]["bar"]);'
PHP Fatal error: Cannot use string offset as an array in Command line
code on line 1

PHP 5.4
./php -r '$s = "abc"; var_dump($s[0]["bar"]);'
string(1) "a"

Isn't it better to raise notice for accessing string by string index?
There is no use to allowing string index access to strings. I think
raising notice is feasible. Isn't it?

String index access is still required since they are often numeric
strings. We could add a notice for non-numeric strings, but the check
would slow things down a bit.

-Rasmus

Would it be possible to have that check only if E_NOTICE is enabled ?
That would allow to limit the cost to development environments
(assuming one could disable E_NOTICEs on production env).

Devis

13 years ago by Alan Knowles — view source

unread

Hi all,

I should think twice before seding mail. "abc" as array index is
converted to 0 since it's not a integer. So with current code is
behave consistently with regards to string to long conversion.

However,

PHP 5.3
php -r '$s = "abc"; var_dump($s[0]["bar"]);'
PHP Fatal error: Cannot use string offset as an array in Command line
code on line 1

PHP 5.4
./php -r '$s = "abc"; var_dump($s[0]["bar"]);'
string(1) "a"

Isn't it better to raise notice for accessing string by string index?
There is no use to allowing string index access to strings. I think
raising notice is feasible. Isn't it?
String index access is still required since they are often numeric
strings. We could add a notice for non-numeric strings, but the check
would slow things down a bit.

-Rasmus

Would it be possible to have that check only if E_NOTICE is enabled ?
That would allow to limit the cost to development environments
(assuming one could disable E_NOTICEs on production env).

Devis
This is a design issue of using the same syntax for arrays and strings,
and accepting string indexes for both, I think this is beginning to
illustrate some of the flaws in that design.

if (isset($_POST['query']['search'])) { << This does not do what you
think (if ?query=a_string..

Although the chained string offset fix is probably the right thing to
do, I think it should be reverted in this version, then this process occur:

5.4

revert chained string offsets
add E_NOTICE to any usage of string index's of string eg . $a =
"string" ; $a["string"] = gives E_NOTICE
users can add (int) if they really need to check string indexes of a
string
isset($string['x']) will return true as present
isset($string['x']['x']) will return false as present

5.5

reapply chained string offsets
add E_FATAL to any usage of string index's of string = eg . $a =
"string" ; $a["string"] = gives E_FATAL ????
make $a["string"] return an empty string - so isset() on chained
string offsets will work as expected.
isset($string['x']) will return false as per the warning on the
previous version..
isset($string['x']['x']) will return false as present

Thoughts.........

Regards
Alan

13 years ago by Ferenc Kovacs — view source

unread

Hi all,

I should think twice before seding mail. "abc" as array index is
converted to 0 since it's not a integer. So with current code is
behave consistently with regards to string to long conversion.

However,

PHP 5.3
php -r '$s = "abc"; var_dump($s[0]["bar"]);'
PHP Fatal error: Cannot use string offset as an array in Command line
code on line 1

PHP 5.4
./php -r '$s = "abc"; var_dump($s[0]["bar"]);'
string(1) "a"

Isn't it better to raise notice for accessing string by string index?
There is no use to allowing string index access to strings. I think
raising notice is feasible. Isn't it?

agree, and I also suggested adding this notice.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

5.4 regression: non-existent sub-sub keys now have values

behave differently, which was a huge WTF and can only be considered a bug. Any code relying on this should be considered a bug too and fixed immediately. I can not think of any legitimate reason for the code to rely on such things.

-- Lester Caine - G8HFL

-- Lester Caine - G8HFL

-- Lester Caine - G8HFL

-- Lester Caine - G8HFL

Cheers,

That just seems really wrong and the work around is awful.

-- Lester Caine - G8HFL

-- Lester Caine - G8HFL

Cheers,

Cheers,

Cheers,

Now I'm sorry somebody used the fact that chained offsets didn't work to do a check "do we have any strings there" but that's not how PHP is supposed to work and it's clearly a side-effect of a bug.

-- Lester Caine - G8HFL

No, it's not going for array element - it applies multiple array operations to a string without checking and relies on a bug to pass it off. That's broken, plain and simple.

-- Lester Caine - G8HFL

-- Lester Caine - G8HFL

-- Lester Caine - G8HFL

-- Lester Caine - G8HFL

I just remembered that there is http://codepad.viper-7.com/ so: 5.3 - fatal error: http://codepad.viper-7.com/GGhoSn 5.4 - works: http://codepad.viper-7.com/WRdyni

I just remembered that there is http://codepad.viper-7.com/ so: 5.3 - fatal error: http://codepad.viper-7.com/GGhoSn 5.4 - works: http://codepad.viper-7.com/WRdyni

Sorry for the long mail.

BEHAVIOR CHANGED: sub-key 'non_existent' is not set. expected: sub-key 1 is set: string(1) "o"

good: sub-sub-key 'sub_sub' is not set. good: sub-sub-key 0 is not set.

BEHAVIOR CHANGED: sub-key 'non_existent' is empty. expected: sub-key 1 is NOT empty: string(1) "o"

V5.0.4 to V5.3.9RC2-dev (as at around 10am GMT on the 23rd Nov 2011). expected: sub-key 'non_existent' is set: string(1) "f" expected: sub-key 1 is set: string(1) "o"

good: sub-sub-key 'sub_sub' is not set. good: sub-sub-key 0 is not set.

expected: sub-key 'non_existent' is not empty: string(1) "f" expected: sub-key 1 is NOT empty: string(1) "o"

V5.4+ (betas and RCs) expected: sub-key 'non_existent' is set: string(1) "f" expected: sub-key 1 is set: string(1) "o"

BEHAVIOR CHANGED: sub-key 'sub_sub' is set: string(1) "f" BEHAVIOR CHANGED: sub-sub-key 0 is set: string(1) "o"

expected: sub-key 'non_existent' is not empty: string(1) "f" expected: sub-key 1 is NOT empty: string(1) "o"

BEHAVIOR CHANGED: sub-key 'non_existent' is not set. expected: sub-key 1 is set: string(1) "o"

good: sub-sub-key 'sub_sub' is not set. good: sub-sub-key 0 is not set.

BEHAVIOR CHANGED: sub-key 'non_existent' is empty. expected: sub-key 1 is NOT empty: string(1) "o"

V5.0.4 to V5.3.9RC2-dev (as at around 10am GMT on the 23rd Nov 2011). expected: sub-key 'non_existent' is set: string(1) "f" expected: sub-key 1 is set: string(1) "o"

good: sub-sub-key 'sub_sub' is not set. good: sub-sub-key 0 is not set.

expected: sub-key 'non_existent' is not empty: string(1) "f" expected: sub-key 1 is NOT empty: string(1) "o"

V5.4+ (betas and RCs) expected: sub-key 'non_existent' is set: string(1) "f" expected: sub-key 1 is set: string(1) "o"

BEHAVIOR CHANGED: sub-key 'sub_sub' is set: string(1) "f" BEHAVIOR CHANGED: sub-sub-key 0 is set: string(1) "o"

expected: sub-key 'non_existent' is not empty: string(1) "f" expected: sub-key 1 is NOT empty: string(1) "o"

behave differently, which was a huge WTF and can only be considered a
bug. Any code relying on this should be considered a bug too and fixed
immediately. I can not think of any legitimate reason for the code to
rely on such things.

--
Lester Caine - G8HFL

--
Lester Caine - G8HFL

--
Lester Caine - G8HFL

--
Lester Caine - G8HFL

--
Lester Caine - G8HFL

--
Lester Caine - G8HFL

Now I'm sorry somebody used the fact that chained offsets didn't work to
do a check "do we have any strings there" but that's not how PHP is
supposed to work and it's clearly a side-effect of a bug.

--
Lester Caine - G8HFL

No, it's not going for array element - it applies multiple array
operations to a string without checking and relies on a bug to pass it
off. That's broken, plain and simple.

--
Lester Caine - G8HFL

--
Lester Caine - G8HFL

--
Lester Caine - G8HFL

--
Lester Caine - G8HFL

I just remembered that there is http://codepad.viper-7.com/
so:
5.3 - fatal error: http://codepad.viper-7.com/GGhoSn
5.4 - works: http://codepad.viper-7.com/WRdyni

I just remembered that there is http://codepad.viper-7.com/
so:
5.3 - fatal error: http://codepad.viper-7.com/GGhoSn
5.4 - works: http://codepad.viper-7.com/WRdyni

BEHAVIOR CHANGED: sub-key 'non_existent' is not set.
expected: sub-key 1 is set: string(1) "o"

good: sub-sub-key 'sub_sub' is not set.
good: sub-sub-key 0 is not set.

BEHAVIOR CHANGED: sub-key 'non_existent' is empty.
expected: sub-key 1 is NOT empty: string(1) "o"

V5.0.4 to V5.3.9RC2-dev (as at around 10am GMT on the 23rd Nov 2011).
expected: sub-key 'non_existent' is set: string(1) "f"
expected: sub-key 1 is set: string(1) "o"

good: sub-sub-key 'sub_sub' is not set.
good: sub-sub-key 0 is not set.

expected: sub-key 'non_existent' is not empty: string(1) "f"
expected: sub-key 1 is NOT empty: string(1) "o"

V5.4+ (betas and RCs)
expected: sub-key 'non_existent' is set: string(1) "f"
expected: sub-key 1 is set: string(1) "o"

BEHAVIOR CHANGED: sub-key 'sub_sub' is set: string(1) "f"
BEHAVIOR CHANGED: sub-sub-key 0 is set: string(1) "o"

expected: sub-key 'non_existent' is not empty: string(1) "f"
expected: sub-key 1 is NOT empty: string(1) "o"

BEHAVIOR CHANGED: sub-key 'non_existent' is not set.
expected: sub-key 1 is set: string(1) "o"

good: sub-sub-key 'sub_sub' is not set.
good: sub-sub-key 0 is not set.

BEHAVIOR CHANGED: sub-key 'non_existent' is empty.
expected: sub-key 1 is NOT empty: string(1) "o"

V5.0.4 to V5.3.9RC2-dev (as at around 10am GMT on the 23rd Nov 2011).
expected: sub-key 'non_existent' is set: string(1) "f"
expected: sub-key 1 is set: string(1) "o"

good: sub-sub-key 'sub_sub' is not set.
good: sub-sub-key 0 is not set.

expected: sub-key 'non_existent' is not empty: string(1) "f"
expected: sub-key 1 is NOT empty: string(1) "o"

V5.4+ (betas and RCs)
expected: sub-key 'non_existent' is set: string(1) "f"
expected: sub-key 1 is set: string(1) "o"

BEHAVIOR CHANGED: sub-key 'sub_sub' is set: string(1) "f"
BEHAVIOR CHANGED: sub-sub-key 0 is set: string(1) "o"

expected: sub-key 'non_existent' is not empty: string(1) "f"
expected: sub-key 1 is NOT empty: string(1) "o"