From d45a73777935c86fc5131955831833d7465b5e9d Mon Sep 17 00:00:00 2001
From: Bharat Mediratta <bharat@menalto.com>
Date: Mon, 21 Jan 2013 01:22:01 -0500
Subject: Update copyright to 2013.  Fixes #1953.

---
 modules/gallery/models/item.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(limited to 'modules/gallery/models/item.php')

diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php
index 8f127532..85aca2c1 100644
--- a/modules/gallery/models/item.php
+++ b/modules/gallery/models/item.php
@@ -1,7 +1,7 @@
 <?php defined("SYSPATH") or die("No direct script access.");
 /**
  * Gallery - a web based photo album viewer and editor
- * Copyright (C) 2000-2012 Bharat Mediratta
+ * Copyright (C) 2000-2013 Bharat Mediratta
  *
  * 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
-- 
cgit v1.2.3


From 9ba9f3953132c5c5de9efb0a4724c7b9300dc9ea Mon Sep 17 00:00:00 2001
From: Bharat Mediratta <bharat@menalto.com>
Date: Wed, 23 Jan 2013 18:24:31 -0500
Subject: Add a "can_add" permission bit similar to the "can_edit" bit to the
 RESTful array representation of Item_Model.  Fixes #1903.

---
 modules/gallery/models/item.php           | 12 ++++++++----
 modules/gallery/tests/Item_Model_Test.php | 11 ++++++++++-
 2 files changed, 18 insertions(+), 5 deletions(-)

(limited to 'modules/gallery/models/item.php')

diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php
index 85aca2c1..bbfeee47 100644
--- a/modules/gallery/models/item.php
+++ b/modules/gallery/models/item.php
@@ -663,7 +663,7 @@ class Item_Model_Core extends ORM_MPTT {
   }
 
   /**
-   * Return a view for movies.  By default this is a Flowplayer v3 <script> tag, but 
+   * Return a view for movies.  By default this is a Flowplayer v3 <script> tag, but
    * movie_img events can override this and provide their own player/view.  If no player/view
    * is found and the movie is unsupported by Flowplayer v3, this returns a simple download link.
    * @param array $extra_attrs
@@ -675,12 +675,12 @@ class Item_Model_Core extends ORM_MPTT {
     $height = $this->height;
     if ($width == 0 || $height == 0) {
       // Not set correctly, likely because ffmpeg isn't available.  Making the window 0x0 causes the
-      // video to be effectively unviewable.  So, let's guess: set width to max_size and guess a 
-      // height (using 4:3 aspect ratio).  Once the video metadata is loaded, js in 
+      // video to be effectively unviewable.  So, let's guess: set width to max_size and guess a
+      // height (using 4:3 aspect ratio).  Once the video metadata is loaded, js in
       // movieplayer.html.php will correct these values.
       $width = $max_size;
       $height = ceil($width * 3/4);
-    } 
+    }
     $attrs = array_merge(array("id" => "g-item-id-{$this->id}"), $extra_attrs,
                          array("class" => "g-movie"));
 
@@ -1073,6 +1073,10 @@ class Item_Model_Core extends ORM_MPTT {
       $data["can_edit"] = access::can("edit", $this);
     }
 
+    if (empty($fields) || isset($fields["can_add"])) {
+      $data["can_add"] = access::can("add", $this);
+    }
+
     // Elide some internal-only data that is going to cause confusion in the client.
     foreach (array("relative_path_cache", "relative_url_cache", "left_ptr", "right_ptr",
                    "thumb_dirty", "resize_dirty", "weight") as $key) {
diff --git a/modules/gallery/tests/Item_Model_Test.php b/modules/gallery/tests/Item_Model_Test.php
index dc4432a6..a7eba1ad 100644
--- a/modules/gallery/tests/Item_Model_Test.php
+++ b/modules/gallery/tests/Item_Model_Test.php
@@ -399,7 +399,16 @@ class Item_Model_Test extends Gallery_Unit_Test_Case {
     $this->assert_false($response["can_edit"]);
   }
 
-  public function first_photo_becomes_album_cover() {
+  public function as_restful_array_with_add_bit_test() {
+    $response = item::root()->as_restful_array();
+    $this->assert_true($response["can_add"]);
+
+    identity::set_active_user(identity::guest());
+    $response = item::root()->as_restful_array();
+    $this->assert_false($response["can_add"]);
+  }
+
+  public function first_photo_becomes_album_cover_test() {
     $album = test::random_album();
     $photo = test::random_photo($album);
     $album->reload();
-- 
cgit v1.2.3


From cc2aed656c7289881ba23e8ec700a4daf7889688 Mon Sep 17 00:00:00 2001
From: shadlaws <shad@shadlaws.com>
Date: Sat, 26 Jan 2013 08:38:31 +0100
Subject: #1946, 1947 - Make altered names/slugs more user-friendly, make
 conflict-finding code check filenames with no extensions - Reduced from four
 places that alter names/slugs to two (one to populate empty slugs, one to
 handle conflicting names/slugs). - For empty slugs, fill with generic
 human-readable name (e.g. "photo") instead of random value. - For conflicting
 names/slugs, add suffix that's sequential (e.g. "-01"), only using random
 after 99 conflicts. - Made conflict-finding code check filenames with no
 extensions. - Renamed _randomize_name_or_slug_on_conflict to
 _check_and_fix_conflicts. - Added unit tests.  Also cleaned up existing unit
 tests to reflect new logic and removed a redundant test. - Added installer
 logic to correct existing items now considered in conflict. - Revised
 gallery_task to look for duplicate names based on new criteria.

---
 installer/install.sql                         |   2 +-
 modules/gallery/helpers/gallery_installer.php |  43 +++++-
 modules/gallery/helpers/gallery_task.php      | 113 +++++++++++++--
 modules/gallery/helpers/item.php              |  63 +++------
 modules/gallery/models/item.php               | 127 +++++++++++------
 modules/gallery/module.info                   |   2 +-
 modules/gallery/tests/Item_Model_Test.php     | 194 ++++++++++++++++++++++----
 7 files changed, 418 insertions(+), 126 deletions(-)

(limited to 'modules/gallery/models/item.php')

diff --git a/installer/install.sql b/installer/install.sql
index b01c5a7c..70cf0c86 100644
--- a/installer/install.sql
+++ b/installer/install.sql
@@ -244,7 +244,7 @@ CREATE TABLE {modules} (
   KEY `weight` (`weight`)
 ) AUTO_INCREMENT=10 DEFAULT CHARSET=utf8;
 /*!40101 SET character_set_client = @saved_cs_client */;
-INSERT INTO {modules} VALUES (1,1,'gallery',53,1);
+INSERT INTO {modules} VALUES (1,1,'gallery',54,1);
 INSERT INTO {modules} VALUES (2,1,'user',4,2);
 INSERT INTO {modules} VALUES (3,1,'comment',7,3);
 INSERT INTO {modules} VALUES (4,1,'organize',4,4);
diff --git a/modules/gallery/helpers/gallery_installer.php b/modules/gallery/helpers/gallery_installer.php
index fe651a4e..86ff7ce5 100644
--- a/modules/gallery/helpers/gallery_installer.php
+++ b/modules/gallery/helpers/gallery_installer.php
@@ -315,7 +315,7 @@ class gallery_installer {
     module::set_var("gallery", "lock_timeout", 1);
     module::set_var("gallery", "movie_extract_frame_time", 3);
 
-    module::set_version("gallery", 53);
+    module::set_version("gallery", 54);
   }
 
   static function upgrade($version) {
@@ -743,6 +743,47 @@ class gallery_installer {
       module::set_var("gallery", "movie_extract_frame_time", 3);
       module::set_version("gallery", $version = 53);
     }
+
+    if ($version == 53) {
+      // In v54, we changed how we check for name and slug conflicts in Item_Model.  Previously,
+      // we checked the whole filename.  As a result, "foo.jpg" and "foo.png" were not considered
+      // conflicting if their slugs were different (a rare case in practice since server_add and
+      // uploader would give them both the same slug "foo").  Now, we check the filename without its
+      // extension.  This upgrade stanza fixes any conflicts where they were previously allowed.
+
+      // This might be slow, but if it times out it can just pick up where it left off.
+
+      // Find and loop through each conflict (e.g. "foo.jpg", "foo.png", and "foo.flv" are one
+      // conflict; "bar.jpg", "bar.png", and "bar.flv" are another)
+      foreach (db::build()
+               ->select_distinct(array("parent_base_name" =>
+                 db::expr("CONCAT(`parent_id`, ':', LOWER(SUBSTR(`name`, 1, LOCATE('.', `name`) - 1)))")))
+               ->select(array("C" => "COUNT(\"*\")"))
+               ->from("items")
+               ->where("type", "<>", "album")
+               ->having("C", ">", 1)
+               ->group_by("parent_base_name")
+               ->execute() as $conflict) {
+        list ($parent_id, $base_name) = explode(":", $conflict->parent_base_name, 2);
+        $base_name_escaped = Database::escape_for_like($base_name);
+        // Loop through the items for each conflict
+        foreach (db::build()
+                 ->from("items")
+                 ->select("id")
+                 ->where("type", "<>", "album")
+                 ->where("parent_id", "=", $parent_id)
+                 ->where("name", "LIKE", "{$base_name_escaped}.%")
+                 ->limit(1000000)  // required to satisfy SQL syntax (no offset without limit)
+                 ->offset(1)       // skips the 0th item
+                 ->execute() as $row) {
+          set_time_limit(30);
+          $item = ORM::factory("item", $row->id);
+          $item->name = $item->name;  // this will force Item_Model to check for conflicts on save
+          $item->save();
+        }
+      }
+      module::set_version("gallery", $version = 54);
+    }
   }
 
   static function uninstall() {
diff --git a/modules/gallery/helpers/gallery_task.php b/modules/gallery/helpers/gallery_task.php
index 82ca9ffc..e986b822 100644
--- a/modules/gallery/helpers/gallery_task.php
+++ b/modules/gallery/helpers/gallery_task.php
@@ -26,11 +26,13 @@ class gallery_task_Core {
   const FIX_STATE_RUN_DUPE_SLUGS = 5;
   const FIX_STATE_START_DUPE_NAMES = 6;
   const FIX_STATE_RUN_DUPE_NAMES = 7;
-  const FIX_STATE_START_REBUILD_ITEM_CACHES = 8;
-  const FIX_STATE_RUN_REBUILD_ITEM_CACHES = 9;
-  const FIX_STATE_START_MISSING_ACCESS_CACHES = 10;
-  const FIX_STATE_RUN_MISSING_ACCESS_CACHES = 11;
-  const FIX_STATE_DONE = 12;
+  const FIX_STATE_START_DUPE_BASE_NAMES = 8;
+  const FIX_STATE_RUN_DUPE_BASE_NAMES = 9;
+  const FIX_STATE_START_REBUILD_ITEM_CACHES = 10;
+  const FIX_STATE_RUN_REBUILD_ITEM_CACHES = 11;
+  const FIX_STATE_START_MISSING_ACCESS_CACHES = 12;
+  const FIX_STATE_RUN_MISSING_ACCESS_CACHES = 13;
+  const FIX_STATE_DONE = 14;
 
   static function available_tasks() {
     $dirty_count = graphics::find_dirty_images_query()->count_records();
@@ -348,8 +350,9 @@ class gallery_task_Core {
       // album audit (permissions and bogus album covers): 1 operation for every album
       $total += db::build()->where("type", "=", "album")->count_records("items");
 
-      // one operation for each missing slug, name and access cache
-      foreach (array("find_dupe_slugs", "find_dupe_names", "find_missing_access_caches") as $func) {
+      // one operation for each dupe slug, dupe name, dupe base name, and missing access cache
+      foreach (array("find_dupe_slugs", "find_dupe_names", "find_dupe_base_names",
+                     "find_missing_access_caches") as $func) {
         foreach (self::$func() as $row) {
           $total++;
         }
@@ -489,11 +492,12 @@ class gallery_task_Core {
           $task->set("stack", implode(" ", $stack));
           $state = self::FIX_STATE_RUN_DUPE_NAMES;
         } else {
-          $state = self::FIX_STATE_START_ALBUMS;
+          $state = self::FIX_STATE_START_DUPE_BASE_NAMES;
         }
         break;
 
       case self::FIX_STATE_RUN_DUPE_NAMES:
+        // NOTE: This does *not* attempt to fix the file system!
         $stack = explode(" ", $task->get("stack"));
         list ($parent_id, $name) = explode(":", array_pop($stack));
 
@@ -505,9 +509,16 @@ class gallery_task_Core {
           ->find_all(1, 1);
         if ($conflicts->count() && $conflict = $conflicts->current()) {
           $task->log("Fixing conflicting name for item id {$conflict->id}");
+          if (!$conflict->is_album() && preg_match("/^(.*)(\.[^\.\/]*?)$/", $conflict->name, $matches)) {
+            $item_base_name = $matches[1];
+            $item_extension = $matches[2]; // includes a leading dot
+          } else {
+            $item_base_name = $conflict->name;
+            $item_extension = "";
+          }
           db::build()
             ->update("items")
-            ->set("name", $name . "-" . (string)rand(1000, 9999))
+            ->set("name", $item_base_name . "-" . (string)rand(1000, 9999) . $item_extension)
             ->where("id", "=", $conflict->id)
             ->execute();
 
@@ -521,6 +532,74 @@ class gallery_task_Core {
         $task->set("stack", implode(" ", $stack));
         $completed++;
 
+        if (empty($stack)) {
+          $state = self::FIX_STATE_START_DUPE_BASE_NAMES;
+        }
+        break;
+
+      case self::FIX_STATE_START_DUPE_BASE_NAMES:
+        $stack = array();
+        foreach (self::find_dupe_base_names() as $row) {
+          list ($parent_id, $base_name) = explode(":", $row->parent_base_name, 2);
+          $stack[] = join(":", array($parent_id, $base_name));
+        }
+        if ($stack) {
+          $task->set("stack", implode(" ", $stack));
+          $state = self::FIX_STATE_RUN_DUPE_BASE_NAMES;
+        } else {
+          $state = self::FIX_STATE_START_ALBUMS;
+        }
+        break;
+
+      case self::FIX_STATE_RUN_DUPE_BASE_NAMES:
+        // NOTE: This *does* attempt to fix the file system!  So, it must go *after* run_dupe_names.
+        $stack = explode(" ", $task->get("stack"));
+        list ($parent_id, $base_name) = explode(":", array_pop($stack));
+        $base_name_escaped = Database::escape_for_like($base_name);
+
+        $fixed = 0;
+        // We want to leave the first one alone and update all conflicts to be random values.
+        $conflicts = ORM::factory("item")
+          ->where("parent_id", "=", $parent_id)
+          ->where("name", "LIKE", "{$base_name_escaped}.%")
+          ->where("type", "<>", "album")
+          ->find_all(1, 1);
+        if ($conflicts->count() && $conflict = $conflicts->current()) {
+          $task->log("Fixing conflicting name for item id {$conflict->id}");
+          if (preg_match("/^(.*)(\.[^\.\/]*?)$/", $conflict->name, $matches)) {
+            $item_base_name = $matches[1]; // unlike $base_name, this always maintains capitalization
+            $item_extension = $matches[2]; // includes a leading dot
+          } else {
+            $item_base_name = $conflict->name;
+            $item_extension = "";
+          }
+          // Unlike conflicts found in run_dupe_names, these items are likely to have an intact
+          // file system.  Let's use the item save logic to rebuild the paths and rename the files
+          // if possible.
+          try {
+            $conflict->name = $item_base_name . "-" . (string)rand(1000, 9999) . $item_extension;
+            $conflict->validate();
+            // If we get here, we're safe to proceed with save
+            $conflict->save();
+          } catch (Exception $e) {
+            // Didn't work.  Edit database directly without fixing file system.
+            db::build()
+              ->update("items")
+              ->set("name", $item_base_name . "-" . (string)rand(1000, 9999) . $item_extension)
+              ->where("id", "=", $conflict->id)
+              ->execute();
+          }
+
+          // We fixed one conflict, but there might be more so put this parent back on the stack
+          // and try again.  We won't consider it completed when we don't fix a conflict.  This
+          // guarantees that we won't spend too long fixing one set of conflicts, and that we
+          // won't stop before all are fixed.
+          $stack[] = "$parent_id:$base_name";
+          break;
+        }
+        $task->set("stack", implode(" ", $stack));
+        $completed++;
+
         if (empty($stack)) {
           $state = self::FIX_STATE_START_ALBUMS;
         }
@@ -669,18 +748,32 @@ class gallery_task_Core {
   }
 
   static function find_dupe_names() {
+    // looking for photos, movies, and albums
     return db::build()
       ->select_distinct(
         array("parent_name" => db::expr("CONCAT(`parent_id`, ':', LOWER(`name`))")))
       ->select("id")
       ->select(array("C" => "COUNT(\"*\")"))
       ->from("items")
-      ->where("type", "<>", "album")
       ->having("C", ">", 1)
       ->group_by("parent_name")
       ->execute();
   }
 
+  static function find_dupe_base_names() {
+    // looking for photos or movies, not albums
+    return db::build()
+      ->select_distinct(
+        array("parent_base_name" => db::expr("CONCAT(`parent_id`, ':', LOWER(SUBSTR(`name`, 1, LOCATE('.', `name`) - 1)))")))
+      ->select("id")
+      ->select(array("C" => "COUNT(\"*\")"))
+      ->from("items")
+      ->where("type", "<>", "album")
+      ->having("C", ">", 1)
+      ->group_by("parent_base_name")
+      ->execute();
+  }
+
   static function find_empty_item_caches($limit) {
     return db::build()
       ->select("items.id")
diff --git a/modules/gallery/helpers/item.php b/modules/gallery/helpers/item.php
index b2c4cadf..386eeb08 100644
--- a/modules/gallery/helpers/item.php
+++ b/modules/gallery/helpers/item.php
@@ -39,55 +39,28 @@ class item_Core {
       }
     }
 
-    $source->parent_id = $target->id;
-
-    // Moving may result in name or slug conflicts.  If that happens, try up to 5 times to pick a
-    // random name (or slug) to avoid the conflict.
     $orig_name = $source->name;
-    $orig_name_filename = pathinfo($source->name, PATHINFO_FILENAME);
-    $orig_name_extension = pathinfo($source->name, PATHINFO_EXTENSION);
-    $orig_slug = $source->slug;
-    for ($i = 0; $i < 5; $i++) {
-      try {
-        $source->save();
-        if ($orig_name != $source->name) {
-          switch ($source->type) {
-          case "album":
-            message::info(
-              t("Album <b>%old_name</b> renamed to <b>%new_name</b> to avoid a conflict",
-                array("old_name" => $orig_name, "new_name" => $source->name)));
-            break;
-
-          case "photo":
-            message::info(
-              t("Photo <b>%old_name</b> renamed to <b>%new_name</b> to avoid a conflict",
-                array("old_name" => $orig_name, "new_name" => $source->name)));
-            break;
+    $source->parent_id = $target->id;
+    $source->save();
+    if ($orig_name != $source->name) {
+      switch ($source->type) {
+      case "album":
+        message::info(
+          t("Album <b>%old_name</b> renamed to <b>%new_name</b> to avoid a conflict",
+            array("old_name" => $orig_name, "new_name" => $source->name)));
+        break;
 
-          case "movie":
-            message::info(
-              t("Movie <b>%old_name</b> renamed to <b>%new_name</b> to avoid a conflict",
-                array("old_name" => $orig_name, "new_name" => $source->name)));
-            break;
-          }
-        }
+      case "photo":
+        message::info(
+          t("Photo <b>%old_name</b> renamed to <b>%new_name</b> to avoid a conflict",
+            array("old_name" => $orig_name, "new_name" => $source->name)));
         break;
-      } catch (ORM_Validation_Exception $e) {
-        $rand = rand(10, 99);
-        $errors = $e->validation->errors();
-        if (isset($errors["name"])) {
-          $source->name = $orig_name_filename . "-{$rand}." . $orig_name_extension;
-          unset($errors["name"]);
-        }
-        if (isset($errors["slug"])) {
-          $source->slug = $orig_slug . "-{$rand}";
-          unset($errors["slug"]);
-        }
 
-        if ($errors) {
-          // There were other validation issues-- we don't know how to handle those
-          throw $e;
-        }
+      case "movie":
+        message::info(
+          t("Movie <b>%old_name</b> renamed to <b>%new_name</b> to avoid a conflict",
+            array("old_name" => $orig_name, "new_name" => $source->name)));
+        break;
       }
     }
 
diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php
index bbfeee47..e4b997aa 100644
--- a/modules/gallery/models/item.php
+++ b/modules/gallery/models/item.php
@@ -338,10 +338,11 @@ class Item_Model_Core extends ORM_MPTT {
         if (empty($this->slug)) {
           $this->slug = item::convert_filename_to_slug(pathinfo($this->name, PATHINFO_FILENAME));
 
-          // If the filename is all invalid characters, then the slug may be empty here.  Pick a
-          // random value.
+          // If the filename is all invalid characters, then the slug may be empty here.  We set a
+          // generic name ("photo", "movie", or "album") based on its type, then rely on
+          // check_and_fix_conflicts to ensure it doesn't conflict with another name.
           if (empty($this->slug)) {
-            $this->slug = (string)rand(1000, 9999);
+            $this->slug = $this->type;
           }
         }
 
@@ -362,7 +363,7 @@ class Item_Model_Core extends ORM_MPTT {
           }
         }
 
-        $this->_randomize_name_or_slug_on_conflict();
+        $this->_check_and_fix_conflicts();
 
         parent::save();
 
@@ -382,16 +383,6 @@ class Item_Model_Core extends ORM_MPTT {
 
         case "photo":
         case "movie":
-          // The thumb or resize may already exist in the case where a movie and a photo generate
-          // a thumbnail of the same name (eg, foo.flv movie and foo.jpg photo will generate
-          // foo.jpg thumbnail).  If that happens, randomize and save again.
-          if (file_exists($this->resize_path()) ||
-              file_exists($this->thumb_path())) {
-            $pi = pathinfo($this->name);
-            $this->name = $pi["filename"] . "-" . random::int() . "." . $pi["extension"];
-            parent::save();
-          }
-
           copy($this->data_file, $this->file_path());
           break;
         }
@@ -435,7 +426,7 @@ class Item_Model_Core extends ORM_MPTT {
           $this->relative_url_cache = null;
         }
 
-        $this->_randomize_name_or_slug_on_conflict();
+        $this->_check_and_fix_conflicts();
 
         parent::save();
 
@@ -528,30 +519,60 @@ class Item_Model_Core extends ORM_MPTT {
 
   /**
    * Check to see if there's another item that occupies the same name or slug that this item
-   * intends to use, and if so choose a new name/slug while preserving the extension.
-   * @todo Improve this.  Random numbers are not user friendly
+   * intends to use, and if so choose a new name/slug while preserving the extension.  Since this
+   * checks the name without its extension, it covers possible collisions with thumbs and resizes
+   * as well (e.g. between the thumbs of movie "foo.flv" and photo "foo.jpg").
    */
-  private function _randomize_name_or_slug_on_conflict() {
-    $base_name = pathinfo($this->name, PATHINFO_FILENAME);
-    $base_ext = pathinfo($this->name, PATHINFO_EXTENSION);
-    $base_slug = $this->slug;
-    while (ORM::factory("item")
-           ->where("parent_id", "=", $this->parent_id)
-           ->where("id", $this->id ? "<>" : "IS NOT", $this->id)
-           ->and_open()
-           ->where("name", "=", $this->name)
-           ->or_where("slug", "=", $this->slug)
-           ->close()
-           ->find()->id) {
-      $rand = random::int();
-      if ($base_ext) {
-        $this->name = "$base_name-$rand.$base_ext";
+  private function _check_and_fix_conflicts() {
+    $suffix_num = 1;
+    $suffix = "";
+    if ($this->is_album()) {
+      while (db::build()
+             ->from("items")
+             ->where("parent_id", "=", $this->parent_id)
+             ->where("id", $this->id ? "<>" : "IS NOT", $this->id)
+             ->and_open()
+             ->where("name", "=", "{$this->name}{$suffix}")
+             ->or_where("slug", "=", "{$this->slug}{$suffix}")
+             ->close()
+             ->count_records()) {
+        $suffix = "-" . (($suffix_num <= 99) ? sprintf("%02d", $suffix_num++) : random::int());
+      }
+      if ($suffix) {
+        $this->name = "{$this->name}{$suffix}";
+        $this->slug = "{$this->slug}{$suffix}";
+        $this->relative_path_cache = null;
+        $this->relative_url_cache = null;
+      }
+    } else {
+      // Split the filename into its base and extension.  This uses a regexp similar to
+      // legal_file::change_extension (which isn't always the same as pathinfo).
+      if (preg_match("/^(.*)(\.[^\.\/]*?)$/", $this->name, $matches)) {
+        $base_name = $matches[1];
+        $extension = $matches[2]; // includes a leading dot
       } else {
-        $this->name = "$base_name-$rand";
+        $base_name = $this->name;
+        $extension = "";
+      }
+      $base_name_escaped = Database::escape_for_like($base_name);
+      // Note: below query uses LIKE with wildcard % at end, which is still sargable (i.e. quick)
+      while (db::build()
+             ->from("items")
+             ->where("parent_id", "=", $this->parent_id)
+             ->where("id", $this->id ? "<>" : "IS NOT", $this->id)
+             ->and_open()
+             ->where("name", "LIKE", "{$base_name_escaped}{$suffix}.%")
+             ->or_where("slug", "=", "{$this->slug}{$suffix}")
+             ->close()
+             ->count_records()) {
+        $suffix = "-" . (($suffix_num <= 99) ? sprintf("%02d", $suffix_num++) : random::int());
+      }
+      if ($suffix) {
+        $this->name = "{$base_name}{$suffix}{$extension}";
+        $this->slug = "{$this->slug}{$suffix}";
+        $this->relative_path_cache = null;
+        $this->relative_url_cache = null;
       }
-      $this->slug = "$base_slug-$rand";
-      $this->relative_path_cache = null;
-      $this->relative_url_cache = null;
     }
   }
 
@@ -871,14 +892,32 @@ class Item_Model_Core extends ORM_MPTT {
       }
     }
 
-    if (db::build()
-        ->from("items")
-        ->where("parent_id", "=", $this->parent_id)
-        ->where("name", "=", $this->name)
-        ->merge_where($this->id ? array(array("id", "<>", $this->id)) : null)
-        ->count_records()) {
-      $v->add_error("name", "conflict");
-      return;
+    if ($this->is_album()) {
+      if (db::build()
+          ->from("items")
+          ->where("parent_id", "=", $this->parent_id)
+          ->where("name", "=", $this->name)
+          ->merge_where($this->id ? array(array("id", "<>", $this->id)) : null)
+          ->count_records()) {
+        $v->add_error("name", "conflict");
+        return;
+      }
+    } else {
+      if (preg_match("/^(.*)(\.[^\.\/]*?)$/", $this->name, $matches)) {
+        $base_name = $matches[1];
+      } else {
+        $base_name = $this->name;
+      }
+      $base_name_escaped = Database::escape_for_like($base_name);
+      if (db::build()
+          ->from("items")
+          ->where("parent_id", "=", $this->parent_id)
+          ->where("name", "LIKE", "{$base_name_escaped}.%")
+          ->merge_where($this->id ? array(array("id", "<>", $this->id)) : null)
+          ->count_records()) {
+        $v->add_error("name", "conflict");
+        return;
+      }
     }
 
     if ($this->parent_id == 1 && Kohana::auto_load("{$this->slug}_Controller")) {
diff --git a/modules/gallery/module.info b/modules/gallery/module.info
index 566ca2eb..b6cefe39 100644
--- a/modules/gallery/module.info
+++ b/modules/gallery/module.info
@@ -1,6 +1,6 @@
 name = "Gallery 3"
 description = "Gallery core application"
-version = 53
+version = 54
 author_name = "Gallery Team"
 author_url = "http://codex.galleryproject.org/Gallery:Team"
 info_url = "http://codex.galleryproject.org/Gallery3:Modules:gallery"
diff --git a/modules/gallery/tests/Item_Model_Test.php b/modules/gallery/tests/Item_Model_Test.php
index c45bbc3c..41361b32 100644
--- a/modules/gallery/tests/Item_Model_Test.php
+++ b/modules/gallery/tests/Item_Model_Test.php
@@ -136,19 +136,6 @@ class Item_Model_Test extends Gallery_Unit_Test_Case {
     $this->assert_true(false, "Shouldn't get here");
   }
 
-  public function item_rename_over_existing_name_gets_uniqified_test() {
-    // Create a test photo
-    $item = test::random_photo();
-    $item2 = test::random_photo();
-
-    $item->name = $item2->name;
-    $item->save();
-
-    // foo.jpg should become foo-####.jpg
-    $this->assert_true(
-      preg_match("/" . str_replace(".jpg", "", $item2->name) . "-\d+\.jpg/", $item->name));
-  }
-
   public function move_album_test() {
     $album2 = test::random_album();
     $album1 = test::random_album($album2);
@@ -205,7 +192,7 @@ class Item_Model_Test extends Gallery_Unit_Test_Case {
     $this->assert_equal($fullsize_file, file_get_contents($photo->file_path()));
   }
 
-  public function move_album_with_conflicting_target_gets_uniqified_test() {
+  public function move_album_with_conflicting_target_gets_uniquified_test() {
     $album = test::random_album();
     $source = test::random_album_unsaved($album);
     $source->name = $album->name;
@@ -217,9 +204,9 @@ class Item_Model_Test extends Gallery_Unit_Test_Case {
     $source->parent_id = item::root()->id;
     $source->save();
 
-    // foo should become foo-####
-    $this->assert_true(preg_match("/{$album->name}-\d+/", $source->name));
-    $this->assert_true(preg_match("/{$album->slug}-\d+/", $source->slug));
+    // foo should become foo-01
+    $this->assert_same("{$album->name}-01", $source->name);
+    $this->assert_same("{$album->slug}-01", $source->slug);
   }
 
   public function move_album_fails_wrong_target_type_test() {
@@ -239,7 +226,7 @@ class Item_Model_Test extends Gallery_Unit_Test_Case {
     $this->assert_true(false, "Shouldn't get here");
   }
 
-  public function move_photo_with_conflicting_target_gets_uniqified_test() {
+  public function move_photo_with_conflicting_target_gets_uniquified_test() {
     $photo1 = test::random_photo();
     $album = test::random_album();
     $photo2 = test::random_photo_unsaved($album);
@@ -247,17 +234,16 @@ class Item_Model_Test extends Gallery_Unit_Test_Case {
     $photo2->save();
 
     // $photo1 and $photo2 have the same name, so if we move $photo1 into the root they should
-    // conflict and get uniqified.
+    // conflict and get uniquified.
 
     $photo2->parent_id = item::root()->id;
     $photo2->save();
 
-    // foo.jpg should become foo-####.jpg
-    $this->assert_true(
-      preg_match("/" . str_replace(".jpg", "", $photo1->name) . "-\d+\.jpg/", $photo2->name));
+    // foo.jpg should become foo-01.jpg
+    $this->assert_same(pathinfo($photo1->name, PATHINFO_FILENAME) . "-01.jpg", $photo2->name);
 
-    // foo should become foo
-    $this->assert_true(preg_match("/{$photo1->slug}/", $photo2->name));
+    // foo should become foo-01
+    $this->assert_same("{$photo1->slug}-01", $photo2->slug);
   }
 
   public function move_album_inside_descendent_fails_test() {
@@ -535,4 +521,164 @@ class Item_Model_Test extends Gallery_Unit_Test_Case {
     $album->name = $album->name . ".foo.bar";
     $album->save();
   }
+
+  public function no_conflict_when_parents_different_test() {
+    $parent1 = test::random_album();
+    $parent2 = test::random_album();
+    $photo1 = test::random_photo($parent1);
+    $photo2 = test::random_photo($parent2);
+
+    $photo2->name = $photo1->name;
+    $photo2->slug = $photo1->slug;
+    $photo2->save();
+
+    // photo2 has same name and slug as photo1 but different parent - no conflict.
+    $this->assert_same($photo1->name, $photo2->name);
+    $this->assert_same($photo1->slug, $photo2->slug);
+  }
+
+  public function fix_conflict_when_names_identical_test() {
+    $parent = test::random_album();
+    $photo1 = test::random_photo($parent);
+    $photo2 = test::random_photo($parent);
+
+    $photo1_orig_base = pathinfo($photo1->name, PATHINFO_FILENAME);
+    $photo2_orig_slug = $photo2->slug;
+
+    $photo2->name = $photo1->name;
+    $photo2->save();
+
+    // photo2 has same name as photo1 - conflict resolved by renaming with -01.
+    $this->assert_same("{$photo1_orig_base}-01.jpg", $photo2->name);
+    $this->assert_same("{$photo2_orig_slug}-01", $photo2->slug);
+  }
+
+  public function fix_conflict_when_slugs_identical_test() {
+    $parent = test::random_album();
+    $photo1 = test::random_photo($parent);
+    $photo2 = test::random_photo($parent);
+
+    $photo2_orig_base = pathinfo($photo2->name, PATHINFO_FILENAME);
+
+    $photo2->slug = $photo1->slug;
+    $photo2->save();
+
+    // photo2 has same slug as photo1 - conflict resolved by renaming with -01.
+    $this->assert_same("{$photo2_orig_base}-01.jpg", $photo2->name);
+    $this->assert_same("{$photo1->slug}-01", $photo2->slug);
+  }
+
+  public function no_conflict_when_parents_different_for_albums_test() {
+    $parent1 = test::random_album();
+    $parent2 = test::random_album();
+    $album1 = test::random_album($parent1);
+    $album2 = test::random_album($parent2);
+
+    $album2->name = $album1->name;
+    $album2->slug = $album1->slug;
+    $album2->save();
+
+    // album2 has same name and slug as album1 but different parent - no conflict.
+    $this->assert_same($album1->name, $album2->name);
+    $this->assert_same($album1->slug, $album2->slug);
+  }
+
+  public function fix_conflict_when_names_identical_for_albums_test() {
+    $parent = test::random_album();
+    $album1 = test::random_album($parent);
+    $album2 = test::random_album($parent);
+
+    $album2_orig_slug = $album2->slug;
+
+    $album2->name = $album1->name;
+    $album2->save();
+
+    // album2 has same name as album1 - conflict resolved by renaming with -01.
+    $this->assert_same("{$album1->name}-01", $album2->name);
+    $this->assert_same("{$album2_orig_slug}-01", $album2->slug);
+  }
+
+  public function fix_conflict_when_slugs_identical_for_albums_test() {
+    $parent = test::random_album();
+    $album1 = test::random_album($parent);
+    $album2 = test::random_album($parent);
+
+    $album2_orig_name = $album2->name;
+
+    $album2->slug = $album1->slug;
+    $album2->save();
+
+    // album2 has same slug as album1 - conflict resolved by renaming with -01.
+    $this->assert_same("{$album2_orig_name}-01", $album2->name);
+    $this->assert_same("{$album1->slug}-01", $album2->slug);
+  }
+
+  public function no_conflict_when_base_names_identical_between_album_and_photo_test() {
+    $parent = test::random_album();
+    $album = test::random_album($parent);
+    $photo = test::random_photo($parent);
+
+    $photo_orig_slug = $photo->slug;
+
+    $photo->name = "{$album->name}.jpg";
+    $photo->save();
+
+    // photo has same base name as album - no conflict.
+    $this->assert_same("{$album->name}.jpg", $photo->name);
+    $this->assert_same($photo_orig_slug, $photo->slug);
+  }
+
+  public function fix_conflict_when_full_names_identical_between_album_and_photo_test() {
+    $parent = test::random_album();
+    $photo = test::random_photo($parent);
+    $album = test::random_album($parent);
+
+    $album_orig_slug = $album->slug;
+
+    $album->name = $photo->name;
+    $album->save();
+
+    // album has same full name as album - conflict resolved by renaming with -01.
+    $this->assert_same("{$photo->name}-01", $album->name);
+    $this->assert_same("{$album_orig_slug}-01", $album->slug);
+  }
+
+  public function fix_conflict_when_slugs_identical_between_album_and_photo_test() {
+    $parent = test::random_album();
+    $album = test::random_album($parent);
+    $photo = test::random_photo($parent);
+
+    $photo_orig_base = pathinfo($photo->name, PATHINFO_FILENAME);
+
+    $photo->slug = $album->slug;
+    $photo->save();
+
+    // photo has same slug as album - conflict resolved by renaming with -01.
+    $this->assert_same("{$photo_orig_base}-01.jpg", $photo->name);
+    $this->assert_same("{$album->slug}-01", $photo->slug);
+  }
+
+  public function fix_conflict_when_base_names_identical_between_jpg_png_flv_test() {
+    $parent = test::random_album();
+    $item1 = test::random_photo($parent);
+    $item2 = test::random_photo($parent);
+    $item3 = test::random_movie($parent);
+
+    $item1_orig_base = pathinfo($item1->name, PATHINFO_FILENAME);
+    $item2_orig_slug = $item2->slug;
+    $item3_orig_slug = $item3->slug;
+
+    $item2->set_data_file(MODPATH . "gallery/images/graphicsmagick.png");
+    $item2->name = "{$item1_orig_base}.png";
+    $item2->save();
+
+    $item3->name = "{$item1_orig_base}.flv";
+    $item3->save();
+
+    // item2 and item3 have same base name as item1 - conflict resolved by renaming with -01 and -02.
+    $this->assert_same("{$item1_orig_base}-01.png", $item2->name);
+    $this->assert_same("{$item2_orig_slug}-01", $item2->slug);
+    $this->assert_same("{$item1_orig_base}-02.flv", $item3->name);
+    $this->assert_same("{$item3_orig_slug}-02", $item3->slug);
+  }
 }
-- 
cgit v1.2.3


From 5fca371a616dba16f955087c4477ee229ee222d0 Mon Sep 17 00:00:00 2001
From: shadlaws <shad@shadlaws.com>
Date: Mon, 28 Jan 2013 23:31:18 +0100
Subject: #1945 - Extend legal_file helper functions. - Added
 get_types_by_extension function, which is a merged version of
 get...types_by_extension functions (similar to get_extensions). - Added
 optional extension argument to get...extensions functions similar to
 get...types_by_extension functions. - Added unit tests.  Now, every
 legal_file function has one. - Restructured helper file to include caches. -
 Added array_unique to get...types (derived from get...types_by_extension,
 which can be many-to-one). - Edited server_add, uploader, and item model to
 use new functionality.

---
 modules/gallery/controllers/uploader.php         |   2 +-
 modules/gallery/helpers/legal_file.php           | 146 +++++++++++++++++------
 modules/gallery/models/item.php                  |  13 +-
 modules/gallery/tests/Legal_File_Helper_Test.php |  57 +++++++++
 modules/server_add/controllers/server_add.php    |   8 +-
 5 files changed, 176 insertions(+), 50 deletions(-)

(limited to 'modules/gallery/models/item.php')

diff --git a/modules/gallery/controllers/uploader.php b/modules/gallery/controllers/uploader.php
index b2ec5b55..55c65c95 100644
--- a/modules/gallery/controllers/uploader.php
+++ b/modules/gallery/controllers/uploader.php
@@ -69,7 +69,7 @@ class Uploader_Controller extends Controller {
 
         $path_info = @pathinfo($temp_filename);
         if (array_key_exists("extension", $path_info) &&
-            in_array(strtolower($path_info["extension"]), legal_file::get_movie_extensions())) {
+            legal_file::get_movie_extensions($path_info["extension"])) {
           $item->type = "movie";
           $item->save();
           log::success("content", t("Added a movie"),
diff --git a/modules/gallery/helpers/legal_file.php b/modules/gallery/helpers/legal_file.php
index 5768cf14..ab9047c8 100644
--- a/modules/gallery/helpers/legal_file.php
+++ b/modules/gallery/helpers/legal_file.php
@@ -18,6 +18,13 @@
  * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA  02110-1301, USA.
  */
 class legal_file_Core {
+  private static $photo_types_by_extension;
+  private static $movie_types_by_extension;
+  private static $photo_extensions;
+  private static $movie_extensions;
+  private static $photo_types;
+  private static $movie_types;
+
   /**
    * Create a default list of allowed photo MIME types paired with their extensions and then let
    * modules modify it.  This is an ordered map, mapping extensions to their MIME types.
@@ -26,21 +33,24 @@ class legal_file_Core {
    * @param string $extension (opt.) - return MIME of extension; if not given, return complete array
    */
   static function get_photo_types_by_extension($extension=null) {
-    $types_by_extension_wrapper = new stdClass();
-    $types_by_extension_wrapper->types_by_extension = array(
-      "jpg" => "image/jpeg", "jpeg" => "image/jpeg", "gif" => "image/gif", "png" => "image/png");
-    module::event("photo_types_by_extension", $types_by_extension_wrapper);
+    if (empty(self::$photo_types_by_extension)) {
+      $types_by_extension_wrapper = new stdClass();
+      $types_by_extension_wrapper->types_by_extension = array(
+        "jpg" => "image/jpeg", "jpeg" => "image/jpeg", "gif" => "image/gif", "png" => "image/png");
+      module::event("photo_types_by_extension", $types_by_extension_wrapper);
+      self::$photo_types_by_extension = $types_by_extension_wrapper->types_by_extension;
+    }
     if ($extension) {
       // return matching MIME type
       $extension = strtolower($extension);
-      if (isset($types_by_extension_wrapper->types_by_extension[$extension])) {
-        return $types_by_extension_wrapper->types_by_extension[$extension];
+      if (isset(self::$photo_types_by_extension[$extension])) {
+        return self::$photo_types_by_extension[$extension];
       } else {
         return null;
       }
     } else {
       // return complete array
-      return $types_by_extension_wrapper->types_by_extension;
+      return self::$photo_types_by_extension;
     }
   }
 
@@ -52,53 +62,111 @@ class legal_file_Core {
    * @param string $extension (opt.) - return MIME of extension; if not given, return complete array
    */
   static function get_movie_types_by_extension($extension=null) {
-    $types_by_extension_wrapper = new stdClass();
-    $types_by_extension_wrapper->types_by_extension = array(
-      "flv" => "video/x-flv", "mp4" => "video/mp4", "m4v" => "video/x-m4v");
-    module::event("movie_types_by_extension", $types_by_extension_wrapper);
+    if (empty(self::$movie_types_by_extension)) {
+      $types_by_extension_wrapper = new stdClass();
+      $types_by_extension_wrapper->types_by_extension = array(
+        "flv" => "video/x-flv", "mp4" => "video/mp4", "m4v" => "video/x-m4v");
+      module::event("movie_types_by_extension", $types_by_extension_wrapper);
+      self::$movie_types_by_extension = $types_by_extension_wrapper->types_by_extension;
+    }
+    if ($extension) {
+      // return matching MIME type
+      $extension = strtolower($extension);
+      if (isset(self::$movie_types_by_extension[$extension])) {
+        return self::$movie_types_by_extension[$extension];
+      } else {
+        return null;
+      }
+    } else {
+      // return complete array
+      return self::$movie_types_by_extension;
+    }
+  }
+
+  /**
+   * Create a merged list of all allowed photo and movie MIME types paired with their extensions.
+   *
+   * @param string $extension (opt.) - return MIME of extension; if not given, return complete array
+   */
+  static function get_types_by_extension($extension=null) {
+    $types_by_extension = legal_file::get_photo_types_by_extension();
+    if (movie::find_ffmpeg()) {
+      $types_by_extension = array_merge($types_by_extension,
+                                        legal_file::get_movie_types_by_extension());
+    }
     if ($extension) {
       // return matching MIME type
       $extension = strtolower($extension);
-      if (isset($types_by_extension_wrapper->types_by_extension[$extension])) {
-        return $types_by_extension_wrapper->types_by_extension[$extension];
+      if (isset($types_by_extension[$extension])) {
+        return $types_by_extension[$extension];
       } else {
         return null;
       }
     } else {
       // return complete array
-      return $types_by_extension_wrapper->types_by_extension;
+      return $types_by_extension;
     }
   }
 
   /**
    * Create a default list of allowed photo extensions and then let modules modify it.
+   *
+   * @param string $extension (opt.) - return true if allowed; if not given, return complete array
    */
-  static function get_photo_extensions() {
-    $extensions_wrapper = new stdClass();
-    $extensions_wrapper->extensions = array_keys(legal_file::get_photo_types_by_extension());
-    module::event("legal_photo_extensions", $extensions_wrapper);
-    return $extensions_wrapper->extensions;
+  static function get_photo_extensions($extension=null) {
+    if (empty(self::$photo_extensions)) {
+      $extensions_wrapper = new stdClass();
+      $extensions_wrapper->extensions = array_keys(legal_file::get_photo_types_by_extension());
+      module::event("legal_photo_extensions", $extensions_wrapper);
+      self::$photo_extensions = $extensions_wrapper->extensions;
+    }
+    if ($extension) {
+      // return true if in array, false if not
+      return in_array(strtolower($extension), self::$photo_extensions);
+    } else {
+      // return complete array
+      return self::$photo_extensions;
+    }
   }
 
   /**
    * Create a default list of allowed movie extensions and then let modules modify it.
+   *
+   * @param string $extension (opt.) - return true if allowed; if not given, return complete array
    */
-  static function get_movie_extensions() {
-    $extensions_wrapper = new stdClass();
-    $extensions_wrapper->extensions = array_keys(legal_file::get_movie_types_by_extension());
-    module::event("legal_movie_extensions", $extensions_wrapper);
-    return $extensions_wrapper->extensions;
+  static function get_movie_extensions($extension=null) {
+    if (empty(self::$movie_extensions)) {
+      $extensions_wrapper = new stdClass();
+      $extensions_wrapper->extensions = array_keys(legal_file::get_movie_types_by_extension());
+      module::event("legal_movie_extensions", $extensions_wrapper);
+      self::$movie_extensions = $extensions_wrapper->extensions;
+    }
+    if ($extension) {
+      // return true if in array, false if not
+      return in_array(strtolower($extension), self::$movie_extensions);
+    } else {
+      // return complete array
+      return self::$movie_extensions;
+    }
   }
 
   /**
    * Create a merged list of all allowed photo and movie extensions.
+   *
+   * @param string $extension (opt.) - return true if allowed; if not given, return complete array
    */
-  static function get_extensions() {
+  static function get_extensions($extension=null) {
     $extensions = legal_file::get_photo_extensions();
     if (movie::find_ffmpeg()) {
       $extensions = array_merge($extensions, legal_file::get_movie_extensions());
     }
-    return $extensions;
+    if ($extension) {
+      // return true if in array, false if not
+      return in_array(strtolower($extension), $extensions);
+    } else {
+      // return complete array
+      return $extensions;
+    }
   }
 
   /**
@@ -119,10 +187,14 @@ class legal_file_Core {
    * (e.g. flv maps to video/x-flv by default, but video/flv is still legal).
    */
   static function get_photo_types() {
-    $types_wrapper = new stdClass();
-    $types_wrapper->types = array_values(legal_file::get_photo_types_by_extension());
-    module::event("legal_photo_types", $types_wrapper);
-    return $types_wrapper->types;
+    if (empty(self::$photo_types)) {
+      $types_wrapper = new stdClass();
+      // Need array_unique since types_by_extension can be many-to-one (e.g. jpeg and jpg).
+      $types_wrapper->types = array_unique(array_values(legal_file::get_photo_types_by_extension()));
+      module::event("legal_photo_types", $types_wrapper);
+      self::$photo_types = $types_wrapper->types;
+    }
+    return self::$photo_types;
   }
 
   /**
@@ -131,11 +203,15 @@ class legal_file_Core {
    * (e.g. flv maps to video/x-flv by default, but video/flv is still legal).
    */
   static function get_movie_types() {
-    $types_wrapper = new stdClass();
-    $types_wrapper->types = array_values(legal_file::get_movie_types_by_extension());
-    $types_wrapper->types[] = "video/flv";
-    module::event("legal_movie_types", $types_wrapper);
-    return $types_wrapper->types;
+    if (empty(self::$movie_types)) {
+      $types_wrapper = new stdClass();
+      // Need array_unique since types_by_extension can be many-to-one (e.g. jpeg and jpg).
+      $types_wrapper->types = array_unique(array_values(legal_file::get_movie_types_by_extension()));
+      $types_wrapper->types[] = "video/flv";
+      module::event("legal_movie_types", $types_wrapper);
+      self::$movie_types = $types_wrapper->types;
+    }
+    return self::$movie_types;
   }
 
   /**
diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php
index e4b997aa..60318c26 100644
--- a/modules/gallery/models/item.php
+++ b/modules/gallery/models/item.php
@@ -879,16 +879,9 @@ class Item_Model_Core extends ORM_MPTT {
         return;
       }
 
-      if ($this->is_photo()) {
-        if (!in_array(strtolower($ext), legal_file::get_photo_extensions())) {
-          $v->add_error("name", "illegal_data_file_extension");
-        }
-      }
-
-      if ($this->is_movie()) {
-        if (!in_array(strtolower($ext), legal_file::get_movie_extensions())) {
-          $v->add_error("name", "illegal_data_file_extension");
-        }
+      if ($this->is_photo() && !legal_file::get_photo_extensions($ext) ||
+          $this->is_movie() && !legal_file::get_movie_extensions($ext)) {
+        $v->add_error("name", "illegal_data_file_extension");
       }
     }
 
diff --git a/modules/gallery/tests/Legal_File_Helper_Test.php b/modules/gallery/tests/Legal_File_Helper_Test.php
index 5db99935..84a29a52 100644
--- a/modules/gallery/tests/Legal_File_Helper_Test.php
+++ b/modules/gallery/tests/Legal_File_Helper_Test.php
@@ -40,6 +40,63 @@ class Legal_File_Helper_Test extends Gallery_Unit_Test_Case {
     $this->assert_equal(3, count(legal_file::get_movie_types_by_extension()));
   }
 
+  public function get_types_by_extension_test() {
+    $this->assert_equal("image/jpeg", legal_file::get_types_by_extension("jpg"));  // photo
+    $this->assert_equal("video/x-flv", legal_file::get_types_by_extension("FLV")); // movie
+    $this->assert_equal(null, legal_file::get_types_by_extension("php"));          // invalid
+    $this->assert_equal(null, legal_file::get_types_by_extension("php.flv"));      // invalid w/ .
+
+    // No extension returns full array
+    $this->assert_equal(7, count(legal_file::get_types_by_extension()));
+  }
+
+  public function get_photo_extensions_test() {
+    $this->assert_equal(true, legal_file::get_photo_extensions("jpg"));      // regular
+    $this->assert_equal(true, legal_file::get_photo_extensions("JPG"));      // all caps
+    $this->assert_equal(true, legal_file::get_photo_extensions("Png"));      // some caps
+    $this->assert_equal(false, legal_file::get_photo_extensions("php"));     // invalid
+    $this->assert_equal(false, legal_file::get_photo_extensions("php.jpg")); // invalid w/ .
+
+    // No extension returns full array
+    $this->assert_equal(4, count(legal_file::get_photo_extensions()));
+  }
+
+  public function get_movie_extensions_test() {
+    $this->assert_equal(true, legal_file::get_movie_extensions("flv"));      // regular
+    $this->assert_equal(true, legal_file::get_movie_extensions("FLV"));      // all caps
+    $this->assert_equal(true, legal_file::get_movie_extensions("Mp4"));      // some caps
+    $this->assert_equal(false, legal_file::get_movie_extensions("php"));     // invalid
+    $this->assert_equal(false, legal_file::get_movie_extensions("php.jpg")); // invalid w/ .
+
+    // No extension returns full array
+    $this->assert_equal(3, count(legal_file::get_movie_extensions()));
+  }
+
+  public function get_extensions_test() {
+    $this->assert_equal(true, legal_file::get_extensions("jpg"));      // photo
+    $this->assert_equal(true, legal_file::get_extensions("FLV"));      // movie
+    $this->assert_equal(false, legal_file::get_extensions("php"));     // invalid
+    $this->assert_equal(false, legal_file::get_extensions("php.jpg")); // invalid w/ .
+
+    // No extension returns full array
+    $this->assert_equal(7, count(legal_file::get_extensions()));
+  }
+
+  public function get_filters_test() {
+    // All 7 extensions both uppercase and lowercase
+    $this->assert_equal(14, count(legal_file::get_filters()));
+  }
+
+  public function get_photo_types_test() {
+    // Note that this is one *less* than photo extensions since jpeg and jpg have the same mime.
+    $this->assert_equal(3, count(legal_file::get_photo_types()));
+  }
+
+  public function get_movie_types_test() {
+    // Note that this is one *more* than movie extensions since video/flv is added.
+    $this->assert_equal(4, count(legal_file::get_movie_types()));
+  }
+
   public function change_extension_test() {
     $this->assert_equal("foo.jpg", legal_file::change_extension("foo.png", "jpg"));
   }
diff --git a/modules/server_add/controllers/server_add.php b/modules/server_add/controllers/server_add.php
index df55d8db..f6e0a944 100644
--- a/modules/server_add/controllers/server_add.php
+++ b/modules/server_add/controllers/server_add.php
@@ -61,7 +61,7 @@ class Server_Add_Controller extends Admin_Controller {
         }
         if (!is_dir($file)) {
           $ext = strtolower(pathinfo($file, PATHINFO_EXTENSION));
-          if (!in_array($ext, legal_file::get_extensions())) {
+          if (!legal_file::get_extensions($ext)) {
             continue;
           }
         }
@@ -169,7 +169,7 @@ class Server_Add_Controller extends Admin_Controller {
           foreach ($child_paths as $child_path) {
             if (!is_dir($child_path)) {
               $ext = strtolower(pathinfo($child_path, PATHINFO_EXTENSION));
-              if (!in_array($ext, legal_file::get_extensions()) || !filesize($child_path)) {
+              if (!legal_file::get_extensions($ext) || !filesize($child_path)) {
                 // Not importable, skip it.
                 continue;
               }
@@ -255,7 +255,7 @@ class Server_Add_Controller extends Admin_Controller {
         } else {
           try {
             $extension = strtolower(pathinfo($name, PATHINFO_EXTENSION));
-            if (in_array($extension, legal_file::get_photo_extensions())) {
+            if (legal_file::get_photo_extensions($extension)) {
               $photo = ORM::factory("item");
               $photo->type = "photo";
               $photo->parent_id = $parent->id;
@@ -265,7 +265,7 @@ class Server_Add_Controller extends Admin_Controller {
               $photo->owner_id = $owner_id;
               $photo->save();
               $entry->item_id = $photo->id;
-            } else if (in_array($extension, legal_file::get_movie_extensions())) {
+            } else if (legal_file::get_movie_extensions($extension)) {
               $movie = ORM::factory("item");
               $movie->type = "movie";
               $movie->parent_id = $parent->id;
-- 
cgit v1.2.3


From cf077425953a6a492dea97eaf1517e7d08c9648f Mon Sep 17 00:00:00 2001
From: shadlaws <shad@shadlaws.com>
Date: Wed, 30 Jan 2013 01:07:36 +0100
Subject: #1968 - Improve album cover generation/removal/etc. - Added stanza to
 Item_Model::save that handles when cover id is null. - Added logic to
 graphics::generate to copy/convert album cover thumbs from their item thumbs
 to ensure they're always jpg, and eliminate the possibility that we
 copy/convert a dirty thumb. - Redirected other places in code where we want
 to do one of the above two things to use these two functions instead
 (gallery_event::item_updated_data_file, item::make_album_cover,
 item::remove_album_cover). - Improved validation in Item_Model so only albums
 can have covers and all covers must be non-albums. - Added unit tests to
 Graphics_Helper_Test.

---
 modules/gallery/helpers/gallery_event.php      |  5 +-
 modules/gallery/helpers/graphics.php           | 78 ++++++++++++++------------
 modules/gallery/helpers/item.php               | 15 +----
 modules/gallery/models/item.php                | 15 ++++-
 modules/gallery/tests/Graphics_Helper_Test.php | 58 +++++++++++++++++++
 5 files changed, 117 insertions(+), 54 deletions(-)

(limited to 'modules/gallery/models/item.php')

diff --git a/modules/gallery/helpers/gallery_event.php b/modules/gallery/helpers/gallery_event.php
index bd9acc1c..ee3c51cc 100644
--- a/modules/gallery/helpers/gallery_event.php
+++ b/modules/gallery/helpers/gallery_event.php
@@ -141,10 +141,9 @@ class gallery_event_Core {
     foreach (ORM::factory("item")
              ->where("album_cover_item_id", "=", $item->id)
              ->find_all() as $target) {
-      copy($item->thumb_path(), $target->thumb_path());
-      $target->thumb_width = $item->thumb_width;
-      $target->thumb_height = $item->thumb_height;
+      $target->thumb_dirty = 1;
       $target->save();
+      graphics::generate($target);
     }
   }
 
diff --git a/modules/gallery/helpers/graphics.php b/modules/gallery/helpers/graphics.php
index 3f5e2d56..19ae1036 100644
--- a/modules/gallery/helpers/graphics.php
+++ b/modules/gallery/helpers/graphics.php
@@ -115,36 +115,12 @@ class graphics_Core {
    * @param Item_Model $item
    */
   static function generate($item) {
-    if ($item->is_album()) {
-      if (!$cover = $item->album_cover()) {
-        // This album has no cover; there's nothing to generate.  Because of an old bug, it's
-        // possible that there's an album cover item id that points to an invalid item.  In that
-        // case, just null out the album cover item id.  It's not optimal to do that at this low
-        // level, but it's not trivial to find these cases quickly in an upgrade script and if we
-        // don't do this, the album may be permanently marked as "needs rebuilding"
-        //
-        // ref: http://sourceforge.net/apps/trac/gallery/ticket/1172
-        //      http://galleryproject.org/node/96926
-        if ($item->album_cover_item_id) {
-          $item->album_cover_item_id = null;
-          $item->save();
-        }
-        return;
-      }
-      $input_file = $cover->file_path();
-      $input_item = $cover;
-    } else {
-      $input_file = $item->file_path();
-      $input_item = $item;
-    }
-
     if ($item->thumb_dirty) {
       $ops["thumb"] = $item->thumb_path();
     }
-    if ($item->resize_dirty && !$item->is_album() && !$item->is_movie()) {
+    if ($item->resize_dirty && $item->is_photo()) {
       $ops["resize"] = $item->resize_path();
     }
-
     if (empty($ops)) {
       $item->thumb_dirty = 0;
       $item->resize_dirty = 0;
@@ -154,9 +130,11 @@ class graphics_Core {
 
     try {
       foreach ($ops as $target => $output_file) {
+        $working_file = $item->file_path();
         // Delete anything that might already be there
         @unlink($output_file);
-        if ($input_item->is_movie()) {
+        switch ($item->type) {
+        case "movie":
           // Run movie_extract_frame events, which can either:
           //  - generate an output file, bypassing the ffmpeg-based movie::extract_frame
           //  - add to the options sent to movie::extract_frame (e.g. change frame extract time,
@@ -164,27 +142,55 @@ class graphics_Core {
           // Note that the args are similar to those of the events in gallery_graphics
           $movie_options_wrapper = new stdClass();
           $movie_options_wrapper->movie_options = array();
-          module::event("movie_extract_frame", $input_file, $output_file,
-                        $movie_options_wrapper, $input_item);
+          module::event("movie_extract_frame", $working_file, $output_file,
+                        $movie_options_wrapper, $item);
           // If no output_file generated by events, run movie::extract_frame with movie_options
           clearstatcache();
           if (@filesize($output_file) == 0) {
             try {
-              movie::extract_frame($input_file, $output_file, $movie_options_wrapper->movie_options);
+              movie::extract_frame($working_file, $output_file, $movie_options_wrapper->movie_options);
             } catch (Exception $e) {
               // Didn't work, likely because of MISSING_FFMPEG - use placeholder
               graphics::_replace_image_with_placeholder($item, $target);
             }
           }
           $working_file = $output_file;
-        } else {
-          $working_file = $input_file;
-        }
 
-        foreach (self::_get_rules($target) as $rule) {
-          $args = array($working_file, $output_file, unserialize($rule->args), $item);
-          call_user_func_array($rule->operation, $args);
-          $working_file = $output_file;
+        case "photo":
+          // Run the graphics rules (for both movies and photos)
+          foreach (self::_get_rules($target) as $rule) {
+            $args = array($working_file, $output_file, unserialize($rule->args), $item);
+            call_user_func_array($rule->operation, $args);
+            $working_file = $output_file;
+          }
+          break;
+
+        case "album":
+          if (!$cover = $item->album_cover()) {
+            // This album has no cover; there's nothing to generate.  Because of an old bug, it's
+            // possible that there's an album cover item id that points to an invalid item.  In that
+            // case, just null out the album cover item id.  It's not optimal to do that at this low
+            // level, but it's not trivial to find these cases quickly in an upgrade script and if we
+            // don't do this, the album may be permanently marked as "needs rebuilding"
+            //
+            // ref: http://sourceforge.net/apps/trac/gallery/ticket/1172
+            //      http://galleryproject.org/node/96926
+            if ($item->album_cover_item_id) {
+              $item->album_cover_item_id = null;
+              $item->save();
+            }
+            return;
+          }
+          if ($cover->thumb_dirty) {
+            graphics::generate($cover);
+          }
+          if (!$cover->thumb_dirty) {
+            // Make the album cover from the cover item's thumb.  Run gallery_graphics::resize with
+            // null options and it will figure out if this is a direct copy or conversion to jpg.
+            $working_file = $cover->thumb_path();
+            gallery_graphics::resize($working_file, $output_file, null, $item);
+          }
+          break;
         }
       }
 
diff --git a/modules/gallery/helpers/item.php b/modules/gallery/helpers/item.php
index 386eeb08..093feb2d 100644
--- a/modules/gallery/helpers/item.php
+++ b/modules/gallery/helpers/item.php
@@ -78,15 +78,9 @@ class item_Core {
 
     model_cache::clear();
     $parent->album_cover_item_id = $item->is_album() ? $item->album_cover_item_id : $item->id;
-    if ($item->thumb_dirty) {
-      $parent->thumb_dirty = 1;
-      graphics::generate($parent);
-    } else {
-      copy($item->thumb_path(), $parent->thumb_path());
-      $parent->thumb_width = $item->thumb_width;
-      $parent->thumb_height = $item->thumb_height;
-    }
     $parent->save();
+    graphics::generate($parent);
+
     $grand_parent = $parent->parent();
     if ($grand_parent && access::can("edit", $grand_parent) &&
         $grand_parent->album_cover_item_id == null)  {
@@ -97,15 +91,10 @@ class item_Core {
   static function remove_album_cover($album) {
     access::required("view", $album);
     access::required("edit", $album);
-    @unlink($album->thumb_path());
 
     model_cache::clear();
     $album->album_cover_item_id = null;
-    $album->thumb_width = 0;
-    $album->thumb_height = 0;
-    $album->thumb_dirty = 1;
     $album->save();
-    graphics::generate($album);
   }
 
   /**
diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php
index 60318c26..f9edd3c6 100644
--- a/modules/gallery/models/item.php
+++ b/modules/gallery/models/item.php
@@ -420,6 +420,15 @@ class Item_Model_Core extends ORM_MPTT {
           }
         }
 
+        // If an album's cover has changed (or been removed), delete any existing album cover,
+        // reset the thumb metadata, and mark the thumb as dirty.
+        if (array_key_exists("album_cover_item_id", $this->changed) && $this->is_album()) {
+          @unlink($original->thumb_path());
+          $this->thumb_dirty = 1;
+          $this->thumb_height = 0;
+          $this->thumb_width = 0;
+        }
+
         if (array_intersect($this->changed, array("parent_id", "name", "slug"))) {
           $original->_build_relative_caches();
           $this->relative_path_cache = null;
@@ -966,10 +975,12 @@ class Item_Model_Core extends ORM_MPTT {
       return;
     }
 
-    if ($this->album_cover_item_id && db::build()
+    if ($this->album_cover_item_id && ($this->is_photo() || $this->is_movie() ||
+        db::build()
         ->from("items")
         ->where("id", "=", $this->album_cover_item_id)
-        ->count_records() != 1) {
+        ->where("type", "<>", "album")
+        ->count_records() != 1)) {
       $v->add_error("album_cover_item_id", "invalid_item");
     }
   }
diff --git a/modules/gallery/tests/Graphics_Helper_Test.php b/modules/gallery/tests/Graphics_Helper_Test.php
index ddcb9dfd..a68822b0 100644
--- a/modules/gallery/tests/Graphics_Helper_Test.php
+++ b/modules/gallery/tests/Graphics_Helper_Test.php
@@ -44,6 +44,39 @@ class Graphics_Helper_Test extends Gallery_Unit_Test_Case {
     $this->assert_equal(0, $movie->thumb_dirty);
   }
 
+  public function generate_album_cover_test() {
+    $album = test::random_album();
+    $photo = test::random_unique_photo($album);
+    $album->reload();
+    // Check that the image was copied directly from item thumb
+    $this->assert_equal(file_get_contents($photo->thumb_path()),
+                        file_get_contents($album->thumb_path()));
+    // Check that the items table got updated
+    $this->assert_equal(array(200, 150), array($album->thumb_width, $album->thumb_height));
+    // Check that the image is not marked dirty
+    $this->assert_equal(0, $album->thumb_dirty);
+  }
+
+  public function generate_album_cover_from_png_test() {
+    $input_file = MODPATH . "gallery/tests/test.jpg";
+    $output_file = TMPPATH . test::random_name() . ".png";
+    gallery_graphics::resize($input_file, $output_file, null, null);
+
+    $album = test::random_album();
+    $photo = test::random_photo_unsaved($album);
+    $photo->set_data_file($output_file);
+    $photo->name = "album_cover_from_png.png";
+    $photo->save();
+    $album->reload();
+    // Check that the image was correctly resized and converted to jpg
+    $this->assert_equal(array(200, 150, "image/jpeg", "jpg"),
+                        photo::get_file_metadata($album->thumb_path()));
+    // Check that the items table got updated
+    $this->assert_equal(array(200, 150), array($album->thumb_width, $album->thumb_height));
+    // Check that the image is not marked dirty
+    $this->assert_equal(0, $album->thumb_dirty);
+  }
+
   public function generate_bad_photo_test() {
     $photo = test::random_photo();
     // At this point, the photo is valid and has a valid resize and thumb.  Make it garble.
@@ -86,4 +119,29 @@ class Graphics_Helper_Test extends Gallery_Unit_Test_Case {
     // Check that the image is *not* marked as dirty
     $this->assert_equal(0, $movie->thumb_dirty);
   }
+
+  public function generate_album_cover_from_bad_photo_test() {
+    $album = test::random_album();
+    $photo = test::random_photo($album);
+    $album->reload();
+    // At this point, the photo is valid and has a valid resize and thumb.  Make it garble.
+    file_put_contents($photo->file_path(), test::lorem_ipsum(200));
+    // Regenerate album from garbled photo.
+    $photo->thumb_dirty = 1;
+    $photo->save();
+    $album->thumb_dirty = 1;
+    try {
+      graphics::generate($album);
+      $this->assert_true(false, "Shouldn't get here");
+    } catch (Exception $e) {
+      // Exception expected
+    }
+    // Check that the image got replaced with a missing image placeholder
+    $this->assert_same(file_get_contents(MODPATH . "gallery/images/missing_photo.jpg"),
+                       file_get_contents($album->thumb_path()));
+    // Check that the items table got updated with new metadata
+    $this->assert_equal(array(200, 200), array($album->thumb_width, $album->thumb_height));
+    // Check that the images are marked as dirty
+    $this->assert_equal(1, $album->thumb_dirty);
+  }
 }
\ No newline at end of file
-- 
cgit v1.2.3


From fd8b8de2420066f907d09fd30ac75d678ef3e0ca Mon Sep 17 00:00:00 2001
From: shadlaws <shad@shadlaws.com>
Date: Thu, 31 Jan 2013 18:59:33 +0100
Subject: #1983: Remove item keys changed for/by graphics::generate from
 significant changes list. There's no need to run through all of the item
 save/validation logic for these entries, which are changed frequently during
 the course of thumb/resize generation.

---
 modules/gallery/models/item.php | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

(limited to 'modules/gallery/models/item.php')

diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php
index f9edd3c6..f80fc53a 100644
--- a/modules/gallery/models/item.php
+++ b/modules/gallery/models/item.php
@@ -318,9 +318,11 @@ class Item_Model_Core extends ORM_MPTT {
    */
   public function save() {
     $significant_changes = $this->changed;
-    unset($significant_changes["view_count"]);
-    unset($significant_changes["relative_url_cache"]);
-    unset($significant_changes["relative_path_cache"]);
+    foreach (array("view_count", "relative_url_cache", "relative_path_cache",
+                   "resize_width", "resize_height", "resize_dirty",
+                   "thumb_width", "thumb_height", "thumb_dirty") as $key) {
+      unset($significant_changes[$key]);
+    }
 
     if ((!empty($this->changed) && $significant_changes) || isset($this->data_file)) {
       $this->updated = time();
-- 
cgit v1.2.3


From 2da4d9dd1f99707778b61a320601921243202d2c Mon Sep 17 00:00:00 2001
From: shadlaws <shad@shadlaws.com>
Date: Thu, 7 Feb 2013 09:49:28 +0100
Subject: #1992 - Improve examples for thumb/resize/file path/url functions in
 item model. Documentation-only changes to examples used in: - Item_Model::url
 - Item_Model::abs_url - Item_Model::file_path - Item_Model::file_url -
 Item_Model::resize_path - Item_Model::resize_url - Item_Model::thumb_path -
 Item_Model::thumb_url

---
 modules/gallery/models/item.php | 64 +++++++++++++++++++++++++++++------------
 1 file changed, 46 insertions(+), 18 deletions(-)

(limited to 'modules/gallery/models/item.php')

diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php
index f80fc53a..197d3057 100644
--- a/modules/gallery/models/item.php
+++ b/modules/gallery/models/item.php
@@ -129,10 +129,11 @@ class Item_Model_Core extends ORM_MPTT {
 
   /**
    * Return the server-relative url to this item, eg:
-   *   /gallery3/index.php/BobsWedding?page=2
-   *   /gallery3/index.php/BobsWedding/Eating-Cake.jpg
+   *   album: /gallery3/index.php/Bobs%20Wedding?page=2
+   *   photo: /gallery3/index.php/Bobs%20Wedding/Eating-Cake
+   *   movie: /gallery3/index.php/Bobs%20Wedding/First-Dance
    *
-   * @param string $query the query string (eg "show=3")
+   * @param string $query the query string (eg "page=2")
    */
   public function url($query=null) {
     $url = url::site($this->relative_url());
@@ -144,10 +145,11 @@ class Item_Model_Core extends ORM_MPTT {
 
   /**
    * Return the full url to this item, eg:
-   *   http://example.com/gallery3/index.php/BobsWedding?page=2
-   *   http://example.com/gallery3/index.php/BobsWedding/Eating-Cake.jpg
+   *   album: http://example.com/gallery3/index.php/Bobs%20Wedding?page=2
+   *   photo: http://example.com/gallery3/index.php/Bobs%20Wedding/Eating-Cake
+   *   movie: http://example.com/gallery3/index.php/Bobs%20Wedding/First-Dance
    *
-   * @param string $query the query string (eg "show=3")
+   * @param string $query the query string (eg "page=2")
    */
   public function abs_url($query=null) {
     $url = url::abs_site($this->relative_url());
@@ -158,16 +160,24 @@ class Item_Model_Core extends ORM_MPTT {
   }
 
   /**
-   * album: /var/albums/album1/album2
-   * photo: /var/albums/album1/album2/photo.jpg
+   * Return the full path to this item's file, eg:
+   *   album: /usr/home/www/gallery3/var/albums/Bobs Wedding
+   *   photo: /usr/home/www/gallery3/var/albums/Bobs Wedding/Eating-Cake.jpg
+   *   movie: /usr/home/www/gallery3/var/albums/Bobs Wedding/First-Dance.mp4
    */
   public function file_path() {
     return VARPATH . "albums/" . urldecode($this->relative_path());
   }
 
   /**
-   * album: http://example.com/gallery3/var/resizes/album1/
-   * photo: http://example.com/gallery3/var/albums/album1/photo.jpg
+   * Return the relative url to this item's file, with cache buster, eg:
+   *   album: var/albums/Bobs%20Wedding?m=1234567890
+   *   photo: var/albums/Bobs%20Wedding/Eating-Cake.jpg?m=1234567890
+   *   movie: var/albums/Bobs%20Wedding/First-Dance.mp4?m=1234567890
+   * If $full_uri==true, return the full url to this item's file, with cache buster, eg:
+   *   album: http://example.com/gallery3/var/albums/Bobs%20Wedding?m=1234567890
+   *   photo: http://example.com/gallery3/var/albums/Bobs%20Wedding/Eating-Cake.jpg?m=1234567890
+   *   movie: http://example.com/gallery3/var/albums/Bobs%20Wedding/First-Dance.mp4?m=1234567890
    */
   public function file_url($full_uri=false) {
     $relative_path = "var/albums/" . $this->relative_path();
@@ -177,8 +187,10 @@ class Item_Model_Core extends ORM_MPTT {
   }
 
   /**
-   * album: /var/resizes/album1/.thumb.jpg
-   * photo: /var/albums/album1/photo.thumb.jpg
+   * Return the full path to this item's thumb, eg:
+   *   album: /usr/home/www/gallery3/var/thumbs/Bobs Wedding/.album.jpg
+   *   photo: /usr/home/www/gallery3/var/thumbs/Bobs Wedding/Eating-Cake.jpg
+   *   movie: /usr/home/www/gallery3/var/thumbs/Bobs Wedding/First-Dance.jpg
    */
   public function thumb_path() {
     $base = VARPATH . "thumbs/" . urldecode($this->relative_path());
@@ -200,8 +212,14 @@ class Item_Model_Core extends ORM_MPTT {
   }
 
   /**
-   * album: http://example.com/gallery3/var/resizes/album1/.thumb.jpg
-   * photo: http://example.com/gallery3/var/albums/album1/photo.thumb.jpg
+   * Return the relative url to this item's thumb, with cache buster, eg:
+   *   album: var/thumbs/Bobs%20Wedding/.album.jpg?m=1234567890
+   *   photo: var/thumbs/Bobs%20Wedding/Eating-Cake.jpg?m=1234567890
+   *   movie: var/thumbs/Bobs%20Wedding/First-Dance.mp4?m=1234567890
+   * If $full_uri==true, return the full url to this item's file, with cache buster, eg:
+   *   album: http://example.com/gallery3/var/thumbs/Bobs%20Wedding/.album.jpg?m=1234567890
+   *   photo: http://example.com/gallery3/var/thumbs/Bobs%20Wedding/Eating-Cake.jpg?m=1234567890
+   *   movie: http://example.com/gallery3/var/thumbs/Bobs%20Wedding/First-Dance.mp4?m=1234567890
    */
   public function thumb_url($full_uri=false) {
     $cache_buster = $this->_cache_buster($this->thumb_path());
@@ -219,8 +237,11 @@ class Item_Model_Core extends ORM_MPTT {
   }
 
   /**
-   * album: /var/resizes/album1/.resize.jpg
-   * photo: /var/albums/album1/photo.resize.jpg
+   * Return the full path to this item's resize, eg:
+   *   album: /usr/home/www/gallery3/var/resizes/Bobs Wedding/.album.jpg      (*)
+   *   photo: /usr/home/www/gallery3/var/resizes/Bobs Wedding/Eating-Cake.jpg
+   *   movie: /usr/home/www/gallery3/var/resizes/Bobs Wedding/First-Dance.mp4 (*)
+   * (*) Since only photos have resizes, album and movie paths are fictitious.
    */
   public function resize_path() {
     return VARPATH . "resizes/" . urldecode($this->relative_path()) .
@@ -228,8 +249,15 @@ class Item_Model_Core extends ORM_MPTT {
   }
 
   /**
-   * album: http://example.com/gallery3/var/resizes/album1/.resize.jpg
-   * photo: http://example.com/gallery3/var/albums/album1/photo.resize.jpg
+   * Return the relative url to this item's resize, with cache buster, eg:
+   *   album: var/resizes/Bobs%20Wedding/.album.jpg?m=1234567890      (*)
+   *   photo: var/resizes/Bobs%20Wedding/Eating-Cake.jpg?m=1234567890
+   *   movie: var/resizes/Bobs%20Wedding/First-Dance.mp4?m=1234567890 (*)
+   * If $full_uri==true, return the full url to this item's file, with cache buster, eg:
+   *   album: http://example.com/gallery3/var/resizes/Bobs%20Wedding/.album.jpg?m=1234567890      (*)
+   *   photo: http://example.com/gallery3/var/resizes/Bobs%20Wedding/Eating-Cake.jpg?m=1234567890
+   *   movie: http://example.com/gallery3/var/resizes/Bobs%20Wedding/First-Dance.mp4?m=1234567890 (*)
+   * (*) Since only photos have resizes, album and movie urls are fictitious.
    */
   public function resize_url($full_uri=false) {
     $relative_path = "var/resizes/" . $this->relative_path();
-- 
cgit v1.2.3


From 0312d1b071bd4434ddb3f82888b0323da6bf3732 Mon Sep 17 00:00:00 2001
From: shadlaws <shad@shadlaws.com>
Date: Fri, 8 Feb 2013 13:51:41 +0100
Subject: #1994 - Make get_file_metadata throw an exception if photo or movie
 is unidentifiable/illegal. - photo & movie helpers: modified to throw
 exceptions when file is known to be unidentifiable/illegal. - item model:
 revised to work with exceptions and be more explicit when the data file is
 invalid. - item model: removed duplicate get_file_metadata call for updated
 items. - admin_watermarks controller: revised to work with exceptions (really
 cleans up logic here). - graphics helper: revised to handle invalid
 placeholders (a nearly-impossible corner case, but still...). - photo & movie
 helper tests: revised to work with exceptions, added new tests for illegal
 files with valid extensions. - item model tests: revised to work with
 exceptions, added new tests for illegal files with valid extensions.

---
 modules/gallery/helpers/gallery_graphics.php       |  5 ++
 modules/gallery/helpers/graphics.php               | 11 +++-
 modules/gallery/helpers/movie.php                  | 10 ++-
 modules/gallery/helpers/photo.php                  |  9 ++-
 modules/gallery/models/item.php                    | 71 +++++++++++++---------
 modules/gallery/tests/Item_Model_Test.php          | 16 ++++-
 modules/gallery/tests/Movie_Helper_Test.php        | 36 +++++++++--
 modules/gallery/tests/Photo_Helper_Test.php        | 18 +++++-
 modules/watermark/controllers/admin_watermarks.php | 13 ++--
 9 files changed, 139 insertions(+), 50 deletions(-)

(limited to 'modules/gallery/models/item.php')

diff --git a/modules/gallery/helpers/gallery_graphics.php b/modules/gallery/helpers/gallery_graphics.php
index b78bd9a7..eb76353f 100644
--- a/modules/gallery/helpers/gallery_graphics.php
+++ b/modules/gallery/helpers/gallery_graphics.php
@@ -172,6 +172,11 @@ class gallery_graphics_Core {
 
       module::event("graphics_composite_completed", $input_file, $output_file, $options, $item);
     } catch (ErrorException $e) {
+      // Unlike rotate and resize, composite catches its exceptions here.  This is because
+      // composite is typically called for watermarks.  If during thumb/resize generation
+      // the watermark fails, we'd still like the image resized, just without its watermark.
+      // If the exception isn't caught here, graphics::generate will replace it with a
+      // placeholder.
       Kohana_Log::add("error", $e->getMessage());
     }
   }
diff --git a/modules/gallery/helpers/graphics.php b/modules/gallery/helpers/graphics.php
index 4df57fba..e34af018 100644
--- a/modules/gallery/helpers/graphics.php
+++ b/modules/gallery/helpers/graphics.php
@@ -224,7 +224,16 @@ class graphics_Core {
         graphics::_replace_image_with_placeholder($item, "resize");
       }
       graphics::_replace_image_with_placeholder($item, "thumb");
-      graphics::_update_item_dimensions($item);
+      try {
+        graphics::_update_item_dimensions($item);
+      } catch (Exception $e) {
+        // Looks like get_file_metadata couldn't identify our placeholders.  We should never get
+        // here, but in the odd case we do, we need to do something.  Let's put in hardcoded values.
+        if ($item->is_photo()) {
+          list ($item->resize_width, $item->resize_height) = array(200, 200);
+        }
+        list ($item->thumb_width, $item->thumb_height) = array(200, 200);
+      }
       $item->save();
       throw $e;
     }
diff --git a/modules/gallery/helpers/movie.php b/modules/gallery/helpers/movie.php
index 6844771b..d4b907a2 100644
--- a/modules/gallery/helpers/movie.php
+++ b/modules/gallery/helpers/movie.php
@@ -192,8 +192,16 @@ class movie_Core {
       $metadata->extension = strtolower($extension);
     }
 
-    // Run movie_get_file_metadata events which can modify the class, then return results.
+    // Run movie_get_file_metadata events which can modify the class.
     module::event("movie_get_file_metadata", $file_path, $metadata);
+
+    // If the post-events results are invalid, throw an exception.  Note that, unlike photos, having
+    // zero width and height isn't considered invalid (as is the case when FFmpeg isn't installed).
+    if (!$metadata->mime_type || !$metadata->extension ||
+        ($metadata->mime_type != legal_file::get_movie_types_by_extension($metadata->extension))) {
+      throw new Exception("@todo ILLEGAL_OR_UNINDENTIFIABLE_FILE");
+    }
+
     return array($metadata->width, $metadata->height, $metadata->mime_type,
                  $metadata->extension, $metadata->duration);
   }
diff --git a/modules/gallery/helpers/photo.php b/modules/gallery/helpers/photo.php
index 51e51507..2d32f0d3 100644
--- a/modules/gallery/helpers/photo.php
+++ b/modules/gallery/helpers/photo.php
@@ -133,8 +133,15 @@ class photo_Core {
       $metadata->height = 0;
     }
 
-    // Run photo_get_file_metadata events which can modify the class, then return results.
+    // Run photo_get_file_metadata events which can modify the class.
     module::event("photo_get_file_metadata", $file_path, $metadata);
+
+    // If the post-events results are invalid, throw an exception.
+    if (!$metadata->width || !$metadata->height || !$metadata->mime_type || !$metadata->extension ||
+        ($metadata->mime_type != legal_file::get_photo_types_by_extension($metadata->extension))) {
+      throw new Exception("@todo ILLEGAL_OR_UNINDENTIFIABLE_FILE");
+    }
+
     return array($metadata->width, $metadata->height, $metadata->mime_type, $metadata->extension);
   }
 }
diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php
index 197d3057..33b8a89d 100644
--- a/modules/gallery/models/item.php
+++ b/modules/gallery/models/item.php
@@ -21,6 +21,7 @@ class Item_Model_Core extends ORM_MPTT {
   protected $children = "items";
   protected $sorting = array();
   public $data_file = null;
+  private $data_file_error = null;
 
   public function __construct($id=null) {
     parent::__construct($id);
@@ -378,18 +379,26 @@ class Item_Model_Core extends ORM_MPTT {
 
         // Get the width, height and mime type from our data file for photos and movies.
         if ($this->is_photo() || $this->is_movie()) {
-          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);
-          }
-
-          // Force an extension onto the name if necessary
-          $pi = pathinfo($this->data_file);
-          if (empty($pi["extension"])) {
-            $this->name = "{$this->name}.$extension";
+          try {
+            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);
+            }
+
+            // Force an extension onto the name if necessary
+            $pi = pathinfo($this->data_file);
+            if (empty($pi["extension"])) {
+              $this->name = "{$this->name}.$extension";
+            }
+
+            // Data file valid - make sure the flag is reset to false.
+            $this->data_file_error = false;
+          } catch (Exception $e) {
+            // Data file invalid - set the flag so it's reported during item validation.
+            $this->data_file_error = true;
           }
         }
 
@@ -436,17 +445,24 @@ class Item_Model_Core extends ORM_MPTT {
         // 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);
+          try {
+            $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);
+            }
+            // Data file valid - make sure the flag is reset to false.
+            $this->data_file_error = false;
+          } catch (Exception $e) {
+            // Data file invalid - set the flag so it's reported during item validation.
+            $this->data_file_error = true;
           }
         }
 
@@ -524,13 +540,6 @@ class Item_Model_Core extends ORM_MPTT {
         // Replace the data file, if requested.
         if ($this->data_file && ($this->is_photo() || $this->is_movie())) {
           copy($this->data_file, $this->file_path());
-
-          // Get the width, height and mime type from our data file for photos and movies.
-          if ($this->is_photo()) {
-            list ($this->width, $this->height) = photo::get_file_metadata($this->file_path());
-          } else if ($this->is_movie()) {
-            list ($this->width, $this->height) = movie::get_file_metadata($this->file_path());
-          }
           $this->thumb_dirty = 1;
           $this->resize_dirty = 1;
         }
@@ -966,6 +975,8 @@ class Item_Model_Core extends ORM_MPTT {
       $v->add_error("name", "bad_data_file_path");
     } else if (filesize($this->data_file) == 0) {
       $v->add_error("name", "empty_data_file");
+    } else if ($this->data_file_error) {
+      $v->add_error("name", "invalid_data_file");
     }
   }
 
diff --git a/modules/gallery/tests/Item_Model_Test.php b/modules/gallery/tests/Item_Model_Test.php
index a1c5bce6..a93498dd 100644
--- a/modules/gallery/tests/Item_Model_Test.php
+++ b/modules/gallery/tests/Item_Model_Test.php
@@ -445,13 +445,25 @@ class Item_Model_Test extends Gallery_Unit_Test_Case {
       $photo->set_data_file(MODPATH . "gallery/tests/Item_Model_Test.php");
       $photo->save();
     } catch (ORM_Validation_Exception $e) {
-      $this->assert_same(array("mime_type" => "invalid", "name" => "illegal_data_file_extension"),
-                         $e->validation->errors());
+      $this->assert_same(array("name" => "illegal_data_file_extension"), $e->validation->errors());
       return;  // pass
     }
     $this->assert_true(false, "Shouldn't get here");
   }
 
+  public function unsafe_data_file_replacement_with_valid_extension_test() {
+    $temp_file = TMPPATH . "masquerading_php.jpg";
+    copy(MODPATH . "gallery/tests/Item_Model_Test.php", $temp_file);
+    try {
+      $photo = test::random_photo();
+      $photo->set_data_file($temp_file);
+      $photo->save();
+    } catch (ORM_Validation_Exception $e) {
+      $this->assert_same(array("name" => "invalid_data_file"), $e->validation->errors());
+      return;  // pass
+    }
+  }
+
   public function urls_test() {
     $photo = test::random_photo();
     $this->assert_true(
diff --git a/modules/gallery/tests/Movie_Helper_Test.php b/modules/gallery/tests/Movie_Helper_Test.php
index 0c262620..03fa2da9 100644
--- a/modules/gallery/tests/Movie_Helper_Test.php
+++ b/modules/gallery/tests/Movie_Helper_Test.php
@@ -64,18 +64,42 @@ class Movie_Helper_Test extends Gallery_Unit_Test_Case {
 
   public function get_file_metadata_with_no_extension_test() {
     copy(MODPATH . "gallery/tests/test.flv", TMPPATH . "test_flv_with_no_extension");
-    $this->assert_equal(array(360, 288, null, null, 6.00),
-                        movie::get_file_metadata(TMPPATH . "test_flv_with_no_extension"));
+    // Since mime type and extension are based solely on the filename, this is considered invalid.
+    try {
+      $metadata = movie::get_file_metadata(TMPPATH . "test_flv_with_no_extension");
+      $this->assert_true(false, "Shouldn't get here");
+    } catch (Exception $e) {
+      // pass
+    }
   }
 
   public function get_file_metadata_with_illegal_extension_test() {
-    $this->assert_equal(array(0, 0, null, null, 0),
-                        movie::get_file_metadata(MODPATH . "gallery/tests/Movie_Helper_Test.php"));
+    try {
+      $metadata = movie::get_file_metadata(MODPATH . "gallery/tests/Movie_Helper_Test.php");
+      $this->assert_true(false, "Shouldn't get here");
+    } catch (Exception $e) {
+      // pass
+    }
   }
 
   public function get_file_metadata_with_illegal_extension_but_valid_file_contents_test() {
     copy(MODPATH . "gallery/tests/test.flv", TMPPATH . "test_flv_with_php_extension.php");
-    $this->assert_equal(array(360, 288, null, null, 6.00),
-                        movie::get_file_metadata(TMPPATH . "test_flv_with_php_extension.php"));
+    // Since mime type and extension are based solely on the filename, this is considered invalid.
+    try {
+      $metadata = movie::get_file_metadata(TMPPATH . "test_flv_with_php_extension.php");
+      $this->assert_true(false, "Shouldn't get here");
+    } catch (Exception $e) {
+      // pass
+    }
+  }
+
+  public function get_file_metadata_with_valid_extension_but_illegal_file_contents_test() {
+    copy(MODPATH . "gallery/tests/Photo_Helper_Test.php", TMPPATH . "test_php_with_flv_extension.flv");
+    // Since mime type and extension are based solely on the filename, this is considered valid.
+    // Of course, FFmpeg cannot extract width, height, or duration from the file.  Note that this
+    // isn't a really a security problem, since the filename doesn't have a php extension and
+    // therefore will never be executed.
+    $this->assert_equal(array(0, 0, "video/x-flv", "flv", 0),
+                        movie::get_file_metadata(TMPPATH . "test_php_with_flv_extension.flv"));
   }
 }
diff --git a/modules/gallery/tests/Photo_Helper_Test.php b/modules/gallery/tests/Photo_Helper_Test.php
index 5207a6db..79b5ccfd 100644
--- a/modules/gallery/tests/Photo_Helper_Test.php
+++ b/modules/gallery/tests/Photo_Helper_Test.php
@@ -40,8 +40,12 @@ class Photo_Helper_Test extends Gallery_Unit_Test_Case {
   }
 
   public function get_file_metadata_with_illegal_extension_test() {
-    $this->assert_equal(array(0, 0, null, null),
-                        photo::get_file_metadata(MODPATH . "gallery/tests/Photo_Helper_Test.php"));
+    try {
+      $metadata = photo::get_file_metadata(MODPATH . "gallery/tests/Photo_Helper_Test.php");
+      $this->assert_true(false, "Shouldn't get here");
+    } catch (Exception $e) {
+      // pass
+    }
   }
 
   public function get_file_metadata_with_illegal_extension_but_valid_file_contents_test() {
@@ -53,4 +57,14 @@ class Photo_Helper_Test extends Gallery_Unit_Test_Case {
     $this->assert_equal(array(1024, 768, "image/jpeg", "jpg"),
                         photo::get_file_metadata(TMPPATH . "test_jpg_with_php_extension.php"));
   }
+
+  public function get_file_metadata_with_valid_extension_but_illegal_file_contents_test() {
+    copy(MODPATH . "gallery/tests/Photo_Helper_Test.php", TMPPATH . "test_php_with_jpg_extension.jpg");
+    try {
+      $metadata = photo::get_file_metadata(TMPPATH . "test_php_with_jpg_extension.jpg");
+      $this->assert_true(false, "Shouldn't get here");
+    } catch (Exception $e) {
+      // pass
+    }
+  }
 }
diff --git a/modules/watermark/controllers/admin_watermarks.php b/modules/watermark/controllers/admin_watermarks.php
index 27c2efc9..59bb7fa9 100644
--- a/modules/watermark/controllers/admin_watermarks.php
+++ b/modules/watermark/controllers/admin_watermarks.php
@@ -102,18 +102,17 @@ class Admin_Watermarks_Controller extends Admin_Controller {
       $name = preg_replace("/uploadfile-[^-]+-(.*)/", '$1', $pathinfo["basename"]);
       $name = legal_file::smash_extensions($name);
 
-      list ($width, $height, $mime_type, $extension) = photo::get_file_metadata($file);
-      if (!$width || !$height || !$mime_type || !$extension ||
-          !legal_file::get_photo_extensions($extension)) {
-        message::error(t("Invalid or unidentifiable image file"));
-        @unlink($file);
-        return;
-      } else {
+      try {
+        list ($width, $height, $mime_type, $extension) = photo::get_file_metadata($file);
         // Force correct, legal extension type on file, which will be of our canonical type
         // (i.e. all lowercase, jpg instead of jpeg, etc.).  This renaming prevents the issues
         // addressed in ticket #1855, where an image that looked valid (header said jpg) with a
         // php extension was previously accepted without changing its extension.
         $name = legal_file::change_extension($name, $extension);
+      } catch (Exception $e) {
+        message::error(t("Invalid or unidentifiable image file"));
+        @unlink($file);
+        return;
       }
 
       rename($file, VARPATH . "modules/watermark/$name");
-- 
cgit v1.2.3


From d04a6fc87d96b70ab0f70414f2ff40d1f1e7f482 Mon Sep 17 00:00:00 2001
From: shadlaws <shad@shadlaws.com>
Date: Tue, 12 Feb 2013 00:37:33 +0100
Subject: #2001 - Make filename sanitizing more consistent. - legal_file -
 added sanitize_filname() to sanitize photo/movie filenames. -
 admin_watermarks - revised add() to use new function. - item model - added
 _process_data_file_info() to validate the data file, get its metadata, and
 sanitize the item name. - item model - revised save() for new items to use
 _process_data_file_info *before* the slug is checked. - item model - revised
 save() for updated items to use _process_data_file_info. - item model -
 revised save() for updated items to sanitize name if changed. - uploader -
 removed call to smash_extensions (item model does this when it calls
 sanitize_filename). - Legal_File_Helper_Test - added unit tests for
 sanitize_filename. - Item_Model_Test - revised existing unit tests based on
 changes. - Item_Model_Test - added new unit tests for names with legal but
 incorrect extensions. - Averted take over by HAL with fix #2001...

---
 modules/gallery/controllers/uploader.php           |   4 -
 modules/gallery/helpers/legal_file.php             |  57 ++++++++++++
 modules/gallery/models/item.php                    |  95 ++++++++++---------
 modules/gallery/tests/Item_Model_Test.php          | 101 ++++++++++-----------
 modules/gallery/tests/Legal_File_Helper_Test.php   |  44 +++++++++
 modules/watermark/controllers/admin_watermarks.php |   9 +-
 6 files changed, 202 insertions(+), 108 deletions(-)

(limited to 'modules/gallery/models/item.php')

diff --git a/modules/gallery/controllers/uploader.php b/modules/gallery/controllers/uploader.php
index 55c65c95..78437071 100644
--- a/modules/gallery/controllers/uploader.php
+++ b/modules/gallery/controllers/uploader.php
@@ -63,10 +63,6 @@ class Uploader_Controller extends Controller {
         $item->parent_id = $album->id;
         $item->set_data_file($temp_filename);
 
-        // Remove double extensions from the filename - they'll be disallowed in the model but if
-        // we don't do it here then it'll result in a failed upload.
-        $item->name = legal_file::smash_extensions($item->name);
-
         $path_info = @pathinfo($temp_filename);
         if (array_key_exists("extension", $path_info) &&
             legal_file::get_movie_extensions($path_info["extension"])) {
diff --git a/modules/gallery/helpers/legal_file.php b/modules/gallery/helpers/legal_file.php
index ef588ceb..5a852f2b 100644
--- a/modules/gallery/helpers/legal_file.php
+++ b/modules/gallery/helpers/legal_file.php
@@ -250,4 +250,61 @@ class legal_file_Core {
     $result .= isset($parts["extension"]) ? "{$parts['filename']}.{$parts['extension']}" : $parts["filename"];
     return $result;
   }
+
+  /**
+   * Sanitize a filename for a given type (given as "photo" or "movie") and a target file format
+   * (given as an extension).  This returns a completely legal and valid filename,
+   * or throws an exception if the type or extension given is invalid or illegal.  It tries to
+   * maintain the filename's original extension even if it's not identical to the given extension
+   * (e.g. don't change "JPG" or "jpeg" to "jpg").
+   *
+   * Note: it is not okay if the extension given is legal but does not match the type (e.g. if
+   * extension is "mp4" and type is "photo", it will throw an exception)
+   *
+   * @param  string $filename  (with no directory)
+   * @param  string $extension (can be uppercase or lowercase)
+   * @param  string $type      (as "photo" or "movie")
+   * @return string sanitized filename (or null if bad extension argument)
+   */
+  static function sanitize_filename($filename, $extension, $type) {
+    // Check if the type is valid - if so, get the mime types of the
+    // original and target extensions; if not, throw an exception.
+    $original_extension = pathinfo($filename, PATHINFO_EXTENSION);
+    switch ($type) {
+      case "photo":
+        $mime_type = legal_file::get_photo_types_by_extension($extension);
+        $original_mime_type = legal_file::get_photo_types_by_extension($original_extension);
+        break;
+      case "movie":
+        $mime_type = legal_file::get_movie_types_by_extension($extension);
+        $original_mime_type = legal_file::get_movie_types_by_extension($original_extension);
+        break;
+      default:
+        throw new Exception("@todo INVALID_TYPE");
+    }
+
+    // Check if the target extension is blank or invalid - if so, throw an exception.
+    if (!$extension || !$mime_type) {
+      throw new Exception("@todo ILLEGAL_EXTENSION");
+    }
+
+    // Check if the mime types of the original and target extensions match - if not, fix it.
+    if (!$original_extension || ($mime_type != $original_mime_type)) {
+      $filename = legal_file::change_extension($filename, $extension);
+    }
+
+    // It should be a filename without a directory - remove all slashes (and backslashes).
+    $filename = str_replace("/", "_", $filename);
+    $filename = str_replace("\\", "_", $filename);
+
+    // Remove extra dots from the filename.  This will also remove extraneous underscores.
+    $filename = legal_file::smash_extensions($filename);
+
+    // It's possible that the filename has no base (e.g. ".jpg") - if so, give it a generic one.
+    if (empty($filename) || (substr($filename, 0, 1) == ".")) {
+      $filename = $type . $filename;  // e.g. "photo.jpg" or "movie.mp4"
+    }
+
+    return $filename;
+  }
 }
diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php
index 33b8a89d..43b9a292 100644
--- a/modules/gallery/models/item.php
+++ b/modules/gallery/models/item.php
@@ -365,6 +365,14 @@ class Item_Model_Core extends ORM_MPTT {
           $this->weight = item::get_max_weight();
         }
 
+        // Process the data file info.
+        if (isset($this->data_file)) {
+          $this->_process_data_file_info();
+        } else if (!$this->is_album()) {
+          // Unless it's an album, new items must have a data file.
+          $this->data_file_error = true;
+        }
+
         // Make an url friendly slug from the name, if necessary
         if (empty($this->slug)) {
           $this->slug = item::convert_filename_to_slug(pathinfo($this->name, PATHINFO_FILENAME));
@@ -377,31 +385,6 @@ class Item_Model_Core extends ORM_MPTT {
           }
         }
 
-        // Get the width, height and mime type from our data file for photos and movies.
-        if ($this->is_photo() || $this->is_movie()) {
-          try {
-            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);
-            }
-
-            // Force an extension onto the name if necessary
-            $pi = pathinfo($this->data_file);
-            if (empty($pi["extension"])) {
-              $this->name = "{$this->name}.$extension";
-            }
-
-            // Data file valid - make sure the flag is reset to false.
-            $this->data_file_error = false;
-          } catch (Exception $e) {
-            // Data file invalid - set the flag so it's reported during item validation.
-            $this->data_file_error = true;
-          }
-        }
-
         $this->_check_and_fix_conflicts();
 
         parent::save();
@@ -439,31 +422,19 @@ class Item_Model_Core extends ORM_MPTT {
         // keep it around.
         $original = ORM::factory("item", $this->id);
 
-        // Preserve the extension of the data file. Many helpers, (e.g. ImageMagick), assume
+        // If we have a new data file, process its info.  This will get its metadata and
+        // 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)) {
-          try {
-            $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);
-            }
-            // Data file valid - make sure the flag is reset to false.
-            $this->data_file_error = false;
-          } catch (Exception $e) {
-            // Data file invalid - set the flag so it's reported during item validation.
-            $this->data_file_error = true;
-          }
+          $this->_process_data_file_info();
+        } else if (!$this->is_album() && array_key_exists("name", $this->changed)) {
+          // There's no new data file, but the name changed.  If it's a photo or movie,
+          // make sure the new name still agrees with the file type.
+          $this->name = legal_file::sanitize_filename($this->name,
+            pathinfo($original->name, PATHINFO_EXTENSION), $this->type);
         }
 
         // If an album's cover has changed (or been removed), delete any existing album cover,
@@ -624,6 +595,40 @@ class Item_Model_Core extends ORM_MPTT {
     }
   }
 
+  /**
+   * Process the data file info.  Get its metadata and extension.
+   * If valid, use it to sanitize the item name and update the
+   * width, height, and mime type.
+   */
+  private function _process_data_file_info() {
+    try {
+      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);
+      } else {
+        // Albums don't have data files.
+        $this->data_file = null;
+        return;
+      }
+
+      // Sanitize the name based on the idenified extension, but only set $this->name if different
+      // to ensure it isn't unnecessarily marked as "changed"
+      $name = legal_file::sanitize_filename($this->name, $extension, $this->type);
+      if ($this->name != $name) {
+        $this->name = $name;
+      }
+
+      // Data file valid - make sure the flag is reset to false.
+      $this->data_file_error = false;
+    } catch (Exception $e) {
+      // Data file invalid - set the flag so it's reported during item validation.
+      $this->data_file_error = true;
+    }
+  }
+
   /**
    * Return the Item_Model representing the cover for this album.
    * @return Item_Model or null if there's no cover
diff --git a/modules/gallery/tests/Item_Model_Test.php b/modules/gallery/tests/Item_Model_Test.php
index a93498dd..fcb5c2ad 100644
--- a/modules/gallery/tests/Item_Model_Test.php
+++ b/modules/gallery/tests/Item_Model_Test.php
@@ -126,14 +126,9 @@ class Item_Model_Test extends Gallery_Unit_Test_Case {
 
   public function item_rename_wont_accept_slash_test() {
     $item = test::random_photo();
-    try {
-      $item->name = test::random_name() . "/";
-      $item->save();
-    } catch (ORM_Validation_Exception $e) {
-      $this->assert_equal(array("name" => "no_slashes"), $e->validation->errors());
-      return;
-    }
-    $this->assert_true(false, "Shouldn't get here");
+    $item->name = "/no_slashes/allowed/";
+    $item->save();
+    $this->assert_equal("no_slashes_allowed.jpg", $item->name);
   }
 
   public function move_album_test() {
@@ -328,30 +323,17 @@ class Item_Model_Test extends Gallery_Unit_Test_Case {
   }
 
   public function photo_files_must_have_an_extension_test() {
-    try {
-      $photo = test::random_photo_unsaved();
-      $photo->mime_type = "image/jpeg";
-      $photo->name = "no_extension";
-      $photo->save();
-    } catch (ORM_Validation_Exception $e) {
-      $this->assert_same(array("name" => "illegal_data_file_extension"), $e->validation->errors());
-      return;  // pass
-    }
-    $this->assert_true(false, "Shouldn't get here");
+    $photo = test::random_photo_unsaved();
+    $photo->name = "no_extension_photo";
+    $photo->save();
+    $this->assert_equal("no_extension_photo.jpg", $photo->name);
   }
 
   public function movie_files_must_have_an_extension_test() {
-    try {
-      $movie = test::random_movie_unsaved();
-      $movie->type = "movie";
-      $movie->mime_type = "video/x-flv";
-      $movie->name = "no_extension";
-      $movie->save();
-    } catch (ORM_Validation_Exception $e) {
-      $this->assert_same(array("name" => "illegal_data_file_extension"), $e->validation->errors());
-      return;  // pass
-    }
-    $this->assert_true(false, "Shouldn't get here");
+    $movie = test::random_movie_unsaved();
+    $movie->name = "no_extension_movie";
+    $movie->save();
+    $this->assert_equal("no_extension_movie.flv", $movie->name);
   }
 
   public function cant_delete_root_album_test() {
@@ -445,7 +427,7 @@ class Item_Model_Test extends Gallery_Unit_Test_Case {
       $photo->set_data_file(MODPATH . "gallery/tests/Item_Model_Test.php");
       $photo->save();
     } catch (ORM_Validation_Exception $e) {
-      $this->assert_same(array("name" => "illegal_data_file_extension"), $e->validation->errors());
+      $this->assert_same(array("name" => "invalid_data_file"), $e->validation->errors());
       return;  // pass
     }
     $this->assert_true(false, "Shouldn't get here");
@@ -462,6 +444,7 @@ class Item_Model_Test extends Gallery_Unit_Test_Case {
       $this->assert_same(array("name" => "invalid_data_file"), $e->validation->errors());
       return;  // pass
     }
+    $this->assert_true(false, "Shouldn't get here");
   }
 
   public function urls_test() {
@@ -493,43 +476,55 @@ class Item_Model_Test extends Gallery_Unit_Test_Case {
       $album->thumb_url() . " is malformed");
   }
 
-  public function legal_extension_test() {
-    foreach (array("test.gif", "test.GIF", "test.Gif", "test.jpeg", "test.JPG") as $name) {
+  public function legal_extension_that_does_match_gets_used_test() {
+    foreach (array("jpg", "JPG", "Jpg", "jpeg") as $extension) {
       $photo = test::random_photo_unsaved(item::root());
-      $photo->name = $name;
+      $photo->name = test::random_name() . ".{$extension}";
       $photo->save();
+      // Should get renamed with the correct jpg extension of the data file.
+      $this->assert_equal($extension, pathinfo($photo->name, PATHINFO_EXTENSION));
     }
   }
 
   public function illegal_extension_test() {
     foreach (array("test.php", "test.PHP", "test.php5", "test.php4",
                    "test.pl", "test.php.png") as $name) {
-      try {
-        $photo = test::random_photo_unsaved(item::root());
-        $photo->name = $name;
-        $photo->save();
-      } catch (ORM_Validation_Exception $e) {
-        $this->assert_equal(array("name" => "illegal_data_file_extension"),
-                            $e->validation->errors());
-        continue;
-      }
-      $this->assert_true(false, "Shouldn't get here");
+      $photo = test::random_photo_unsaved(item::root());
+      $photo->name = $name;
+      $photo->save();
+      // Should get renamed with the correct jpg extension of the data file.
+      $this->assert_equal("jpg", pathinfo($photo->name, PATHINFO_EXTENSION));
     }
   }
 
   public function cant_rename_to_illegal_extension_test() {
     foreach (array("test.php.test", "test.php", "test.PHP",
                    "test.php5", "test.php4", "test.pl") as $name) {
-      try {
-        $photo = test::random_photo(item::root());
-        $photo->name = $name;
-        $photo->save();
-      } catch (ORM_Validation_Exception $e) {
-        $this->assert_equal(array("name" => "illegal_data_file_extension"),
-                            $e->validation->errors());
-        continue;
-      }
-      $this->assert_true(false, "Shouldn't get here");
+      $photo = test::random_photo(item::root());
+      $photo->name = $name;
+      $photo->save();
+      // Should get renamed with the correct jpg extension of the data file.
+      $this->assert_equal("jpg", pathinfo($photo->name, PATHINFO_EXTENSION));
+    }
+  }
+
+  public function legal_extension_that_doesnt_match_gets_fixed_test() {
+    foreach (array("test.png", "test.mp4", "test.GIF") as $name) {
+      $photo = test::random_photo_unsaved(item::root());
+      $photo->name = $name;
+      $photo->save();
+      // Should get renamed with the correct jpg extension of the data file.
+      $this->assert_equal("jpg", pathinfo($photo->name, PATHINFO_EXTENSION));
+    }
+  }
+
+  public function rename_to_legal_extension_that_doesnt_match_gets_fixed_test() {
+    foreach (array("test.png", "test.mp4", "test.GIF") as $name) {
+      $photo = test::random_photo(item::root());
+      $photo->name = $name;
+      $photo->save();
+      // Should get renamed with the correct jpg extension of the data file.
+      $this->assert_equal("jpg", pathinfo($photo->name, PATHINFO_EXTENSION));
     }
   }
 
diff --git a/modules/gallery/tests/Legal_File_Helper_Test.php b/modules/gallery/tests/Legal_File_Helper_Test.php
index 203d5616..7ed5214b 100644
--- a/modules/gallery/tests/Legal_File_Helper_Test.php
+++ b/modules/gallery/tests/Legal_File_Helper_Test.php
@@ -150,4 +150,48 @@ class Legal_File_Helper_Test extends Gallery_Unit_Test_Case {
     $this->assert_equal("", legal_file::smash_extensions(""));
     $this->assert_equal(null, legal_file::smash_extensions(null));
   }
+
+  public function sanitize_filename_with_no_rename_test() {
+    $this->assert_equal("foo.jpeg", legal_file::sanitize_filename("foo.jpeg", "jpg", "photo"));
+    $this->assert_equal("foo.jpg", legal_file::sanitize_filename("foo.jpg", "jpeg", "photo"));
+    $this->assert_equal("foo.MP4", legal_file::sanitize_filename("foo.MP4", "mp4", "movie"));
+    $this->assert_equal("foo.mp4", legal_file::sanitize_filename("foo.mp4", "MP4", "movie"));
+  }
+
+  public function sanitize_filename_with_corrected_extension_test() {
+    $this->assert_equal("foo.jpg", legal_file::sanitize_filename("foo.png", "jpg", "photo"));
+    $this->assert_equal("foo.MP4", legal_file::sanitize_filename("foo.jpg", "MP4", "movie"));
+    $this->assert_equal("foo.jpg", legal_file::sanitize_filename("foo.php", "jpg", "photo"));
+  }
+
+  public function sanitize_filename_with_non_standard_chars_and_dots_test() {
+    $this->assert_equal("foo.jpg", legal_file::sanitize_filename("foo", "jpg", "photo"));
+    $this->assert_equal("foo.mp4", legal_file::sanitize_filename("foo.", "mp4", "movie"));
+    $this->assert_equal("foo.jpeg", legal_file::sanitize_filename(".foo.jpeg", "jpg", "photo"));
+    $this->assert_equal("foo_2013_02_10.jpeg",
+      legal_file::sanitize_filename("foo.2013/02/10.jpeg", "jpg", "photo"));
+    $this->assert_equal("foo_bar_baz.jpg",
+      legal_file::sanitize_filename("...foo...bar..baz...png", "jpg", "photo"));
+    $this->assert_equal("j'écris@un#nom_bizarre(mais quand_même_ça_passe.jpg",
+      legal_file::sanitize_filename("/j'écris@un#nom/bizarre(mais quand.même/ça_passe.\$ÇÀ@€#_", "jpg", "photo"));
+  }
+
+  public function sanitize_filename_with_no_base_name_test() {
+    $this->assert_equal("photo.jpg", legal_file::sanitize_filename(".png", "jpg", "photo"));
+    $this->assert_equal("movie.mp4", legal_file::sanitize_filename("__..__", "mp4", "movie"));
+    $this->assert_equal("photo.jpg", legal_file::sanitize_filename(".", "jpg", "photo"));
+    $this->assert_equal("movie.mp4", legal_file::sanitize_filename(null, "mp4", "movie"));
+  }
+
+  public function sanitize_filename_with_invalid_arguments_test() {
+    foreach (array("flv" => "photo", "jpg" => "movie", "php" => "photo",
+                   null => "movie", "jpg" => "album", "jpg" => null) as $extension => $type) {
+      try {
+        legal_file::sanitize_filename("foo.jpg", $extension, $type);
+        $this->assert_true(false, "Shouldn't get here");
+      } catch (Exception $e) {
+        // pass
+      }
+    }
+  }
 }
\ No newline at end of file
diff --git a/modules/watermark/controllers/admin_watermarks.php b/modules/watermark/controllers/admin_watermarks.php
index 59bb7fa9..b058d6a5 100644
--- a/modules/watermark/controllers/admin_watermarks.php
+++ b/modules/watermark/controllers/admin_watermarks.php
@@ -97,18 +97,15 @@ class Admin_Watermarks_Controller extends Admin_Controller {
     // validation logic will correctly reject it.  So, we skip validation when we're running tests.
     if (TEST_MODE || $form->validate()) {
       $file = $_POST["file"];
-      $pathinfo = pathinfo($file);
       // Forge prefixes files with "uploadfile-xxxxxxx" for uniqueness
-      $name = preg_replace("/uploadfile-[^-]+-(.*)/", '$1', $pathinfo["basename"]);
-      $name = legal_file::smash_extensions($name);
+      $name = preg_replace("/uploadfile-[^-]+-(.*)/", '$1', basename($file));
 
       try {
         list ($width, $height, $mime_type, $extension) = photo::get_file_metadata($file);
-        // Force correct, legal extension type on file, which will be of our canonical type
-        // (i.e. all lowercase, jpg instead of jpeg, etc.).  This renaming prevents the issues
+        // Sanitize filename, which ensures a valid extension.  This renaming prevents the issues
         // addressed in ticket #1855, where an image that looked valid (header said jpg) with a
         // php extension was previously accepted without changing its extension.
-        $name = legal_file::change_extension($name, $extension);
+        $name = legal_file::sanitize_filename($name, $extension, "photo");
       } catch (Exception $e) {
         message::error(t("Invalid or unidentifiable image file"));
         @unlink($file);
-- 
cgit v1.2.3