-
Notifications
You must be signed in to change notification settings - Fork 209
PHPC-1545: Require object or array type for "kmsProviders" option #1089
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but did you want to do this for schemaMap
and extraOptions
as well (per my last comment in PHPC-1545)? If so, you can repurpose the Manager test file as "invalid autoEncryption options" and handle all three in sequence.
That’s what I get for just skimming over comments. I’ll also apply those for other options in this PR 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM but I'd suggest renaming some of the tests for createClientEncryption()
(I may have missed the other cases in previous reviews).
@@ -2907,8 +2917,8 @@ static mongoc_client_encryption_opts_t* phongo_clientencryption_opts_from_zval(m | |||
zval* kms_providers = php_array_fetchc(options, "kmsProviders"); | |||
bson_t bson_providers = BSON_INITIALIZER; | |||
|
|||
if (Z_TYPE_P(kms_providers) != IS_ARRAY) { | |||
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"kmsProviders\" encryption option to be an array"); | |||
if (Z_TYPE_P(kms_providers) != IS_ARRAY && Z_TYPE_P(kms_providers) != IS_OBJECT) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I forgot we were already checking for an array type here. Good to know both instances of "kmsProviders" handling are consistent now!
@@ -0,0 +1,36 @@ | |||
--TEST-- | |||
MongoDB\Driver\ClientEncryption::__construct() with invalid option types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be a test for Manager::createClientEncryption()
since the constructor is disabled? I see this is clientEncryption-ctor-error-002.phpt
, so perhaps move all of those over to the Manager name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes. Those were left over from when I initially built this with a public constructor. I moved them to the manager folder and renamed them accordingly.
https://jira.mongodb.org/browse/PHPC-1545