Introduce get_properties_for #3579
Conversation
This handler allows getting the object properties for a particular purpose, such as array casting, serialization, etc.
This resolves the long-standing issue where var_dump a DateTime (etc) object makes a number of additional properties accessible, which may also change other behaviors as a side-effect.
Instead overload get_properties_for for a few specific cases such as array casts. This resolves the issue where ArrayObject get_properties may violate engine invariants in some cases.
nikic
force-pushed
the
get_properties_for
branch
from
41658fb to
ee0dcce
Compare
There was a problem hiding this comment.
Looks fine to me, except I wouldn't try to keep compatibility (object handlers API is going to be broken by typed properties anyway), and would add argument to existing get_properties() handler.
I also plan some optimizations that are going to break object handlers API.
| @@ -670,9 +670,6 @@ static zend_always_inline uint32_t zval_gc_info(uint32_t gc_type_info) { | |||
| #define Z_OBJPROP(zval) Z_OBJ_HT((zval))->get_properties(&(zval)) | |||
| #define Z_OBJPROP_P(zval_p) Z_OBJPROP(*(zval_p)) | |||
There was a problem hiding this comment.
I would add additional "for" argument to existing get_properties() handler, and change this into Z_OBJ_HT((zval))->get_properties(&(zval), ZEND_PROP_PURPOSE_DEFAULT)
There was a problem hiding this comment.
Actually, we might need additional ZEND_PROP_PURPOSE_FOREACH_READ and ZEND_PROP_PURPOSE_FOREACH_WRITE to avoid useless duplications in SPL ArrayObject.
|
@dstogov The reason why I did not merge get_properties and get_properties_for is that the memory management works differently for them. get_properties simply returns the HT, while the result of get_properties_for must be released. If we wanted to combine them, we'd have to (among other things) drop the HASH_OF API, as it's not compatible with get_properties_for. Or alternatively we'd have to use different memory management for different purposes, some requiring freeing and some not. That seems somewhat confusing. |
|
@nikic Do you see any other problems with this? |
|
@nikic It seems, something was broken by this. :( /home/dmitry/www/bench/ZendFramework-1.12.3-minimal/library/Zend/Application/Bootstrap/Bootstrap.php(89) : Fatal error - Uncaught Error: Call to a member function getDefaultModule() on null in /home/dmitry/www/bench/ZendFramework-1.12.3-minimal/library/Zend/Application/Bootstrap/Bootstrap.php:89 I'll try to dig deeper. |
|
@dstogov It uses |
|
@nikic can you fix this? Breaking ZF is not a good idea. |
This is a different attempt at #3574 /cc @dstogov.
The level of BC break for ArrayObject here is pretty much the same, but I think this approach is cleaner and also covers some other cases (e.g. DateTime).
The reason why it does not provide full BC either is that the
get_properties_forreturn value has to be released (it may be temporary), while theget_propertiesreturn value does not. As such I can't just use it as a drop in forHASH_OFetc. I could go through all the code usingHASH_OFandARRAY_OR_OBJECTand similar, but I'm rather skeptical that it is worthwhile.