-
Notifications
You must be signed in to change notification settings - Fork 266
PHPLIB-1206 Allow global registration of GridFS buckets with gridfs:// protocol
#1138
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
16f9cc2 to
e982b85
Compare
A couple of high level questions about the API. Let's take this code from the PR description:
$bucket = $client->selectDatabase('test_db')->selectGridFSBucket(); $bucket->registerAsDefaultStreamWrapper(); file_put_contents("gridfs://test_db/fs.files/filename.txt", 'Hello GridFS!');
If we're already registering a bucket as the default stream wrapper, why does the URI still contain the database and bucket names? The bucket instance is tied to a client, database, and bucket, so I'd expect to use something like this:
file_put_contents('gridfs://filename.txt', 'Hello GridFS!');
This might create conflicts with us using GridFS internally. However, we could work around that by allowing to registerer a new stream wrapper for each bucket:
$client->selectDatabase('test_db')->selectGridFSBucket()->registerStreamWrapper('foo'); $client->selectDatabase('test_db')->selectGridFSBucket(['name' => 'cache'])->registerStreamWrapper('cache'); file_put_contents('foo://filename.txt', 'Hello GridFS!'); // Writes to the fs.files collection file_put_contents('cache://filename.txt', 'Hello GridFS!'); // Writes to the cache.files collection
Not only would this make accessing files easier, but it would also allow for using more than a single bucket if users so desire.
I'm thinking to an other path pattern: gridfs://<bucket-alias>/<filename>
<bucket-alias>: name associated with a closure that get the context from thestream_opendata ($path,$mode,$options).<filename>: the path associated to the file
The context resolver will be a map instead of a single closure.
We can reuse the same StreamWrapper class and the same protocol gridfs.
04a1cc8 to
6ebcc42
Compare
gridfs:// protocol (追記ここまで)
ed857c2 to
cf4c665
Compare
8891ad0 to
41c915b
Compare
docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt
Outdated
Show resolved
Hide resolved
docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt
Outdated
Show resolved
Hide resolved
docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt
Outdated
Show resolved
Hide resolved
docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt
Outdated
Show resolved
Hide resolved
docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt
Outdated
Show resolved
Hide resolved
94e3848 to
002e0d4
Compare
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.
Two outstanding comments about url_stat(), which GitHub thinks are outdated.
docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt
Outdated
Show resolved
Hide resolved
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.
I'll defer to you and Andreas to decide if disableMD5 should be changed.
docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt
Outdated
Show resolved
Hide resolved
docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt
Outdated
Show resolved
Hide resolved
This CI failure looks relevant:
1) MongoDB\Tests\GridFS\StreamWrapperFunctionalTest::testOpenSpecificRevision
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'version 1'
+'version 0'
/home/runner/work/mongo-php-library/mongo-php-library/tests/GridFS/StreamWrapperFunctionalTest.php:300
I noticed some other CI failures related to Atlas Search spec tests failing to match error messages, but that's probably related to DRIVERS-2794 as it only fails on mongodb-latest.
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.
@jmikola Added this 1ms delay to fix #1138 (comment)
c408130 to
8470f88
Compare
* master: PHPLIB-1323 Implement `unlink` for GridFS stream wrapper (mongodb#1206) PHPLIB-1330: Sync tests for failCommand errorLabels reqs (mongodb#1214) PHPLIB-1246: Test PHP 8.3 on Evergreen (mongodb#1213) PHPLIB-1324 Implement `rename` for GridFS stream wrapper (mongodb#1207) PHPLIB-1248 Add examples on GridFS (mongodb#1196) Deprecate setting GridFS disableMD5 to false explicitly (mongodb#1205) PHPLIB-1326: Use more permissive top-level runOnRequirements (mongodb#1210) PHPLIB-1206 Add bucket alises for context resolver using GridFS StreamWrapper (mongodb#1138) Bump actions/upload-artifact from 3 to 4 (mongodb#1208) PHPLIB-1275: Replace apiargs usage in docs with extracts (mongodb#1203) Fix title formatting in Client::removeSubscriber() docs (mongodb#1204) PHPLIB-1304: Pull mongohouse image from ECR repo (mongodb#1202) Fix evergreen failures (mongodb#1200) Enable workflows to run for GitHub Merge Queue (mongodb#1199) PHPLIB-1313 Ensure the GridFS stream is saved when the script ends (mongodb#1197) PHPLIB-1309 Add addSubscriber/removeSubscriber to Client class to ease configuration (mongodb#1195) Master is now 1.18-dev
Uh oh!
There was an error while loading. Please reload this page.
Fix PHPLIB-1206
Fix #1137
Allows to register an alias for the stream wrapper. File urls looks like:
Where
<bucket-alias>is an alias for the bucketTodo:
Bucket::registerGlobalStreamWrapperAlias