Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
PHPORM-186 GridFS adapter for Filesystem #2985
PHPORM-186 GridFS adapter for Filesystem #2985
Changes from 6 commits
49dd91a
59129f9
04e9c86
0780822
9c7b6ba
3815890
e633d9a
e67b592
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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: I think you can delete "the disk"
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.
S: remove the article to match the other descriptions
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.
S: Reword so this sentence is a little less subjective
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.
S: introduce the code
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.
S: I'd stay consistent with using either "filename" or "file name"
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.
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.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.
We don't use the leading
/
in the PHP GridFS documentation. I don't know what to write in a note.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.
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).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.
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.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.
Flysystem always adds the prefix to the file name stored in MongoDB.