ext/soap: Various refactorings #14579
Conversation
|
The signature change of __doRequest doesn't seem right due to the one_way behaviour. |
Girgias
force-pushed
the
soap-refactoring1
branch
from
3a73a31 to
badb561
Compare
Girgias
force-pushed
the
soap-refactoring1
branch
2 times, most recently
from
88331fe to
3b6d72e
Compare
ext/soap/soap.c
Outdated
| zval *headers = NULL; | ||
| zval *output_headers = NULL; | ||
|
|
||
| if (zend_parse_parameters(ZEND_NUM_ARGS(), "Sh|h!zz", &function, &args, &options, &headers, &output_headers) == FAILURE) { |
There was a problem hiding this comment.
I do not think the everything needs to use fast ZPP but hot paths like this might benefit it wdyt ? does not need to be in same PR though
There was a problem hiding this comment.
I think it makes sense, I need to double check if we have a fast ZPP check for object_of_class/array/null as it would improve the handlings of the headers arg.
|
Looks ok but definitively needs another pair of eyes, quite some changes. |
ext/soap/soap.c
Outdated
| if (UNEXPECTED(Z_TYPE_P(faultcode) == IS_NULL)) { | ||
| faultcode_val = zend_empty_string; | ||
| } else { | ||
| ZEND_ASSERT(Z_TYPE_P(faultcode) == IS_STRING); | ||
| faultcode_val = Z_STR_P(faultcode); | ||
| } |
There was a problem hiding this comment.
Nit: We could inverse the condition here for extra safety:
if (EXPECTED(Z_TYPE_P(faultcode) == IS_STRING)) {
faultcode_val = Z_STR_P(faultcode);
} else {
ZEND_ASSERT(Z_TYPE_P(faultcode) == IS_NULL);
faultcode_val = zend_empty_string;
}
There was a problem hiding this comment.
The refactoring looks ok to me.
The reason for the error handling change makes sense, but I would like to see other opinions on this.
This is a lot of changes, and the split in commits makes it easier to review, but I think that submitting two separate PRs would have been even better as the changes are not really related (one PR for pure refactoring, and another for the behavior-affecting changes).
…::__constructor() Use ValueErrors as the conditions checked are programming errors Also narrow down bailout mechanism use in it as only the call to get_sdl() requires us to use the bailout mechanism.
…::__constructor() Use ValueErrors as the conditions checked are programming errors Also narrow down bailout mechanism use in it as only the call to get_sdl() requires us to use the bailout mechanism.
Girgias
force-pushed
the
soap-refactoring1
branch
from
3b6d72e to
eee6d3a
Compare
|
I have merged the refactoring manually out of the PR, the PR basically only has the bailout conversions and the refactoring of the I was planning on doing different PRs, but I was just working on all the things kinda at the same time as the codebase is somewhat convoluted so trying to figure it out took me on a journey. |
|
I think the only controversial change is the one in SoapServer, as this changes the behaviour from printing a SOAP Fault XML error to a PHP exception. The other cases either convert a fatal error to an exception, or convert one exception to another type. |
Right, the reason this is done is such that you can develop this in co-operation with a SoapClient, which expects XML responses. It's probably better to keep the old behaviour. The SOAP extension is quite rigid to change I have noticed, because the APIs haven't changed in a long long time. EDIT: perhaps if it's consistently done for all of the SOAP code then it may be fine. I'm not sure. |
Cleaning up the extension code and attempting to slightly modernize it.
This PR should be reviewed commit by commit, as they are all independent of each other.
Part of the rationale is to move away from the bailout mechanism and use standard exceptions/errors.
I think the conversions done here are non-problematic, as they indicate programming errors while attempting to create a server or a client, which IMHO should not result in an XML SOAP Fault being returned.