[WAIT] PHPC-1076: Expose result document for failed commands via CommandFailedEvent #818
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
derickr
approved these changes
Apr 26, 2018
| @@ -152,6 +175,7 @@ static zend_function_entry php_phongo_commandfailedevent_me[] = { | |||
| PHP_ME(CommandFailedEvent, getError, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL) | |||
| PHP_ME(CommandFailedEvent, getDurationMicros, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL) | |||
| PHP_ME(CommandFailedEvent, getOperationId, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL) | |||
| PHP_ME(CommandFailedEvent, getReply, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL) | |||
There was a problem hiding this comment.
I see what you did here, indenting it nicely in between the one before and after — however, I don't think our intention was ever to stair-case-indent these in the first place, and it might just have slipped by our review radar when we introduced clang-format.
This can be addressed by using the following patch:
--- a/src/MongoDB/Monitoring/CommandFailedEvent.c
+++ b/src/MongoDB/Monitoring/CommandFailedEvent.c
@@ -147,15 +147,17 @@ ZEND_BEGIN_ARG_INFO_EX(ai_CommandFailedEvent_void, 0, 0, 0)
ZEND_END_ARG_INFO()
static zend_function_entry php_phongo_commandfailedevent_me[] = {
+ /* clang-format off */
ZEND_NAMED_ME(__construct, PHP_FN(MongoDB_disabled___construct), ai_CommandFailedEvent_void, ZEND_ACC_PRIVATE | ZEND_ACC_FINAL)
- PHP_ME(CommandFailedEvent, getCommandName, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
- PHP_ME(CommandFailedEvent, getError, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
- PHP_ME(CommandFailedEvent, getDurationMicros, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
- PHP_ME(CommandFailedEvent, getOperationId, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
- PHP_ME(CommandFailedEvent, getRequestId, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
- PHP_ME(CommandFailedEvent, getServer, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
- ZEND_NAMED_ME(__wakeup, PHP_FN(MongoDB_disabled___wakeup), ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
- PHP_FE_END
+ PHP_ME(CommandFailedEvent, getCommandName, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
+ PHP_ME(CommandFailedEvent, getError, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
+ PHP_ME(CommandFailedEvent, getDurationMicros, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
+ PHP_ME(CommandFailedEvent, getOperationId, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
+ PHP_ME(CommandFailedEvent, getRequestId, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
+ PHP_ME(CommandFailedEvent, getServer, ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
+ ZEND_NAMED_ME(__wakeup, PHP_FN(MongoDB_disabled___wakeup), ai_CommandFailedEvent_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL)
+ PHP_FE_END
+ /* clang-format on */
};
/* }}} */
I will make a patch to do that for all places — but that might mean a conflict here. For now, I think you can keep it and I'll deal with it in my formatting patch.
| @@ -2229,6 +2231,7 @@ static void php_phongo_command_failed(const mongoc_apm_command_failed_t* event) | |||
| object_init_ex(&p_event->z_error, phongo_exception_from_mongoc_domain(tmp_error.domain, tmp_error.code)); | |||
| zend_update_property_string(default_exception_ce, &p_event->z_error, ZEND_STRL("message"), tmp_error.message TSRMLS_CC); | |||
| zend_update_property_long(default_exception_ce, &p_event->z_error, ZEND_STRL("code"), tmp_error.code TSRMLS_CC); | |||
|
|
|||
There was a problem hiding this comment.
I don't think you should add this extra white space line here.
jmikola
reviewed
Apr 26, 2018
| @@ -2214,11 +2214,13 @@ static void php_phongo_command_failed(const mongoc_apm_command_failed_t* event) | |||
| p_event->operation_id = mongoc_apm_command_failed_get_operation_id(event); | |||
| p_event->request_id = mongoc_apm_command_failed_get_request_id(event); | |||
| p_event->duration_micros = mongoc_apm_command_failed_get_duration(event); | |||
| p_event->reply = bson_copy(mongoc_apm_command_failed_get_reply(event)); | |||
There was a problem hiding this comment.
The build currently fails because mongoc_apm_command_failed_get_reply() does not exist in the libmongoc submodule. You'll need to do the following to address this:
- Bump the libmongoc submodule to its master branch (or the commit where this function first shows up). The PHPC commit for this can use this issue number. For prior art, see: a49c71c#diff-25d902c24283ab8cfbac54dfa101ad31
- In
config.m4, you'll also need to bump lines that check the version of libmongoc when used as a system library. See:Line 289 in 94c7487
Note that libmongoc's master branch (1.10-dev at the moment) incorporates libbson, so I'm not suggesting you bump the libbson submodule since there is nothing to bump to. I assume libmongoc 1.10-dev will still play nice with libbson 1.9 (for now), but if only updating libmongoc causes issues, we can work through a solution in more detail.
kvwalker
changed the title
|
Superseded by #840. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://jira.mongodb.org/browse/PHPC-1076
These changes depend on libmongoc 1.10.0, but I was able to test them locally and verify that they work.