Custom _id for GridFS documents (#2487) #2489
Conversation
|
It might make sense to change the property name from |
|
|
Hi @andrey-tech, thanks for the PR! since this is a feature, can you please target to |
|
@franmomu Done. |
| @@ -108,6 +110,14 @@ private function prepareOptions(?UploadOptions $uploadOptions = null): array | |||
| $chunkSizeBytes = $uploadOptions->chunkSizeBytes ?: $this->class->getChunkSizeBytes(); | |||
| $options = []; | |||
|
|
|||
| if ($uploadOptions->id !== null) { | |||
| $options['_id'] = $this->class->getDatabaseIdentifierValue($uploadOptions->id); | |||
There was a problem hiding this comment.
Would be great to have a test that covers type transformation
There was a problem hiding this comment.
MongoDB documentation lists ObjectId as just one of the common variants for the _id field. So I removed type transformation from method prepareOptions() of class DefaultGridFSRepository for more flexibility. Test that covers this type transformation not needed anymore.
| public $id; | ||
|
|
||
| /** @var bool|null */ | ||
| public $disableMD5; |
There was a problem hiding this comment.
Do you have a specific use-case for the disableMD5 option? The reason I omitted this was that the specification marks the option as deprecated and it will be removed from the PHP driver in the next major release. I would prefer not adding it in now, as we'd have to do another BC break for 3.x then (as opposed to breaking it once for people migrating to ODM 2.0).
There was a problem hiding this comment.
This one's on me (#2487 (comment)) I didn't know it's deprecated :)
There was a problem hiding this comment.
Hehe, yes. We deprecated it since using MD5 is forbidden when being compliant with FIPS, and personally I wouldn't rely on MD5 for checksums anymore. @andrey-tech do you mind dropping it? I don't think we should introduce the option at this point, especially since we haven't had anyone ask for it.
There was a problem hiding this comment.
@alcaeus I don't mind.
There was a problem hiding this comment.
@malarzm Do you mind dropping it?
There was a problem hiding this comment.
@andrey-tech since it's deprecated and it was not added on purpose, we should not add it
There was a problem hiding this comment.
The deprecated option disableMD5 has been removed from this PR.
composer.json
Outdated
| @@ -32,7 +32,7 @@ | |||
| "doctrine/persistence": "^2.4 || ^3.0", | |||
| "friendsofphp/proxy-manager-lts": "^1.0", | |||
| "jean85/pretty-package-versions": "^1.3.0 || ^2.0.1", | |||
| "mongodb/mongodb": "^1.2.0", | |||
| "mongodb/mongodb": "^1.4.0", | |||
There was a problem hiding this comment.
Might as well bump this to 1.15 while you're at it. 1.4 is ancient and no longer supported, we should encourage people to upgrade :)
There was a problem hiding this comment.
I don't mind :)
There was a problem hiding this comment.
@malarzm Do you mind to set ^1.15.0?
There was a problem hiding this comment.
I set mongodb/mongodb to ^1.15.0.
|
@alcaeus I think many people are still stuck on older versions and I wouldn't force them to update to v1.15 yet, especially that it requires updating the PHP extension via https://packagist.org/packages/mongodb/mongodb/stats#major/1 |
|
Bumping to |
| @@ -37,7 +37,7 @@ jobs: | |||
| - dependencies: "lowest" | |||
| php-version: "7.2" | |||
| mongodb-version: "3.6" | |||
| driver-version: "1.5.0" | |||
| driver-version: "1.10.0" | |||
There was a problem hiding this comment.
mongodb/mongodb 1.10 requires ext-mongodb 1.11 - we've corrected this with 1.15 to restore version number parity.
There was a problem hiding this comment.
Noted, thanks! Some builds are canceled because of ubuntu-18.04 being deprecated but 20.04 doesn't play well with the mongodb-labs/drivers-evergreen-tools action. Let's look into that in a separate PR.
IonBazan
force-pushed
the
issue-2487
branch
2 times, most recently
from
9a25306 to
ee120a2
Compare
|
Thanks @andrey-tech! |
Summary
Added two missing options to
UploadOptions:id(mixed): File document identifier. Defaults to a new ObjectId.-disableMD5(boolean): When true, no MD5 sum will be generated for the stored file. Defaults tofalse.Themongodb/mongodb^1.4.0 is required for optiondisableMD5.