Skip to content

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Apr 21, 2020

We add a get_properties handler which complements the already
existing has_property and read_propertyhandlers.

We add a `get_properties` handler which complements the already
existing `has_property` and `read_property`handlers.
@cmb69
Copy link
Member Author

cmb69 commented Apr 21, 2020

cc @beberlei

@beberlei
Copy link
Contributor

@cmb69 awesome! i didn't know about this, weird that this hasn't come up earlier with reflection being used for IDE /static analysis stub generation regularly.

@cmb69 cmb69 added the Bug label Apr 21, 2020
@kocsismate
Copy link
Member

kocsismate commented Apr 21, 2020

While we are at it, I was wonderingearlier if it would be possible (or worth) to also generate class, constant, and/or property registration code from our stubs?

UPDATE: these doesn't seem nicely doable :/ E.g. I now see that exposed constants are usually registered with a value of a built-in constant. Also, properties are given a handler whose name would be a bit weird to maintain in the stubs (?)

@cmb69
Copy link
Member Author

cmb69 commented Apr 21, 2020

For default properties which do not have special handling, it might be possible to generate zend_declare_property_*() calls. However, none of the DOM properties is a default property; they're all dynamic.

@cmb69
Copy link
Member Author

cmb69 commented Apr 27, 2020

Applied as 6bc8f7e. Thanks!

@cmb69 cmb69 closed this Apr 27, 2020
@cmb69 cmb69 deleted the cmb/79065 branch April 27, 2020 08:30
@nikic
Copy link
Member

nikic commented Apr 28, 2020

I've reverted this change, because it causes assertion failures in PhpUnit:

php: /home/nikic/php-7.4/Zend/zend_types.h:1039: zend_gc_delref: Assertion `p->refcount > 0' failed.
==25569== 
==25569== Process terminating with default action of signal 6 (SIGABRT)
==25569==    at 0xA21BE97: raise (raise.c:51)
==25569==    by 0xA21D800: abort (abort.c:79)
==25569==    by 0xA20D399: __assert_fail_base (assert.c:92)
==25569==    by 0xA20D411: __assert_fail (assert.c:101)
==25569==    by 0x93ED1E: zend_gc_delref (zend_types.h:1039)
==25569==    by 0x93EFC0: i_zval_ptr_dtor (zend_variables.h:43)
==25569==    by 0x93F1E2: zval_ptr_dtor (zend_variables.c:84)
==25569==    by 0x95869A: _zend_hash_add_or_update_i (zend_hash.c:757)
==25569==    by 0x958CE3: zend_hash_update (zend_hash.c:879)
==25569==    by 0x3F544D: dom_get_properties (php_dom.c:427)
==25569==    by 0x99526C: zend_std_get_gc (zend_object_handlers.c:123)
==25569==    by 0x97B8A5: gc_scan_black (zend_gc.c:708)

Generally, I would recommend against overloading get_properties, because it never ends well. We introduced get_properties_for in 7.4 to allow targeted overloading. Reflection support was omitted from that intentionally. (DOM properties are like __get, and get the same level of Reflection. This may change if we introduce first-class accessors.)

@cmb69
Copy link
Member Author

cmb69 commented Apr 28, 2020

Thanks @nikic, I was not really aware of that. I have re-opened https://bugs.php.net/79065, and changed it to documentation problem.

@sebastianbergmann
Copy link
Contributor

I've reverted this change, because it causes assertion failures in PhpUnit:

This type of assertion failures. After reading the reverting commit's message I thought PHPUnit's assertions were failing :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants