PHPORM-186 GridFS adapter for Filesystem #2985

GromNaN · 2024-05-30T16:55:11Z

Integration of Flysystem GridFS Adapter for Laravel File Storage.

The configuration is similar to the Symfony Flysystem Bundle as it provides various way to configure the bucket:

using the database connection (default connection by default)
setting a service name for the bucket instance
setting a factory function for the bucket instance

Unlike the Symfony Bundle, I do not provide instantiation of a new MongoDB\Client from the configuration as we always have a database connection with Eloquent.

Checklist

Add tests and ensure they pass
Add an entry to the CHANGELOG.md file
Update documentation for new features

docs/filesystems.txt

jmikola · 2024-05-31T17:17:18Z

docs/filesystems.txt

+           'bucket' => 'fs',
+           'prefix' => '',
+           'read-only' => false,
+           'throw' => false,


Is this the complete list of supported options, with default values repeated just so users know what they can set? Since you have a table below with all options, I'd consider reducing this code example to just the necessary options.

Why not, but personally I find the complete example more synthetic and quicker to read, or even copy and paste.
I understand that we want developers to use the default minimal config, so that's what I'm going to keep.

I find the complete example more synthetic and quicker to read

Same. If this example was annotated with comments to call out default values (and thus assist user's with reducing their own config) I think it'd stand well on its own and we wouldn't even need the table that follows.

Added optional settings in comments. Keeping the table for details.

docs/filesystems.txt

src/MongoDBServiceProvider.php

tests/FilesystemsTest.php

jmikola · 2024-05-31T17:35:37Z

tests/FilesystemsTest.php

+        ]);
+
+        $disk = Storage::disk('gridfs-prefix');
+        $disk->put('hello/world.txt', 'Hello World!');


I may be mistaken and/or misunderstanding something, but aren't all files supposed to be prefixed with a leading directory separator?

The directory separator is always added between the prefix and the file name by the PathPrefixer.

I recall that the PathPrefixer adds it, but I thought the docs might present the canonical form. I just noted the absence of a leading / is consistent with examples in the Laravel Filesystem docs.

When the prefix is empty, the file name in the fs.files collection does not have the leading /. The only situation you can get a leading / is by having it in the prefix.

composer.json

norareidy

I added some comments, mostly about wording and introducing the code examples in the text. Let me know if you have any questions!

docs/filesystems.txt

norareidy · 2024-05-31T18:57:03Z

docs/filesystems.txt

+Configuration
+-------------
+
+Before using the GridFS driver, you will need to install the Flysystem GridFS package via the Composer package manager:


S: we try to avoid the future tense and "via" in the docs. Also, I'd edit this sentence slightly so it introduces the code example:

Suggested change

Before using the GridFS driver, you will need to install the Flysystem GridFS package via the Composer package manager:

Before using the GridFS driver, install the Flysystem GridFS package through the Composer package manager by running the following command:

docs/filesystems.txt

jmikola · 2024-06-03T19:59:47Z

docs/filesystems.txt

+   $bucket = $app['db']->connection('mongodb')->getMongoDB()->selectGridFSBucket();
+
+   // Download the last but one version of a file
+   $bucket->openDownloadStreamByName('hello.txt', ['revision' => -2])


Since PathPrefixer doesn't apply here, would users need to write /hello.txt, or do the prefixes only kick in when a path is used? I expect this could be a pain point for users, that may warrant a note.

We don't use the leading / in the PHP GridFS documentation. I don't know what to write in a note.

There's no prefixing done in PHPLIB, so the filename is used exactly as provided.

My question here is: would hello.txt here refer to the same file written by Flysystem, or would you need to use /hello.txt? It's not clear to me if Flysystem adds a prefix to every file name or just those that appear to use paths (i.e. already contain a / somewhere in the middle of the string).

Perhaps this is answered by #2985 (comment). In that case, I suppose the note would be to remind users that prefixing is entirely handled by Flysystem. So if they're using a non-empty prefix option they'll need to ensure that filenames are prefixed manually when using PHPLIB directly.

Flysystem always adds the prefix to the file name stored in MongoDB.

norareidy

LGTM with some comments!

norareidy · 2024-06-04T13:50:06Z

docs/filesystems.txt

+       ],
+   ],
+
+You can configure the disk the following settings in ``config/filesystems.php``:


I: I think you can delete "the disk"

Suggested change

You can configure the disk the following settings in ``config/filesystems.php``:

You can configure the following settings in ``config/filesystems.php``:

norareidy · 2024-06-04T13:52:10Z

docs/filesystems.txt

+     - **Required**. Specifies the filesystem driver to use. Must be ``gridfs`` for MongoDB.
+
+   * - ``connection``
+     - The database connection used to store jobs. It must be a ``mongodb`` connection. The driver uses the default connection if a connection is not specified.


S: remove the article to match the other descriptions

Suggested change

- The database connection used to store jobs. It must be a ``mongodb`` connection. The driver uses the default connection if a connection is not specified.

- Database connection used to store jobs. It must be a ``mongodb`` connection. The driver uses the default connection if a connection is not specified.

norareidy · 2024-06-04T13:52:37Z

docs/filesystems.txt

+   * - ``prefix``
+     - Specifies a prefix for the name of the files that are stored in the bucket. Using a distinct bucket is recommended
+       in order to store the files in a different collection, instead of using a prefix.
+       The prefix should not start with a leading slash ``/``.


S: Add parentheses around the slash

Suggested change

The prefix should not start with a leading slash ``/``.

The prefix should not start with a leading slash (``/``).

norareidy · 2024-06-04T13:58:48Z

docs/filesystems.txt

+Usage
+-----
+
+A benefit of using Laravel Filesystem is that it provides a common interface


S: Reword so this sentence is a little less subjective

Suggested change

A benefit of using Laravel Filesystem is that it provides a common interface

Laravel Filesystem provides a common interface

norareidy · 2024-06-04T14:03:12Z

docs/filesystems.txt

+A benefit of using Laravel Filesystem is that it provides a common interface
+for all the supported file systems. You can use the ``gridfs`` disk in the same
+way as the ``local`` disk.
+


S: introduce the code

Suggested change

The following example writes a file to the ``gridfs`` disk, then reads the file:

norareidy · 2024-06-04T14:09:27Z

docs/filesystems.txt

+File names in GridFS are metadata in file documents, which are identified by
+unique ObjectIDs. If multiple documents share the same file name, they are


S: since this is the first time that file metadata documents are mentioned, I think it would be helpful to describe them

Suggested change

File names in GridFS are metadata in file documents, which are identified by

unique ObjectIDs. If multiple documents share the same file name, they are

GridFS creates file documents for each uploaded file. These documents contain metadata, including the file name and a

unique ObjectId. If multiple documents share the same file name, they are

norareidy · 2024-06-04T14:10:37Z

docs/filesystems.txt

+- Deleting a file deletes all the revisions of this file name
+
+The GridFS Adapter for Flysystem does not provide access to a specific revision
+of a filename. You must use the


S: I'd stay consistent with using either "filename" or "file name"

Suggested change

of a filename. You must use the

of a file name. You must use the

norareidy · 2024-06-04T14:18:00Z

docs/filesystems.txt

+   If you use a prefix the Filesystem component, you will have to handle it
+   by yourself when using the GridFS API directly.


S: clarify the first part of this sentence ("If you use a prefix the Filesystem component" part)

Q: what do you mean by "handle it by yourself" - could you clarify that in the note?

Suggested change

If you use a prefix the Filesystem component, you will have to handle it

by yourself when using the GridFS API directly.

If you specify the ``prefix`` GridFS setting, you will have to handle it

by yourself when using the GridFS API directly.

GromNaN · 2024-06-04T15:05:00Z

LGTM with some comments!

I applied your suggested changes in a new PR: #2993

GromNaN force-pushed the PHPORM-186 branch 2 times, most recently from aa72f30 to 9bb2081 Compare May 30, 2024 22:25

github-actions bot added the docs label May 30, 2024

GromNaN added enhancement and removed docs labels May 30, 2024

GromNaN requested a review from jmikola May 30, 2024 22:30

GromNaN marked this pull request as ready for review May 30, 2024 22:30

GromNaN requested review from a team as code owners May 30, 2024 22:30

GromNaN requested review from rustagir and norareidy May 30, 2024 22:30

GromNaN added the docs label May 30, 2024

GromNaN force-pushed the PHPORM-186 branch from 37653c0 to ef8c79e Compare May 30, 2024 22:50

GromNaN added this to the 4.5 milestone May 31, 2024

GromNaN changed the base branch from 4.4 to 4.5 May 31, 2024 07:27

GromNaN added 3 commits May 31, 2024 09:27

GridFS adapter for Filesystem

49dd91a

Add tests and remove Client instanciation from Configuration

59129f9

Add docs

04e9c86

GromNaN force-pushed the PHPORM-186 branch from ef8c79e to 82e80ea Compare May 31, 2024 07:28

Remove file

0780822

GromNaN force-pushed the PHPORM-186 branch from 82e80ea to 0780822 Compare May 31, 2024 07:29

jmikola reviewed May 31, 2024

View reviewed changes

norareidy requested changes May 31, 2024

View reviewed changes

GromNaN added 2 commits June 3, 2024 11:53

Docs review

9c7b6ba

Prevent cascading resolution of the bucket option

3815890

GromNaN requested review from norareidy and jmikola June 3, 2024 10:09

jmikola reviewed Jun 3, 2024

View reviewed changes

Docs review

e633d9a

GromNaN requested a review from jmikola June 4, 2024 07:54

Add note about prefix with gridfs api

e67b592

GromNaN force-pushed the PHPORM-186 branch from b381163 to e67b592 Compare June 4, 2024 13:31

jmikola approved these changes Jun 4, 2024

View reviewed changes

GromNaN enabled auto-merge (squash) June 4, 2024 13:32

norareidy approved these changes Jun 4, 2024

View reviewed changes

GromNaN merged commit 847e3c7 into mongodb:4.5 Jun 4, 2024
26 checks passed

GromNaN deleted the PHPORM-186 branch June 4, 2024 14:59

GromNaN mentioned this pull request Jun 4, 2024

PHPORM-186 Review of GridFS docs #2993

Merged

GromNaN mentioned this pull request Jun 4, 2024

DOCSP-39849: revise job batching docs #2994

Merged

3 tasks

GromNaN mentioned this pull request Jun 18, 2024

Add link to filesystems docs #3009

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHPORM-186 GridFS adapter for Filesystem #2985

PHPORM-186 GridFS adapter for Filesystem #2985

GromNaN commented May 30, 2024 •

edited

Loading

jmikola May 31, 2024

GromNaN Jun 3, 2024

jmikola Jun 3, 2024

GromNaN Jun 4, 2024

jmikola May 31, 2024

GromNaN May 31, 2024

jmikola Jun 3, 2024

GromNaN Jun 4, 2024

norareidy left a comment

norareidy May 31, 2024

jmikola Jun 3, 2024

GromNaN Jun 4, 2024

jmikola Jun 4, 2024

jmikola Jun 4, 2024 •

edited

Loading

GromNaN Jun 4, 2024

norareidy left a comment

norareidy Jun 4, 2024

norareidy Jun 4, 2024

norareidy Jun 4, 2024

norareidy Jun 4, 2024

norareidy Jun 4, 2024

norareidy Jun 4, 2024

norareidy Jun 4, 2024

norareidy Jun 4, 2024

GromNaN commented Jun 4, 2024

	Before using the GridFS driver, you will need to install the Flysystem GridFS package via the Composer package manager:
	Before using the GridFS driver, install the Flysystem GridFS package through the Composer package manager by running the following command:

	You can configure the disk the following settings in ``config/filesystems.php``:
	You can configure the following settings in ``config/filesystems.php``:

	- The database connection used to store jobs. It must be a ``mongodb`` connection. The driver uses the default connection if a connection is not specified.
	- Database connection used to store jobs. It must be a ``mongodb`` connection. The driver uses the default connection if a connection is not specified.

	The prefix should not start with a leading slash ``/``.
	The prefix should not start with a leading slash (``/``).

	A benefit of using Laravel Filesystem is that it provides a common interface
	Laravel Filesystem provides a common interface


	The following example writes a file to the ``gridfs`` disk, then reads the file:

		File names in GridFS are metadata in file documents, which are identified by
		unique ObjectIDs. If multiple documents share the same file name, they are

	of a filename. You must use the
	of a file name. You must use the

		If you use a prefix the Filesystem component, you will have to handle it
		by yourself when using the GridFS API directly.

PHPORM-186 GridFS adapter for Filesystem #2985

PHPORM-186 GridFS adapter for Filesystem #2985

Conversation

GromNaN commented May 30, 2024 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norareidy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norareidy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GromNaN commented Jun 4, 2024

GromNaN commented May 30, 2024 •

edited

Loading

jmikola Jun 4, 2024 •

edited

Loading