Call cast_object handler from get_properties_for

[iluuu1994]

Fixes GH-11547

[dstogov]

Looks fine.

[ju1ius]

Hi @iluuu1994,

I just tested this change and, while I understand why you chose this implementation, I'm not sure it's actually an improvement...

Before this change, while having to override get_properties_for for array casts was counter-intuitive and forced you to override another handler, at least the array cast operation had one and only one location.

After this change:

• If I don't need to override get_properties_for for anything else than array casts, then good, I now only need to override cast_object.
• But if I also to override get_properties_for for some reason, I must now remember manually to delegate the ZEND_PROP_PURPOSE_ARRAY_CAST operation to my custom cast_object handler...
• And If I need to override get_properties_for but not cast_object, I still have to take care to manually delegate the ZEND_PROP_PURPOSE_ARRAY_CAST operation to zend_std_get_properties_for...

So I feel the situation might now be even more confusing for extension authors, especially given none of this is actually documented.

A clean way to do this could be to just remove the ZEND_PROP_PURPOSE_ARRAY_CAST constant and make the ZEND_CAST vm handler always call cast_object, but this might have implications that I'm not aware of...

What do you think?

[ju1ius]

Coming back at it, in fact this PR doesn't really fix #11547.

This check in the VM handler skips zend_get_properties_for entirely. So basically if your object has no properties hashtable and does not override get_properties_for nor get_properties, the cast_object handler is never called with IS_ARRAY.

In other words, in order for cast_object to be called for array casts, you have to either:

• allow dynamic properties
• override get_properties_for
• override get_properties

Which confirms that this issue cannot be fixed without some changes in the VM.

In the light of this (IMO):

1. this PR should be reverted, and
2. cast_object handler is never called for array cast #11547 should either:
   • be reopened if properly fixing it is deemed desirable
   • be closed as wontfix otherwise

[iluuu1994]

@ju1ius Hmm, you're right. I missed that this code is duplicated in the VM. I would rather not add another check there, because it will unnecessarily slow it down. Thus I think I will revert the change. We may improve the internals documentation instead.

[iluuu1994]

Reverted the commit here: efc73f2

[ju1ius]

I would rather not add another check there, because it will unnecessarily slow it down.

Technically, I don't think another check is needed. The check just has to happen in the VM handler instead (I'm not sure a simple pointer comparison would significantly slow things down...). But this would most probably be a BC break and a bigger changeset, so yeah...

We may improve the internals documentation instead.

Oh man, is it needed 😄 !

Thus I think I will revert the change.

Thanks for looking into this btw, and sorry for not being able to test this sooner.

[iluuu1994]

@ju1ius Yeah, what I meant is an additional check in the VM. The VM is generally more performance critical than other code since it gets called a lot. So we'd have to be checking for both special get_properties_for, get_properties and cast_object to skip the specialized zend_std_build_object_properties_array in the VM. It would likely not be by a lot, but given that we can already achieve this in extensions in a admittedly somewhat confusing way, it's likely not worth the change.