[Refactoring] Proposed refactoring of FileUpload.class.inc #3476

nicolasbrossard · 2018-02-07T19:46:08Z

This PR is the proposed interface for classes that handle file uploads. It also contains a default implementation that could be used as the base class for all handlers in LORIS that will process one or more uploaded files. I have coded an implementation for the imaging uploader as a proof of concept and things seem to work fine (will be the object of another PR). After review, I can start refactoring all modules in LORIS so that this new class is used every time uploaded files are processed.

PapillonMcGill

well structured!
I haven't tested the code

xlecours

Review for DefaultFilesUploadHandler.class.inc
Comments, suggestions and change requests

xlecours · 2018-02-12T14:32:11Z

php/libraries/DefaultFilesUploadHandler.class.inc

+     * @param int       $dirGroup       if directory $moveDirPath has to be created
+     *                                  it will have this access group.
+     *
+     * @return nothing.


This do net need to be specified. And it implicitly return the new instance of that class.

Funny it passes phpcs...Anyway, I'll remove it.

I'm surprised it passed Travis. I thought phpcs complains if a constructor declares a return?

xlecours · 2018-02-12T14:33:15Z

php/libraries/DefaultFilesUploadHandler.class.inc

+     * Default directory to move the uploaded files to if none is explicitely
+     * specified.
+     */
+    public static $DEFAULT_MOVE_DIR = __DIR__ . "../user_uploads";


Is this directory created in the install script?

xlecours · 2018-02-12T14:35:52Z

php/libraries/DefaultFilesUploadHandler.class.inc

+    ) {
+        $this->_lorisForm      = $lorisForm;
+        $this->_moveDirPath    = is_null($moveDirPath)
+            ? self::$DEFAULT_MOVE_DIR : $moveDirPath;


Null coalesce operator would make it fit in one line.
http://php.net/manual/fr/migration70.new-features.php#migration70.new-features.null-coalesce-op

Agreed. Will change.

xlecours · 2018-02-12T14:37:34Z

php/libraries/DefaultFilesUploadHandler.class.inc

+     *                         uploaded file.
+     * @param string $errorMsg error message text.
+     *
+     * @return nothing.


NULL would be more accurate

I have no idea why phpcs forces me to have a return statement for a "void" method...Will change to @return NULL as requested.

I don't think @return NULL is correct unless it's explicitly returning a value of null. Check what phan does, if PHPCS is allowing anything.. I'm fairly certain @return void is the most robust thing for static analysis tools.

xlecours · 2018-02-12T14:38:46Z

php/libraries/DefaultFilesUploadHandler.class.inc

+    /**
+     * Accessor for field $_dirGroup.
+     *
+     * @return int value for field $_dirGroup.


Change int to string

xlecours · 2018-02-12T15:10:23Z

php/libraries/DefaultFilesUploadHandler.class.inc

+        try {
+            if ($this->isValid()) {
+                foreach ($this->getFiles() as $field => $file) {
+                    if ($file !== false) {


isValid would be false in that case. This if statement can be removed

Actually this was put there for a reason: to allow handling of optional files (i.e. those that might be uploaded but do not necessarily have to). The isValid() method would return true in case one of the optional files is missing and the if(...) code would still be executed. If it was not implemented that way, derived classes would have to override the process method to handle optional files. Makes sense?

Then isValid should handle optional files. And you could consider using the final keyword if the behavior is critical.

xlecours · 2018-02-12T15:21:29Z

php/libraries/DefaultFilesUploadHandler.class.inc

+                                }
+                            }
+                            // File name exists. Make sure it refers to a directory.
+                        } else if (!is_dir($newDir)) {


This could have been checked in the constructor and throw an exception if !(new SPLFileInfo("$moveDir"))->isWritable()

xlecours · 2018-02-12T15:24:01Z

php/libraries/DefaultFilesUploadHandler.class.inc

+                        }
+
+                        // If everything went OK so far
+                        if (!isset($this->_errors[$field])) {


This if statement could be skipped if a continue was added after the addError call

xlecours · 2018-02-12T15:30:25Z

php/libraries/DefaultFilesUploadHandler.class.inc

+     *
+     * @return SplInfo the path information for the new file location.
+     */
+    public function getNewLocation(string $tmpFilePath) : SplFileInfo


By passing the LorisFormFileElement instead of its tmp_name attribute, the foreach loop could be avoided. And this could instanciate the SPLFileInfo and check if it is writable before returning it. throw an exception maybe.

xlecours · 2018-02-12T15:32:30Z

php/libraries/DefaultFilesUploadHandler.class.inc

+                        $newDir  = $newFile->getPath();
+
+                        // Check to see if directory exists
+                        if (!file_exists($newDir)) {


This whole block that check if the directory exists and create it if not should be encapsulated in a function so this processAll() would look less convoluted.

Good idea. I'll refactor.

xlecours · 2018-02-12T15:38:38Z

php/libraries/FilesUploadHandler.class.inc

+     * (i.e. at the very end of processAll()). Cleanup code typically is put into
+     * this function.
+     *
+     * @return nothing.


xlecours · 2018-02-12T15:38:44Z

php/libraries/FilesUploadHandler.class.inc

+     *               the file was successfully moved to its new location, the value
+     *               for this parameter will be null.
+     *
+     * @return nothing.


What does PHPCS say it should be? I'm surprised it passed travis with "nothing."

Yes, it passes Travis and phpcs does not complain. I'll replace with @return NULL if that is the convention.

@return void should be used to indicate the function doesn't return a value. @return Null indicate that the function return the null value.

Docs says:

If the return is omitted the value NULL will be returned.

http://php.net/manual/en/functions.returning-values.php

I would prefer void since it reminds me of C++ void main (void)

I prefer void too but since the doc says that a void method returns NULL, I think that I have to use "@return NULL"...

Ok for null, unlike C, php function always return something.

xlecours · 2018-02-12T15:41:09Z

php/libraries/LorisForm.class.inc

@@ -2224,6 +2257,19 @@ class LorisForm
 */
 class LorisFormElement
 {
+    var $el;


protected instead of var

xlecours · 2018-02-12T15:50:17Z

php/libraries/FilesUploadHandler.class.inc

+     *
+     * @return SplFileInfo all the information regarding the new file location.
+     */
+    public function getNewLocation(string $fileName) : SplFileInfo;


Does it need to be public? I don't think it needs to be in the interface even.

driusan

feedback on DefaultFilesUploadHandler

driusan · 2018-02-12T16:04:40Z

php/libraries/DefaultFilesUploadHandler.class.inc

+    /**
+     *  The LorisForm used to upload the file(s).
+     */
+    private $_lorisForm;


This adds unnecessary coupling to LorisForm.

The places where it is used, it would make more sense to use a PSR7 ServerRequestInterface to get the files out of.

The coupling is intentional. The purpose of the entire class is to provide default behaviour for entities that handle files uploaded via a LorisForm (which is what will be the case in LORIS). I think the coupling had to be avoided in the interface, so that you could have a concrete class that implements it but still uploads files via a mechanism other than LorisForm.

driusan · 2018-02-12T16:10:38Z

php/libraries/DefaultFilesUploadHandler.class.inc

+    /**
+     * When creating $_moveDirPath, these are the access rights to give it
+     * (should be an octal number).
+     */


Is it a string representing an octal number or is it an int? If the latter, the parenthesis is meaningless. Numbers don't have number bases as they're passed around in memory.

The comment was more an indication of the literal representation that should be used for this number so that the calling code is easy to read and very UNIX-like. 0755 and 493 (0755 in decimal form) are valid arguments for this method and produce the same result, but 0755 is easier to read. I can remove the comment if you think it is misleading though.

driusan · 2018-02-12T16:12:14Z

php/libraries/DefaultFilesUploadHandler.class.inc

+     */
+    private $_dirPermissions;
+    /**
+     * When creating $_moveDirPath, give it this access group.


What's an "access group"?

Should be file group instead.

driusan · 2018-02-12T16:19:49Z

php/libraries/DefaultFilesUploadHandler.class.inc

+     * All the properties of the uploaded files. Equivalent to the $_FILES
+     * superglobal.
+     */
+    private $_files = null;


It would make more sense to me to base this on \Psr\Http\Message\UploadedFileInterface than on the _FILES superglobal.

driusan · 2018-02-12T16:30:50Z

php/libraries/DefaultFilesUploadHandler.class.inc

+     * in this array is a SplFileInfo. Each key is the name of the LorisForm
+     * field used to upload the file.
+     */
+    private $_movedFiles;


This (and most of the things that have higher than private getter/setters) should be protected.

driusan · 2018-02-12T16:43:23Z

php/libraries/DefaultFilesUploadHandler.class.inc

+     * in the LorisForm, then the upload is considered invalid and the error for that
+     * field will be set to false. This method assumes that all the fields used for
+     * file uploads have a value (i.e. no "optional" upload is allowed). This
+     * behavior can be overridden bu subclasses should they need to.


driusan · 2018-02-12T16:46:27Z

php/libraries/DefaultFilesUploadHandler.class.inc

+     *
+     * @return nothing.
+     */
+    public function processAll()


I think this would be a lot more robust if it took a parameter of what needs to be processed (maybe defaulting to 'all' if null), rather than always needing to process all files. What if there's multiple handlers on a page that process different things?

Maybe add another method, say shouldProcess($file) (or something along those lines, with a default implementation that always returns true) and have process() call shouldProcess() to determine if a file should be processed or not?

driusan · 2018-02-12T16:51:09Z

php/libraries/DefaultFilesUploadHandler.class.inc

+     */
+    public function processAll()
+    {
+        try {


This will partially process the array and leave things in an inconsistent state if something in the middle throws an exception.

It's debatable whether the try should be moved inside the loop or not, but regardless the content of the loop should be moved to a protected processFile($file) : bool (with some appropriate type for $file, or maybe throwing an exception instead of returning a bool) which is called for each file, so that a mechanism can be put in place for the programmer to determine how to deal with error cases.

You do have access to the _errors and _movedFiles array so you could perform any "cleanup" in processingDone if anything goes seriously wrong (provided the exception is passed to processingDone, which is not the case right now). This would provide similar behaviour IMHO. I do see your point though. I am kind of on the fence regarding this...

driusan · 2018-02-12T16:52:31Z

php/libraries/DefaultFilesUploadHandler.class.inc

+     *
+     * @return nothing.
+     */
+    public function onFileMoveEvent(


It might be a good idea to declare this as abstract rather than having a default which does nothing.

I think providing a default no-op method makes sense (e.g for handlers that only store files in specific locations and do not do anything else)

driusan · 2018-02-12T16:53:07Z

php/libraries/DefaultFilesUploadHandler.class.inc

+     * nothing by default. Subclasses should override this method with any cleanup
+     * code they require.
+     *
+     * @return nothing.


Needs to specify that it's called regardless of whether or not the processing was successful.

driusan · 2018-02-12T17:05:23Z

@nicolasbrossard Can you break this into 2 PRs: 1 that just has the interface so that the appropriate design can be discussed, and then once that one goes in a second one to provide the default implementation?

I think the reviews on this are mixing design and implementation concerns which muddies the waters a bit (partially because github is showing the implementation before the interface in the diff..)

nicolasbrossard · 2018-02-12T17:42:27Z

Break this into 2PRs: OK

ridz1208 · 2018-03-21T15:52:36Z

@nicolasbrossard what is the status of this ?
can this one be closed ?

nicolasbrossard · 2018-03-21T16:05:27Z

@ridz yes, this PR can be closed. #3514 replaces it.

ridz1208 · 2018-03-21T16:09:15Z

Closig this since it's being broken down into multiple smaller PRs

[Refactoring] Proposed refactoring of FileUpload.class.inc

25eff71

nicolasbrossard requested review from xlecours and ridz1208 February 7, 2018 19:46

nicolasbrossard added the [branch] minor label Feb 7, 2018

Fixed broken Travis build

c829515

PapillonMcGill approved these changes Feb 9, 2018

View reviewed changes

xlecours requested changes Feb 12, 2018

View reviewed changes

driusan requested changes Feb 12, 2018

View reviewed changes

ridz1208 mentioned this pull request Feb 22, 2018

[Libraries] Refactor File_Upload Class #2917

Closed

7 tasks

nicolasbrossard mentioned this pull request Feb 27, 2018

Interface for classes that handle file uploads #3514

Closed

ridz1208 closed this Mar 21, 2018

[Refactoring] Proposed refactoring of FileUpload.class.inc #3476

[Refactoring] Proposed refactoring of FileUpload.class.inc #3476

Conversation

nicolasbrossard commented Feb 7, 2018

PapillonMcGill left a comment

Choose a reason for hiding this comment

xlecours 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

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

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

PapillonMcGill Feb 12, 2018 • edited Loading

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

driusan 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

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

driusan commented Feb 12, 2018

nicolasbrossard commented Feb 12, 2018

ridz1208 commented Mar 21, 2018

nicolasbrossard commented Mar 21, 2018

ridz1208 commented Mar 21, 2018

PapillonMcGill Feb 12, 2018 •

edited

Loading