-
Notifications
You must be signed in to change notification settings - Fork 175
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
Interface for classes that handle file uploads #3514
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,129 @@ | ||
<?php | ||
/** | ||
* An interface containing methods for classes used to handle file | ||
* uploads from a client to a server. | ||
* | ||
* PHP Version 7 | ||
* | ||
* @category Main | ||
* @package Main | ||
* @author Nicolas Brossard <nicolasbrossard.mni@gmail.com> | ||
* @license http://www.gnu.org/licenses/gpl-3.0.txt GPLv3 | ||
* @link https://www.github.com/aces/Loris/ | ||
*/ | ||
|
||
/** | ||
* Handles file uploads from a client to a server. The methods listed in this | ||
* interface were created in order to encapsulate the usual things that happen | ||
* when a set of files is uploaded to a server, namely: | ||
* | ||
* 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do these need to be all files and not (potentially) some files? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
* 3. Actual processing of the uploaded files (e.g. update of database | ||
* tables, etc...) | ||
* | ||
* @category Main | ||
* @package Main | ||
* @author Nicolas Brossard <nicolasbrossard.mni@gmail.com> | ||
* @license http://www.gnu.org/licenses/gpl-3.0.txt GPLv3 | ||
* @link https://www.github.com/aces/Loris/ | ||
*/ | ||
interface FilesUploadHandler | ||
{ | ||
/** | ||
* Returns the properties associated to each file uploaded on the | ||
* server in an array identical to $_FILES. | ||
* | ||
* @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
/** | ||
* Validates the set of uploaded files. If a given file is invalid, the | ||
* validation message associated to it will be stored in the array returned | ||
* by getErrors(). | ||
* | ||
* @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the function should take what it's validating as a parameter rather than depending on implicit state. I wanted to suggest |
||
|
||
/** | ||
* Returns any validation or processing errors associated to each uploaded file. | ||
* | ||
* @return array list of error messages for each uploaded file. | ||
*/ | ||
public function getErrors() : array; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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) |
||
|
||
/** | ||
* Returns the final locations of all files that were successfully moved. This | ||
* method will not return anything meaningful until the move operation is done. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There should be a well-specified definition of what "nothing meaningful" means (an empty array? null?), and there needs to be a way to determine if you can rely on the output of this or not. Maybe it should throw an exception if the move isn't done?) |
||
* | ||
* @return SplFileInfo[] final locations of all files that were successfully | ||
* moved. | ||
*/ | ||
public function getMovedFiles() : array; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. getMovedFileLocations (or similar), it's not returning the files themselves. |
||
|
||
/** | ||
* Main entry point for classes implementing this interface. Processes all | ||
* uploaded files. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 processAll(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why does this need to be all? Why can't it take the files that it's being asked to process as a parameter? (If both this and the validation function were explicitly passed the fileset, then the getFiles/setFiles/withFiles would become unnecessary and the interface could be further simplified.) |
||
|
||
/** | ||
* Takes a file that was uploaded on the server and moves it from its temporary | ||
* location to a permament location. | ||
* | ||
* @param string $tmpFileName temporary path to the uploaded file on the server. | ||
* | ||
* @return NULL. | ||
*/ | ||
public function move(string $tmpFileName); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What exception? The process function doesn't say anything about exceptions.. and who's responsibility is it to call this? |
||
* | ||
* @param string $file the temporary path the uploaded file has on the server. | ||
* @param Exception $e the exception that was thrown when processing the file | ||
* passed as argument. | ||
* | ||
* @return NULL. | ||
*/ | ||
public function onProcessError(string $file, Exception $e); | ||
|
||
/** | ||
* This function is called each time a file is moved to its permanent location | ||
* on the server. This is where the actual processing of the uploaded file should | ||
* take place. | ||
* | ||
* @param string $oldPath temporary path on the server for a given uploaded | ||
* 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the string type hint won't allow you to pass a null in this parameter, it needs to be the empty string. |
||
* @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
* | ||
* @return NULL. | ||
*/ | ||
public function onFileMoveEvent( | ||
string $oldPath, string $newPath=null, string $errorMsg=null | ||
); | ||
|
||
/** | ||
* This function is called once all the uploaded files have been processed | ||
* (i.e. at the very end of processAll()). Cleanup code typically is put into | ||
* this function. Note that this method will always be called, regardless of | ||
* whether an error occured during file processing or not. | ||
* | ||
* @return NULL. | ||
*/ | ||
public function processingDone(); | ||
} | ||
?> |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ..."