ext/soap: Refactor userland function calling code - #19055
Conversation
| zend_function *header_fn = zend_hash_find_ptr_lc(function_table, Z_STR(h->function_name)); | ||
| if (UNEXPECTED(header_fn == NULL)) { | ||
| if (soap_obj_ce && soap_obj_ce->__call) { | ||
| header_fn = zend_get_call_trampoline_func(soap_obj_ce, Z_STR(function_name), false); |
There was a problem hiding this comment.
When using zend_get_call_trampoline_func you should also release the trampoline function when you're done with it. This usually doesn't cause problems because as long as you're outside of a trampoline call, this function will store the trampoline in EG(trampoline). It should be possible to test this by making a soap call from within a trampoline.
There was a problem hiding this comment.
I must be honest that I'm not exactly certain how to write a test for SOAP, do I need to "call" the trampoline from the XML?
There was a problem hiding this comment.
You'll instantiate a SoapServer class and use setObject with a class that has a __call implementation.
The SoapServer::handle() function should be called from within another class's __call implementation.
That way, there are 2 trampolines active, and so the one invoked in this C code will use a heap-allocated function structure.
There was a problem hiding this comment.
That said, I'm starting to wonder whether the manual trampoline handling will make the code more complex than it already was
There was a problem hiding this comment.
I'm working on something more long term to see if I can get rid of the function_name zval of the FCI so manual trampoline handling is something that is a bit annoying, I agree.
Not sure if there should be a new API with a "backing" zval.
f55a976 to
66d6c68
Compare
| if (service->soap_functions.ft == NULL) { | ||
| service->soap_functions.functions_all = false; | ||
| service->soap_functions.ft = zend_new_array(0); | ||
| ALLOC_HASHTABLE(service->soap_functions.ft); | ||
| /* This hashtable contains zend_function pointers so doesn't need a destructor */ | ||
| zend_hash_init(service->soap_functions.ft, 0, NULL, NULL, false); | ||
| } |
There was a problem hiding this comment.
This snippet is repeated 2 times, and the allocation with the comment 3 times. I wonder if this can be split off to a helper function (e.g. "soap_ensure_functions_table" or something alike). Could be left for a follow-up
| return; | ||
|
|
||
| if (soap_obj) { | ||
| /* This is because the object might define a __call() magic method */ |
There was a problem hiding this comment.
I think you need a better explanation than "This is", what is "this" ;)
| if (soap_obj) { | ||
| /* This is because the object might define a __call() magic method */ | ||
| zend_result method_call_result = zend_call_method_if_exists(Z_OBJ_P(soap_obj), Z_STR(h->function_name), &h->retval, h->num_params, h->parameters); | ||
| if (UNEXPECTED(method_call_result == FAILURE)) { |
There was a problem hiding this comment.
I wonder if you can factor out the "mustunderstand" stuff
66d6c68 to
3a3ce33
Compare
|
definitely not ready to merge, couple of remarks. |
3a3ce33 to
4f51fe0
Compare
| soap_obj = NULL; | ||
| } | ||
| } | ||
| soap_free_server_object(service, soap_obj); |
There was a problem hiding this comment.
I'm a bit confused here why we don't discard the php output?
| /* This is because the object might define a __call() magic method */ | ||
| zend_result method_call_result = zend_call_method_if_exists(Z_OBJ_P(soap_obj), Z_STR(function_name), &retval, num_params, params); | ||
| if (UNEXPECTED(method_call_result == FAILURE)) { | ||
| zend_throw_error(NULL, "Call to undefined method %s::%s()", ZSTR_VAL(Z_OBJCE_P(soap_obj)->name), Z_STRVAL(function_name)); |
There was a problem hiding this comment.
we kind of lose precision, whether it really does not exist or the method is private. I m unsure but maybe zend_is_callable* might fit better wdyt ?
There was a problem hiding this comment.
I think I'd rather have a new zend_call_method_if_exists_ex() API which has an char **error which handles the error message.
I'll do this in a different PR and then rebase. :)
There was a problem hiding this comment.
@devnexen Let me know if you prefer this approach with the new API.
4f51fe0 to
e279103
Compare
3 participants
Commits should be reviewed in order.
This gets rid of some more
call_user_functionAPI calls and reduces the amount of string copying.