#2058 - Separate uploader logic into Kohana-, Item-Model-, and Uploadify-specific functions.

- Added _process_upload() to handle upload file validation with Kohana.
- Added _add_item() to handle item creation and validation with the item model (and logs).
- Removed these pieces from add_photo(), which is now rather Uploadify-specific.
- Used $_FILES to get filename instead of assuming it's substr(basename($tmp_name), 10).
- No net functional changes - works just like before.

1 files changed, 95 insertions, 42 deletions

diff --git a/modules/gallery/controllers/uploader.php b/modules/gallery/controllers/uploader.php
index c708db92..54e1476b 100644
--- a/modules/gallery/controllers/uploader.php
+++ b/modules/gallery/controllers/uploader.php
@@ -47,52 +47,25 @@ class Uploader_Controller extends Controller {
 
     $form = $this->_get_add_form($album);
 
-    // Uploadify adds its own field to the form, so validate that separately.
-    $file_validation = new Validation($_FILES);
-    $file_validation->add_rules(
-      "Filedata", "upload::valid",  "upload::required",
-      "upload::type[" . implode(",", legal_file::get_extensions()) . "]");
-
-    if ($form->validate() && $file_validation->validate()) {
-      $temp_filename = upload::save("Filedata");
-      system::delete_later($temp_filename);
+    if ($form->validate()) {
+      // Uploadify puts the result in $_FILES["Filedata"] - process it.
       try {
-        $item = ORM::factory("item");
-        $item->name = substr(basename($temp_filename), 10);  // Skip unique identifier Kohana adds
-        $item->title = item::convert_filename_to_title($item->name);
-        $item->parent_id = $album->id;
-        $item->set_data_file($temp_filename);
-
-        $path_info = @pathinfo($temp_filename);
-        if (array_key_exists("extension", $path_info) &&
-            legal_file::get_movie_extensions($path_info["extension"])) {
-          $item->type = "movie";
-          $item->save();
-          log::success("content", t("Added a movie"),
-                       html::anchor("movies/$item->id", t("view movie")));
-        } else {
-          $item->type = "photo";
-          $item->save();
-          log::success("content", t("Added a photo"),
-                       html::anchor("photos/$item->id", t("view photo")));
-        }
+        list ($tmp_name, $name) = $this->_process_upload("Filedata");
+      } catch (Exception $e) {
+        header("HTTP/1.1 400 Bad Request");
+        print "ERROR: " . $e->getMessage();
+        return;
+      }
 
+      // We have a valid upload file (of unknown type) - build an item from it.
+      try {
+        $item = $this->_add_item($id, $tmp_name, $name);
         module::event("add_photos_form_completed", $item, $form);
+        print "FILEID: $item->id";
       } catch (Exception $e) {
-        // The Flash uploader has no good way of reporting complex errors, so just keep it simple.
-        Kohana_Log::add("error", $e->getMessage() . "\n" . $e->getTraceAsString());
-
-        // Ugh.  I hate to use instanceof, But this beats catching the exception separately since
-        // we mostly want to treat it the same way as all other exceptions
-        if ($e instanceof ORM_Validation_Exception) {
-          Kohana_Log::add("error", "Validation errors: " . print_r($e->validation->errors(), 1));
-        }
-
         header("HTTP/1.1 500 Internal Server Error");
         print "ERROR: " . $e->getMessage();
-        return;
       }
-      print "FILEID: $item->id";
     } else {
       header("HTTP/1.1 400 Bad Request");
       print "ERROR: " . t("Invalid upload");
@@ -107,17 +80,17 @@ class Uploader_Controller extends Controller {
                (int)$success_count,
                array("error" => (int)$error_count));
     } else {
-      print t2("Uploaded %count photo", "Uploaded %count photos", $success_count);}
+      print t2("Uploaded %count photo", "Uploaded %count photos", $success_count);
+    }
   }
 
   public function finish() {
     access::verify_csrf();
-
     batch::stop();
     json::reply(array("result" => "success"));
   }
 
-  private function _get_add_form($album)  {
+  private function _get_add_form($album) {
     $form = new Forge("uploader/finish", "", "post", array("id" => "g-add-photos-form"));
     $group = $form->group("add_photos")
       ->label(t("Add photos to %album_title", array("album_title" => html::purify($album->title))));
@@ -145,4 +118,84 @@ class Uploader_Controller extends Controller {
 
     return $form;
   }
+
+  /**
+   * Process the uploaded file.  This handles the interface with Kohana's upload and validation
+   * code, and marks the new temp file for deletion on shutdown.  It returns the temp file path
+   * (tmp_name) and filename (name), analogous to their respective $_FILES elements.
+   * If the upload is invalid, it will throw an exception.  Note that no type-checking (e.g. jpg,
+   * mp4,...) is performed here.
+   * @TODO: consider moving this to a common controller which is extended by various uploaders.
+   *
+   * @param  string name of $_FILES input
+   * @return array  array($tmp_name, $name)
+   */
+  private function _process_upload($file) {
+    // Validate file data.  At this point, any file extension is still valid.
+    $file_validation = new Validation($_FILES);
+    $file_validation->add_rules($file, "upload::valid",  "upload::required");
+    if (!$file_validation->validate()) {
+      throw new Exception(t("Invalid upload"));
+    }
+
+    // Save temp file and mark for deletion when done.
+    $tmp_name = upload::save($file);
+    system::delete_later($tmp_name);
+
+    // Get uploaded filename.  This is different than tmp_name since it hasn't been uniquified.
+    $name = $_FILES[$file]["name"];
+
+    return array($tmp_name, $name);
+  }
+
+  /**
+   * Add photo or movie from upload.  Once we have a valid file, this generates an item model
+   * from it.  It returns the item model on success, and throws an exception and adds log entries
+   * on failure.
+   * @TODO: consider moving this to a common controller which is extended by various uploaders.
+   *
+   * @param  int    parent album id
+   * @param  string temp file path (analogous to $_FILES[...]["tmp_name"])
+   * @param  string filename       (analogous to $_FILES[...]["name"])
+   * @return object new item model
+   */
+  private function _add_item($album_id, $tmp_name, $name) {
+    $extension = pathinfo($name, PATHINFO_EXTENSION);
+
+    try {
+      $item = ORM::factory("item");
+      $item->name = $name;
+      $item->title = item::convert_filename_to_title($name);
+      $item->parent_id = $album_id;
+      $item->set_data_file($tmp_name);
+
+      if (!$extension) {
+        throw new Exception(t("Uploaded file has no extension"));
+      } else if (legal_file::get_photo_extensions($extension)) {
+        $item->type = "photo";
+        $item->save();
+        log::success("content", t("Added a photo"),
+                     html::anchor("photos/$item->id", t("view photo")));
+      } else if (movie::allow_uploads() && legal_file::get_movie_extensions($extension)) {
+        $item->type = "movie";
+        $item->save();
+        log::success("content", t("Added a movie"),
+                     html::anchor("movies/$item->id", t("view movie")));
+      } else {
+        throw new Exception(t("Uploaded file has illegal extension"));
+      }
+    } catch (Exception $e) {
+      // Log errors then re-throw exception.
+      Kohana_Log::add("error", $e->getMessage() . "\n" . $e->getTraceAsString());
+
+      // If we have a validation error, add an additional log entry.
+      if ($e instanceof ORM_Validation_Exception) {
+        Kohana_Log::add("error", "Validation errors: " . print_r($e->validation->errors(), 1));
+      }
+
+      throw $e;
+    }
+
+    return $item;
+  }
 }