Merge pull request #51 from chadparry/rawphoto-squash

Chad Parry's framework enhancement to support replacing an image with a new version of a different mime type.  This includes code that centralizes file type management and fixes some latent issues around the relative url and path caching getting out of date.

8 files changed, 157 insertions, 42 deletions

diff --git a/modules/gallery/controllers/admin_graphics.php b/modules/gallery/controllers/admin_graphics.php
index a2d19d4a..a8a7cdc0 100644
--- a/modules/gallery/controllers/admin_graphics.php
+++ b/modules/gallery/controllers/admin_graphics.php
@@ -40,6 +40,8 @@ class Admin_Graphics_Controller extends Admin_Controller {
       $msg = t("Changed graphics toolkit to: %toolkit", array("toolkit" => $tk->$toolkit_id->name));
       message::success($msg);
       log::success("graphics", $msg);
+
+      module::event("graphics_toolkit_change", $toolkit_id);
     }
 
     url::redirect("admin/graphics");
diff --git a/modules/gallery/controllers/uploader.php b/modules/gallery/controllers/uploader.php
index 6b1455e4..9c2bf7d7 100644
--- a/modules/gallery/controllers/uploader.php
+++ b/modules/gallery/controllers/uploader.php
@@ -51,7 +51,7 @@ class Uploader_Controller extends Controller {
     $file_validation = new Validation($_FILES);
     $file_validation->add_rules(
       "Filedata", "upload::valid",  "upload::required",
-      "upload::type[gif,jpg,jpeg,png,flv,mp4,m4v]");
+      "upload::type[" . implode(",", legal_file::get_extensions()) . "]");
 
     if ($form->validate() && $file_validation->validate()) {
       $temp_filename = upload::save("Filedata");
diff --git a/modules/gallery/helpers/graphics.php b/modules/gallery/helpers/graphics.php
index 39c87fbd..3548faa1 100644
--- a/modules/gallery/helpers/graphics.php
+++ b/modules/gallery/helpers/graphics.php
@@ -170,14 +170,8 @@ class graphics_Core {
 
         foreach (self::_get_rules($target) as $rule) {
           $args = array($working_file, $output_file, unserialize($rule->args), $item);
-          try {
-            call_user_func_array($rule->operation, $args);
-            $working_file = $output_file;
-          } catch (Exception $e) {
-            // Ignore this rule and move on.
-            Kohana_Log::add("error", "Caught exception processing image: {$item->title}\n" .
-                            $e->getMessage() . "\n" . $e->getTraceAsString());
-          }
+          call_user_func_array($rule->operation, $args);
+          $working_file = $output_file;
         }
       }
 
diff --git a/modules/gallery/helpers/legal_file.php b/modules/gallery/helpers/legal_file.php
new file mode 100644
index 00000000..d78efdda
--- /dev/null
+++ b/modules/gallery/helpers/legal_file.php
@@ -0,0 +1,83 @@
+<?php defined("SYSPATH") or die("No direct script access.");
+/**
+ * Gallery - a web based photo album viewer and editor
+ * Copyright (C) 2011 Chad Parry
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or (at
+ * your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+class legal_file_Core {
+  /**
+   * Create a default list of allowed photo extensions and then let modules modify it.
+   */
+  static function get_photo_extensions() {
+    $extensions_wrapper = new stdClass();
+    $extensions_wrapper->extensions = array("gif", "jpg", "jpeg", "png");
+    module::event("legal_photo_extensions", $extensions_wrapper);
+    return $extensions_wrapper->extensions;
+  }
+
+  /**
+   * Create a default list of allowed movie extensions and then let modules modify it.
+   */
+  static function get_movie_extensions() {
+    $extensions_wrapper = new stdClass();
+    $extensions_wrapper->extensions = array("flv", "mp4", "m4v");
+    module::event("legal_movie_extensions", $extensions_wrapper);
+    return $extensions_wrapper->extensions;
+  }
+
+  /**
+   * Create a merged list of all allowed photo and movie extensions.
+   */
+  static function get_extensions() {
+    $extensions = legal_file::get_photo_extensions();
+    if (movie::find_ffmpeg()) {
+      $extensions = array_merge($extensions, legal_file::get_movie_extensions());
+    }
+    return $extensions;
+  }
+
+  /**
+   * Create a merged list of all photo and movie filename filters,
+   * (e.g. "*.gif"), based on allowed extensions.
+   */
+  static function get_filters() {
+    $filters = array();
+    foreach (legal_file::get_extensions() as $extension) {
+      array_push($filters, "*." . $extension, "*." . strtoupper($extension));
+    }
+    return $filters;
+  }
+
+  /**
+   * Create a default list of allowed photo MIME types and then let modules modify it.
+   */
+  static function get_photo_types() {
+    $types_wrapper = new stdClass();
+    $types_wrapper->types = array("image/jpeg", "image/gif", "image/png");
+    module::event("legal_photo_types", $types_wrapper);
+    return $types_wrapper->types;
+  }
+
+  /**
+   * Create a default list of allowed movie MIME types and then let modules modify it.
+   */
+  static function get_movie_types() {
+    $types_wrapper = new stdClass();
+    $types_wrapper->types = array("video/flv", "video/x-flv", "video/mp4");
+    module::event("legal_movie_types", $types_wrapper);
+    return $types_wrapper->types;
+  }
+}
diff --git a/modules/gallery/libraries/Form_Uploadify.php b/modules/gallery/libraries/Form_Uploadify.php
index 3e35e380..450320b3 100644
--- a/modules/gallery/libraries/Form_Uploadify.php
+++ b/modules/gallery/libraries/Form_Uploadify.php
@@ -47,6 +47,7 @@ class Form_Uploadify_Core extends Form_Input {
     $v->script_data = $this->data["script_data"];
     $v->simultaneous_upload_limit = module::get_var("gallery", "simultaneous_upload_limit");
     $v->movies_allowed = (bool) movie::find_ffmpeg();
+    $v->extensions = legal_file::get_filters();
     $v->suhosin_session_encrypt = (bool) ini_get("suhosin.session.encrypt");
 
     list ($toolkit_max_filesize_bytes, $toolkit_max_filesize) = graphics::max_filesize();
diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php
index 2a5e6894..93e97af6 100644
--- a/modules/gallery/models/item.php
+++ b/modules/gallery/models/item.php
@@ -408,6 +408,27 @@ class Item_Model_Core extends ORM_MPTT {
         // If any significant fields have changed, load up a copy of the original item and
         // keep it around.
         $original = ORM::factory("item", $this->id);
+
+        // Preserve the extension of the data file. Many helpers, (e.g. ImageMagick), assume
+        // the MIME type from the extension. So when we adopt the new data file, it's important
+        // to adopt the new extension. That ensures that the item's extension is always
+        // appropriate for its data. We don't try to preserve the name of the data file, though,
+        // because the name is typically a temporary randomly-generated name.
+        if (isset($this->data_file)) {
+          $extension = pathinfo($this->data_file, PATHINFO_EXTENSION);
+          $new_name = pathinfo($this->name, PATHINFO_FILENAME) . ".$extension";
+          if (!empty($extension) && strcmp($this->name, $new_name)) {
+            $this->name = $new_name;
+          }
+          if ($this->is_photo()) {
+            list ($this->width, $this->height, $this->mime_type, $extension) =
+              photo::get_file_metadata($this->data_file);
+          } else if ($this->is_movie()) {
+            list ($this->width, $this->height, $this->mime_type, $extension) =
+              movie::get_file_metadata($this->data_file);
+          }
+        }
+
         if (array_intersect($this->changed, array("parent_id", "name", "slug"))) {
           $original->_build_relative_caches();
           $this->relative_path_cache = null;
@@ -429,8 +450,19 @@ class Item_Model_Core extends ORM_MPTT {
         }
 
         if ($original->parent_id != $this->parent_id || $original->name != $this->name) {
+          $this->_build_relative_caches();
+          // If there is a data file, then we want to preserve both the old data and the new data.
+          // (Third-party event handlers would like access to both). The old data file will be
+          // accessible via the $original item, and the new one via $this item. But in that case,
+          // we don't want to rename the original as below, because the old data would end up being
+          // clobbered by the new data file. Also, the rename isn't necessary, because the new item
+          // data is coming from the data file anyway. So we only perform the rename if there isn't
+          // a data file. Another way to solve this would be to copy the original file rather than
+          // conditionally rename it, but a copy would cost far more than the rename.
+          if (!isset($this->data_file)) {
+            @rename($original->file_path(), $this->file_path());
+          }
           // Move all of the items associated data files
-          @rename($original->file_path(), $this->file_path());
           if ($this->is_album()) {
             @rename(dirname($original->resize_path()), dirname($this->resize_path()));
             @rename(dirname($original->thumb_path()), dirname($this->thumb_path()));
@@ -460,8 +492,6 @@ class Item_Model_Core extends ORM_MPTT {
         }
 
         // Replace the data file, if requested.
-        // @todo: we don't handle the case where you swap in a file of a different mime type
-        //        should we prevent that in validation?  or in set_data_file()
         if ($this->data_file && ($this->is_photo() || $this->is_movie())) {
           copy($this->data_file, $this->file_path());
 
@@ -481,6 +511,9 @@ class Item_Model_Core extends ORM_MPTT {
           // Null out the data file variable here, otherwise this event will trigger another
           // save() which will think that we're doing another file move.
           $this->data_file = null;
+          if ($original->file_path() != $this->file_path()) {
+            @unlink($original->file_path());
+          }
           module::event("item_updated_data_file", $this);
         }
       }
@@ -517,6 +550,8 @@ class Item_Model_Core extends ORM_MPTT {
         $this->name = "$base_name-$rand";
       }
       $this->slug = "$base_slug-$rand";
+      $this->relative_path_cache = null;
+      $this->relative_url_cache = null;
     }
   }
 
@@ -768,16 +803,7 @@ class Item_Model_Core extends ORM_MPTT {
     }
 
     if ($this->is_movie() || $this->is_photo()) {
-      if ($this->loaded()) {
-        // Existing items can't change their extension
-        $original = ORM::factory("item", $this->id);
-        $new_ext = pathinfo($this->name, PATHINFO_EXTENSION);
-        $old_ext = pathinfo($original->name, PATHINFO_EXTENSION);
-        if (strcasecmp($new_ext, $old_ext)) {
-          $v->add_error("name", "illegal_data_file_extension");
-          return;
-        }
-      } else {
+      if (!$this->loaded()) {
         // New items must have an extension
         $ext = pathinfo($this->name, PATHINFO_EXTENSION);
         if (!$ext) {
@@ -785,9 +811,10 @@ class Item_Model_Core extends ORM_MPTT {
           return;
         }
 
-        if ($this->is_movie() && !preg_match("/^(flv|mp4|m4v)$/i", $ext)) {
-          $v->add_error("name", "illegal_data_file_extension");
-        } else if ($this->is_photo() && !preg_match("/^(gif|jpg|jpeg|png)$/i", $ext)) {
+        if ($this->is_photo() &&
+            !in_array(strtolower($ext), array_map("strtolower", legal_file::get_photo_extensions())) ||
+            $this->is_movie() &&
+            !in_array(strtolower($ext), array_map("strtolower", legal_file::get_movie_extensions()))) {
           $v->add_error("name", "illegal_data_file_extension");
         }
       }
@@ -813,17 +840,6 @@ class Item_Model_Core extends ORM_MPTT {
     } else if (filesize($this->data_file) == 0) {
       $v->add_error("name", "empty_data_file");
     }
-
-    if ($this->loaded()) {
-      if ($this->is_photo()) {
-        list ($a, $b, $mime_type) = photo::get_file_metadata($this->data_file);
-      } else if ($this->is_movie()) {
-        list ($a, $b, $mime_type) = movie::get_file_metadata($this->data_file);
-      }
-      if ($mime_type != $this->mime_type) {
-        $v->add_error("name", "cant_change_mime_type");
-      }
-    }
   }
 
   /**
@@ -877,9 +893,9 @@ class Item_Model_Core extends ORM_MPTT {
     switch($field) {
     case "mime_type":
       if ($this->is_movie()) {
-        $legal_values = array("video/flv", "video/x-flv", "video/mp4");
-      } if ($this->is_photo()) {
-        $legal_values = array("image/jpeg", "image/gif", "image/png");
+        $legal_values = legal_file::get_movie_types();
+      } else if ($this->is_photo()) {
+        $legal_values = legal_file::get_photo_types();
       }
       break;
 
diff --git a/modules/gallery/tests/Item_Model_Test.php b/modules/gallery/tests/Item_Model_Test.php
index 968d7510..19ab8ec4 100644
--- a/modules/gallery/tests/Item_Model_Test.php
+++ b/modules/gallery/tests/Item_Model_Test.php
@@ -394,15 +394,34 @@ class Item_Model_Test extends Gallery_Unit_Test_Case {
     $this->assert_equal(20337, filesize($photo->file_path()));
   }
 
-  public function replacement_data_file_must_be_same_mime_type_test() {
+  public function replace_data_file_type_test() {
     // Random photo is modules/gallery/tests/test.jpg
     $photo = test::random_photo();
+    $this->assert_equal(1024, $photo->width);
+    $this->assert_equal(768, $photo->height);
+    $this->assert_equal(6232, filesize($photo->file_path()));
+    $this->assert_equal("image/jpeg", $photo->mime_type);
+    $orig_name = $photo->name;
+
+    // Random photo is gallery/images/graphicsmagick.png is 104x76 and 1486 bytes
     $photo->set_data_file(MODPATH . "gallery/images/graphicsmagick.png");
+    $photo->save();
+
+    $this->assert_equal(104, $photo->width);
+    $this->assert_equal(76, $photo->height);
+    $this->assert_equal(1486, filesize($photo->file_path()));
+    $this->assert_equal("image/png", $photo->mime_type);
+    $this->assert_equal("png", pathinfo($photo->name, PATHINFO_EXTENSION));
+    $this->assert_equal(pathinfo($orig_name, PATHINFO_FILENAME), pathinfo($photo->name, PATHINFO_FILENAME));
+  }
 
+  public function unsafe_data_file_replacement_test() {
     try {
+      $photo = test::random_photo();
+      $photo->set_data_file(MODPATH . "gallery/tests/Item_Model_Test.php");
       $photo->save();
     } catch (ORM_Validation_Exception $e) {
-      $this->assert_same(array("name" => "cant_change_mime_type"), $e->validation->errors());
+      $this->assert_same(array("mime_type" => "invalid"), $e->validation->errors());
       return;  // pass
     }
     $this->assert_true(false, "Shouldn't get here");
diff --git a/modules/gallery/views/form_uploadify.html.php b/modules/gallery/views/form_uploadify.html.php
index 83dfcc68..ba4a3621 100644
--- a/modules/gallery/views/form_uploadify.html.php
+++ b/modules/gallery/views/form_uploadify.html.php
@@ -28,7 +28,7 @@
         uploader: "<?= url::file("lib/uploadify/uploadify.swf") ?>",
         script: "<?= url::site("uploader/add_photo/{$album->id}") ?>",
         scriptData: <?= json_encode($script_data) ?>,
-        fileExt: "*.gif;*.jpg;*.jpeg;*.png;*.GIF;*.JPG;*.JPEG;*.PNG<? if ($movies_allowed): ?>;*.flv;*.mp4;*.m4v;*.FLV;*.MP4;*.M4V<? endif ?>",
+        fileExt: "<?= implode(";", $extensions) ?>",
         fileDesc: <?= t("Photos and movies")->for_js() ?>,
         cancelImg: "<?= url::file("lib/uploadify/cancel.png") ?>",
         simUploadLimit: <?= $simultaneous_upload_limit ?>,