I'm wondering if we can improve on this by collecting all IDs, then deleting all files with the given filename, followed by removing all chunks for the IDs we previously found.

While there's always a chance to end up in an undesirable state, e.g. by having older revisions of files remaining, the suggestion above would reduce this by instructing the server to delete all file documents with a given name, ensuring that future requests for such files would not produce any result. Thus, if there was an error while deleting any chunks, we wouldn't end with older revisions left over.

I will implement a solution consistent with rename #1207 (comment)

This would definitely benefit from some bulk strategy to remove the fs.files documents first (likely using $in on multiple _id values), and then cleaning up the orphan chunks. I think it'd also benefit from being addressed in the GridFS spec, since we'd probably want to have a consistent way to address this in any driver that wishes to do so. And codifying that in a spec would ensure we're not overlooking some edge case.

I implemented the bulk delete

1. get all file ids by filename
2. delete all files by id
3. delete all chunks by file ids

The limitation is if there is more revisions that supported by a single command body.

This is zero in case of WriteConcern=0 or not file to update.

I implemented the bulk delete

1. get all file ids by filename
2. delete all files by id
3. delete all chunks by file ids

The limitation is if there is more revisions that supported by a single command body.

I realize this is an internal class, so we're not actually introducing a public API here, but a delete() method that affects multiple revisions seems odd, which is otherwise an abstraction for writing a single revision.

In the PR description, you said:

The context resolver mechanism introduced in #1138 doesn't give direct access to the CollectionWrapper which is required for this feature.

If we look at stream_open, it accesses a CollectionWrapper after resolving the path to an array of context options. Couldn't the same approach be used here to access a CollectionWrapper directly?

I felt deep down that using ReadableStream and WritableStream wasn't correct. I refactored by adding a private getContext method to use CollectionWrapper directly for rename and unlink.

Bucket::resolveStreamContext() executes findFileByFilenameAndRevision() for read modes in order to fetch the file document. Would it make sense to use 'w' here and avoid the unnecessary query?

I understand rename() needs to use 'r' since it does read the filename from the file document; however, there may be a refactoring opportunity there if you'd just like to trust the $fromPath string. I suppose it's a matter of intentionally NOP-ing on a FileNotFoundException or executing an update query that may not match anything -- so I'd be find leaving rename() as-is.

The idea of using r mode was to check that the file exists before doing operations, to throw a FileNotFoundException. This could be optimized by checking the number of updated documents after the update, but that's not feasible if Write Concern = 0 (edge case).

I reverted to throw a FileNotFoundException in rename and unlink, instead of silently returning false. This behavior is consistent with S3 stream wrapper.

@jmikola removing or renaming a single revision is not supported. Even if the option gridfs[revision] is set.

Edge case: if $fromPath and $toPath are identical, the file exists but you'll throw FileNotFoundException. That may warrant reverting to an "r" mode when selecting the context to throw eagerly.

Alternatively, you could have updateFilenameForFilename() return the matched count and assume that the update was successful or a NOP. The only normal scenario where the matched and modified counts wouldn't be equals is if the paths were the same, in which case you could probably NOP earlier and avoid calling updateFilenameForFilename() altogether (FWIW, PHP's rename() returns true in this case, but I dunno if there's some internal NOP behavior).

Good catch. I updated the code to return the matched count, and added a test for renaming with the same filename (existing and not existing).

Not sure if this is relevant, but the ternary here only checks for "r" and stream_open() may call this with "rb". Can you invoke functions here, or are you restricted on syntax here?

The logical or works. I also added the @return annotation so PHPStorm understands it.