Everything

php

Home » Project » Newsgroups » php.internals » Bug 28879: Populating arrays with resources and objects as offsets »

Bug 28879: Populating arrays with resources and objects as offsets

php.internals

Sara Golemon

22 years ago

http://bugs.php.net/28879

The fixes are really simple (and the ZE1 branch is no more complicated), but
I just wanted to double check that this kind of behavior change should be
done.  On the one hand it makes things consistent (which is good), on the
other hand the two "problems" it "solves" are fairly insipid to begin with.
Would this raise a BC issue?

Andi/Zeev?

Index: Zend/zend_execute.c
===================================================================
RCS file: /repository/ZendEngine2/zend_execute.c,v
retrieving revision 1.649
diff -u -r1.649 zend_execute.c
--- Zend/zend_execute.c	18 Jun 2004 18:33:46 -0000	1.649
+++ Zend/zend_execute.c	22 Jun 2004 16:38:27 -0000
@@ -3391,6 +3391,7 @@
case IS_DOUBLE:
zend_hash_index_update(array_ptr->value.ht, (long) offset->value.dval,
&expr_ptr, sizeof(zval *), NULL);
break;
+			case IS_RESOURCE:
case IS_LONG:
case IS_BOOL:
zend_hash_index_update(array_ptr->value.ht, offset->value.lval,
&expr_ptr, sizeof(zval *), NULL);
@@ -3402,6 +3403,7 @@
zend_hash_update(array_ptr->value.ht, "", sizeof(""), &expr_ptr,
sizeof(zval *), NULL);
break;
default:
+				zend_error(E_WARNING, "Illegal offset type");
zval_ptr_dtor(&expr_ptr);
/* do nothing */
break;

Andi Gutmans

22 years ago

This isn't a bug fix, but the behavior we chose.
Basically only integers and strings are supported as array offsets.
As you guessed, we converted doubles and bools to integers (possibly
loosing info in doubles).
As resources is something abstract, I'm not really sure we should "fix"
this. Then again, it probably can't do much harm.
Objects can't be converted to integers because two different objects can
have the same object id! I mentioned it a few times on this list.

Andi

At 09:41 AM 6/22/2004 -0700, Sara Golemon wrote:

Sara Golemon

22 years ago

> > http://bugs.php.net/28879
> >
> This isn't a bug fix, but the behavior we chose.
> Basically only integers and strings are supported as array offsets.
> As you guessed, we converted doubles and bools to integers (possibly
> loosing info in doubles).
> As resources is something abstract, I'm not really sure we should "fix"
> this. Then again, it probably can't do much harm.
>

For myself, I agree with you.  Using resources as array offsets just feels
wrong, and I'm more than happy to leave that be.  My only concern there was
consistency with $arr[$res] = $val; behavior which *does* pickup the lval
and has since ages long past.

But since there's no BC need to allow resources in array initialization,
then sure, leave it to the scripter to cast it to an int if that's REALLY
what they want. (Again "why?" comes to mind).

> Objects can't be converted to integers because two different objects can
> have the same object id! I mentioned it a few times on this list.
>

Nor would I suggest we do that.  The portion of that bug report which refers
to objects is only asking why an error is thrown in $arr[$obj] = $val; style
while $arr = array($obj => $val); simply fails silently.

Here my reservation would be in throwing errors where there previously
wasn't one (this would apply to both Objects and Arrays as offsets).  If
this kind of scripting error is going to cause that name/value pair to
simply be ignored however, then it seems as though some notification should
be raised.

The $arr[$obj] = $val; style raises E_WARNING, so it makes sense (to me) to
do the same with the $arr = array($obj => $val); syntax.

-Sara

Unnamed Person

22 years ago

"Sara Golemon" <pollita@php.net> writes:

> Here my reservation would be in throwing errors where there previously
> wasn't one (this would apply to both Objects and Arrays as offsets).

Prior to release of PHP5 is most certainly the right time to make this sort of
change.  There are already other PHP code changes required when porting to
PHP5, so adding warnings/errors where they didn't exist previously, to
indicate an undocumented "feature" that was (likely) being misused, is fine
IMHO.  Fixing that sort of thing will become part of the porting process to
PHP5, and will possibly avert a hidden bug.

Derrell

Aidan Lister

22 years ago

> But since there's no BC need to allow resources in array initialization,
> then sure, leave it to the scripter to cast it to an int if that's REALLY
> what they want. (Again "why?" comes to mind).

PEAR::DB uses this method - I'm seeing a million errors on sites that have
updated to latest CVS version.

You're forced to i use this method if you want to link a resource back to
it's creation information.

If you have an array of IPs, and you create an array of resources from each
IP, a third array linking the IP back to the casted interger value of the
resource is required if you want to get the IP back. (This is very confusing
to write).

Here's an example:

<?php
$servers[] = "139.134.253.11:27025";
$servers[] = "203.114.130.7:27015";

foreach ($servers as $server)
{
list($ip, $port) = explode(':', $server);
$sock = @fsockopen("udp://" . $ip, $port, $errno, $errstr, 1);
if ($sock !== false)
{
$sockets[] = $sock;
// Create an array so we can link the socket back to the server
$server_keys[$sock] = array_search($server, $servers);

}
}

while (stream_select($r, $w = null, $e = null, 0, calctimeout($maxtime,
$starttime)) !== 0)
{
++$i;
foreach ($r as $socket)
{
$response = stream_get_contents($socket);
// Put the response into an array with the original IP as the key
$results[$server_keys[$socket]][] = $response;
}
}
?>

I hope that demonstrates the purpose properly.

Ofcourse this method can still be used, it just requires an extra cast
(which is a cleaner solution anyway).

Regards,
Aidan

Daniel Convissor

22 years ago

Hi:

On Thu, Jun 24, 2004 at 11:33:54AM +1000, Aidan Lister wrote:

> > But since there's no BC need to allow resources in array initialization,
> > then sure, leave it to the scripter to cast it to an int if that's REALLY
> > what they want. (Again "why?" comes to mind).
>
> PEAR::DB uses this method - I'm seeing a million errors on sites that have
> updated to latest CVS version.

I've just updated PEAR DB in CVS to cast resources to int before use as
array indexes.  The change will be available in the 1.6.5 release, which
should be out in the near future.

--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

Andi Gutmans

22 years ago

Maybe we should move this to an E_STRICT? E_WARNING is more consistent with
existing warnings but I wouldn't want to be over strict, especially when
making this change a couple of weeks before release.
We could escalate the warning for 5.1 if we feel we need to do so.

Andi

At 11:33 AM 6/24/2004 +1000, Aidan Lister wrote:

Sara Golemon

22 years ago

> Maybe we should move this to an E_STRICT? E_WARNING is more consistent

with

> existing warnings but I wouldn't want to be over strict, especially when
> making this change a couple of weeks before release.
> We could escalate the warning for 5.1 if we feel we need to do so.
>

While I have a....layer of skepticism about the approach Aidan described, I
can see the wisdom in easing into error throwing.  Let's go ahed and start
with E_STRICT and discuss an elevation to either E_NOTICE (similar to
implicitly casting unknown constants to strings), or E_WARNING.

-Sara

Andi Gutmans

22 years ago

At 10:15 AM 6/22/2004 -0700, Sara Golemon wrote:

> > > http://bugs.php.net/28879
> > >
> > This isn't a bug fix, but the behavior we chose.
> > Basically only integers and strings are supported as array offsets.
> > As you guessed, we converted doubles and bools to integers (possibly
> > loosing info in doubles).
> > As resources is something abstract, I'm not really sure we should "fix"
> > this. Then again, it probably can't do much harm.
> >
>For myself, I agree with you.  Using resources as array offsets just feels
>wrong, and I'm more than happy to leave that be.  My only concern there was
>consistency with $arr[$res] = $val; behavior which *does* pickup the lval
>and has since ages long past.
>
>But since there's no BC need to allow resources in array initialization,
>then sure, leave it to the scripter to cast it to an int if that's REALLY
>what they want. (Again "why?" comes to mind).

You are right, which is a sucky inconsistency. Instead of "fixing" array
initialization, maybe the right thing to do, is to add an E_WARNING to
$arr[$res] instead? Or at least an E_STRICT.

> > Objects can't be converted to integers because two different objects can
> > have the same object id! I mentioned it a few times on this list.
> >
>Nor would I suggest we do that.  The portion of that bug report which refers
>to objects is only asking why an error is thrown in $arr[$obj] = $val; style
>while $arr = array($obj => $val); simply fails silently.
>
>Here my reservation would be in throwing errors where there previously
>wasn't one (this would apply to both Objects and Arrays as offsets).  If
>this kind of scripting error is going to cause that name/value pair to
>simply be ignored however, then it seems as though some notification should
>be raised.
>
>The $arr[$obj] = $val; style raises E_WARNING, so it makes sense (to me) to
>do the same with the $arr = array($obj => $val); syntax.

Yep, I agree. We should issue an E_WARNING in this case similar to
$arr[$obj]. We should need to copy the illegal offset error msg.

Andi

Sara Golemon

22 years ago

> >For myself, I agree with you.  Using resources as array offsets just

feels

> >wrong, and I'm more than happy to leave that be.  My only concern there

was

> >consistency with $arr[$res] = $val; behavior which *does* pickup the lval
> >and has since ages long past.
> >
> >But since there's no BC need to allow resources in array initialization,
> >then sure, leave it to the scripter to cast it to an int if that's REALLY
> >what they want. (Again "why?" comes to mind).
>
> You are right, which is a sucky inconsistency. Instead of "fixing" array
> initialization, maybe the right thing to do, is to add an E_WARNING to
> $arr[$res] instead? Or at least an E_STRICT.
>

I can get behind that approach.  That only leaves the severity level and
error text to be decided.  Given the behavior gap between these two, I vote
for E_WARNING.  How does this sound:

Index: Zend/zend_execute.c
===================================================================
RCS file: /repository/ZendEngine2/zend_execute.c,v
retrieving revision 1.649
diff -u -r1.649 zend_execute.c
--- Zend/zend_execute.c 18 Jun 2004 18:33:46 -0000 1.649
+++ Zend/zend_execute.c 23 Jun 2004 16:30:51 -0000
@@ -839,8 +839,10 @@
}
}
break;
-  case IS_DOUBLE:
case IS_RESOURCE:
+   zend_error(E_WARNING, "Resource ID#%d used as offset, casting to integer
(%d)", dim->value.lval, dim->value.lval);
+   /* Fall Through */
+  case IS_DOUBLE:
case IS_BOOL:
case IS_LONG: {
long index;
@@ -3402,6 +3404,7 @@
zend_hash_update(array_ptr->value.ht, "", sizeof(""), &expr_ptr,
sizeof(zval *), NULL);
break;
default:
+    zend_error(E_WARNING, "Illegal offset type");
zval_ptr_dtor(&expr_ptr);
/* do nothing */
break;

> >The $arr[$obj] = $val; style raises E_WARNING, so it makes sense (to me)

to

> >do the same with the $arr = array($obj => $val); syntax.
>
> Yep, I agree. We should issue an E_WARNING in this case similar to
> $arr[$obj]. We should need to copy the illegal offset error msg.
>

See second block of above diff...

-Sara

Andi Gutmans

22 years ago

As far as I'm concerned you can go ahead and commit this.

At 09:34 AM 6/23/2004 -0700, Sara Golemon wrote:

Wez Furlong

22 years ago

COM/VARIANT objects can have a default integer value when casted in an
integer context.

--Wez.

----- Original Message -----
From: "Andi Gutmans" <andi@zend.com>
To: "Sara Golemon" <pollita@php.net>; <internals@lists.php.net>
Sent: Tuesday, June 22, 2004 7:02 PM
Subject: Re: [PHP-DEV] Bug 28879: Populating arrays with resources and
objects as offsets

> This isn't a bug fix, but the behavior we chose.
> Basically only integers and strings are supported as array offsets.
> As you guessed, we converted doubles and bools to integers (possibly
> loosing info in doubles).
> As resources is something abstract, I'm not really sure we should "fix"
> this. Then again, it probably can't do much harm.
> Objects can't be converted to integers because two different objects can
> have the same object id! I mentioned it a few times on this list.
>
> Andi
>
> At 09:41 AM 6/22/2004 -0700, Sara Golemon wrote:
> >http://bugs.php.net/28879
> >
> >The fixes are really simple (and the ZE1 branch is no more complicated),

but

> >I just wanted to double check that this kind of behavior change should be
> >done.  On the one hand it makes things consistent (which is good), on the
> >other hand the two "problems" it "solves" are fairly insipid to begin

with.

> >Would this raise a BC issue?
> >
> >Andi/Zeev?
> >
> >Index: Zend/zend_execute.c
> >===================================================================
> >RCS file: /repository/ZendEngine2/zend_execute.c,v
> >retrieving revision 1.649
> >diff -u -r1.649 zend_execute.c
> >--- Zend/zend_execute.c 18 Jun 2004 18:33:46 -0000      1.649
> >+++ Zend/zend_execute.c 22 Jun 2004 16:38:27 -0000
> >@@ -3391,6 +3391,7 @@
> >                         case IS_DOUBLE:
> >
> >zend_hash_index_update(array_ptr->value.ht, (long) offset->value.dval,
> >&expr_ptr, sizeof(zval *), NULL);
> >                                 break;
> >+                       case IS_RESOURCE:
> >                         case IS_LONG:
> >                         case IS_BOOL:
> >
> >zend_hash_index_update(array_ptr->value.ht, offset->value.lval,
> >&expr_ptr, sizeof(zval *), NULL);
> >@@ -3402,6 +3403,7 @@
> >                                 zend_hash_update(array_ptr->value.ht,

"",

> > sizeof(""), &expr_ptr,
> >sizeof(zval *), NULL);
> >                                 break;
> >                         default:
> >+                               zend_error(E_WARNING, "Illegal offset

type");

Andi Gutmans

22 years ago

Don't quite understand what you are saying? Does the recent patch affect you?
Are you talking about explicit casting?

Andi

At 01:32 AM 6/27/2004 +0200, Wez Furlong wrote:

Wez Furlong

22 years ago

I didn't really see the patch; I was just stating that a com object
(well, IDispatch) can be seen as being equal to the value of its
default property when the engine calls its cast handler.  It would be
cool if we could use objects for string offsets and have their cast
handlers called (raising an appropriate warning if they have no cast
handler); I have no idea if this worked before or works after the
patch.

--Wez.

On Sun, 27 Jun 2004 07:24:35 +0100, Andi Gutmans <andi@zend.com> wrote: