PHPC-1057: Refactor option parsing for execute methods #707
Conversation
|
@derickr Note that Travis builds are failing because |
php_phongo.c
Outdated
| } | ||
|
|
||
| if (Z_TYPE_P(options) != IS_ARRAY) { | ||
| phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected options to be array, %s given", zend_get_type_by_const(Z_TYPE_P(options))); |
There was a problem hiding this comment.
Should use the new macro to show object name.
php_phongo.c
Outdated
| } | ||
| } | ||
| if (Z_TYPE_P(option) != IS_OBJECT || !instanceof_function(Z_OBJCE_P(option), php_phongo_readconcern_ce TSRMLS_CC)) { | ||
| phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"readConcern\" option to be %s, %s given", ZSTR_VAL(php_phongo_readconcern_ce->name), zend_get_type_by_const(Z_TYPE_P(option))); |
There was a problem hiding this comment.
ditto, and I suspect in other places too
| $bulk->insert(['wc' => 1]); | ||
| $manager->executeBulkWrite(NS, $bulk, ['readPreference' => 'foo']); | ||
| $bulk->insert(['x' => 1]); | ||
| $manager->executeBulkWrite(NS, $bulk, ['writeConcern' => new stdClass]); | ||
| }, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n"; |
There was a problem hiding this comment.
You should try to minimize changes to tests. There was no need to change the order, and to change "wc" to "x" for the writeConcern options for example.
There was a problem hiding this comment.
Noted re: "wc" to "x". The order changed as I basically wrote one test for all options and then copy/pasted it throughout all of the myriad of execute method tests, stripping out irrelevant options for each test based on what the method took. I made sure to leave string and object test cases for each invalid option, though, and used just a string for unknown options (as you originally had).
Looks like |
|
Rebased on master and #680 and using the new |
Execute options are now parsed via php_array functions, which makes the options case-sensitive. A helper function is now used to convert legacy options for the original three execute methods into an array. This simplifies option parsing in php_phongo.c. Additionally, Manager methods now perform their own server selection, which means that execute functions in php_phongo.c can rely on a server_id being provided. Tests have been updated to expect a serverId option when relevant (e.g. query opts). Additionally, error tests for execute methods have been improved so that invalid values for all known options are tested.
These tests were extracted from previous tests that combined invalid/unknown options.
Since Manager methods no longer pass -1 to execute functions in php_phongo.c, server_id should always be unsigned.
| PHONGO_OPTION_WRITE_CONCERN = 0x04, | ||
| PHONGO_COMMAND_RAW = 0x07, | ||
| PHONGO_COMMAND_READ = 0x03, | ||
| PHONGO_COMMAND_WRITE = 0x04, |
There was a problem hiding this comment.
Are you sure you can have duplicate enum values?
There was a problem hiding this comment.
Yessir. See this Stack Overflow thread.
https://jira.mongodb.org/browse/PHPC-1057
https://jira.mongodb.org/browse/PHPC-1066
This depends on the initial session implementation in #680. I've split up the PRs to make reviewing a bit easier, although I'll note that the option parsing refactoring in this PR was necessary to finish the session API (
phongo_execute_queryin particular).Since I ended up improving the error tests to cover all invalid, known options, I ended up moving (and enhancing) tests for unknown options into separate files. Those split tests for unknown options now have
XFAILblocks depending on PHPC-1066. There are no other changes related to PHPC-1066 in this PR, but I didn't want to lose track of any existing test cases we had.The other two commits for fixing titles in server tests and updating
server_idtouint32_tare just small cleanup tasks.