From d45a73777935c86fc5131955831833d7465b5e9d Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 21 Jan 2013 01:22:01 -0500 Subject: Update copyright to 2013. Fixes #1953. --- modules/gallery/helpers/item.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'modules/gallery/helpers/item.php') diff --git a/modules/gallery/helpers/item.php b/modules/gallery/helpers/item.php index b739e8bd..20a865d1 100644 --- a/modules/gallery/helpers/item.php +++ b/modules/gallery/helpers/item.php @@ -1,7 +1,7 @@ Date: Wed, 23 Jan 2013 21:33:19 -0500 Subject: Extract reweighting logic out of Organize_Controller into item::reweight_all_children as an API and write a test for it. Work in progress on #1914. --- modules/gallery/helpers/item.php | 12 ++++++++++++ modules/gallery/tests/Item_Helper_Test.php | 15 +++++++++++++++ modules/organize/controllers/organize.php | 8 +------- 3 files changed, 28 insertions(+), 7 deletions(-) (limited to 'modules/gallery/helpers/item.php') diff --git a/modules/gallery/helpers/item.php b/modules/gallery/helpers/item.php index 20a865d1..b2c4cadf 100644 --- a/modules/gallery/helpers/item.php +++ b/modules/gallery/helpers/item.php @@ -437,4 +437,16 @@ class item_Core { } return call_user_func_array($callback, $args); } + + /** + * Reset all child weights of a given album to a monotonically increasing sequence based on the + * current sort order of the album. + */ + static function resequence_child_weights($album) { + $weight = 0; + foreach ($album->children() as $child) { + $child->weight = ++$weight; + $child->save(); + } + } } \ No newline at end of file diff --git a/modules/gallery/tests/Item_Helper_Test.php b/modules/gallery/tests/Item_Helper_Test.php index 0c08d1af..63081884 100644 --- a/modules/gallery/tests/Item_Helper_Test.php +++ b/modules/gallery/tests/Item_Helper_Test.php @@ -235,4 +235,19 @@ class Item_Helper_Test extends Gallery_Unit_Test_Case { $level3b->id, item::find_by_relative_url("{$level1->slug}/{$level2b->slug}/{$level3b->slug}")->id); } + + public function resequence_child_weights_test() { + $album = test::random_album(); + $photo1 = test::random_photo($album); + $photo2 = test::random_photo($album); + $this->assert_true($photo2->weight > $photo1->weight); + + $album->reload(); + $album->sort_order = "DESC"; + $album->save(); + item::resequence_child_weights($album); + + $this->assert_equal(2, $photo1->reload()->weight); + $this->assert_equal(1, $photo2->reload()->weight); + } } diff --git a/modules/organize/controllers/organize.php b/modules/organize/controllers/organize.php index 97280489..37920020 100644 --- a/modules/organize/controllers/organize.php +++ b/modules/organize/controllers/organize.php @@ -128,13 +128,7 @@ class Organize_Controller extends Controller { access::required("edit", $album); if ($album->sort_column != "weight") { - // Force all the weights into the current order before changing the order to manual - $weight = 0; - foreach ($album->children() as $child) { - $child->weight = ++$weight; - $child->save(); - } - + item::resequence_child_weights($album); $album->sort_column = "weight"; $album->sort_order = "ASC"; $album->save(); -- cgit v1.2.3 From cc2aed656c7289881ba23e8ec700a4daf7889688 Mon Sep 17 00:00:00 2001 From: shadlaws 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/helpers/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 %old_name renamed to %new_name to avoid a conflict", - array("old_name" => $orig_name, "new_name" => $source->name))); - break; - - case "photo": - message::info( - t("Photo %old_name renamed to %new_name 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 %old_name renamed to %new_name to avoid a conflict", + array("old_name" => $orig_name, "new_name" => $source->name))); + break; - case "movie": - message::info( - t("Movie %old_name renamed to %new_name to avoid a conflict", - array("old_name" => $orig_name, "new_name" => $source->name))); - break; - } - } + case "photo": + message::info( + t("Photo %old_name renamed to %new_name 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 %old_name renamed to %new_name 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 cf077425953a6a492dea97eaf1517e7d08c9648f Mon Sep 17 00:00:00 2001 From: shadlaws 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/helpers/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 cff1e76e8da2055f9faf7449222b43a686014b1c Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Wed, 30 Jan 2013 21:08:36 -0500 Subject: When changing the album cover, find and retarget any other albums which were using the old item as their album cover. Fixes #1978. --- modules/gallery/helpers/item.php | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) (limited to 'modules/gallery/helpers/item.php') diff --git a/modules/gallery/helpers/item.php b/modules/gallery/helpers/item.php index 093feb2d..8d3b4354 100644 --- a/modules/gallery/helpers/item.php +++ b/modules/gallery/helpers/item.php @@ -76,16 +76,35 @@ class item_Core { access::required("view", $parent); access::required("edit", $parent); + $old_album_cover_id = $parent->album_cover_item_id; + model_cache::clear(); $parent->album_cover_item_id = $item->is_album() ? $item->album_cover_item_id : $item->id; $parent->save(); graphics::generate($parent); + // Walk up the parent hierarchy and set album covers if necessary $grand_parent = $parent->parent(); if ($grand_parent && access::can("edit", $grand_parent) && $grand_parent->album_cover_item_id == null) { item::make_album_cover($parent); } + + // When albums are album covers themselves, we hotlink directly to the target item. This + // means that when we change an album cover, the grandparent may have a deep link to the old + // album cover. So find any albums that had the old item as their album cover and switch them + // over to the new item. + if ($old_album_cover_id) { + foreach (ORM::factory("item") + ->where("album_cover_item_id", "=", $old_album_cover_id) + ->find_all() as $other_album) { + if (access::can("edit", $other_album)) { + $other_album->album_cover_item_id = $parent->album_cover_item_id; + $other_album->save(); + graphics::generate($other_album); + } + } + } } static function remove_album_cover($album) { -- cgit v1.2.3 From 8d15e5cb2eed56bbd894ff5578d081deee8ca255 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Wed, 30 Jan 2013 21:42:47 -0500 Subject: Follow-in to cff1e76e8da2055f9faf7449222b43a686014b1c for #1978 Restrict which album cover ids we swap over to the hierarchy of the current album, otherwise we can wind up in sticky situations with hierarchical album cover chains. Eg, you have a hierarchy like this: root -> A1 -> A2 --> A3 -> P1 A4 -> P2 P1 is the album cover for its entire hierarchy. But then you swap A2's album cover for A3 making this: root -> A1 -> A2 + A3 -> P1 \-> A4 -> P2 Since A1, A2 and A3 all had P1 as their album cover item id. Now we're swapping it over to P2 but we want to leave P1 as A3's album cover item id. So only look at A4's hierarchy and ignore its peers. --- modules/gallery/helpers/item.php | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) (limited to 'modules/gallery/helpers/item.php') diff --git a/modules/gallery/helpers/item.php b/modules/gallery/helpers/item.php index 8d3b4354..975d46e5 100644 --- a/modules/gallery/helpers/item.php +++ b/modules/gallery/helpers/item.php @@ -92,16 +92,15 @@ class item_Core { // When albums are album covers themselves, we hotlink directly to the target item. This // means that when we change an album cover, the grandparent may have a deep link to the old - // album cover. So find any albums that had the old item as their album cover and switch them - // over to the new item. + // album cover. So find any parent albums that had the old item as their album cover and + // switch them over to the new item. if ($old_album_cover_id) { - foreach (ORM::factory("item") - ->where("album_cover_item_id", "=", $old_album_cover_id) - ->find_all() as $other_album) { - if (access::can("edit", $other_album)) { - $other_album->album_cover_item_id = $parent->album_cover_item_id; - $other_album->save(); - graphics::generate($other_album); + foreach ($item->parents(array(array("album_cover_item_id", "=", $old_album_cover_id))) + as $ancestor) { + if (access::can("edit", $ancestor)) { + $ancestor->album_cover_item_id = $parent->album_cover_item_id; + $ancestor->save(); + graphics::generate($ancestor); } } } -- cgit v1.2.3 From f83ed5f8716663a45c9d8e8118bbcf0e2849c3fb Mon Sep 17 00:00:00 2001 From: shadlaws Date: Thu, 31 Jan 2013 17:18:39 +0100 Subject: #1982 - Add placeholder for albums with no album cover. - Added missing_album_cover.jpg placeholder image. - Modified the graphics helper to use it. Calling graphics::generate will copy it. - Modified item::remove_album_cover and gallery_event::item_created to run graphics::generate. - Added unit test to Graphics_Helper_Test. --- modules/gallery/helpers/gallery_event.php | 20 ++++++++++---------- modules/gallery/helpers/graphics.php | 10 +++++++--- modules/gallery/helpers/item.php | 1 + modules/gallery/images/missing_album_cover.jpg | Bin 0 -> 4453 bytes modules/gallery/tests/Graphics_Helper_Test.php | 11 +++++++++++ 5 files changed, 29 insertions(+), 13 deletions(-) create mode 100644 modules/gallery/images/missing_album_cover.jpg (limited to 'modules/gallery/helpers/item.php') diff --git a/modules/gallery/helpers/gallery_event.php b/modules/gallery/helpers/gallery_event.php index 4bbeccc2..aeb1c7eb 100644 --- a/modules/gallery/helpers/gallery_event.php +++ b/modules/gallery/helpers/gallery_event.php @@ -86,17 +86,17 @@ class gallery_event_Core { static function item_created($item) { access::add_item($item); - if ($item->is_photo() || $item->is_movie()) { - // Build our thumbnail/resizes. - try { - graphics::generate($item); - } catch (Exception $e) { - log::error("graphics", t("Couldn't create a thumbnail or resize for %item_title", - array("item_title" => $item->title)), - html::anchor($item->abs_url(), t("details"))); - Kohana_Log::add("error", $e->getMessage() . "\n" . $e->getTraceAsString()); - } + // Build our thumbnail/resizes. + try { + graphics::generate($item); + } catch (Exception $e) { + log::error("graphics", t("Couldn't create a thumbnail or resize for %item_title", + array("item_title" => $item->title)), + html::anchor($item->abs_url(), t("details"))); + Kohana_Log::add("error", $e->getMessage() . "\n" . $e->getTraceAsString()); + } + if ($item->is_photo() || $item->is_movie()) { // If the parent has no cover item, make this it. $parent = $item->parent(); if (access::can("edit", $parent) && $parent->album_cover_item_id == null) { diff --git a/modules/gallery/helpers/graphics.php b/modules/gallery/helpers/graphics.php index 19ae1036..7c8e89d5 100644 --- a/modules/gallery/helpers/graphics.php +++ b/modules/gallery/helpers/graphics.php @@ -152,6 +152,7 @@ class graphics_Core { } catch (Exception $e) { // Didn't work, likely because of MISSING_FFMPEG - use placeholder graphics::_replace_image_with_placeholder($item, $target); + break; } } $working_file = $output_file; @@ -167,7 +168,7 @@ class graphics_Core { case "album": if (!$cover = $item->album_cover()) { - // This album has no cover; there's nothing to generate. Because of an old bug, it's + // This album has no cover; copy its placeholder image. 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 @@ -179,7 +180,8 @@ class graphics_Core { $item->album_cover_item_id = null; $item->save(); } - return; + graphics::_replace_image_with_placeholder($item, $target); + break; } if ($cover->thumb_dirty) { graphics::generate($cover); @@ -238,7 +240,9 @@ class graphics_Core { } private static function _replace_image_with_placeholder($item, $target) { - if ($item->is_movie() || ($item->is_album() && $item->album_cover()->is_movie())) { + if ($item->is_album() && !$item->album_cover_item_id) { + $input_path = MODPATH . "gallery/images/missing_album_cover.jpg"; + } else if ($item->is_movie() || ($item->is_album() && $item->album_cover()->is_movie())) { $input_path = MODPATH . "gallery/images/missing_movie.jpg"; } else { $input_path = MODPATH . "gallery/images/missing_photo.jpg"; diff --git a/modules/gallery/helpers/item.php b/modules/gallery/helpers/item.php index 975d46e5..9882a9c5 100644 --- a/modules/gallery/helpers/item.php +++ b/modules/gallery/helpers/item.php @@ -113,6 +113,7 @@ class item_Core { model_cache::clear(); $album->album_cover_item_id = null; $album->save(); + graphics::generate($album); } /** diff --git a/modules/gallery/images/missing_album_cover.jpg b/modules/gallery/images/missing_album_cover.jpg new file mode 100644 index 00000000..bdddeec5 Binary files /dev/null and b/modules/gallery/images/missing_album_cover.jpg differ diff --git a/modules/gallery/tests/Graphics_Helper_Test.php b/modules/gallery/tests/Graphics_Helper_Test.php index a68822b0..2cf5caa7 100644 --- a/modules/gallery/tests/Graphics_Helper_Test.php +++ b/modules/gallery/tests/Graphics_Helper_Test.php @@ -77,6 +77,17 @@ class Graphics_Helper_Test extends Gallery_Unit_Test_Case { $this->assert_equal(0, $album->thumb_dirty); } + public function generate_album_cover_for_empty_album_test() { + $album = test::random_album(); + // Check that the album cover is the missing image placeholder + $this->assert_same(file_get_contents(MODPATH . "gallery/images/missing_album_cover.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 image is *not* marked as 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. -- cgit v1.2.3