From 251e9d5c8f727b886676e010481a6090ddac028c Mon Sep 17 00:00:00 2001 From: shadlaws Date: Tue, 26 Feb 2013 18:39:59 +0100 Subject: #2010 - Revise item::find_by_path to search for jpg-converted items. - added extra $var_subdir argument to item::find_by_path. - changed item::find_by_path to use $var_subdir to detect if we should look for a jpg-converted item or not (e.g. movie thumbs) - moved the album thumb detection to item::find_by_path to ensure it knows to look for an exact album match. - added more sanity checks to item::find_by_path (now has fewer false positive possibilities). - updated file_proxy to remove the need to guess different movie files. - updated File_Proxy_Controller - new sanity checks catch previously undetected bug. - added additional unit tests for item::find_by_path. --- modules/gallery/controllers/file_proxy.php | 20 +-- modules/gallery/helpers/item.php | 81 +++++++++-- .../gallery/tests/File_Proxy_Controller_Test.php | 2 +- modules/gallery/tests/Item_Helper_Test.php | 159 +++++++++++++++++++-- 4 files changed, 220 insertions(+), 42 deletions(-) (limited to 'modules/gallery') diff --git a/modules/gallery/controllers/file_proxy.php b/modules/gallery/controllers/file_proxy.php index 7e5d0038..ac558a71 100644 --- a/modules/gallery/controllers/file_proxy.php +++ b/modules/gallery/controllers/file_proxy.php @@ -66,24 +66,8 @@ class File_Proxy_Controller extends Controller { throw $e; } - // If the last element is .album.jpg, pop that off since it's not a real item - $path = preg_replace("|/.album.jpg$|", "", $path); - - $item = item::find_by_path($path); - if (!$item->loaded()) { - // We didn't turn it up. If we're looking for a .jpg then it's it's possible that we're - // requesting the thumbnail for a movie. In that case, the movie file would - // have been converted to a .jpg. So try some alternate types: - if (preg_match('/.jpg$/', $path)) { - foreach (legal_file::get_movie_extensions() as $ext) { - $movie_path = preg_replace('/.jpg$/', ".$ext", $path); - $item = item::find_by_path($movie_path); - if ($item->loaded()) { - break; - } - } - } - } + // Get the item model using the path and type (which corresponds to a var subdir) + $item = item::find_by_path($path, $type); if (!$item->loaded()) { $e = new Kohana_404_Exception(); diff --git a/modules/gallery/helpers/item.php b/modules/gallery/helpers/item.php index 9882a9c5..bbbc81d6 100644 --- a/modules/gallery/helpers/item.php +++ b/modules/gallery/helpers/item.php @@ -203,10 +203,18 @@ class item_Core { /** * Find an item by its path. If there's no match, return an empty Item_Model. * NOTE: the caller is responsible for performing security checks on the resulting item. + * + * In addition to $path, $var_subdir can be specified ("albums", "resizes", or "thumbs"). This + * corresponds to the file's directory in var, which is what's used in file_proxy. By specifying + * this, we can be smarter about items whose formats get converted (e.g. movies that get jpg + * thumbs). If omitted, it defaults to "albums" which looks for identical matches between $path + * and the item name, just like pre-v3.1 behavior. + * * @param string $path + * @param string $var_subdir * @return object Item_Model */ - static function find_by_path($path) { + static function find_by_path($path, $var_subdir="albums") { $path = trim($path, "/"); // The root path name is NULL not "", hence this workaround. @@ -214,35 +222,80 @@ class item_Core { return item::root(); } + $search_full_name = true; + $album_thumb = false; + if (($var_subdir == "thumbs") && preg_match("|^(.*)/\.album\.jpg$|", $path, $matches)) { + // It's an album thumb - remove "/.album.jpg" from the path. + $path = $matches[1]; + $album_thumb = true; + } else if (($var_subdir != "albums") && preg_match("/^(.*)\.jpg$/", $path, $matches)) { + // Item itself could be non-jpg (e.g. movies) - remove .jpg from path, don't search full name. + $path = $matches[1]; + $search_full_name = false; + } + // Check to see if there's an item in the database with a matching relative_path_cache value. - // Since that field is urlencoded, we must urlencoded the components of the path. + // Since that field is urlencoded, we must urlencode the components of the path. foreach (explode("/", $path) as $part) { $encoded_array[] = rawurlencode($part); } $encoded_path = join("/", $encoded_array); - $item = ORM::factory("item") - ->where("relative_path_cache", "=", $encoded_path) - ->find(); - if ($item->loaded()) { - return $item; + if ($search_full_name) { + $item = ORM::factory("item") + ->where("relative_path_cache", "=", $encoded_path) + ->find(); + // See if the item was found and if it should have been found. + if ($item->loaded() && + (($var_subdir == "albums") || $item->is_photo() || $album_thumb)) { + return $item; + } + } else { + // Note that the below query uses LIKE with wildcard % at end, which is still sargable and + // therefore still takes advantage of the indexed relative_path_cache (i.e. still quick). + $item = ORM::factory("item") + ->where("relative_path_cache", "LIKE", Database::escape_for_like($encoded_path) . ".%") + ->find(); + // See if the item was found and should be a jpg. + if ($item->loaded() && + (($item->is_movie() && ($var_subdir == "thumbs")) || + ($item->is_photo() && (preg_match("/^(.*)\.jpg$/", $item->name))))) { + return $item; + } } // Since the relative_path_cache field is a cache, it can be unavailable. If we don't find // anything, fall back to checking the path the hard way. $paths = explode("/", $path); - foreach (ORM::factory("item") - ->where("name", "=", end($paths)) - ->where("level", "=", count($paths) + 1) - ->find_all() as $item) { - if (urldecode($item->relative_path()) == $path) { - return $item; + if ($search_full_name) { + foreach (ORM::factory("item") + ->where("name", "=", end($paths)) + ->where("level", "=", count($paths) + 1) + ->find_all() as $item) { + // See if the item was found and if it should have been found. + if ((urldecode($item->relative_path()) == $path) && + (($var_subdir == "albums") || $item->is_photo() || $album_thumb)) { + return $item; + } + } + } else { + foreach (ORM::factory("item") + ->where("name", "LIKE", Database::escape_for_like(end($paths)) . ".%") + ->where("level", "=", count($paths) + 1) + ->find_all() as $item) { + // Compare relative_path without extension (regexp same as legal_file::change_extension), + // see if it should be a jpg. + if ((preg_replace("/\.[^\.\/]*?$/", "", urldecode($item->relative_path())) == $path) && + (($item->is_movie() && ($var_subdir == "thumbs")) || + ($item->is_photo() && (preg_match("/^(.*)\.jpg$/", $item->name))))) { + return $item; + } } } + // Nothing found - return an empty item model. return new Item_Model(); } - /** * Locate an item using the URL. We assume that the url is in the form /a/b/c where each * component matches up with an item slug. If there's no match, return an empty Item_Model diff --git a/modules/gallery/tests/File_Proxy_Controller_Test.php b/modules/gallery/tests/File_Proxy_Controller_Test.php index 562100e4..06068d62 100644 --- a/modules/gallery/tests/File_Proxy_Controller_Test.php +++ b/modules/gallery/tests/File_Proxy_Controller_Test.php @@ -66,7 +66,7 @@ class File_Proxy_Controller_Test extends Gallery_Unit_Test_Case { public function movie_thumbnails_are_jpgs_test() { $movie = test::random_movie(); $name = legal_file::change_extension($movie->name, "jpg"); - $_SERVER["REQUEST_URI"] = url::file("var/thumbs/{$movie->name}"); + $_SERVER["REQUEST_URI"] = url::file("var/thumbs/$name"); $controller = new File_Proxy_Controller(); $this->assert_same($movie->thumb_path(), $controller->__call("", array())); } diff --git a/modules/gallery/tests/Item_Helper_Test.php b/modules/gallery/tests/Item_Helper_Test.php index f5b99bec..f4995c53 100644 --- a/modules/gallery/tests/Item_Helper_Test.php +++ b/modules/gallery/tests/Item_Helper_Test.php @@ -164,11 +164,9 @@ class Item_Helper_Test extends Gallery_Unit_Test_Case { $this->assert_same(item::root()->id, item::find_by_path("")->id); // Verify that we don't get confused by the part names, using the fallback code. - db::build() - ->update("items") - ->set(array("relative_path_cache" => null)) - ->where("id", "IN", array($level3->id, $level3b->id)) - ->execute(); + self::_remove_relative_path_caches(); + self::_remove_relative_path_caches(); + $this->assert_same( $level3->id, item::find_by_path("{$level1->name}/{$level2->name}/{$level3->name}")->id); @@ -180,11 +178,154 @@ class Item_Helper_Test extends Gallery_Unit_Test_Case { // Verify that we don't get false positives $this->assert_false( item::find_by_path("foo/bar/baz")->loaded()); + } - // Verify that the fallback code works - $this->assert_same( - $level3b->id, - item::find_by_path("{$level1->name}/{$level2b->name}/{$level3b->name}")->id); + public function find_by_path_with_jpg_test() { + $parent = test::random_album(); + $jpg = test::random_photo($parent); + + $jpg_path = "{$parent->name}/{$jpg->name}"; + $flv_path = legal_file::change_extension($jpg_path, "flv"); + + // Check normal operation. + $this->assert_equal($jpg->id, item::find_by_path($jpg_path, "albums")->id); + $this->assert_equal($jpg->id, item::find_by_path($jpg_path, "resizes")->id); + $this->assert_equal($jpg->id, item::find_by_path($jpg_path, "thumbs")->id); + $this->assert_equal($jpg->id, item::find_by_path($jpg_path)->id); + + // Check that we don't get false positives. + $this->assert_equal(null, item::find_by_path($flv_path, "albums")->id); + $this->assert_equal(null, item::find_by_path($flv_path, "resizes")->id); + $this->assert_equal(null, item::find_by_path($flv_path, "thumbs")->id); + $this->assert_equal(null, item::find_by_path($flv_path)->id); + + // Check normal operation without relative path cache. + self::_remove_relative_path_caches(); + $this->assert_equal($jpg->id, item::find_by_path($jpg_path, "albums")->id); + self::_remove_relative_path_caches(); + $this->assert_equal($jpg->id, item::find_by_path($jpg_path, "resizes")->id); + self::_remove_relative_path_caches(); + $this->assert_equal($jpg->id, item::find_by_path($jpg_path, "thumbs")->id); + self::_remove_relative_path_caches(); + $this->assert_equal($jpg->id, item::find_by_path($jpg_path)->id); + + // Check that we don't get false positives without relative path cache. + self::_remove_relative_path_caches(); + $this->assert_equal(null, item::find_by_path($flv_path, "albums")->id); + $this->assert_equal(null, item::find_by_path($flv_path, "resizes")->id); + $this->assert_equal(null, item::find_by_path($flv_path, "thumbs")->id); + $this->assert_equal(null, item::find_by_path($flv_path)->id); + } + + public function find_by_path_with_png_test() { + $parent = test::random_album(); + $png = test::random_photo_unsaved($parent); + $png->set_data_file(MODPATH . "gallery/images/graphicsmagick.png"); + $png->save(); + + $png_path = "{$parent->name}/{$png->name}"; + $jpg_path = legal_file::change_extension($png_path, "jpg"); + + // Check normal operation. + $this->assert_equal($png->id, item::find_by_path($png_path, "albums")->id); + $this->assert_equal($png->id, item::find_by_path($png_path, "resizes")->id); + $this->assert_equal($png->id, item::find_by_path($png_path, "thumbs")->id); + $this->assert_equal($png->id, item::find_by_path($png_path)->id); + + // Check that we don't get false positives. + $this->assert_equal(null, item::find_by_path($jpg_path, "albums")->id); + $this->assert_equal(null, item::find_by_path($jpg_path, "resizes")->id); + $this->assert_equal(null, item::find_by_path($jpg_path, "thumbs")->id); + $this->assert_equal(null, item::find_by_path($jpg_path)->id); + + // Check normal operation without relative path cache. + self::_remove_relative_path_caches(); + $this->assert_equal($png->id, item::find_by_path($png_path, "albums")->id); + self::_remove_relative_path_caches(); + $this->assert_equal($png->id, item::find_by_path($png_path, "resizes")->id); + self::_remove_relative_path_caches(); + $this->assert_equal($png->id, item::find_by_path($png_path, "thumbs")->id); + self::_remove_relative_path_caches(); + $this->assert_equal($png->id, item::find_by_path($png_path)->id); + + // Check that we don't get false positives without relative path cache. + self::_remove_relative_path_caches(); + $this->assert_equal(null, item::find_by_path($jpg_path, "albums")->id); + $this->assert_equal(null, item::find_by_path($jpg_path, "resizes")->id); + $this->assert_equal(null, item::find_by_path($jpg_path, "thumbs")->id); + $this->assert_equal(null, item::find_by_path($jpg_path)->id); + } + + public function find_by_path_with_flv_test() { + $parent = test::random_album(); + $flv = test::random_movie($parent); + + $flv_path = "{$parent->name}/{$flv->name}"; + $jpg_path = legal_file::change_extension($flv_path, "jpg"); + + // Check normal operation. + $this->assert_equal($flv->id, item::find_by_path($flv_path, "albums")->id); + $this->assert_equal($flv->id, item::find_by_path($jpg_path, "thumbs")->id); + $this->assert_equal($flv->id, item::find_by_path($flv_path)->id); + + // Check that we don't get false positives. + $this->assert_equal(null, item::find_by_path($jpg_path, "albums")->id); + $this->assert_equal(null, item::find_by_path($flv_path, "thumbs")->id); + $this->assert_equal(null, item::find_by_path($jpg_path)->id); + + // Check normal operation without relative path cache. + self::_remove_relative_path_caches(); + $this->assert_equal($flv->id, item::find_by_path($flv_path, "albums")->id); + self::_remove_relative_path_caches(); + $this->assert_equal($flv->id, item::find_by_path($jpg_path, "thumbs")->id); + self::_remove_relative_path_caches(); + $this->assert_equal($flv->id, item::find_by_path($flv_path)->id); + + // Check that we don't get false positives without relative path cache. + self::_remove_relative_path_caches(); + $this->assert_equal(null, item::find_by_path($jpg_path, "albums")->id); + $this->assert_equal(null, item::find_by_path($flv_path, "thumbs")->id); + $this->assert_equal(null, item::find_by_path($jpg_path)->id); + } + + public function find_by_path_with_album_test() { + $parent = test::random_album(); + $album = test::random_movie($parent); + + $album_path = "{$parent->name}/{$album->name}"; + $thumb_path = "{$album_path}/.album.jpg"; + + // Check normal operation. + $this->assert_equal($album->id, item::find_by_path($album_path, "albums")->id); + $this->assert_equal($album->id, item::find_by_path($thumb_path, "thumbs")->id); + $this->assert_equal($album->id, item::find_by_path($album_path)->id); + + // Check that we don't get false positives. + $this->assert_equal(null, item::find_by_path($thumb_path, "albums")->id); + $this->assert_equal(null, item::find_by_path($album_path, "thumbs")->id); + $this->assert_equal(null, item::find_by_path($thumb_path)->id); + + // Check normal operation without relative path cache. + self::_remove_relative_path_caches(); + $this->assert_equal($album->id, item::find_by_path($album_path, "albums")->id); + self::_remove_relative_path_caches(); + $this->assert_equal($album->id, item::find_by_path($thumb_path, "thumbs")->id); + self::_remove_relative_path_caches(); + $this->assert_equal($album->id, item::find_by_path($album_path)->id); + + // Check that we don't get false positives without relative path cache. + self::_remove_relative_path_caches(); + $this->assert_equal(null, item::find_by_path($thumb_path, "albums")->id); + $this->assert_equal(null, item::find_by_path($album_path, "thumbs")->id); + $this->assert_equal(null, item::find_by_path($thumb_path)->id); + } + + private function _remove_relative_path_caches() { + // This gets used *many* times in the find_by_path tests above to check the fallback code. + db::build() + ->update("items") + ->set("relative_path_cache", null) + ->execute(); } public function find_by_relative_url_test() { -- cgit v1.2.3