From bf085a1a176f32546f86988049e0c3f809842ce7 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Sat, 16 Jan 2010 00:51:31 -0800 Subject: Convert photo uploading over to the new model based validation approach. - Rearrange Simple_Uploader_Controller::add_photo() to validate the form early in the process, and switch to using model based validation. - Move thumbnail generation into gallery_event::item_created() so that it's decoupled from the model. - Delete photo::create() and move all of its logic into Item_Model::save(). - Add Item_Model::$data_file to track the data file associated with new movies and photos. - Do some cleanup on the validation callbacks -- it turns out the 2nd argument is the field name not the value. --- modules/gallery/controllers/simple_uploader.php | 40 ++++++++++++++----------- 1 file changed, 23 insertions(+), 17 deletions(-) (limited to 'modules/gallery/controllers/simple_uploader.php') diff --git a/modules/gallery/controllers/simple_uploader.php b/modules/gallery/controllers/simple_uploader.php index 5d32e35f..7a7e7557 100644 --- a/modules/gallery/controllers/simple_uploader.php +++ b/modules/gallery/controllers/simple_uploader.php @@ -40,39 +40,45 @@ class Simple_Uploader_Controller extends Controller { access::required("add", $album); access::verify_csrf(); + // The Flash uploader not call /start directly, so simulate it here for now. + if (!batch::in_progress()) { + batch::start(); + } + + $form = $this->_get_add_form($album); + + // Uploadify adds its own field to the form, so validate that separately. $file_validation = new Validation($_FILES); $file_validation->add_rules( "Filedata", "upload::valid", "upload::required", "upload::type[gif,jpg,jpeg,png,flv,mp4]"); - if ($file_validation->validate()) { - // SimpleUploader.swf does not yet call /start directly, so simulate it here for now. - if (!batch::in_progress()) { - batch::start(); - } + if ($form->validate() && $file_validation->validate()) { $temp_filename = upload::save("Filedata"); try { - $name = substr(basename($temp_filename), 10); // Skip unique identifier Kohana adds - $title = item::convert_filename_to_title($name); + $item = ORM::factory("item"); + $item->name = substr(basename($temp_filename), 10); // Skip unique identifier Kohana adds + $item->title = item::convert_filename_to_title($item->name); + $item->parent_id = $album->id; + $item->set_data_file($temp_filename); + $path_info = @pathinfo($temp_filename); if (array_key_exists("extension", $path_info) && in_array(strtolower($path_info["extension"]), array("flv", "mp4"))) { - $item = movie::create($album, $temp_filename, $name, $title); + $item->type = "movie"; + $item->save(); log::success("content", t("Added a movie"), html::anchor("movies/$item->id", t("view movie"))); } else { - $item = photo::create($album, $temp_filename, $name, $title); + $item->type = "photo"; + $item->save(); log::success("content", t("Added a photo"), html::anchor("photos/$item->id", t("view photo"))); } - // We currently have no way of showing errors if validation fails, so only call our event - // handlers if validation passes. - $form = $this->_get_add_form($album); - if ($form->validate()) { - module::event("add_photos_form_completed", $item, $form); - } + module::event("add_photos_form_completed", $item, $form); } catch (Exception $e) { - Kohana_Log::add("alert", $e->__toString()); + // The Flash uploader has no good way of reporting complex errors, so just keep it simple. + Kohana_Log::add("error", $e->getMessage() . "\n" . $e->getTraceAsString()); if (file_exists($temp_filename)) { unlink($temp_filename); } @@ -84,7 +90,7 @@ class Simple_Uploader_Controller extends Controller { print "FILEID: $item->id"; } else { header("HTTP/1.1 400 Bad Request"); - print "ERROR: " . t("Invalid Upload"); + print "ERROR: " . t("Invalid upload"); } } -- cgit v1.2.3 From efdb73cb986002806dfe3c9241f792652e4b56fa Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Sat, 16 Jan 2010 12:00:50 -0800 Subject: Make movie creation use model based validation. Move movie related logic from movie::create() into Item_Model --- modules/gallery/controllers/simple_uploader.php | 7 ++ modules/gallery/helpers/movie.php | 103 ------------------------ modules/gallery/models/item.php | 77 +++++++++++------- 3 files changed, 55 insertions(+), 132 deletions(-) (limited to 'modules/gallery/controllers/simple_uploader.php') diff --git a/modules/gallery/controllers/simple_uploader.php b/modules/gallery/controllers/simple_uploader.php index 7a7e7557..16d1d241 100644 --- a/modules/gallery/controllers/simple_uploader.php +++ b/modules/gallery/controllers/simple_uploader.php @@ -79,6 +79,13 @@ class Simple_Uploader_Controller extends Controller { } catch (Exception $e) { // The Flash uploader has no good way of reporting complex errors, so just keep it simple. Kohana_Log::add("error", $e->getMessage() . "\n" . $e->getTraceAsString()); + + // Ugh. I hate to use instanceof, But this beats catching the exception separately since + // we mostly want to treat it the same way as all other exceptions + if ($e instanceof ORM_Validation_Exception) { + Kohana_Log::add("error", "Validation errors: " . print_r($e->validation->errors(), 1)); + } + if (file_exists($temp_filename)) { unlink($temp_filename); } diff --git a/modules/gallery/helpers/movie.php b/modules/gallery/helpers/movie.php index b0d24f68..0a27ac94 100644 --- a/modules/gallery/helpers/movie.php +++ b/modules/gallery/helpers/movie.php @@ -24,109 +24,6 @@ * Note: by design, this class does not do any permission checking. */ class movie_Core { - /** - * Create a new movie. - * @param integer $parent_id id of parent album - * @param string $filename path to the photo file on disk - * @param string $name the filename to use for this photo in the album - * @param integer $title the title of the new photo - * @param string $description (optional) the longer description of this photo - * @param string $slug (optional) the url component for this photo - * @return Item_Model - */ - static function create($parent, $filename, $name, $title, - $description=null, $owner_id=null, $slug=null) { - if (!$parent->loaded() || !$parent->is_album()) { - throw new Exception("@todo INVALID_PARENT"); - } - - if (!is_file($filename)) { - throw new Exception("@todo MISSING_MOVIE_FILE"); - } - - if (strpos($name, "/")) { - throw new Exception("@todo NAME_CANNOT_CONTAIN_SLASH"); - } - - // We don't allow trailing periods as a security measure - // ref: http://dev.kohanaphp.com/issues/684 - if (rtrim($name, ".") != $name) { - throw new Exception("@todo NAME_CANNOT_END_IN_PERIOD"); - } - - try { - $movie_info = movie::getmoviesize($filename); - } catch (Exception $e) { - // Assuming this is MISSING_FFMPEG for now - $movie_info = getimagesize(MODPATH . "gallery/images/missing_movie.png"); - } - - // Force an extension onto the name - $pi = pathinfo($filename); - if (empty($pi["extension"])) { - $pi["extension"] = image_type_to_extension($movie_info[2], false); - $name .= "." . $pi["extension"]; - } - - if (empty($slug)) { - $slug = item::convert_filename_to_slug($name); - } - - $movie = ORM::factory("item"); - $movie->type = "movie"; - $movie->title = $title; - $movie->description = $description; - $movie->name = $name; - $movie->owner_id = $owner_id ? $owner_id : identity::active_user()->id; - $movie->width = $movie_info[0]; - $movie->height = $movie_info[1]; - $movie->mime_type = strtolower($pi["extension"]) == "mp4" ? "video/mp4" : "video/x-flv"; - $movie->thumb_dirty = 1; - $movie->resize_dirty = 1; - $movie->sort_column = "weight"; - $movie->slug = $slug; - - // Randomize the name if there's a conflict - // @todo Improve this. Random numbers are not user friendly - while (ORM::factory("item") - ->where("parent_id", "=", $parent->id) - ->and_open() - ->where("name", "=", $movie->name) - ->or_where("slug", "=", $movie->slug) - ->close() - ->find()->id) { - $rand = rand(); - $movie->name = "{$name}.$rand.{$pi['extension']}"; - $movie->slug = "{$slug}-$rand"; - } - - // This saves the photo - $movie->add_to_parent($parent); - - // If the thumb or resize already exists then rename it - if (file_exists($movie->resize_path()) || - file_exists($movie->thumb_path())) { - $movie->name = $pi["filename"] . "-" . rand() . "." . $pi["extension"]; - $movie->save(); - } - - copy($filename, $movie->file_path()); - - // @todo: publish this from inside Item_Model::save() when we refactor to the point where - // there's only one save() happening here. - module::event("item_created", $movie); - - // Build our thumbnail - graphics::generate($movie); - - // If the parent has no cover item, make this it. - if (access::can("edit", $parent) && $parent->album_cover_item_id == null) { - item::make_album_cover($movie); - } - - return $movie; - } - static function get_edit_form($movie) { $form = new Forge("movies/update/$movie->id", "", "post", array("id" => "g-edit-movie-form")); $form->hidden("from_id"); diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index a9607699..c007afeb 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -376,7 +376,6 @@ class Item_Model extends ORM_MPTT { unset($significant_changes["relative_url_cache"]); unset($significant_changes["relative_path_cache"]); - if (!empty($this->changed) && $significant_changes) { $this->updated = time(); if (!$this->loaded()) { @@ -403,23 +402,32 @@ class Item_Model extends ORM_MPTT { $this->slug = trim($tmp, "-"); } + // Get the width, height and mime type from our data file for photos and movies. if ($this->is_movie() || $this->is_photo()) { - $image_info = getimagesize($this->data_file); + $pi = pathinfo($this->data_file); if ($this->is_photo()) { + $image_info = getimagesize($this->data_file); $this->width = $image_info[0]; $this->height = $image_info[1]; - $this->mime_type = - empty($image_info['mime']) ? "application/unknown" : $image_info['mime']; - } + $this->mime_type = $image_info["mime"]; - // Force an extension onto the name if necessary - $pi = pathinfo($this->data_file); - if (empty($pi["extension"])) { - $pi["extension"] = image_type_to_extension($image_info[2], false); - $this->name .= "." . $pi["extension"]; - } + // Force an extension onto the name if necessary + if (empty($pi["extension"])) { + $pi["extension"] = image_type_to_extension($image_info[2], false); + $this->name .= "." . $pi["extension"]; + } + } else { + list ($this->width, $this->height) = movie::getmoviesize($this->data_file); + // No extension? Assume FLV. + if (empty($pi["extension"])) { + $pi["extension"] = "flv"; + $this->name .= "." . $pi["extension"]; + } + + $this->mime_type = strtolower($pi["extension"]) == "mp4" ? "video/mp4" : "video/x-flv"; + } } // Randomize the name or slug if there's a conflict. Preserve the extension. @@ -445,8 +453,9 @@ class Item_Model extends ORM_MPTT { parent::save(); - // Build our url caches and save again. If we could depend on a save happening later we - // could defer this 2nd save. + // Build our url caches, then save again. We have to do this after it's already been + // saved once because we use only information from the database to build the paths. If we + // could depend on a save happening later we could defer this 2nd save. $this->_build_relative_caches(); parent::save(); @@ -459,6 +468,7 @@ class Item_Model extends ORM_MPTT { break; 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. @@ -746,26 +756,27 @@ class Item_Model extends ORM_MPTT { * Add some custom per-instance rules. */ public function validate($array=null) { + // validate() is recursive, only modify the rules on the outermost call. if (!$array) { - // The root item has different rules for the name and slug. if ($this->id == 1) { + // Root album can't have a name or slug $this->rules["name"] = array("rules" => array("length[0]")); $this->rules["slug"] = array("rules" => array("length[0]")); + } else { + // Layer some callbacks on top of the existing rules + $this->rules["name"]["callbacks"] = array(array($this, "valid_name")); + $this->rules["slug"]["callbacks"] = array(array($this, "valid_slug")); } - // Names and slugs can't conflict - $this->rules["name"]["callbacks"][] = array($this, "valid_name"); - $this->rules["slug"]["callbacks"][] = array($this, "valid_slug"); - } + // Movies and photos must have data files + if (($this->is_photo() || $this->is_movie()) && !$this->loaded()) { + $this->rules["name"]["callbacks"][] = array($this, "valid_data_file"); + } - // Movies and photos must have data files - if (($this->is_photo() || $this->is_movie()) && !$this->loaded()) { - $this->rules["name"]["callbacks"][] = array($this, "valid_data_file"); + // All items must have a legal parent + $this->rules["parent_id"]["callbacks"] = array(array($this, "valid_parent")); } - // All items must have a legal parent - $this->rules["parent_id"]["callbacks"][] = array($this, "valid_parent"); - parent::validate($array); } @@ -801,11 +812,19 @@ class Item_Model extends ORM_MPTT { } if ($this->is_movie() || $this->is_photo()) { - $new_ext = pathinfo($this->name, PATHINFO_EXTENSION); - $old_ext = pathinfo($this->original()->name, PATHINFO_EXTENSION); - if (strcasecmp($new_ext, $old_ext)) { - $v->add_error("name", "illegal_extension"); - return; + if ($this->loaded()) { + // Existing items can't change their extension + $new_ext = pathinfo($this->name, PATHINFO_EXTENSION); + $old_ext = pathinfo($this->original()->name, PATHINFO_EXTENSION); + if (strcasecmp($new_ext, $old_ext)) { + $v->add_error("name", "illegal_extension"); + return; + } + } else { + // New items must have an extension + if (!pathinfo($this->name, PATHINFO_EXTENSION)) { + $v->add_error("name", "illegal_extension"); + } } } -- cgit v1.2.3