PHPC-348: Check for errors after calling bson_to_zval() #1034
Conversation
|
@jmikola heads up, I've changed all instances to return early to avoid subsequent operations if we're going to throw an exception anyways. I figured this is most sensible considering we don't want multiple exceptions to be thrown as they would create confusion. As mentioned, there are a few more instances in |
src/MongoDB/Command.c
Outdated
| php_phongo_bson_to_zval(bson_get_data(intern->bson), intern->bson->len, &zv); | ||
| if (!php_phongo_bson_to_zval(bson_get_data(intern->bson), intern->bson->len, &zv)) { | ||
| zval_ptr_dtor(&zv); | ||
| return Z_ARRVAL(retval); |
There was a problem hiding this comment.
Thoughts on using goto done; and adding a done: label above the original return Z_ARRVAL(retval); line at the end of this function? It'd be closer to what we do in other places with failure/cleanup jumps.
I wouldn't suggest this for cases where we're simply returning (e.g. Javascript::getScope()), but I think helps avoid duplication when we're returning an expression, which could get out of sync. If so, other get_debug_info handlers could get the same treatment.
Happy to defer to you on this. You're also allowed to berate me for publicly advocating for a goto statement :)
There was a problem hiding this comment.
That said, use of goto as a replacement for finally and failure returns is established in the driver, so I don't mind adding those here as well 👍
https://jira.mongodb.org/browse/PHPC-348
Note: there are three occurrences of
php_phongo_bson_to_zval_exinCursor.c(1, 2, 3) where I wasn't sure if freeing the result was a good idea.When used in the
debug_infofunctions, I thought it was best to addnullvalues and continue assembling the output, but we may want to return without processing any further properties. I'm not quite sure what the preferred course of action is.