#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.

3 files changed, 163 insertions, 56 deletions

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();
 
@@ -522,6 +533,74 @@ class gallery_task_Core {
         $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;
         }
         break;
@@ -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;
       }
     }