Skip to content

PHPC-1144: Use server-side error code for BulkWriteException #785

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 22, 2018

Conversation

kvwalker
Copy link
Contributor

@kvwalker kvwalker requested review from jmikola and derickr March 22, 2018 18:32
php_phongo.c Outdated
@@ -671,7 +671,7 @@ bool phongo_execute_bulk_write(mongoc_client_t* client, const char* namespace, p
if (!success) {
if ((error.domain == MONGOC_ERROR_COMMAND && error.code != MONGOC_ERROR_COMMAND_INVALID_ARG) ||
error.domain == MONGOC_ERROR_SERVER || error.domain == MONGOC_ERROR_WRITE_CONCERN) {
phongo_throw_exception(PHONGO_ERROR_WRITE_FAILED TSRMLS_CC, "%s", error.message);
zend_throw_exception(php_phongo_bulkwriteexception_ce, error.message, error.code TSRMLS_CC);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent of this, I think (error.domain == MONGOC_ERROR_COMMAND && error.code != MONGOC_ERROR_COMMAND_INVALID_ARG) in the above condition is probably redundant now that we're using libmongoc error API v2. I think I missed this in 1ad6d75#diff-c06c6e1c9374aecabcf544157f9d0c26L705.

If we leave that in place, I suppose there's a small chance we might use a BulkWriteException for a MONGOC_ERROR_PROTOCOL_BAD_WIRE_VERSION code under the MONGOC_ERROR_COMMAND domain.

Could you address it in a second commit associated with PHPC-1090?

@kvwalker kvwalker merged commit 110d13e into mongodb:master Mar 22, 2018
kvwalker added a commit that referenced this pull request Mar 22, 2018
@kvwalker kvwalker deleted the PHPC-1144 branch March 22, 2018 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants