Ignore disableMD5 option as md5 field is removed from the spec
#1502
Conversation
| // GridFS deprecated fields are removed | ||
| 'gridfs/gridfs-upload-disableMD5: upload when length is 0 sans MD5' => 'Deprecated fields are removed', | ||
| 'gridfs/gridfs-upload-disableMD5: upload when length is 1 sans MD5' => 'Deprecated fields are removed', | ||
| 'gridfs/gridfs-upload: upload when contentType is provided' => 'Deprecated fields are removed', |
There was a problem hiding this comment.
Noted that this test was removed in mongodb/specifications@12be2df.
tests/UnifiedSpecTests/Operation.php
Outdated
| @@ -769,6 +769,10 @@ private function executeForSession(Session $session) | |||
| private function executeForBucket(Bucket $bucket) | |||
| { | |||
| $args = $this->prepareArguments(); | |||
|
|
|||
| // "md5" field is removed from the spec, option "disableMD5" is ignored | |||
| unset($args['disableMD5']); | |||
There was a problem hiding this comment.
Is this needed to allow running upload-disableMD5.yml, which still exists in the upstream repository?
Those tests all assert md5: { $$exists: false }, so I'm not sure what it would matter. Wouldn't PHPLIB 2.x just ignore the option (vs. a possible deprecation notice in 1.x)?
There was a problem hiding this comment.
v2.x ignore the option.
- If not set: md5 field is not set
- If set to false: md5 field is not set ...
In v1.x, we talked about deprecating not setting disableMD5: true, but that was rejected because of the burden of having to set an option for something that is not used.
There was a problem hiding this comment.
I likely misunderstood the point of this change. Is it just being done to satisfy the Util.php check for unsupported options? If so, it'd be helpful to make a note of that in this comment (since I've just demonstrated how likely I am to misunderstand the purpose of this unset() ^_^).
No objections to merging after that.
There was a problem hiding this comment.
After investigations, disableMD5 option is documented in the spec. So I added the option to the list of allowed options, with a comment.
There was a problem hiding this comment.
So I restored the option disableMD5 that I removed from the allowed options in #1398. Note that the spec document the option to create a bucket, not to call the upload method.
There was a problem hiding this comment.
If spec tests actually use a disableMD5 option for upload operations, I think that's an oversight in the spec itself. The option should probably be added to File Upload with the same note it has on the corresponding bucket option (that it's deprecated and transitional).
Defer to you if that's worth doing at this point.
GromNaN
force-pushed
the
ignore-disableMD5
branch
2 times, most recently
from
15fc90a to
962c806
Compare
GromNaN
force-pushed
the
ignore-disableMD5
branch
from
962c806 to
2cd6387
Compare
| @@ -499,7 +499,7 @@ private static function prepareCollectionOrDatabaseOptions(array $options): arra | |||
|
|
|||
| private static function prepareBucketOptions(array $options): array | |||
| { | |||
| Util::assertHasOnlyKeys($options, ['bucketName', 'chunkSizeBytes', 'readConcern', 'readPreference', 'writeConcern']); | |||
| Util::assertHasOnlyKeys($options, ['bucketName', 'chunkSizeBytes', 'disableMD5', 'readConcern', 'readPreference', 'writeConcern']); | |||
There was a problem hiding this comment.
Per my earlier comment about no tests using bucketOptions, I doubt this change is required. But if you'd like to keep it, I think it warrants the same comment I suggested below in Util.php so any future reader understands why this option is still here.
There was a problem hiding this comment.
You are correct, this option is not necessary for the existing spec tests. But it should be allowed according to the spec.
| Util::assertHasOnlyKeys($options, ['bucketName', 'chunkSizeBytes', 'disableMD5', 'readConcern', 'readPreference', 'writeConcern']); | |
| Util::assertHasOnlyKeys($options, ['bucketName', 'chunkSizeBytes', 'readConcern', 'readPreference', 'writeConcern']); |
Co-authored-by: Jeremy Mikola <[email protected]>
The
disableMD5option is ignored. The behavior is to never write themd5field, which is supported by upload tests.Related to
md5,contentType,aliasesspecifications#1683 Removed test oncontentTypeoptionmd5field from the library