Interface for classes that handle file uploads #3514
Conversation
|
You are right: it should be #3476. Corrected. |
| */ | ||
|
|
||
| /** | ||
| * Handles file uploads from a client to a server. The methods listed in this |
There was a problem hiding this comment.
Minor grammatical nits/editing: The first sentence is missing a noun (should be "FilesUploadHandler handles files uploads [...]", and the second sentence has a lot of superfluous wording. It can just be something like "The interface encapsulates the usual things that happen when a set of files is uploaded to a server, namely [...]", though "the usual things" seems a little informal for a class doc, so maybe just: "The interface encapsulates the following behaviours that happen when a set of files is uploaded: [...]" would be even better.
But like I said, that's nitpicking. (Maybe @PapillonMcGill has thoughts on a good way to word it. Her reviews of the spec/README PRs generally had good technical writing suggestions.)
There was a problem hiding this comment.
I would go with @driusan suggestions: "The interface encapsulates the following behaviours that happen when a set of files is uploaded: [...]" for the second phrase.
Suggestion for the first phrase is also good or "The FilesUploadHandler interface handles ..." but my first choice would be "This interface handles ..."
| * | ||
| * 1. Validation of all uploaded files | ||
| * 2. Transfer of all files to a designated (permanent) location on the | ||
| * server. |
There was a problem hiding this comment.
Why do these need to be all files and not (potentially) some files?
There was a problem hiding this comment.
I would effectively remove the "all"
Also, if you put an ending . at the end of point 2, it would also require ending . at the end of other points
| * @return array the properties for each file uploaded (one array for | ||
| * each uploaded file, see $_FILES). | ||
| */ | ||
| public function getFiles() : array; |
There was a problem hiding this comment.
I made this comment on the other PR, but I think an iterable of PSR7 UploadedFileInterface's is better than an array of _FILES (most of the other comments on this review build on reasons why), but in addition to that, I'm not sure if this is even required, since once the transition to a PSR7/15 based router is finished, this will essentially just be a duplicate of ServerRequestInterface->getUploadedFiles() in a different format.
What I think the interface itself needs is a way to inject which files it's handling (ie either an immutable withFiles or a mutable setFiles, my preference would be the former), rather than making assumptions about the execution environment.
| * @return bool true if the set of files is valid and should be processed, false | ||
| * otherwise. | ||
| */ | ||
| public function isValid() : bool; |
There was a problem hiding this comment.
I think the function should take what it's validating as a parameter rather than depending on implicit state. I wanted to suggest isFileValid(UploadedFileInterface) : bool, but potentially there might be interactions between files affecting validity, so maybe isFileSetValid(UploadedFileInterface[]) : bool (or whatever the type ends up being based on above..)
| * | ||
| * @return array list of error messages for each uploaded file. | ||
| */ | ||
| public function getErrors() : array; |
There was a problem hiding this comment.
What happens if process or validate is called multiple times? Does one error list get lost? And how do you know if it's a processing or validation error? At the very least, this should be getLastErrors(), but to make things more atomic maybe the validation should return an error list rather than a bool and the process should return a (potentially different) error list rather than null.
Also, this should specify if it's talking about a numeric or associative array of error messages (and if the latter, what the key is)
|
|
||
| /** | ||
| * Main entry point for classes implementing this interface. Processes all | ||
| * uploaded files. |
There was a problem hiding this comment.
Is it required that a processAll implementation calls isValid, or is it required that a user manually call isValid before they call processAll?
(If it's the former, I don't think isValid even needs to be in this interface, the processAll function just needs to document that it must validate the files before moving them.)
| * | ||
| * @return NULL. | ||
| */ | ||
| public function move(string $tmpFileName); |
There was a problem hiding this comment.
UploadedFileInterface would be better than a string here too, I think, since then you have more metadata than the file name to make your decisions on (and the moveTo() function that's already part of the PSR7 interface would be helpful for implementations.)
|
|
||
| /** | ||
| * Handles the exception that was thrown when processing an upload file with a | ||
| * given temporary path. |
There was a problem hiding this comment.
What exception? The process function doesn't say anything about exceptions.. and who's responsibility is it to call this?
| * file. | ||
| * @param string $newPath new (permanent) path for the uploaded file. If | ||
| * the file could not actually be moved to this new location, the | ||
| * value of this parameter will be null. |
There was a problem hiding this comment.
the string type hint won't allow you to pass a null in this parameter, it needs to be the empty string.
| * value of this parameter will be null. | ||
| * @param string $errorMsg the error that occured during the move operation. If | ||
| * the file was successfully moved to its new location, the value | ||
| * for this parameter will be null. |
There was a problem hiding this comment.
This parameter makes me confused about the interaction/chronology between move and onFileMoveEvent. Has the file already been moved? Is it about to be moved? What is someone going to do with a string error message in this handler?
zaliqarosli
added
the
State: Needs work
|
closed in favour of linked PR |
|
You forgot to link it.. closed in favour of #4181 |
…loads in LORIS (#4181) This PR picks up on #3514 and introduces an attempt at creating a generic File Upload (Validator) class for use in LORIS file uploads. See attached issue for more details. The newly Reactified instrument_manager is used as an example to demonstrate how this class should be used.
This PR is the result of a request to break #3476 in two. The first part (this PR) is a proposal for an interface that could be used by all classes handling files uploaded on a server. I'll leave #3476 open for now but close it once we merge this.