Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
GromNaN merged 1 commit into mongodb:master from GromNaN:PHPLIB-1206
Dec 18, 2023

Conversation

@GromNaN
Copy link
Member

@GromNaN GromNaN commented Jul 21, 2023
edited
Loading

Fix PHPLIB-1206
Fix #1137

Allows to register an alias for the stream wrapper. File urls looks like:

gridfs://<bucket-alias>/<filename>

Where <bucket-alias> is an alias for the bucket

$client = new Client();
$bucket = $client->selectDatabase('test_db')->selectGridFSBucket();
$bucket->registerGlobalStreamWrapperAlias('mybucket');
file_put_contents("gridfs://mybucket/filename.txt", 'Hello GridFS!');
if (file_exists("gridfs://mybucket/filename.txt")) {
 echo file_get_contents("gridfs://mybucket/filename.txt") . PHP_EOL;
}

Todo:

  • Add reference doc for the method Bucket::registerGlobalStreamWrapperAlias

@GromNaN GromNaN requested a review from jmikola July 24, 2023 11:25
@GromNaN GromNaN marked this pull request as ready for review July 24, 2023 12:56
@GromNaN GromNaN marked this pull request as draft July 25, 2023 06:47
Copy link
Member

alcaeus commented Nov 27, 2023
edited
Loading

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.

Copy link
Member Author

GromNaN commented Nov 27, 2023

I'm thinking to an other path pattern: gridfs://<bucket-alias>/<filename>

  • <bucket-alias>: name associated with a closure that get the context from the stream_open data ($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.

@GromNaN GromNaN force-pushed the PHPLIB-1206 branch 2 times, most recently from 04a1cc8 to 6ebcc42 Compare November 28, 2023 21:50
@GromNaN GromNaN changed the title (削除) PHPLIB-1206 Add default bucket for GridFS StreamWrapper (削除ここまで) (追記) PHPLIB-1206 Allow global registration of GridFS buckets with gridfs:// protocol (追記ここまで) Nov 30, 2023
@GromNaN GromNaN changed the title (削除) PHPLIB-1206 Allow global registration of GridFS buckets with gridfs:// protocol (削除ここまで) (追記) PHPLIB-1206 Allow global registration of GridFS buckets with gridfs:// protocol (追記ここまで) Nov 30, 2023
@GromNaN GromNaN force-pushed the PHPLIB-1206 branch 2 times, most recently from ed857c2 to cf4c665 Compare December 6, 2023 13:55
@GromNaN GromNaN marked this pull request as ready for review December 6, 2023 15:07
@GromNaN GromNaN force-pushed the PHPLIB-1206 branch 5 times, most recently from 8891ad0 to 41c915b Compare December 13, 2023 14:32
@GromNaN GromNaN force-pushed the PHPLIB-1206 branch 2 times, most recently from 94e3848 to 002e0d4 Compare December 13, 2023 16:20
Copy link
Member

@jmikola jmikola left a 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.

Copy link
Member

@jmikola jmikola left a 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.

Copy link
Member

jmikola commented Dec 15, 2023

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.


// Insert 3 revisions, wait 1ms between each to ensure they have different uploadDate
file_put_contents($filename, 'version 0');
usleep(1000);
Copy link
Member Author

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)

jmikola reacted with thumbs up emoji
@GromNaN GromNaN merged commit b1147cd into mongodb:master Dec 18, 2023
@GromNaN GromNaN deleted the PHPLIB-1206 branch December 18, 2023 10:01
alcaeus added a commit to alcaeus/mongo-php-library that referenced this pull request Jan 15, 2024
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmikola jmikola jmikola approved these changes

@alcaeus alcaeus alcaeus approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

The StreamWrapper class is currently unusable without the bucket

AltStyle によって変換されたページ (->オリジナル) /