cast_object handler is never called for array cast

[ju1ius]

Description

The following code:

static zend_class_entry *foo_ce;
static zend_object_handlers foo_handlers;

PHP_MINIT_FUNCTION(my_extension) {
  // Class declaration boilerplate omitted
  foo_ce = register_class_Foo();
  // object handlers initialization boilerplate omitted
  foo_handlers.cast_object = foo_cast_object;
  foo_ce.default_object_handlers = &foo_handlers;
}

static zend_result foo_cast_object(zend_object *readobj, zval *writeobj, int type) {
  switch (type) {
    case IS_LONG:
    case _IS_NUMBER:
      ZVAL_LONG(writeobj, 42);
      return SUCCESS;
    case IS_ARRAY:
      ZVAL_LONG(writeobj, 42);
      convert_to_array(writeobj);
      return SUCCESS;
    default:
      return FAILURE;
  }
}

<?php
$foo = new Foo;
var_dump((int)$foo);
var_dump((array)$foo);

Resulted in this output:

int(42)
array(0) {
}

But I expected this output instead:

int(42)
array(1) {
  [0]=>
  int(42)
}

So it seems that the VM unconditionally uses the result of object->handlers->get_properties_for() without checking beforehand if object->handlers->cast_object() returns something.

Not exactly a bug per se, but rather yet another quirk whose fixing would improve the usability the extension API, IMO.

PHP Version

8.3-dev

Operating System

No response

[Girgias]

I agree, but I don't know if people do not rely on the fact that they can use (array) cast to get the properties of any object for serialisation. We now have get_mangled_object_vars() as of PHP 7.4.

What we might need to do is deprecate using an (array) to convert objects that do not have such a handler...

@Ocramius @nicolas-grekas as you are two people that I know who have (ab)used this feature, would changing the behaviour of this in 8.3 be an issue?

[nicolas-grekas]

What really matters is keeping the property of (array) to not call any userland code.

deprecate using an (array) to convert objects that do not have such a handler

Not sure what this means, but keeping the array-cast as is for userland is critically needed.

About extensions, it's quite common for the engine to allow them to do unexpected things that userland can't do. Not sure it's a good thing though.

[Girgias]

What really matters is keeping the property of (array) to not call any userland code.

Well, in theory an extension could decide to call a userland function during the cast, now do I expect this to happen, not really.

deprecate using an (array) to convert objects that do not have such a handler

Not sure what this means, but keeping the array-cast as is for userland is critically needed.

Can't all cases of this be converted to using the get_mangled_object_vars() function?

About extensions, it's quite common for the engine to allow them to do unexpected things that userland can't do. Not sure it's a good thing though.

Right, but arguably the current casting behaviour for object is somewhat dumb. And yes extensions can do whacky stuff which is why I'm asking what the impact of changing this specific behaviour would be :D

[nicolas-grekas]

Extensions would better provide an explicit method instead of breaking current expectations IMHO :)

[Ocramius]

Agreed: explicit API over magic hook into such a low-level language detail.

[ju1ius]

I agree, but I don't know if people do not rely on the fact that they can use (array) cast to get the properties of any object for serialisation.

Just to be clear, I wasn't asking to change the default userland-visible behaviour in any way. I'm just saying that as an extension author, it is counter-intuitive that an array cast has to be done in the get_properties_for handler rather than in cast_object.

I may be wrong, but I think the default behaviour can be preserved by doing some checks in the VM (and convert_to_array), i.e (pseudocode):

if (obj->handlers->cast_object) {
  if (obj->handlers->cast_object(obj, result, IS_ARRAY) == SUCCESS) return result;
  if (EG(exception)) return NULL;
}
return zend_get_properties_for(obj, ZEND_PROP_PURPOSE_ARRAY_CAST);

[iluuu1994]

iluuu1994#89

That seems fine to me. The Symfony/Wordpress benchmarks show no overhead for the additional check. This can break an existing assumption that cast_object won't be called for IS_ARRAY, where it may trigger an assertion, abort(), or whatever. So this is still breaking to some extent.