PHPC-1195: BulkWrite::update() should append arrayFilters as array #842
Conversation
jmikola
changed the title
src/MongoDB/BulkWrite.c
Outdated
| @@ -73,6 +73,67 @@ static inline bool php_phongo_bulkwrite_update_has_operators(bson_t* bupdate) /* | |||
| return false; | |||
| } /* }}} */ | |||
|
|
|||
| static inline bool php_phongo_bulkwrite_bson_array_has_valid_keys(bson_t* array) /* {{{ */ | |||
There was a problem hiding this comment.
Can you explain with a comment what a "valid key" is?
These tests were only using Mongo Orchestration to ensure a 3.6 server. skip_if_server_version() can be used instead.
jmikola
changed the title
src/MongoDB/BulkWrite.c
Outdated
| } | ||
|
|
||
| if (!php_phongo_bulkwrite_bson_array_has_valid_keys(&b)) { | ||
| phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "\"%s\" option has invalid keys for a BSON array", zarr_key, zend_get_type_by_const(Z_TYPE_P(value))); |
There was a problem hiding this comment.
This has an extra zend_get_type_by_const — Travis says so too: https://travis-ci.org/mongodb/mongo-php-driver/jobs/383741731#L1198
| @@ -73,6 +73,70 @@ static inline bool php_phongo_bulkwrite_update_has_operators(bson_t* bupdate) /* | |||
| return false; | |||
| } /* }}} */ | |||
|
|
|||
| /* Returns whether the BSON array's keys are a sequence of integer strings | |||
| * starting with "0". BSON_APPEND_ARRAY considers it the caller's responsibility | |||
| * to ensure that the array's keys are properly formatted. */ | |||
There was a problem hiding this comment.
And this is an equivalent to bson-encode.c's php_phongo_is_array_or_document? Should they be placed together perhaps? I don't mind, so please do what makes sense for you.
There was a problem hiding this comment.
php_phongo_is_array_or_document() operates on a zval, while this validates a bson_t.
We can't work with the zval here, as we're not inspecting it until it's already been converted to a bson_t. In theory, this allows a Serializable value to produce a BSON array for this option.
| <?php START('THROWAWAY', ["version" => "36-release"]); CLEANUP(THROWAWAY); ?> | ||
| <?php skip_if_not_live(); ?> | ||
| <?php skip_if_server_version('<', '3.6'); ?> | ||
| <?php skip_if_not_clean(); ?> |
There was a problem hiding this comment.
This is a bit misleading, innit? skip_if_not_clean will also clean first?
There was a problem hiding this comment.
This change was made during #787.
Technically, the function attempts to drop the test collection and skips the test if that fails for some reason. I'll admit it's unlike other skip_if functions because it attempts to change the state of the system; however, it can actually skip if something goes wrong. In the past, CLEAN() ignored any errors.
If you have some ideas for renaming or revising this, a new ticket would be best 👍
https://jira.mongodb.org/browse/PHPC-1195
Note: this is waiting on CDRIVER-2661. Once that is merged, this PR can be updated to bump the libmongoc submodule to 1.10-dev (assuming the issue is also backported to the
r1.10branch).