Add ZPP macros for class name or object parameters #5647
Conversation
kocsismate
force-pushed
the
zpp-str-or-object
branch
2 times, most recently
from
ac1b52d to
c198253
Compare
Zend/zend_builtin_functions.c
Outdated
| } | ||
| ZEND_PARSE_PARAMETERS_START(0, 1) | ||
| Z_PARAM_OPTIONAL | ||
| Z_PARAM_STR_OR_OBJ(str, object) |
There was a problem hiding this comment.
@nikic In the newest implementation, I do not use Z_PARAM_STR_OR_OBJ_OR_NULL() anymore. Hopefully we will be able to deprecate calling this function with no argument.
Zend/zend_builtin_functions.c
Outdated
| if (object) { | ||
| ce = object->ce; | ||
| } else if (str) { | ||
| ce = zend_lookup_class(str); |
There was a problem hiding this comment.
I'm a bit concerned about this. This means we now call zend_lookup_class(), which also includes an autoloader call, on things like numbers. This seems potentially problematic. Especially if we consider how this is going to extend other similar functions, like method_exists(). The latter could historically (prior to PHP 8) be called on non-strings/objects and just returned false in that case. And people did do that (based on Symfony), and these calls would now start invoking the autoloader.
As such, I think we really should be treating these "object or class name" arguments differently. Either by just handling the types strictly, as method_exists/property_exists now do, or by actually adding a special fastzpp case for it (we already have one for just class names). In that case we'd start throwing on unknown classes though, rather than just returning false.
There was a problem hiding this comment.
@nikic I implemented a class name or object ZPP macro. After pushing the change, I've found out that it currently has some issues, but before I start fixing them (e.g. it returns the class name rather than the ce), I'd like to be sure if it's the right way to go. Do you think this macro is a good idea? Or should we just use your other suggestion?
There was a problem hiding this comment.
@nikic I've started to look into the resource migration of openssl. As it turned out, a string|object-of-type ZPP macro will be needed there (e.g. most X.509 related functions would need it), so my question is if I could also add this macro besides the class name or object in this PR?
There was a problem hiding this comment.
Seems like a reasonable addition for that use-case. Let's make a separate PR though.
kocsismate
force-pushed
the
zpp-str-or-object
branch
from
654009c to
d895ebf
Compare
kocsismate
changed the title
Zend/zend_API.h
Outdated
| static zend_always_inline int zend_parse_arg_str_or_obj( | ||
| zval *arg, zend_string **dest_str, zend_object **dest_object, int allow_null | ||
| static zend_always_inline int zend_parse_arg_class_name_or_obj( | ||
| zval *arg, zend_string **dest_str, zend_object **dest_object, int num, int allow_null |
There was a problem hiding this comment.
We should make this class_or_obj instead with zend_class_entry ** destination.
There was a problem hiding this comment.
In particular because that will save a duplicate zend_lookup_class, which is not exactly cheap.
There was a problem hiding this comment.
Yes, I realized this issue when I applied the macro in get_parent_class(), but before fixing it, I wanted to be sure that using an usual ZPP-mechanism is ok in this case :)
There was a problem hiding this comment.
At least for get_parent_class() the behavior looks pretty sensible to me. We also already have Z_PARAM_CLASS, so it should fit in well with existing semantics.
Zend/zend_API.h
Outdated
| ) { | ||
| zend_class_entry *class_entry; | ||
|
|
||
| if (EXPECTED(Z_TYPE_P(arg) == IS_STRING) && (class_entry = zend_lookup_class(Z_STR_P(arg)))) { |
There was a problem hiding this comment.
The second condition should be inside the if, to avoid duplicate checks.
kocsismate
force-pushed
the
zpp-str-or-object
branch
from
d895ebf to
f373a84
Compare
kocsismate
force-pushed
the
zpp-str-or-object
branch
from
f373a84 to
99b1076
Compare
| } else if (allow_null && EXPECTED(Z_TYPE_P(arg) == IS_NULL)) { | ||
| *destination = NULL; | ||
| } else { | ||
| *destination = NULL; |
There was a problem hiding this comment.
This line can be dropped, we don't initialize in the failure case.
Additionally, add ?object ZPP macros for convenience.