From b3e328c9ff4c3e19df4b6d18da947b759fe0c201 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Thu, 14 Jan 2010 21:04:09 -0800 Subject: Begin the process of converting to model based validation. Right now only Albums_Controller::update() supports the pattern. All form and controller based validation happening when editing an album has been moved over. Model based validation means that our REST controllers share the same validation as web controllers. We'll have consistency enforced at the model level, which is a Good Thing. The basic pattern is now: 1) Rules are in the model 2) ORM::validate() (which is called by ORM::save() but you can call it directly, too) checks the model for all the rules and throws an ORM_Validation_Exception if there are failures 3) Actions are no longer taken when you call Item_Model::__set(). Instead, they're all queued up and executed when you call Item_Model::save(). Notes: - item::validate_xxx() functions are now in Item_Model:: - We still call $form->validate() because the form can have rules (and forms triggered by events will likely continue to have rules. --- modules/gallery/models/item.php | 139 +++++++++++++++++++++++++--------------- 1 file changed, 87 insertions(+), 52 deletions(-) (limited to 'modules/gallery/models') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index 4a3d26e9..19bdf655 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -21,11 +21,11 @@ class Item_Model extends ORM_MPTT { protected $children = 'items'; protected $sorting = array(); - var $form_rules = array( - "name" => "required|length[0,255]", - "title" => "required|length[0,255]", - "description" => "length[0,65535]", - "slug" => "required|length[0,255]" + var $rules = array( + "name" => array("rules" => array("length[0,255]", "required")), + "title" => array("rules" => array("length[0,255]", "required")), + "slug" => array("rules" => array("length[0,255]", "required")), + "description" => array("rules" => array("length[0,65535]")) ); /** @@ -146,21 +146,12 @@ class Item_Model extends ORM_MPTT { } /** - * Rename the underlying file for this item to a new name. Move all the files. This requires a - * save. + * Rename the underlying file for this item to a new name and move all related files. * * @chainable */ - public function rename($new_name) { - if ($new_name == $this->name) { - return; - } - - if (strpos($new_name, "/")) { - throw new Exception("@todo NAME_CANNOT_CONTAIN_SLASH"); - } - - $old_relative_path = urldecode($this->relative_path()); + private function rename($new_name) { + $old_relative_path = urldecode($this->original()->relative_path()); $new_relative_path = dirname($old_relative_path) . "/" . $new_name; if (file_exists(VARPATH . "albums/$new_relative_path")) { throw new Exception("@todo INVALID_RENAME_FILE_EXISTS: $new_relative_path"); @@ -178,18 +169,6 @@ class Item_Model extends ORM_MPTT { @rename(VARPATH . "thumbs/$old_relative_path", VARPATH . "thumbs/$new_relative_path"); } - $this->name = $new_name; - - if ($this->is_album()) { - db::build() - ->update("items") - ->set("relative_url_cache", null) - ->set("relative_path_cache", null) - ->where("left_ptr", ">", $this->left_ptr) - ->where("right_ptr", "<", $this->right_ptr) - ->execute(); - } - return $this; } @@ -375,29 +354,6 @@ class Item_Model extends ORM_MPTT { } } - /** - * @see ORM::__set() - */ - public function __set($column, $value) { - if ($column == "name") { - $this->relative_path_cache = null; - } else if ($column == "slug") { - if ($this->slug != $value) { - // Clear the relative url cache for this item and all children - $this->relative_url_cache = null; - if ($this->is_album()) { - db::build() - ->update("items") - ->set("relative_url_cache", null) - ->where("left_ptr", ">", $this->left_ptr) - ->where("right_ptr", "<", $this->right_ptr) - ->execute(); - } - } - } - parent::__set($column, $value); - } - /** * @see ORM::save() */ @@ -414,9 +370,34 @@ class Item_Model extends ORM_MPTT { $this->weight = item::get_max_weight(); } else { $send_event = 1; + + if ($this->original()->name != $this->name) { + $this->rename($this->name); + $this->relative_path_cache = null; + } + + if ($this->original()->slug != $this->slug) { + // Clear the relative url cache for this item and all children + $this->relative_url_cache = null; + } + + // Changing the name or the slug ripples downwards + if ($this->is_album() && + ($this->original()->name != $this->name || + $this->original()->slug != $this->slug)) { + db::build() + ->update("items") + ->set("relative_url_cache", null) + ->set("relative_path_cache", null) + ->where("left_ptr", ">", $this->left_ptr) + ->where("right_ptr", "<", $this->right_ptr) + ->execute(); + } } } + parent::save(); + if (isset($send_event)) { module::event("item_updated", $this->original(), $this); } @@ -655,4 +636,58 @@ class Item_Model extends ORM_MPTT { } return parent::descendants($limit, $offset, $where, $order_by); } + + /** + * Add some custom per-instance rules. + */ + public function validate($array=null) { + if (!$array) { + // The root item has different rules for the name and slug. + if ($this->id == 1) { + $this->rules["name"]["rules"][] = "length[0]"; + $this->rules["slug"]["rules"][] = "length[0]"; + } + + // Names and slugs can't conflict + $this->rules["name"]["callbacks"][] = array($this, "valid_name"); + $this->rules["slug"]["callbacks"][] = array($this, "valid_slug"); + } + + parent::validate($array); + } + + /** + * Validate that the desired slug does not conflict. + */ + public function valid_slug(Validation $v, $value) { + if (preg_match("/[^A-Za-z0-9-_]/", $value)) { + $v->add_error("slug", "not_url_safe"); + } else if ($row = db::build() + ->from("items") + ->where("parent_id", "=", $this->parent_id) + ->where("id", "<>", $this->id) + ->where("slug", "=", $this->slug) + ->count_records()) { + $v->add_error("slug", "conflict"); + } + } + + /** + * Validate the item name. It can't conflict with other names, can't contain slashes or + * trailing periods. + */ + public function valid_name(Validation $v, $value) { + if (strpos($value, "/") !== false) { + $v->add_error("name", "no_slashes"); + } else if (rtrim($value, ".") !== $value) { + $v->add_error("name", "no_trailing_period"); + } else if ($row = db::build() + ->from("items") + ->where("parent_id", "=", $this->parent_id) + ->where("id", "<>", $this->id) + ->where("name", "=", $this->name) + ->count_records()) { + $v->add_error("name", "conflict"); + } + } } -- cgit v1.2.3 From 1a557ce5a6e367b37e95915290c092b769ed206a Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 15 Jan 2010 10:36:56 -0800 Subject: Use $value in valid_xxx() functions instead of the member field. They're equivalent, but it's more intuitive this way. --- modules/gallery/models/item.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'modules/gallery/models') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index 19bdf655..9edc65ce 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -666,7 +666,7 @@ class Item_Model extends ORM_MPTT { ->from("items") ->where("parent_id", "=", $this->parent_id) ->where("id", "<>", $this->id) - ->where("slug", "=", $this->slug) + ->where("slug", "=", $value) ->count_records()) { $v->add_error("slug", "conflict"); } @@ -685,7 +685,7 @@ class Item_Model extends ORM_MPTT { ->from("items") ->where("parent_id", "=", $this->parent_id) ->where("id", "<>", $this->id) - ->where("name", "=", $this->name) + ->where("name", "=", $value) ->count_records()) { $v->add_error("name", "conflict"); } -- cgit v1.2.3 From 94f58e8b65b78cafed8f07f70a48b7b271cfc212 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 15 Jan 2010 10:48:39 -0800 Subject: Move setting Item_Model::rand_key into Item_Model::save() since it's business logic. --- modules/gallery/helpers/album.php | 1 - modules/gallery/helpers/movie.php | 1 - modules/gallery/helpers/photo.php | 1 - modules/gallery/models/item.php | 1 + 4 files changed, 1 insertion(+), 3 deletions(-) (limited to 'modules/gallery/models') diff --git a/modules/gallery/helpers/album.php b/modules/gallery/helpers/album.php index 477f1945..52759414 100644 --- a/modules/gallery/helpers/album.php +++ b/modules/gallery/helpers/album.php @@ -61,7 +61,6 @@ class album_Core { $album->thumb_dirty = 1; $album->resize_dirty = 1; $album->slug = $slug; - $album->rand_key = ((float)mt_rand()) / (float)mt_getrandmax(); $album->sort_column = "created"; $album->sort_order = "ASC"; diff --git a/modules/gallery/helpers/movie.php b/modules/gallery/helpers/movie.php index 01859924..b0d24f68 100644 --- a/modules/gallery/helpers/movie.php +++ b/modules/gallery/helpers/movie.php @@ -85,7 +85,6 @@ class movie_Core { $movie->resize_dirty = 1; $movie->sort_column = "weight"; $movie->slug = $slug; - $movie->rand_key = ((float)mt_rand()) / (float)mt_getrandmax(); // Randomize the name if there's a conflict // @todo Improve this. Random numbers are not user friendly diff --git a/modules/gallery/helpers/photo.php b/modules/gallery/helpers/photo.php index 4e20e610..aeae7f56 100644 --- a/modules/gallery/helpers/photo.php +++ b/modules/gallery/helpers/photo.php @@ -84,7 +84,6 @@ class photo_Core { $photo->resize_dirty = 1; $photo->sort_column = "weight"; $photo->slug = $slug; - $photo->rand_key = ((float)mt_rand()) / (float)mt_getrandmax(); // Randomize the name or slug if there's a conflict // @todo Improve this. Random numbers are not user friendly diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index 9edc65ce..33b36ff1 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -368,6 +368,7 @@ class Item_Model extends ORM_MPTT { if (!$this->loaded()) { $this->created = $this->updated; $this->weight = item::get_max_weight(); + $this->rand_key = ((float)mt_rand()) / (float)mt_getrandmax(); } else { $send_event = 1; -- cgit v1.2.3 From 50e3cc5837df7b0ae8e2d43a3dacee7500ba6db8 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 15 Jan 2010 12:15:20 -0800 Subject: Move model business logic out of album::create() and into Item_Model::save(). This makes creating albums similar to editing them and makes it difficult to create an album poorly. I expect to be able to remove a lot of code from the photo and movie helper because it's duplicated here. In order to do this, I refactored ORM_MPTT::add_to_parent() into ORM_MPTT::save() so we now add it to the parent when we do save. This allows us to call save() only once which saves a database call per add. The Albums_Controller logic is roughly the same as before. Haven't updated the tests yet, they're going to fail miserably since many of them depend on album::create() which is now gone. --- modules/gallery/controllers/albums.php | 42 +++++++++++-------- modules/gallery/helpers/album.php | 67 ------------------------------ modules/gallery/libraries/ORM_MPTT.php | 59 +++++++++++++------------- modules/gallery/models/item.php | 75 +++++++++++++++++++++++++++++----- 4 files changed, 118 insertions(+), 125 deletions(-) (limited to 'modules/gallery/models') diff --git a/modules/gallery/controllers/albums.php b/modules/gallery/controllers/albums.php index 9f8c16ea..7658a913 100644 --- a/modules/gallery/controllers/albums.php +++ b/modules/gallery/controllers/albums.php @@ -96,29 +96,35 @@ class Albums_Controller extends Items_Controller { access::required("add", $album); $form = album::get_add_form($album); - if ($form->validate()) { - $new_album = album::create( - $album, - $form->add_album->inputs["name"]->value, - $form->add_album->title->value ? - $form->add_album->title->value : $form->add_album->inputs["name"]->value, - $form->add_album->description->value, - identity::active_user()->id, - $form->add_album->slug->value); + try { + $valid = $form->validate(); + $album = ORM::factory("item"); + $album->type = "album"; + $album->parent_id = $parent_id; + $album->name = $form->add_album->inputs["name"]->value; + $album->title = $form->add_album->title->value ? + $form->add_album->title->value : $form->add_album->inputs["name"]->value; + $album->description = $form->add_album->description->value; + $album->slug = $form->add_album->slug->value; + $album->validate(); + } catch (ORM_Validation_Exception $e) { + // Translate ORM validation errors into form error messages + foreach ($e->validation->errors() as $key => $error) { + $form->add_album->inputs[$key]->add_error($error, 1); + } + $valid = false; + } + if ($valid) { + $album->save(); log::success("content", "Created an album", - html::anchor("albums/$new_album->id", "view album")); + html::anchor("albums/$album->id", "view album")); message::success(t("Created album %album_title", - array("album_title" => html::purify($new_album->title)))); + array("album_title" => html::purify($album->title)))); - print json_encode( - array("result" => "success", - "location" => $new_album->url())); + print json_encode(array("result" => "success", "location" => $album->url())); } else { - print json_encode( - array( - "result" => "error", - "form" => $form->__toString())); + print json_encode(array("result" => "error", "form" => (string) $form)); } } diff --git a/modules/gallery/helpers/album.php b/modules/gallery/helpers/album.php index 52759414..e99770e9 100644 --- a/modules/gallery/helpers/album.php +++ b/modules/gallery/helpers/album.php @@ -24,71 +24,6 @@ * Note: by design, this class does not do any permission checking. */ class album_Core { - /** - * Create a new album. - * @param integer $parent_id id of parent album - * @param string $name the name of this new album (it will become the directory name on disk) - * @param integer $title the title of the new album - * @param string $description (optional) the longer description of this album - * @param string $slug (optional) the url component for this photo - * @return Item_Model - */ - static function create($parent, $name, $title, $description=null, $owner_id=null, $slug=null) { - if (!$parent->loaded() || !$parent->is_album()) { - throw new Exception("@todo INVALID_PARENT"); - } - - 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"); - } - - if (empty($slug)) { - $slug = item::convert_filename_to_slug($name); - } - - $album = ORM::factory("item"); - $album->type = "album"; - $album->title = $title; - $album->description = $description; - $album->name = $name; - $album->owner_id = $owner_id; - $album->thumb_dirty = 1; - $album->resize_dirty = 1; - $album->slug = $slug; - $album->sort_column = "created"; - $album->sort_order = "ASC"; - - // Randomize the name or slug 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", "=", $album->name) - ->or_where("slug", "=", $album->slug) - ->close() - ->find()->id) { - $rand = rand(); - $album->name = "{$name}-$rand"; - $album->slug = "{$slug}-$rand"; - } - - $album = $album->add_to_parent($parent); - mkdir($album->file_path()); - mkdir(dirname($album->thumb_path())); - mkdir(dirname($album->resize_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", $album); - - return $album; - } static function get_add_form($parent) { $form = new Forge("albums/create/{$parent->id}", "", "post", array("id" => "g-add-album-form")); @@ -97,10 +32,8 @@ class album_Core { $group->input("title")->label(t("Title")); $group->textarea("description")->label(t("Description")); $group->input("name")->label(t("Directory name")) - ->callback("item::validate_no_slashes") ->error_messages("no_slashes", t("The directory name can't contain the \"/\" character")); $group->input("slug")->label(t("Internet Address")) - ->callback("item::validate_url_safe") ->error_messages( "not_url_safe", t("The internet address should contain only letters, numbers, hyphens and underscores")); diff --git a/modules/gallery/libraries/ORM_MPTT.php b/modules/gallery/libraries/ORM_MPTT.php index ed77cac9..46ae0af8 100644 --- a/modules/gallery/libraries/ORM_MPTT.php +++ b/modules/gallery/libraries/ORM_MPTT.php @@ -40,44 +40,43 @@ class ORM_MPTT_Core extends ORM { } /** - * Add this node as a child of the parent provided. + * Overload ORM::save() to update the MPTT tree when we add new items to the hierarchy. * * @chainable - * @param integer $parent_id the id of the parent node - * @return ORM + * @return ORM */ - function add_to_parent($parent) { - $this->lock(); - $parent->reload(); // Assume that the prior lock holder may have changed the parent + function save() { + if (!$this->loaded()) { + $this->lock(); + $parent = ORM::factory("item")->where("id", "=", $this->parent_id)->find(); - try { - // Make a hole in the parent for this new item - $this->db_builder - ->update($this->table_name) - ->set("left_ptr", new Database_Expression("`left_ptr` + 2")) - ->where("left_ptr", ">=", $parent->right_ptr) - ->execute(); - $this->db_builder - ->update($this->table_name) - ->set("right_ptr", new Database_Expression("`right_ptr` + 2")) - ->where("right_ptr", ">=", $parent->right_ptr) - ->execute(); - $parent->right_ptr += 2; + try { + // Make a hole in the parent for this new item + $this->db_builder + ->update($this->table_name) + ->set("left_ptr", new Database_Expression("`left_ptr` + 2")) + ->where("left_ptr", ">=", $parent->right_ptr) + ->execute(); + $this->db_builder + ->update($this->table_name) + ->set("right_ptr", new Database_Expression("`right_ptr` + 2")) + ->where("right_ptr", ">=", $parent->right_ptr) + ->execute(); + $parent->right_ptr += 2; - // Insert this item into the hole - $this->left_ptr = $parent->right_ptr - 2; - $this->right_ptr = $parent->right_ptr - 1; - $this->parent_id = $parent->id; - $this->level = $parent->level + 1; - $this->save(); - $parent->reload(); - } catch (Exception $e) { + // Insert this item into the hole + $this->left_ptr = $parent->right_ptr - 2; + $this->right_ptr = $parent->right_ptr - 1; + $this->parent_id = $parent->id; + $this->level = $parent->level + 1; + } catch (Exception $e) { + $this->unlock(); + throw $e; + } $this->unlock(); - throw $e; } - $this->unlock(); - return $this; + return parent::save(); } /** diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index 33b36ff1..e929f30d 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -25,7 +25,8 @@ class Item_Model extends ORM_MPTT { "name" => array("rules" => array("length[0,255]", "required")), "title" => array("rules" => array("length[0,255]", "required")), "slug" => array("rules" => array("length[0,255]", "required")), - "description" => array("rules" => array("length[0,65535]")) + "description" => array("rules" => array("length[0,65535]")), + "parent_id" => array("rules" => array("Item_Model::valid_parent")) ); /** @@ -355,7 +356,10 @@ class Item_Model extends ORM_MPTT { } /** + * Handle any business logic necessary to create an item. * @see ORM::save() + * + * @return ORM Item_Model */ public function save() { $significant_changes = $this->changed; @@ -366,12 +370,55 @@ class Item_Model extends ORM_MPTT { if (!empty($this->changed) && $significant_changes) { $this->updated = time(); if (!$this->loaded()) { + // Create a new item. Use whatever fields are set, and specify defaults for the rest. $this->created = $this->updated; $this->weight = item::get_max_weight(); $this->rand_key = ((float)mt_rand()) / (float)mt_getrandmax(); - } else { - $send_event = 1; + $this->thumb_dirty = 1; + $this->resize_dirty = 1; + if (empty($this->sort_column)) { + $this->sort_column = "created"; + } + if (empty($this->sort_order)) { + $this->sort_order = "ASC"; + } + if (empty($this->owner_id)) { + $this->owner_id = identity::active_user()->id; + } + if (empty($this->slug)) { + $tmp = pathinfo($this->name, PATHINFO_FILENAME); + $tmp = preg_replace("/[^A-Za-z0-9-_]+/", "-", $tmp); + $this->slug = trim($tmp, "-"); + } + + // Randomize the name or slug if there's a conflict + // @todo Improve this. Random numbers are not user friendly + $base_name = $this->name; + $base_slug = $this->slug; + while (ORM::factory("item") + ->where("parent_id", "=", $this->parent_id) + ->and_open() + ->where("name", "=", $this->name) + ->or_where("slug", "=", $this->slug) + ->close() + ->find()->id) { + $rand = rand(); + $this->name = "$base_name-$rand"; + $this->slug = "$base_slug-$rand"; + } + + parent::save(); + // Call this after we finish saving so that the paths are correct. + if ($this->is_album()) { + mkdir($this->file_path()); + mkdir(dirname($this->thumb_path())); + mkdir(dirname($this->resize_path())); + } + + module::event("item_created", $this); + } else { + // Update an existing item if ($this->original()->name != $this->name) { $this->rename($this->name); $this->relative_path_cache = null; @@ -394,14 +441,11 @@ class Item_Model extends ORM_MPTT { ->where("right_ptr", "<", $this->right_ptr) ->execute(); } + parent::save(); + module::event("item_updated", $this->original(), $this); } } - parent::save(); - - if (isset($send_event)) { - module::event("item_updated", $this->original(), $this); - } return $this; } @@ -663,7 +707,7 @@ class Item_Model extends ORM_MPTT { public function valid_slug(Validation $v, $value) { if (preg_match("/[^A-Za-z0-9-_]/", $value)) { $v->add_error("slug", "not_url_safe"); - } else if ($row = db::build() + } else if (db::build() ->from("items") ->where("parent_id", "=", $this->parent_id) ->where("id", "<>", $this->id) @@ -682,7 +726,7 @@ class Item_Model extends ORM_MPTT { $v->add_error("name", "no_slashes"); } else if (rtrim($value, ".") !== $value) { $v->add_error("name", "no_trailing_period"); - } else if ($row = db::build() + } else if (db::build() ->from("items") ->where("parent_id", "=", $this->parent_id) ->where("id", "<>", $this->id) @@ -691,4 +735,15 @@ class Item_Model extends ORM_MPTT { $v->add_error("name", "conflict"); } } + + /** + * Make sure that the parent id refers to an album. + */ + static function valid_parent($value) { + return db::build() + ->from("items") + ->where("id", "=", $value) + ->where("type", "=", "album") + ->count_records() == 1; + } } -- cgit v1.2.3 From 654b103355f1bda15246e651fa91f3c9e08c3901 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 15 Jan 2010 13:41:46 -0800 Subject: Validate the model type. --- modules/gallery/models/item.php | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) (limited to 'modules/gallery/models') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index e929f30d..395ba52c 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -26,7 +26,8 @@ class Item_Model extends ORM_MPTT { "title" => array("rules" => array("length[0,255]", "required")), "slug" => array("rules" => array("length[0,255]", "required")), "description" => array("rules" => array("length[0,65535]")), - "parent_id" => array("rules" => array("Item_Model::valid_parent")) + "parent_id" => array("rules" => array("Item_Model::valid_parent")), + "type" => array("rules" => array("Item_Model::valid_type")), ); /** @@ -736,6 +737,13 @@ class Item_Model extends ORM_MPTT { } } + /** + * Make sure that the type is valid. + */ + static function valid_type($value) { + return in_array($value, array("album", "photo", "movie")); + } + /** * Make sure that the parent id refers to an album. */ -- cgit v1.2.3 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 ++++--- modules/gallery/helpers/gallery_event.php | 16 +++ modules/gallery/helpers/photo.php | 112 ------------------- modules/gallery/models/item.php | 139 +++++++++++++++++++----- 4 files changed, 151 insertions(+), 156 deletions(-) (limited to 'modules/gallery/models') 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"); } } diff --git a/modules/gallery/helpers/gallery_event.php b/modules/gallery/helpers/gallery_event.php index 679d65c2..9452e855 100644 --- a/modules/gallery/helpers/gallery_event.php +++ b/modules/gallery/helpers/gallery_event.php @@ -73,6 +73,22 @@ 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::failure("Unable to create a thumbnail for item id {$item->id}"); + Kohana_Log::add("error", $e->getMessage() . "\n" . $e->getTraceAsString()); + } + + // If the parent has no cover item, make this it. + $parent = $item->parent(); + if (access::can("edit", $parent) && $parent->album_cover_item_id == null) { + item::make_album_cover($item); + } + } } static function item_deleted($item) { diff --git a/modules/gallery/helpers/photo.php b/modules/gallery/helpers/photo.php index aeae7f56..74e30409 100644 --- a/modules/gallery/helpers/photo.php +++ b/modules/gallery/helpers/photo.php @@ -24,118 +24,6 @@ * Note: by design, this class does not do any permission checking. */ class photo_Core { - /** - * Create a new photo. - * @param integer $parent 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_IMAGE_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"); - } - - if (filesize($filename) == 0) { - throw new Exception("@todo EMPTY_INPUT_FILE"); - } - - $image_info = getimagesize($filename); - - // Force an extension onto the name - $pi = pathinfo($filename); - if (empty($pi["extension"])) { - $pi["extension"] = image_type_to_extension($image_info[2], false); - $name .= "." . $pi["extension"]; - } - - if (empty($slug)) { - $slug = item::convert_filename_to_slug($name); - } - - $photo = ORM::factory("item"); - $photo->type = "photo"; - $photo->title = $title; - $photo->description = $description; - $photo->name = $name; - $photo->owner_id = $owner_id ? $owner_id : identity::active_user()->id; - $photo->width = $image_info[0]; - $photo->height = $image_info[1]; - $photo->mime_type = empty($image_info['mime']) ? "application/unknown" : $image_info['mime']; - $photo->thumb_dirty = 1; - $photo->resize_dirty = 1; - $photo->sort_column = "weight"; - $photo->slug = $slug; - - // Randomize the name or slug 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", "=", $photo->name) - ->or_where("slug", "=", $photo->slug) - ->close() - ->find()->id) { - $rand = rand(); - $photo->name = "{$name}.$rand.{$pi['extension']}"; - $photo->slug = "{$slug}-$rand"; - } - - // This saves the photo - $photo->add_to_parent($parent); - - /* - * If the thumb or resize already exists then rename it. We need to do this after the save - * because the resize_path and thumb_path both call relative_path which caches the - * path. Before add_to_parent the relative path will be incorrect. - */ - if (file_exists($photo->resize_path()) || - file_exists($photo->thumb_path())) { - $photo->name = $pi["filename"] . "-" . rand() . "." . $pi["extension"]; - $photo->save(); - } - - copy($filename, $photo->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", $photo); - - // Build our thumbnail/resizes. If we fail to build thumbnail/resize we assume that the image - // is bad in some way and discard it. - try { - graphics::generate($photo); - } catch (Exception $e) { - $photo->delete(); - throw $e; - } - - // 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($photo); - } - - return $photo; - } - static function get_edit_form($photo) { $form = new Forge("photos/update/$photo->id", "", "post", array("id" => "g-edit-photo-form")); $form->hidden("from_id"); diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index 46b0304e..977b9771 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -20,13 +20,13 @@ class Item_Model extends ORM_MPTT { protected $children = 'items'; protected $sorting = array(); + protected $data_file = null; var $rules = array( "name" => array("rules" => array("length[0,255]", "required")), "title" => array("rules" => array("length[0,255]", "required")), "slug" => array("rules" => array("length[0,255]", "required")), "description" => array("rules" => array("length[0,65535]")), - "parent_id" => array("rules" => array("Item_Model::valid_parent")), "type" => array("rules" => array("Item_Model::valid_type")), ); @@ -174,6 +174,14 @@ class Item_Model extends ORM_MPTT { return $this; } + /** + * Specify the path to the data file associated with this item. To actually associate it, + * you still have to call save(). + */ + public function set_data_file($data_file) { + $this->data_file = $data_file; + } + /** * Return the server-relative url to this item, eg: * /gallery3/index.php/BobsWedding?page=2 @@ -304,7 +312,7 @@ class Item_Model extends ORM_MPTT { } $this->relative_path_cache = implode($names, "/"); $this->relative_url_cache = implode($slugs, "/"); - $this->save(); + return $this; } /** @@ -319,7 +327,7 @@ class Item_Model extends ORM_MPTT { } if (!isset($this->relative_path_cache)) { - $this->_build_relative_caches(); + $this->_build_relative_caches()->save(); } return $this->relative_path_cache; } @@ -334,7 +342,7 @@ class Item_Model extends ORM_MPTT { } if (!isset($this->relative_url_cache)) { - $this->_build_relative_caches(); + $this->_build_relative_caches()->save(); } return $this->relative_url_cache; } @@ -368,6 +376,7 @@ 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()) { @@ -386,15 +395,37 @@ class Item_Model extends ORM_MPTT { if (empty($this->owner_id)) { $this->owner_id = identity::active_user()->id; } + + // Make an url friendly slug from the name, if necessary if (empty($this->slug)) { $tmp = pathinfo($this->name, PATHINFO_FILENAME); $tmp = preg_replace("/[^A-Za-z0-9-_]+/", "-", $tmp); $this->slug = trim($tmp, "-"); } - // Randomize the name or slug if there's a conflict + if ($this->is_movie() || $this->is_photo()) { + $image_info = getimagesize($this->data_file); + + if ($this->is_photo()) { + $this->width = $image_info[0]; + $this->height = $image_info[1]; + $this->mime_type = + empty($image_info['mime']) ? "application/unknown" : $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"]; + } + + } + + // Randomize the name or slug if there's a conflict. Preserve the extension. // @todo Improve this. Random numbers are not user friendly - $base_name = $this->name; + $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) @@ -404,19 +435,46 @@ class Item_Model extends ORM_MPTT { ->close() ->find()->id) { $rand = rand(); - $this->name = "$base_name-$rand"; + if ($base_ext) { + $this->name = "$base_name-$rand.$base_ext"; + } else { + $this->name = "$base_name-$rand"; + } $this->slug = "$base_slug-$rand"; } parent::save(); - // Call this after we finish saving so that the paths are correct. - if ($this->is_album()) { + // Build our url caches and save again. If we could depend on a save happening later we + // could defer this 2nd save. + $this->_build_relative_caches(); + parent::save(); + + // Take any actions that we can only do once all our paths are set correctly after saving. + switch ($this->type) { + case "album": mkdir($this->file_path()); mkdir(dirname($this->thumb_path())); mkdir(dirname($this->resize_path())); + break; + + case "photo": + // 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"] . "-" . rand() . "." . $pi["extension"]; + parent::save(); + } + + copy($this->data_file, $this->file_path()); + break; } + // This will almost definitely trigger another save, so put it at the end so that we're + // tail recursive. module::event("item_created", $this); } else { // Update an existing item @@ -691,8 +749,8 @@ class Item_Model extends ORM_MPTT { if (!$array) { // The root item has different rules for the name and slug. if ($this->id == 1) { - $this->rules["name"]["rules"][] = "length[0]"; - $this->rules["slug"]["rules"][] = "length[0]"; + $this->rules["name"] = array("rules" => array("length[0]")); + $this->rules["slug"] = array("rules" => array("length[0]")); } // Names and slugs can't conflict @@ -700,20 +758,28 @@ class Item_Model extends ORM_MPTT { $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"); + } + + // All items must have a legal parent + $this->rules["parent_id"]["callbacks"][] = array($this, "valid_parent"); + parent::validate($array); } /** * Validate that the desired slug does not conflict. */ - public function valid_slug(Validation $v, $value) { - if (preg_match("/[^A-Za-z0-9-_]/", $value)) { + public function valid_slug(Validation $v, $field) { + if (preg_match("/[^A-Za-z0-9-_]/", $this->slug)) { $v->add_error("slug", "not_url_safe"); } else if (db::build() ->from("items") ->where("parent_id", "=", $this->parent_id) ->where("id", "<>", $this->id) - ->where("slug", "=", $value) + ->where("slug", "=", $this->slug) ->count_records()) { $v->add_error("slug", "conflict"); } @@ -723,36 +789,55 @@ class Item_Model extends ORM_MPTT { * Validate the item name. It can't conflict with other names, can't contain slashes or * trailing periods. */ - public function valid_name(Validation $v, $value) { - if (strpos($value, "/") !== false) { + public function valid_name(Validation $v, $field) { + if (strpos($this->name, "/") !== false) { $v->add_error("name", "no_slashes"); - } else if (rtrim($value, ".") !== $value) { + } else if (rtrim($this->name, ".") !== $this->name) { $v->add_error("name", "no_trailing_period"); } else if (db::build() ->from("items") ->where("parent_id", "=", $this->parent_id) ->where("id", "<>", $this->id) - ->where("name", "=", $value) + ->where("name", "=", $this->name) ->count_records()) { $v->add_error("name", "conflict"); } } /** - * Make sure that the type is valid. + * Make sure that the data file is well formed (it exists and isn't empty). */ - static function valid_type($value) { - return in_array($value, array("album", "photo", "movie")); + public function valid_data_file(Validation $v, $field) { + if (!is_file($this->data_file)) { + $v->add_error("file", "bad_path"); + } else if (filesize($this->data_file) == 0) { + $v->add_error("file", "empty_file"); + } } /** * Make sure that the parent id refers to an album. */ - static function valid_parent($value) { - return db::build() - ->from("items") - ->where("id", "=", $value) - ->where("type", "=", "album") - ->count_records() == 1; + public function valid_parent(Validation $v, $field) { + if ($this->id == 1) { + if ($this->parent_id != 0) { + $v->add_error("parent_id", "invalid"); + } + } else { + if (db::build() + ->from("items") + ->where("id", "=", $this->parent_id) + ->where("type", "=", "album") + ->count_records() != 1) { + $v->add_error("parent_id", "invalid"); + } + } + } + + /** + * Make sure that the type is valid. + */ + static function valid_type($value) { + return in_array($value, array("album", "photo", "movie")); } } -- cgit v1.2.3 From 9f6dba723842cc16dd3f3787d232028c6c0c2e19 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Sat, 16 Jan 2010 11:12:19 -0800 Subject: Check for illegal extensions in valid_name() Fix a bug where we were not calling valid_data_file correctly. --- modules/gallery/models/item.php | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) (limited to 'modules/gallery/models') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index 977b9771..a9607699 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -759,7 +759,7 @@ class Item_Model extends ORM_MPTT { } // Movies and photos must have data files - if ($this->is_photo() || $this->is_movie() && !$this->loaded()) { + if (($this->is_photo() || $this->is_movie()) && !$this->loaded()) { $this->rules["name"]["callbacks"][] = array($this, "valid_data_file"); } @@ -792,14 +792,29 @@ class Item_Model extends ORM_MPTT { public function valid_name(Validation $v, $field) { if (strpos($this->name, "/") !== false) { $v->add_error("name", "no_slashes"); - } else if (rtrim($this->name, ".") !== $this->name) { + return; + } + + if (rtrim($this->name, ".") !== $this->name) { $v->add_error("name", "no_trailing_period"); - } else if (db::build() - ->from("items") - ->where("parent_id", "=", $this->parent_id) - ->where("id", "<>", $this->id) - ->where("name", "=", $this->name) - ->count_records()) { + return; + } + + 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 (db::build() + ->from("items") + ->where("parent_id", "=", $this->parent_id) + ->where("id", "<>", $this->id) + ->where("name", "=", $this->name) + ->count_records()) { $v->add_error("name", "conflict"); } } -- 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/models') 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 From a5aacfa4a650ed5830331b8c81f68ea1625cd3ba Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Sat, 16 Jan 2010 18:10:40 -0800 Subject: Don't forget to save when we make insignificant chagnes only. --- modules/gallery/models/item.php | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'modules/gallery/models') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index c007afeb..56ee321a 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -514,6 +514,10 @@ class Item_Model extends ORM_MPTT { parent::save(); module::event("item_updated", $original, $this); } + } else if (!empty($this->changed)) { + // Insignificant changes only. Don't fire events or do any special checking to try to keep + // this lightweight. + parent::save(); } return $this; -- cgit v1.2.3 From fdcb4a1f32d8a7d153462da00129524ffa0f69b8 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Sat, 16 Jan 2010 18:16:47 -0800 Subject: PHPdoc. --- modules/gallery/models/item.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'modules/gallery/models') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index 56ee321a..869e3b27 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -365,7 +365,7 @@ class Item_Model extends ORM_MPTT { } /** - * Handle any business logic necessary to create an item. + * Handle any business logic necessary to create or modify an item. * @see ORM::save() * * @return ORM Item_Model -- cgit v1.2.3 From 4f8c98a7bc89911427a6fd97b8ed6ef6f41a2835 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Sun, 17 Jan 2010 12:13:25 -0800 Subject: Move rules entirely down into validate() so that we can be more sophisticated but keep all our rules in one place. Add rules for most fields. --- modules/gallery/models/item.php | 116 +++++++++++++++++++++++++++++++--------- 1 file changed, 91 insertions(+), 25 deletions(-) (limited to 'modules/gallery/models') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index 869e3b27..04120f10 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -18,18 +18,10 @@ * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA. */ class Item_Model extends ORM_MPTT { - protected $children = 'items'; + protected $children = "items"; protected $sorting = array(); protected $data_file = null; - var $rules = array( - "name" => array("rules" => array("length[0,255]", "required")), - "title" => array("rules" => array("length[0,255]", "required")), - "slug" => array("rules" => array("length[0,255]", "required")), - "description" => array("rules" => array("length[0,65535]")), - "type" => array("rules" => array("Item_Model::valid_type")), - ); - /** * Add a set of restrictions to any following queries to restrict access only to items * viewable by the active user. @@ -757,28 +749,41 @@ class Item_Model extends ORM_MPTT { } /** - * Add some custom per-instance rules. + * Specify our rules here so that we have access to the instance of this model. */ public function validate($array=null) { - // validate() is recursive, only modify the rules on the outermost call. if (!$array) { + $this->rules = array( + "album_cover_item_id" => array("callbacks" => array(array($this, "valid_item"))), + "description" => array("rules" => array("length[0,65535]")), + "left_ptr" => array("callbacks" => array(array($this, "internal_only"))), + "level" => array("callbacks" => array(array($this, "internal_only"))), + "mime_type" => array("callbacks" => array(array($this, "valid_field"))), + "name" => array("rules" => array("length[0,255]", "required"), + "callbacks" => array(array($this, "valid_name"))), + "parent_id" => array("callbacks" => array(array($this, "valid_parent"))), + "rand_key" => array("rule" => array("decimal")), + "right_ptr" => array("callbacks" => array(array($this, "internal_only"))), + "slug" => array("rules" => array("length[0,255]", "required"), + "callbacks" => array(array($this, "valid_slug"))), + "sort_column" => array("callbacks" => array(array($this, "valid_field"))), + "sort_order" => array("callbacks" => array(array($this, "valid_field"))), + "title" => array("rules" => array("length[0,255]", "required")), + "type" => array("callbacks" => array(array($this, "read_only"), + array($this, "valid_field"))), + ); + + // Conditional rules if ($this->id == 1) { - // Root album can't have a name or slug + // Root album can't have a name or slug so replace the rules $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")); } // 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")); } parent::validate($array); @@ -821,13 +826,13 @@ class Item_Model extends ORM_MPTT { $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"); + $v->add_error("name", "illegal_data_file_extension"); return; } } else { // New items must have an extension if (!pathinfo($this->name, PATHINFO_EXTENSION)) { - $v->add_error("name", "illegal_extension"); + $v->add_error("name", "illegal_data_file_extension"); } } } @@ -847,9 +852,9 @@ class Item_Model extends ORM_MPTT { */ public function valid_data_file(Validation $v, $field) { if (!is_file($this->data_file)) { - $v->add_error("file", "bad_path"); + $v->add_error("name", "bad_data_file_path"); } else if (filesize($this->data_file) == 0) { - $v->add_error("file", "empty_file"); + $v->add_error("name", "empty_data_file"); } } @@ -872,10 +877,71 @@ class Item_Model extends ORM_MPTT { } } + /** + * Make sure the field refers to a valid item by id, or is null. + */ + public function valid_item(Validation $v, $field) { + if ($this->$field && db::build() + ->from("items") + ->where("id", "=", $this->$field) + ->count_records() != 1) { + $v->add_error($field, "invalid_item"); + } + } + /** * Make sure that the type is valid. */ - static function valid_type($value) { - return in_array($value, array("album", "photo", "movie")); + public function valid_field(Validation $v, $field) { + switch($field) { + case "mime_type": + if ($this->is_movie()) { + $legal_values = array("video/flv", "video/mp4"); + } if ($this->is_photo()) { + $legal_values = array("image/jpeg", "image/gif", "image/png"); + } + break; + + case "sort_column": + if (!array_key_exists($this->sort_column, $this->object)) { + $v->add_error($field, "invalid"); + } + break; + + case "sort_order": + $legal_values = array("ASC", "DESC", "asc", "desc"); + break; + + case "type": + $legal_values = array("album", "photo", "movie"); + break; + + default: + $v->add_error($field, "unvalidated_field"); + break; + } + + if (isset($legal_values) && !in_array($this->$field, $legal_values)) { + $v->add_error($field, "invalid"); + } + } + + /** + * This field cannot be changed externally, it can only be changed inside save() after + * validation has been performed. + */ + public function internal_only(Validation $v, $field) { + if ($this->original()->$field != $this->$field) { + $v->add_error($field, "internal_only"); + } + } + + /** + * This field cannot be changed after it's been set. + */ + public function read_only(Validation $v, $field) { + if ($this->loaded() && $this->original()->$field != $this->$field) { + $v->add_error($field, "read_only"); + } } } -- cgit v1.2.3 From afb3fa71b9aea16a02c13f75d7999069a4ae9d21 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Sun, 17 Jan 2010 16:58:47 -0800 Subject: Get rid of internal_only designation -- it's too hard to enforce cleanly. --- modules/gallery/models/item.php | 13 ------------- 1 file changed, 13 deletions(-) (limited to 'modules/gallery/models') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index 04120f10..453a3525 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -756,14 +756,11 @@ class Item_Model extends ORM_MPTT { $this->rules = array( "album_cover_item_id" => array("callbacks" => array(array($this, "valid_item"))), "description" => array("rules" => array("length[0,65535]")), - "left_ptr" => array("callbacks" => array(array($this, "internal_only"))), - "level" => array("callbacks" => array(array($this, "internal_only"))), "mime_type" => array("callbacks" => array(array($this, "valid_field"))), "name" => array("rules" => array("length[0,255]", "required"), "callbacks" => array(array($this, "valid_name"))), "parent_id" => array("callbacks" => array(array($this, "valid_parent"))), "rand_key" => array("rule" => array("decimal")), - "right_ptr" => array("callbacks" => array(array($this, "internal_only"))), "slug" => array("rules" => array("length[0,255]", "required"), "callbacks" => array(array($this, "valid_slug"))), "sort_column" => array("callbacks" => array(array($this, "valid_field"))), @@ -926,16 +923,6 @@ class Item_Model extends ORM_MPTT { } } - /** - * This field cannot be changed externally, it can only be changed inside save() after - * validation has been performed. - */ - public function internal_only(Validation $v, $field) { - if ($this->original()->$field != $this->$field) { - $v->add_error($field, "internal_only"); - } - } - /** * This field cannot be changed after it's been set. */ -- cgit v1.2.3 From 39bb08db2847d2c6faf945ccf901bc34edbca355 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Sun, 17 Jan 2010 20:02:30 -0800 Subject: Make set_data_file() chainable. --- modules/gallery/models/item.php | 2 ++ 1 file changed, 2 insertions(+) (limited to 'modules/gallery/models') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index 453a3525..fcd9b8e6 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -169,9 +169,11 @@ class Item_Model extends ORM_MPTT { /** * Specify the path to the data file associated with this item. To actually associate it, * you still have to call save(). + * @chainable */ public function set_data_file($data_file) { $this->data_file = $data_file; + return $this; } /** -- cgit v1.2.3 From 1cfee16e38b483d97dddc723b383f5c8cef0f229 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 18 Jan 2010 13:09:58 -0800 Subject: In valid_name, don't query on the id if it's null. --- modules/gallery/models/item.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'modules/gallery/models') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index fcd9b8e6..e04b1314 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -839,8 +839,8 @@ class Item_Model extends ORM_MPTT { if (db::build() ->from("items") ->where("parent_id", "=", $this->parent_id) - ->where("id", "<>", $this->id) ->where("name", "=", $this->name) + ->merge_where($this->id ? array(array("id", "<>", $this->id)) : null) ->count_records()) { $v->add_error("name", "conflict"); } -- cgit v1.2.3 From 0e2f4a7a372567ba1916e2533ce24ee6e1cc7d36 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 18 Jan 2010 20:13:46 -0800 Subject: Fix renaming in save() by moving the actual rename action under parent::save(). This is consistent with other changes because all filesystem operations happen after the database change is committed. Also, inline rename() since it's fairly simple now. --- modules/gallery/models/item.php | 73 +++++++++++++++++++++-------------------- 1 file changed, 38 insertions(+), 35 deletions(-) (limited to 'modules/gallery/models') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index e04b1314..b3f749eb 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -139,33 +139,6 @@ class Item_Model extends ORM_MPTT { return $this; } - /** - * Rename the underlying file for this item to a new name and move all related files. - * - * @chainable - */ - private function rename($new_name) { - $old_relative_path = urldecode($this->original()->relative_path()); - $new_relative_path = dirname($old_relative_path) . "/" . $new_name; - if (file_exists(VARPATH . "albums/$new_relative_path")) { - throw new Exception("@todo INVALID_RENAME_FILE_EXISTS: $new_relative_path"); - } - - @rename(VARPATH . "albums/$old_relative_path", VARPATH . "albums/$new_relative_path"); - @rename(VARPATH . "resizes/$old_relative_path", VARPATH . "resizes/$new_relative_path"); - if ($this->is_movie()) { - // Movie thumbnails have a .jpg extension - $old_relative_thumb_path = preg_replace("/...$/", "jpg", $old_relative_path); - $new_relative_thumb_path = preg_replace("/...$/", "jpg", $new_relative_path); - @rename(VARPATH . "thumbs/$old_relative_thumb_path", - VARPATH . "thumbs/$new_relative_thumb_path"); - } else { - @rename(VARPATH . "thumbs/$old_relative_path", VARPATH . "thumbs/$new_relative_path"); - } - - return $this; - } - /** * Specify the path to the data file associated with this item. To actually associate it, * you still have to call save(). @@ -482,20 +455,35 @@ class Item_Model extends ORM_MPTT { module::event("item_created", $this); } else { // Update an existing item - if ($this->original()->name != $this->name) { - $this->rename($this->name); + + // The new values have to be valid before we do anything with them. If we make any + // other changes before we call parent::save() below, we'll have to validate those changes + // again. But, we can't take any action on these values until we know they're ok so this + // is unavoidable. + if (!$this->_valid) { + $this->validate(); + } + + $original = clone $this->original(); + + if ($original->name != $this->name) { + // Get the old relative path for when we rename below + if (!isset($this->relative_path_cache)) { + $this->_build_relative_caches(); // but don't call save() + } + $old_relative_path = $this->relative_path_cache; $this->relative_path_cache = null; } - if ($this->original()->slug != $this->slug) { - // Clear the relative url cache for this item and all children + if ($original->slug != $this->slug) { $this->relative_url_cache = null; } + parent::save(); + // Changing the name or the slug ripples downwards if ($this->is_album() && - ($this->original()->name != $this->name || - $this->original()->slug != $this->slug)) { + ($original->name != $this->name || $original->slug != $this->slug)) { db::build() ->update("items") ->set("relative_url_cache", null) @@ -504,8 +492,23 @@ class Item_Model extends ORM_MPTT { ->where("right_ptr", "<", $this->right_ptr) ->execute(); } - $original = clone $this->original(); - parent::save(); + + // If we renamed this item, move all of its associated data files. + if ($original->name != $this->name) { + $relative_path = urldecode($this->relative_path()); + @rename(VARPATH . "albums/$old_relative_path", VARPATH . "albums/$relative_path"); + @rename(VARPATH . "resizes/$old_relative_path", VARPATH . "resizes/$relative_path"); + if ($this->is_movie()) { + // Movie thumbnails have a .jpg extension + $old_relative_thumb_path = preg_replace("/...$/", "jpg", $old_relative_path); + $relative_thumb_path = preg_replace("/...$/", "jpg", $relative_path); + @rename(VARPATH . "thumbs/$old_relative_thumb_path", + VARPATH . "thumbs/$relative_thumb_path"); + } else { + @rename(VARPATH . "thumbs/$old_relative_path", VARPATH . "thumbs/$relative_path"); + } + } + module::event("item_updated", $original, $this); } } else if (!empty($this->changed)) { -- cgit v1.2.3 From 703882f4df712089e02e14baf8970ab26b62c24f Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 18 Jan 2010 22:50:50 -0800 Subject: Update valid_parent() to disallow moving an item inside its own hierarchy. Move move_to() inside save() --- modules/gallery/models/item.php | 102 +++++++++++++--------------------------- 1 file changed, 33 insertions(+), 69 deletions(-) (limited to 'modules/gallery/models') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index b3f749eb..e2f7dc5e 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -91,54 +91,6 @@ class Item_Model extends ORM_MPTT { module::event("item_deleted", $old); } - /** - * Move this item to the specified target. - * @chainable - * @param Item_Model $target Target item (must be an album) - * @return ORM_MPTT - */ - function move_to($target) { - if (!$target->is_album()) { - throw new Exception("@todo INVALID_MOVE_TYPE $target->type"); - } - - if (file_exists($target_file = "{$target->file_path()}/$this->name")) { - throw new Exception("@todo INVALID_MOVE_TARGET_EXISTS: $target_file"); - } - - if ($this->id == 1) { - throw new Exception("@todo INVALID_SOURCE root album"); - } - - $original_path = $this->file_path(); - $original_resize_path = $this->resize_path(); - $original_thumb_path = $this->thumb_path(); - $original_parent = $this->parent(); - - parent::move_to($target, true); - model_cache::clear(); - $this->relative_path_cache = null; - - rename($original_path, $this->file_path()); - if ($this->is_album()) { - @rename(dirname($original_resize_path), dirname($this->resize_path())); - @rename(dirname($original_thumb_path), dirname($this->thumb_path())); - db::build() - ->update("items") - ->set("relative_path_cache", null) - ->set("relative_url_cache", null) - ->where("left_ptr", ">", $this->left_ptr) - ->where("right_ptr", "<", $this->right_ptr) - ->execute(); - } else { - @rename($original_resize_path, $this->resize_path()); - @rename($original_thumb_path, $this->thumb_path()); - } - - module::event("item_moved", $this, $original_parent); - return $this; - } - /** * Specify the path to the data file associated with this item. To actually associate it, * you still have to call save(). @@ -466,12 +418,12 @@ class Item_Model extends ORM_MPTT { $original = clone $this->original(); - if ($original->name != $this->name) { - // Get the old relative path for when we rename below + if ($original->name != $this->name || $original->parent_id != $this->parent_id) { + // Get the old relative path for when we rename or move below if (!isset($this->relative_path_cache)) { $this->_build_relative_caches(); // but don't call save() } - $old_relative_path = $this->relative_path_cache; + $before_save = clone $this; $this->relative_path_cache = null; } @@ -481,9 +433,33 @@ class Item_Model extends ORM_MPTT { parent::save(); - // Changing the name or the slug ripples downwards + if ($original->parent_id != $this->parent_id || $original->name != $this->name) { + if ($original->parent_id != $this->parent_id) { + // Move the ORM pointers around + parent::move_to($this->parent()); + } + + // Move all of the items associated data files + @rename($before_save->file_path(), $this->file_path()); + if ($this->is_album()) { + @rename(dirname($before_save->resize_path()), dirname($this->resize_path())); + @rename(dirname($before_save->thumb_path()), dirname($this->thumb_path())); + } else { + @rename($before_save->resize_path(), $this->resize_path()); + @rename($before_save->thumb_path(), $this->thumb_path()); + } + + if ($original->parent_id != $this->parent_id) { + // This will result in 2 events since we'll still fire the item_updated event below + module::event("item_moved", $this, $original->parent()); + } + } + + // Changing the name, slug or parent ripples downwards if ($this->is_album() && - ($original->name != $this->name || $original->slug != $this->slug)) { + ($original->name != $this->name || + $original->slug != $this->slug || + $original->parent_id != $this->parent_id)) { db::build() ->update("items") ->set("relative_url_cache", null) @@ -493,22 +469,6 @@ class Item_Model extends ORM_MPTT { ->execute(); } - // If we renamed this item, move all of its associated data files. - if ($original->name != $this->name) { - $relative_path = urldecode($this->relative_path()); - @rename(VARPATH . "albums/$old_relative_path", VARPATH . "albums/$relative_path"); - @rename(VARPATH . "resizes/$old_relative_path", VARPATH . "resizes/$relative_path"); - if ($this->is_movie()) { - // Movie thumbnails have a .jpg extension - $old_relative_thumb_path = preg_replace("/...$/", "jpg", $old_relative_path); - $relative_thumb_path = preg_replace("/...$/", "jpg", $relative_path); - @rename(VARPATH . "thumbs/$old_relative_thumb_path", - VARPATH . "thumbs/$relative_thumb_path"); - } else { - @rename(VARPATH . "thumbs/$old_relative_path", VARPATH . "thumbs/$relative_path"); - } - } - module::event("item_updated", $original, $this); } } else if (!empty($this->changed)) { @@ -873,6 +833,10 @@ class Item_Model extends ORM_MPTT { ->from("items") ->where("id", "=", $this->parent_id) ->where("type", "=", "album") + ->and_open() + ->where("left_ptr", "<", $this->left_ptr) + ->or_where("right_ptr", ">", $this->right_ptr) + ->close() ->count_records() != 1) { $v->add_error("parent_id", "invalid"); } -- cgit v1.2.3 From afe2128bb07938d47e98857b897d9d917062831d Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Tue, 19 Jan 2010 19:30:18 -0800 Subject: Make video/x-flv a valid movie mime_type --- modules/gallery/models/item.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'modules/gallery/models') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index e2f7dc5e..036ad796 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -862,7 +862,7 @@ class Item_Model extends ORM_MPTT { switch($field) { case "mime_type": if ($this->is_movie()) { - $legal_values = array("video/flv", "video/mp4"); + $legal_values = array("video/flv", "video/x-flv", "video/mp4"); } if ($this->is_photo()) { $legal_values = array("image/jpeg", "image/gif", "image/png"); } -- cgit v1.2.3 From 6aee6cde2519b125a3b1209d3a6cd441b5d3c526 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Tue, 19 Jan 2010 20:53:21 -0800 Subject: Move data initialization into the constructor so that it happens before validate() is called, which is important with our two phase web controllers. Make valid_parent smarter about moving existing items, vs new items. --- modules/gallery/models/item.php | 54 +++++++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 21 deletions(-) (limited to 'modules/gallery/models') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index 036ad796..a7f73d0e 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -22,6 +22,21 @@ class Item_Model extends ORM_MPTT { protected $sorting = array(); protected $data_file = null; + public function __construct($id=null) { + parent::__construct($id); + + if (!$this->loaded()) { + // Set reasonable defaults + $this->created = time(); + $this->rand_key = ((float)mt_rand()) / (float)mt_getrandmax(); + $this->thumb_dirty = 1; + $this->resize_dirty = 1; + $this->sort_column = "created"; + $this->sort_order = "ASC"; + $this->owner_id = identity::active_user()->id; + } + } + /** * Add a set of restrictions to any following queries to restrict access only to items * viewable by the active user. @@ -298,20 +313,12 @@ class Item_Model extends ORM_MPTT { if (!empty($this->changed) && $significant_changes) { $this->updated = time(); if (!$this->loaded()) { - // Create a new item. Use whatever fields are set, and specify defaults for the rest. - $this->created = $this->updated; - $this->weight = item::get_max_weight(); - $this->rand_key = ((float)mt_rand()) / (float)mt_getrandmax(); - $this->thumb_dirty = 1; - $this->resize_dirty = 1; - if (empty($this->sort_column)) { - $this->sort_column = "created"; - } - if (empty($this->sort_order)) { - $this->sort_order = "ASC"; - } - if (empty($this->owner_id)) { - $this->owner_id = identity::active_user()->id; + // Create a new item. + + // Set a weight if it's missing. We don't do this in the constructor because it's not a + // simple assignment. + if (empty($this->weight)) { + $this->weight = item::get_max_weight(); } // Make an url friendly slug from the name, if necessary @@ -829,15 +836,20 @@ class Item_Model extends ORM_MPTT { $v->add_error("parent_id", "invalid"); } } else { - if (db::build() - ->from("items") - ->where("id", "=", $this->parent_id) - ->where("type", "=", "album") - ->and_open() + $query = db::build() + ->from("items") + ->where("id", "=", $this->parent_id) + ->where("type", "=", "album"); + + // If this is an existing item, make sure the new parent is not part of our hierarchy + if ($this->loaded()) { + $query->and_open() ->where("left_ptr", "<", $this->left_ptr) ->or_where("right_ptr", ">", $this->right_ptr) - ->close() - ->count_records() != 1) { + ->close(); + } + + if ($query->count_records() != 1) { $v->add_error("parent_id", "invalid"); } } -- cgit v1.2.3 From e39c8df19fc0dadcfe65cb8a3ed6529648c6c9cf Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Tue, 19 Jan 2010 21:20:36 -0800 Subject: Fix some validation checks to check to see if the original was loaded before deciding whether or not we changed a value. Change valid_name to be cascading, not parallel. --- modules/gallery/models/item.php | 18 +++++------------- modules/gallery/tests/Item_Model_Test.php | 16 ++++++++++++++-- 2 files changed, 19 insertions(+), 15 deletions(-) (limited to 'modules/gallery/models') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index a7f73d0e..58ff86ed 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -782,21 +782,15 @@ class Item_Model extends ORM_MPTT { if (strpos($this->name, "/") !== false) { $v->add_error("name", "no_slashes"); return; - } - - if (rtrim($this->name, ".") !== $this->name) { + } else if (rtrim($this->name, ".") !== $this->name) { $v->add_error("name", "no_trailing_period"); - return; - } - - if ($this->is_movie() || $this->is_photo()) { - if ($this->loaded()) { + } else if ($this->is_movie() || $this->is_photo()) { + if ($this->original()->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_data_file_extension"); - return; } } else { // New items must have an extension @@ -804,9 +798,7 @@ class Item_Model extends ORM_MPTT { $v->add_error("name", "illegal_data_file_extension"); } } - } - - if (db::build() + } else if (db::build() ->from("items") ->where("parent_id", "=", $this->parent_id) ->where("name", "=", $this->name) @@ -908,7 +900,7 @@ class Item_Model extends ORM_MPTT { * This field cannot be changed after it's been set. */ public function read_only(Validation $v, $field) { - if ($this->loaded() && $this->original()->$field != $this->$field) { + if ($this->original()->loaded() && $this->original()->$field != $this->$field) { $v->add_error($field, "read_only"); } } diff --git a/modules/gallery/tests/Item_Model_Test.php b/modules/gallery/tests/Item_Model_Test.php index afb131fc..284491a0 100644 --- a/modules/gallery/tests/Item_Model_Test.php +++ b/modules/gallery/tests/Item_Model_Test.php @@ -320,9 +320,8 @@ class Item_Model_Test extends Unit_Test_Case { } public function slug_is_url_safe_test() { - $album = test::random_album_unsaved(); - try { + $album = test::random_album_unsaved(); $album->slug = "illegal chars! !@#@#$!@~"; $album->save(); $this->assert_true(false, "Shouldn't be able to save"); @@ -334,4 +333,17 @@ class Item_Model_Test extends Unit_Test_Case { $album->slug = "the_quick_brown_fox"; $album->save(); } + + public function cant_change_item_type_test() { + $photo = test::random_photo(); + try { + $photo->type = "movie"; + $photo->mime_type = "video/x-flv"; + $photo->save(); + } catch (ORM_Validation_Exception $e) { + $this->assert_same(array("type" => "read_only"), $e->validation->errors()); + return; // pass + } + $this->assert_true(false, "Shouldn't get here"); + } } -- cgit v1.2.3 From 995faaa27fc870589e346f7ef65e0bd7cabfe047 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Wed, 20 Jan 2010 22:45:19 -0800 Subject: Stop using MY_ORM::original(). It's got very odd semantics and we are not capturing all cases for setting and resetting $original, which leads to some weird and hard to reproduce behavior. Instead, if we need the original just reload it from the database. This may result in a somewhat excessive load in places, but we'll have to fix that in a later optimization pass. --- modules/gallery/models/item.php | 104 +++++++++++++++--------------- modules/gallery/tests/Item_Model_Test.php | 1 + 2 files changed, 54 insertions(+), 51 deletions(-) (limited to 'modules/gallery/models') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index 58ff86ed..19c379ab 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -415,65 +415,58 @@ class Item_Model extends ORM_MPTT { } else { // Update an existing item - // The new values have to be valid before we do anything with them. If we make any - // other changes before we call parent::save() below, we'll have to validate those changes - // again. But, we can't take any action on these values until we know they're ok so this - // is unavoidable. - if (!$this->_valid) { - $this->validate(); - } - - $original = clone $this->original(); - - if ($original->name != $this->name || $original->parent_id != $this->parent_id) { - // Get the old relative path for when we rename or move below - if (!isset($this->relative_path_cache)) { - $this->_build_relative_caches(); // but don't call save() - } - $before_save = clone $this; + // If any significant fields have changed, load up a copy of the original item and + // keep it around. + if (array_intersect($this->changed, array("parent_id", "name", "slug"))) { + $original = ORM::factory("item")->where("id", "=", $this->id)->find(); + $original->_build_relative_caches(); $this->relative_path_cache = null; - } - - if ($original->slug != $this->slug) { $this->relative_url_cache = null; } parent::save(); - if ($original->parent_id != $this->parent_id || $original->name != $this->name) { + // Now update the filesystem and any database caches if there were significant value + // changes. If anything past this point fails, then we'll have an inconsistent database + // so this code should be as robust as we can make it. + if (isset($original)) { + // Update the MPTT pointers, if necessary. We have to do this before we generate any + // cached paths! if ($original->parent_id != $this->parent_id) { - // Move the ORM pointers around parent::move_to($this->parent()); } - // Move all of the items associated data files - @rename($before_save->file_path(), $this->file_path()); - if ($this->is_album()) { - @rename(dirname($before_save->resize_path()), dirname($this->resize_path())); - @rename(dirname($before_save->thumb_path()), dirname($this->thumb_path())); - } else { - @rename($before_save->resize_path(), $this->resize_path()); - @rename($before_save->thumb_path(), $this->thumb_path()); - } + if ($original->parent_id != $this->parent_id || $original->name != $this->name) { + // Move all of the items associated data files + @rename($original->file_path(), $this->file_path()); + if ($this->is_album()) { + @rename(dirname($original->resize_path()), dirname($this->resize_path())); + @rename(dirname($original->thumb_path()), dirname($this->thumb_path())); + } else { + @rename($original->resize_path(), $this->resize_path()); + @rename($original->thumb_path(), $this->thumb_path()); + } - if ($original->parent_id != $this->parent_id) { - // This will result in 2 events since we'll still fire the item_updated event below - module::event("item_moved", $this, $original->parent()); + + if ($original->parent_id != $this->parent_id) { + // This will result in 2 events since we'll still fire the item_updated event below + module::event("item_moved", $this, $original->parent()); + } } - } - // Changing the name, slug or parent ripples downwards - if ($this->is_album() && - ($original->name != $this->name || - $original->slug != $this->slug || - $original->parent_id != $this->parent_id)) { - db::build() - ->update("items") - ->set("relative_url_cache", null) - ->set("relative_path_cache", null) - ->where("left_ptr", ">", $this->left_ptr) - ->where("right_ptr", "<", $this->right_ptr) - ->execute(); + // Changing the name, slug or parent ripples downwards + if ($this->is_album() && + ($original->name != $this->name || + $original->slug != $this->slug || + $original->parent_id != $this->parent_id)) { + db::build() + ->update("items") + ->set("relative_url_cache", null) + ->set("relative_path_cache", null) + ->where("left_ptr", ">", $this->left_ptr) + ->where("right_ptr", "<", $this->right_ptr) + ->execute(); + } } module::event("item_updated", $original, $this); @@ -784,27 +777,36 @@ class Item_Model extends ORM_MPTT { return; } else if (rtrim($this->name, ".") !== $this->name) { $v->add_error("name", "no_trailing_period"); - } else if ($this->is_movie() || $this->is_photo()) { - if ($this->original()->loaded()) { + return; + } + + if ($this->is_movie() || $this->is_photo()) { + if ($this->loaded()) { // Existing items can't change their extension + $original = ORM::factory("item")->where("id", "=", $this->id)->find(); $new_ext = pathinfo($this->name, PATHINFO_EXTENSION); - $old_ext = pathinfo($this->original()->name, PATHINFO_EXTENSION); + $old_ext = pathinfo($original->name, PATHINFO_EXTENSION); if (strcasecmp($new_ext, $old_ext)) { $v->add_error("name", "illegal_data_file_extension"); + return; } } else { // New items must have an extension if (!pathinfo($this->name, PATHINFO_EXTENSION)) { $v->add_error("name", "illegal_data_file_extension"); + return; } } - } else if (db::build() + } + + 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; } } @@ -900,7 +902,7 @@ class Item_Model extends ORM_MPTT { * This field cannot be changed after it's been set. */ public function read_only(Validation $v, $field) { - if ($this->original()->loaded() && $this->original()->$field != $this->$field) { + if ($this->loaded() && isset($this->changed[$field])) { $v->add_error($field, "read_only"); } } diff --git a/modules/gallery/tests/Item_Model_Test.php b/modules/gallery/tests/Item_Model_Test.php index 9ea74b16..efad660e 100644 --- a/modules/gallery/tests/Item_Model_Test.php +++ b/modules/gallery/tests/Item_Model_Test.php @@ -139,6 +139,7 @@ class Item_Model_Test extends Gallery_Unit_Test_Case { try { $item->name = $item2->name; + print "SAVE\n"; $item->save(); } catch (ORM_Validation_Exception $e) { $this->assert_true(in_array("conflict", $e->validation->errors())); -- cgit v1.2.3 From dde429f71e536d66565eb6aa78bf58c39a243abb Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Wed, 20 Jan 2010 23:49:20 -0800 Subject: Whitespace. --- modules/gallery/models/item.php | 1 - 1 file changed, 1 deletion(-) (limited to 'modules/gallery/models') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index 19c379ab..51857440 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -447,7 +447,6 @@ class Item_Model extends ORM_MPTT { @rename($original->thumb_path(), $this->thumb_path()); } - if ($original->parent_id != $this->parent_id) { // This will result in 2 events since we'll still fire the item_updated event below module::event("item_moved", $this, $original->parent()); -- cgit v1.2.3 From 953c9283ad039adde0d131f2a0e563642d3dcbd1 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 25 Jan 2010 23:39:24 -0800 Subject: Always keep the original around when updating existing items, because we need it for the item_updated event. --- modules/gallery/models/item.php | 65 ++++++++++++++++++++--------------------- 1 file changed, 32 insertions(+), 33 deletions(-) (limited to 'modules/gallery/models') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index 51857440..4f5105b1 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -417,8 +417,8 @@ class Item_Model extends ORM_MPTT { // If any significant fields have changed, load up a copy of the original item and // keep it around. + $original = ORM::factory("item")->where("id", "=", $this->id)->find(); if (array_intersect($this->changed, array("parent_id", "name", "slug"))) { - $original = ORM::factory("item")->where("id", "=", $this->id)->find(); $original->_build_relative_caches(); $this->relative_path_cache = null; $this->relative_url_cache = null; @@ -429,45 +429,44 @@ class Item_Model extends ORM_MPTT { // Now update the filesystem and any database caches if there were significant value // changes. If anything past this point fails, then we'll have an inconsistent database // so this code should be as robust as we can make it. - if (isset($original)) { - // Update the MPTT pointers, if necessary. We have to do this before we generate any - // cached paths! - if ($original->parent_id != $this->parent_id) { - parent::move_to($this->parent()); - } - if ($original->parent_id != $this->parent_id || $original->name != $this->name) { - // Move all of the items associated data files - @rename($original->file_path(), $this->file_path()); - if ($this->is_album()) { - @rename(dirname($original->resize_path()), dirname($this->resize_path())); - @rename(dirname($original->thumb_path()), dirname($this->thumb_path())); - } else { - @rename($original->resize_path(), $this->resize_path()); - @rename($original->thumb_path(), $this->thumb_path()); - } + // Update the MPTT pointers, if necessary. We have to do this before we generate any + // cached paths! + if ($original->parent_id != $this->parent_id) { + parent::move_to($this->parent()); + } - if ($original->parent_id != $this->parent_id) { - // This will result in 2 events since we'll still fire the item_updated event below - module::event("item_moved", $this, $original->parent()); - } + if ($original->parent_id != $this->parent_id || $original->name != $this->name) { + // Move all of the items associated data files + @rename($original->file_path(), $this->file_path()); + if ($this->is_album()) { + @rename(dirname($original->resize_path()), dirname($this->resize_path())); + @rename(dirname($original->thumb_path()), dirname($this->thumb_path())); + } else { + @rename($original->resize_path(), $this->resize_path()); + @rename($original->thumb_path(), $this->thumb_path()); } - // Changing the name, slug or parent ripples downwards - if ($this->is_album() && - ($original->name != $this->name || - $original->slug != $this->slug || - $original->parent_id != $this->parent_id)) { - db::build() - ->update("items") - ->set("relative_url_cache", null) - ->set("relative_path_cache", null) - ->where("left_ptr", ">", $this->left_ptr) - ->where("right_ptr", "<", $this->right_ptr) - ->execute(); + if ($original->parent_id != $this->parent_id) { + // This will result in 2 events since we'll still fire the item_updated event below + module::event("item_moved", $this, $original->parent()); } } + // Changing the name, slug or parent ripples downwards + if ($this->is_album() && + ($original->name != $this->name || + $original->slug != $this->slug || + $original->parent_id != $this->parent_id)) { + db::build() + ->update("items") + ->set("relative_url_cache", null) + ->set("relative_path_cache", null) + ->where("left_ptr", ">", $this->left_ptr) + ->where("right_ptr", "<", $this->right_ptr) + ->execute(); + } + module::event("item_updated", $original, $this); } } else if (!empty($this->changed)) { -- cgit v1.2.3 From 5c68519d9233eabe98fce34fc00cdd4595f03ded Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Tue, 26 Jan 2010 00:23:45 -0800 Subject: Specialize the album cover id check to allow the root album to have no album cover. --- modules/gallery/models/item.php | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) (limited to 'modules/gallery/models') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index 4f5105b1..e4ff0bfb 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -717,7 +717,7 @@ class Item_Model extends ORM_MPTT { public function validate($array=null) { if (!$array) { $this->rules = array( - "album_cover_item_id" => array("callbacks" => array(array($this, "valid_item"))), + "album_cover_item_id" => array("callbacks" => array(array($this, "valid_album_cover"))), "description" => array("rules" => array("length[0,65535]")), "mime_type" => array("callbacks" => array(array($this, "valid_field"))), "name" => array("rules" => array("length[0,255]", "required"), @@ -848,14 +848,18 @@ class Item_Model extends ORM_MPTT { } /** - * Make sure the field refers to a valid item by id, or is null. + * Make sure the album cover item id refers to a valid item, or is null. */ - public function valid_item(Validation $v, $field) { - if ($this->$field && db::build() + public function valid_album_cover(Validation $v, $field) { + if ($this->id == 1) { + return; + } + + if ($this->album_cover_item_id && db::build() ->from("items") - ->where("id", "=", $this->$field) + ->where("id", "=", $this->album_cover_item_id) ->count_records() != 1) { - $v->add_error($field, "invalid_item"); + $v->add_error("album_cover_item_id", "invalid_item"); } } -- cgit v1.2.3 From 212633d05a5a8abb77a744a69c61d6e3051b73c5 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Wed, 27 Jan 2010 21:52:18 -0800 Subject: Prevent accidentally deleting the root album. --- modules/gallery/models/item.php | 6 ++++++ modules/gallery/tests/Item_Model_Test.php | 10 ++++++++++ 2 files changed, 16 insertions(+) (limited to 'modules/gallery/models') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index e4ff0bfb..9706d61f 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -71,6 +71,12 @@ class Item_Model extends ORM_MPTT { } public function delete() { + if ($this->id == 1) { + $v = new Validation(array("id")); + $v->add_error("id", "cant_delete_root_album"); + ORM_Validation_Exception::handle_validation($this->table_name, $v); + } + $old = clone $this; module::event("item_before_delete", $this); diff --git a/modules/gallery/tests/Item_Model_Test.php b/modules/gallery/tests/Item_Model_Test.php index 1e77076a..eb9ecc99 100644 --- a/modules/gallery/tests/Item_Model_Test.php +++ b/modules/gallery/tests/Item_Model_Test.php @@ -336,4 +336,14 @@ class Item_Model_Test extends Gallery_Unit_Test_Case { } $this->assert_true(false, "Shouldn't get here"); } + + public function cant_delete_root_album_test() { + try { + item::root()->delete(); + } catch (ORM_Validation_Exception $e) { + $this->assert_same(array("id" => "cant_delete_root_album"), $e->validation->errors()); + return; // pass + } + $this->assert_true(false, "Shouldn't get here"); + } } -- cgit v1.2.3 From 4b32a71afc7650fe7bdd02ba384c8914f60538f3 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Wed, 27 Jan 2010 22:34:11 -0800 Subject: Convert back to using ORM::factory(..., $id) instead of calling where(). --- modules/comment/models/comment.php | 2 +- modules/comment/tests/Comment_Event_Test.php | 2 +- modules/g2_import/controllers/g2.php | 2 +- modules/g2_import/helpers/g2_import.php | 11 +++++------ modules/gallery/helpers/gallery_installer.php | 2 +- modules/gallery/helpers/item_rest.php | 2 +- modules/gallery/libraries/ORM_MPTT.php | 2 +- modules/gallery/models/item.php | 4 ++-- modules/server_add/controllers/server_add.php | 13 ++++++------- modules/tag/helpers/item_tags_rest.php | 2 +- modules/tag/helpers/tag_item_rest.php | 4 ++-- modules/tag/helpers/tag_items_rest.php | 2 +- modules/tag/helpers/tag_rest.php | 2 +- modules/user/models/group.php | 2 +- modules/user/models/user.php | 2 +- 15 files changed, 26 insertions(+), 28 deletions(-) (limited to 'modules/gallery/models') diff --git a/modules/comment/models/comment.php b/modules/comment/models/comment.php index 43c4148f..8be022b5 100644 --- a/modules/comment/models/comment.php +++ b/modules/comment/models/comment.php @@ -108,7 +108,7 @@ class Comment_Model extends ORM { module::event("comment_created", $this); } else { // Updated comment - $original = ORM::factory("comment")->where("id", "=", $this->id)->find(); + $original = ORM::factory("comment", $this->id); $visible_change = $original->state == "published" || $this->state == "published"; parent::save(); module::event("comment_updated", $original, $this); diff --git a/modules/comment/tests/Comment_Event_Test.php b/modules/comment/tests/Comment_Event_Test.php index 27272055..08f55b3f 100644 --- a/modules/comment/tests/Comment_Event_Test.php +++ b/modules/comment/tests/Comment_Event_Test.php @@ -30,6 +30,6 @@ class Comment_Event_Test extends Gallery_Unit_Test_Case { $album->delete(); - $this->assert_false(ORM::factory("comment")->where("id", "=", $comment->id)->find()->loaded()); + $this->assert_false(ORM::factory("comment", $comment->id)->loaded()); } } diff --git a/modules/g2_import/controllers/g2.php b/modules/g2_import/controllers/g2.php index 3e002758..5fd4400c 100644 --- a/modules/g2_import/controllers/g2.php +++ b/modules/g2_import/controllers/g2.php @@ -50,7 +50,7 @@ class G2_Controller extends Admin_Controller { throw new Kohana_404_Exception(); } - $item = ORM::factory("item")->where("id", "=", $g2_map->g3_id)->find(); + $item = ORM::factory("item", $g2_map->g3_id); if (!$item->loaded() || !access::can("view", $item)) { throw new Kohana_404_Exception(); } diff --git a/modules/g2_import/helpers/g2_import.php b/modules/g2_import/helpers/g2_import.php index 74164305..fa95e547 100644 --- a/modules/g2_import/helpers/g2_import.php +++ b/modules/g2_import/helpers/g2_import.php @@ -358,8 +358,7 @@ class g2_import_Core { if ($g2_album->getParentId() == null) { return t("Skipping Gallery 2 root album"); } - $parent_album = - ORM::factory("item")->where("id", "=", self::map($g2_album->getParentId()))->find(); + $parent_album = ORM::factory("item", self::map($g2_album->getParentId())); $album = ORM::factory("item"); $album->type = "album"; @@ -423,8 +422,8 @@ class g2_import_Core { } $item_id = self::map($g2_source->getId()); if ($item_id) { - $item = ORM::factory("item")->where("id", "=", $item_id)->find(); - $g2_album = ORM::factory("item")->where("id", "=", $g3_album_id)->find(); + $item = ORM::factory("item", $item_id); + $g2_album = ORM::factory("item", $g3_album_id); $g2_album->album_cover_item_id = $item->id; $g2_album->thumb_dirty = 1; $g2_album->view_count = g2(GalleryCoreApi::fetchItemViewCount($g2_album_id)); @@ -452,7 +451,7 @@ class g2_import_Core { array("id" => $g2_item_id, "exception" => (string)$e)); } - $parent = ORM::factory("item")->where("id", "=", self::map($g2_item->getParentId()))->find(); + $parent = ORM::factory("item", self::map($g2_item->getParentId())); $g2_type = $g2_item->getEntityType(); $corrupt = 0; @@ -633,7 +632,7 @@ class g2_import_Core { GalleryCoreApi::requireOnce("modules/tags/classes/TagsHelper.class"); $g2_item_id = array_shift($queue); - $g3_item = ORM::factory("item")->where("id", "=", self::map($g2_item_id))->find(); + $g3_item = ORM::factory("item", self::map($g2_item_id)); if (!$g3_item->loaded()) { return; } diff --git a/modules/gallery/helpers/gallery_installer.php b/modules/gallery/helpers/gallery_installer.php index aa297236..bfab4645 100644 --- a/modules/gallery/helpers/gallery_installer.php +++ b/modules/gallery/helpers/gallery_installer.php @@ -228,7 +228,7 @@ class gallery_installer { "updated" => $now, "weight" => 1)) ->execute(); - $root = ORM::factory("item")->where("id", "=", 1)->find(); + $root = ORM::factory("item", 1); access::add_item($root); module::set_var("gallery", "active_site_theme", "wind"); diff --git a/modules/gallery/helpers/item_rest.php b/modules/gallery/helpers/item_rest.php index 2236fbbb..d5ca1456 100644 --- a/modules/gallery/helpers/item_rest.php +++ b/modules/gallery/helpers/item_rest.php @@ -145,7 +145,7 @@ class item_rest_Core { } static function resolve($id) { - $item = ORM::factory("item")->where("id", "=", $id)->find(); + $item = ORM::factory("item", $id); if (!access::can("view", $item)) { throw new Kohana_404_Exception(); } diff --git a/modules/gallery/libraries/ORM_MPTT.php b/modules/gallery/libraries/ORM_MPTT.php index a7bb24ea..83f9b51e 100644 --- a/modules/gallery/libraries/ORM_MPTT.php +++ b/modules/gallery/libraries/ORM_MPTT.php @@ -48,7 +48,7 @@ class ORM_MPTT_Core extends ORM { function save() { if (!$this->loaded()) { $this->lock(); - $parent = ORM::factory("item")->where("id", "=", $this->parent_id)->find(); + $parent = ORM::factory("item", $this->parent_id); try { // Make a hole in the parent for this new item diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index 9706d61f..ae1b6608 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -423,7 +423,7 @@ class Item_Model extends ORM_MPTT { // If any significant fields have changed, load up a copy of the original item and // keep it around. - $original = ORM::factory("item")->where("id", "=", $this->id)->find(); + $original = ORM::factory("item", $this->id); if (array_intersect($this->changed, array("parent_id", "name", "slug"))) { $original->_build_relative_caches(); $this->relative_path_cache = null; @@ -787,7 +787,7 @@ class Item_Model extends ORM_MPTT { if ($this->is_movie() || $this->is_photo()) { if ($this->loaded()) { // Existing items can't change their extension - $original = ORM::factory("item")->where("id", "=", $this->id)->find(); + $original = ORM::factory("item", $this->id); $new_ext = pathinfo($this->name, PATHINFO_EXTENSION); $old_ext = pathinfo($original->name, PATHINFO_EXTENSION); if (strcasecmp($new_ext, $old_ext)) { diff --git a/modules/server_add/controllers/server_add.php b/modules/server_add/controllers/server_add.php index 4d6d5dfe..287855b6 100644 --- a/modules/server_add/controllers/server_add.php +++ b/modules/server_add/controllers/server_add.php @@ -24,7 +24,7 @@ class Server_Add_Controller extends Admin_Controller { $files[] = $path; } - $item = ORM::factory("item")->where("id", "=", $id)->find(); + $item = ORM::factory("item", $id); $view = new View("server_add_tree_dialog.html"); $view->item = $item; $view->tree = new View("server_add_tree.html"); @@ -78,7 +78,7 @@ class Server_Add_Controller extends Admin_Controller { */ public function start() { access::verify_csrf(); - $item = ORM::factory("item")->where("id", "=", Input::instance()->get("item_id"))->find(); + $item = ORM::factory("item", Input::instance()->get("item_id")); foreach (Input::instance()->post("paths") as $path) { if (server_add::is_valid_path($path)) { @@ -104,7 +104,7 @@ class Server_Add_Controller extends Admin_Controller { function run($task_id) { access::verify_csrf(); - $task = ORM::factory("task")->where("id", "=", $task_id)->find(); + $task = ORM::factory("task", $task_id); if (!$task->loaded() || $task->owner_id != identity::active_user()->id) { access::forbidden(); } @@ -216,12 +216,11 @@ class Server_Add_Controller extends Admin_Controller { // Look up the parent item for this entry. By now it should exist, but if none was // specified, then this belongs as a child of the current item. - $parent_entry = - ORM::factory("server_add_file")->where("id", "=", $entry->parent_id)->find(); + $parent_entry = ORM::factory("server_add_file", $entry->parent_id); if (!$parent_entry->loaded()) { - $parent = ORM::factory("item")->where("id", "=", $task->get("item_id"))->find(); + $parent = ORM::factory("item", $task->get("item_id")); } else { - $parent = ORM::factory("item")->where("id", "=", $parent_entry->item_id)->find(); + $parent = ORM::factory("item", $parent_entry->item_id); } $name = basename($entry->file); diff --git a/modules/tag/helpers/item_tags_rest.php b/modules/tag/helpers/item_tags_rest.php index ce814f77..43e2cef0 100644 --- a/modules/tag/helpers/item_tags_rest.php +++ b/modules/tag/helpers/item_tags_rest.php @@ -50,7 +50,7 @@ class item_tags_rest_Core { } static function resolve($id) { - $item = ORM::factory("item")->where("id", "=", $id)->find(); + $item = ORM::factory("item", $id); if (!access::can("view", $item)) { throw new Kohana_404_Exception(); } diff --git a/modules/tag/helpers/tag_item_rest.php b/modules/tag/helpers/tag_item_rest.php index cd9bb6fe..60d37437 100644 --- a/modules/tag/helpers/tag_item_rest.php +++ b/modules/tag/helpers/tag_item_rest.php @@ -35,8 +35,8 @@ class tag_item_rest_Core { static function resolve($tuple) { list ($tag_id, $item_id) = split(",", $tuple); - $tag = ORM::factory("tag")->where("id", "=", $tag_id)->find(); - $item = ORM::factory("item")->where("id", "=", $item_id)->find(); + $tag = ORM::factory("tag", $tag_id); + $item = ORM::factory("item", $item_id); if (!$tag->loaded() || !$item->loaded() || !$tag->has($item)) { throw new Kohana_404_Exception(); } diff --git a/modules/tag/helpers/tag_items_rest.php b/modules/tag/helpers/tag_items_rest.php index 369a8d83..ef563ac6 100644 --- a/modules/tag/helpers/tag_items_rest.php +++ b/modules/tag/helpers/tag_items_rest.php @@ -52,7 +52,7 @@ class tag_items_rest_Core { } static function resolve($id) { - return ORM::factory("tag")->where("id", "=", $id)->find(); + return ORM::factory("tag", $id); } static function url($tag) { diff --git a/modules/tag/helpers/tag_rest.php b/modules/tag/helpers/tag_rest.php index 7143daa9..4879cf63 100644 --- a/modules/tag/helpers/tag_rest.php +++ b/modules/tag/helpers/tag_rest.php @@ -77,7 +77,7 @@ class tag_rest_Core { } static function resolve($id) { - $tag = ORM::factory("tag")->where("id", "=", $id)->find(); + $tag = ORM::factory("tag", $id); if (!$tag->loaded()) { throw new Kohana_404_Exception(); } diff --git a/modules/user/models/group.php b/modules/user/models/group.php index 85114ede..851e72e6 100644 --- a/modules/user/models/group.php +++ b/modules/user/models/group.php @@ -55,7 +55,7 @@ class Group_Model extends ORM implements Group_Definition { module::event("group_created", $this); } else { // Updated group - $original = ORM::factory("group")->where("id", "=", $this->id)->find(); + $original = ORM::factory("group", $this->id); parent::save(); module::event("group_updated", $original, $this); } diff --git a/modules/user/models/user.php b/modules/user/models/user.php index 7c97bae7..78c31047 100644 --- a/modules/user/models/user.php +++ b/modules/user/models/user.php @@ -99,7 +99,7 @@ class User_Model extends ORM implements User_Definition { module::event("user_created", $this); } else { // Updated user - $original = ORM::factory("user")->where("id", "=", $this->id)->find(); + $original = ORM::factory("user", $this->id); parent::save(); module::event("user_updated", $original, $this); } -- cgit v1.2.3 From c4e360431564627003e4c7864b5dd5a07297e91e Mon Sep 17 00:00:00 2001 From: Tim Almdal Date: Fri, 29 Jan 2010 14:04:27 -0800 Subject: Strongly type the argument list to the model::validate method. --- modules/comment/models/comment.php | 2 +- modules/gallery/models/item.php | 2 +- modules/user/models/group.php | 2 +- modules/user/models/user.php | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) (limited to 'modules/gallery/models') diff --git a/modules/comment/models/comment.php b/modules/comment/models/comment.php index 8be022b5..add15ce8 100644 --- a/modules/comment/models/comment.php +++ b/modules/comment/models/comment.php @@ -56,7 +56,7 @@ class Comment_Model extends ORM { /** * Add some custom per-instance rules. */ - public function validate($array=null) { + public function validate(Validation $array=null) { // validate() is recursive, only modify the rules on the outermost call. if (!$array) { $this->rules = array( diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index ae1b6608..ae6e4cc9 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -720,7 +720,7 @@ class Item_Model extends ORM_MPTT { /** * Specify our rules here so that we have access to the instance of this model. */ - public function validate($array=null) { + public function validate(Validation $array=null) { if (!$array) { $this->rules = array( "album_cover_item_id" => array("callbacks" => array(array($this, "valid_album_cover"))), diff --git a/modules/user/models/group.php b/modules/user/models/group.php index 851e72e6..82843ad1 100644 --- a/modules/user/models/group.php +++ b/modules/user/models/group.php @@ -37,7 +37,7 @@ class Group_Model extends ORM implements Group_Definition { /** * Specify our rules here so that we have access to the instance of this model. */ - public function validate($array=null) { + public function validate(Validation $array=null) { // validate() is recursive, only modify the rules on the outermost call. if (!$array) { $this->rules = array( diff --git a/modules/user/models/user.php b/modules/user/models/user.php index 78c31047..0cd634ea 100644 --- a/modules/user/models/user.php +++ b/modules/user/models/user.php @@ -62,7 +62,7 @@ class User_Model extends ORM implements User_Definition { /** * Specify our rules here so that we have access to the instance of this model. */ - public function validate($array=null) { + public function validate(Validation $array=null) { // validate() is recursive, only modify the rules on the outermost call. if (!$array) { $this->rules = array( -- cgit v1.2.3 From 2bfcec9620814a6f3d0163a174d7ba90efef369d Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Sat, 30 Jan 2010 19:48:57 -0800 Subject: Prevent brute force login attacks by reducing login attempts to 1 per minute after there have been 5 consecutive failed login attempts. Fix for ticket #589. --- modules/gallery/controllers/login.php | 7 ++--- modules/gallery/helpers/auth.php | 45 ++++++++++++++++++++++++++- modules/gallery/helpers/gallery_event.php | 5 +++ modules/gallery/helpers/gallery_installer.php | 22 ++++++++++++- modules/gallery/models/failed_login.php | 20 ++++++++++++ modules/gallery/module.info | 3 +- 6 files changed, 94 insertions(+), 8 deletions(-) create mode 100644 modules/gallery/models/failed_login.php (limited to 'modules/gallery/models') diff --git a/modules/gallery/controllers/login.php b/modules/gallery/controllers/login.php index cfe86cfb..1426f0d8 100644 --- a/modules/gallery/controllers/login.php +++ b/modules/gallery/controllers/login.php @@ -62,11 +62,10 @@ class Login_Controller extends Controller { if ($valid) { $user = identity::lookup_user_by_name($form->login->inputs["name"]->value); if (empty($user) || !identity::is_correct_password($user, $form->login->password->value)) { - log::warning( - "user", - t("Failed login for %name", - array("name" => $form->login->inputs["name"]->value))); $form->login->inputs["name"]->add_error("invalid_login", 1); + $name = $form->login->inputs["name"]->value; + log::warning("user", t("Failed login for %name", array("name" => $name))); + module::event("user_login_failed", $name); $valid = false; } } diff --git a/modules/gallery/helpers/auth.php b/modules/gallery/helpers/auth.php index f7d4f7e8..e112f127 100644 --- a/modules/gallery/helpers/auth.php +++ b/modules/gallery/helpers/auth.php @@ -22,7 +22,10 @@ class auth_Core { $form = new Forge($url, "", "post", array("id" => "g-login-form")); $form->set_attr('class', "g-narrow"); $group = $form->group("login")->label(t("Login")); - $group->input("name")->label(t("Username"))->id("g-username")->class(null); + $group->input("name")->label(t("Username"))->id("g-username")->class(null) + ->callback("auth::validate_too_many_failed_logins") + ->error_messages( + "too_many_failed_logins", t("Too many failed login attempts. Try again later")); $group->password("password")->label(t("Password"))->id("g-password")->class(null); $group->inputs["name"]->error_messages("invalid_login", t("Invalid name or password")); $group->submit("")->value(t("Login")); @@ -55,4 +58,44 @@ class auth_Core { array("url" => user_profile::url($user->id), "user_name" => html::clean($user->name)))); } + + /** + * After there have been 5 failed login attempts, any failure leads to getting locked out for a + * minute. + */ + static function validate_too_many_failed_logins($name_input) { + $failed_login = ORM::factory("failed_login") + ->where("name", "=", $name_input->value) + ->find(); + if ($failed_login->loaded() && + $failed_login->count > 5 && + (time() - $failed_login->time < 60)) { + $name_input->add_error("too_many_failed_logins", 1); + } + } + + /** + * Record a failed login for this user + */ + static function record_failed_login($name) { + $failed_login = ORM::factory("failed_login") + ->where("name", "=", $name) + ->find(); + if (!$failed_login->loaded()) { + $failed_login->name = $name; + } + $failed_login->time = time(); + $failed_login->count++; + $failed_login->save(); + } + + /** + * Clear any failed logins for this user + */ + static function record_successful_login($user) { + db::build() + ->delete("failed_logins") + ->where("name", "=", $user->name) + ->execute(); + } } \ No newline at end of file diff --git a/modules/gallery/helpers/gallery_event.php b/modules/gallery/helpers/gallery_event.php index 80452276..6479e2c3 100644 --- a/modules/gallery/helpers/gallery_event.php +++ b/modules/gallery/helpers/gallery_event.php @@ -110,6 +110,11 @@ class gallery_event_Core { graphics::choose_default_toolkit(); module::clear_var("gallery", "choose_default_tookit"); } + auth::record_successful_login($user); + } + + static function user_login_failed($name) { + auth::record_failed_login($name); } static function item_index_data($item, $data) { diff --git a/modules/gallery/helpers/gallery_installer.php b/modules/gallery/helpers/gallery_installer.php index d2378d64..cf701ed4 100644 --- a/modules/gallery/helpers/gallery_installer.php +++ b/modules/gallery/helpers/gallery_installer.php @@ -42,6 +42,14 @@ class gallery_installer { KEY (`tags`)) DEFAULT CHARSET=utf8;"); + $db->query("CREATE TABLE {failed_logins} ( + `id` int(9) NOT NULL auto_increment, + `count` int(9) NOT NULL, + `name` varchar(255) NOT NULL, + `time` int(9) NOT NULL, + PRIMARY KEY (`id`)) + DEFAULT CHARSET=utf8;"); + $db->query("CREATE TABLE {graphics_rules} ( `id` int(9) NOT NULL auto_increment, `active` BOOLEAN default 0, @@ -276,7 +284,7 @@ class gallery_installer { // @todo this string needs to be picked up by l10n_scanner module::set_var("gallery", "credits", "Powered by Gallery %version"); module::set_var("gallery", "simultaneous_upload_limit", 5); - module::set_version("gallery", 22); + module::set_version("gallery", 23); } static function upgrade($version) { @@ -485,6 +493,17 @@ class gallery_installer { } module::set_version("gallery", $version = 23); } + + if ($version = 23) { + $db->query("CREATE TABLE {failed_logins} ( + `id` int(9) NOT NULL auto_increment, + `count` int(9) NOT NULL, + `name` varchar(255) NOT NULL, + `time` int(9) NOT NULL, + PRIMARY KEY (`id`)) + DEFAULT CHARSET=utf8;"); + module::set_version("gallery", $version = 24); + } } static function uninstall() { @@ -493,6 +512,7 @@ class gallery_installer { $db->query("DROP TABLE IF EXISTS {access_intents}"); $db->query("DROP TABLE IF EXISTS {graphics_rules}"); $db->query("DROP TABLE IF EXISTS {incoming_translations}"); + $db->query("DROP TABLE IF EXISTS {failed_logins}"); $db->query("DROP TABLE IF EXISTS {items}"); $db->query("DROP TABLE IF EXISTS {logs}"); $db->query("DROP TABLE IF EXISTS {modules}"); diff --git a/modules/gallery/models/failed_login.php b/modules/gallery/models/failed_login.php new file mode 100644 index 00000000..0b84c295 --- /dev/null +++ b/modules/gallery/models/failed_login.php @@ -0,0 +1,20 @@ + Date: Sat, 30 Jan 2010 23:36:11 -0800 Subject: Add Item_Model::as_restful_array() for convenience. --- modules/gallery/models/item.php | 17 +++++++++++++++++ modules/gallery/tests/Item_Model_Test.php | 12 ++++++++++++ 2 files changed, 29 insertions(+) (limited to 'modules/gallery/models') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index ae6e4cc9..fd121a5a 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -914,4 +914,21 @@ class Item_Model extends ORM_MPTT { $v->add_error($field, "read_only"); } } + + /** + * Same as ORM::as_array() but convert id fields into their RESTful form. + */ + public function as_restful_array() { + // Convert item ids to rest URLs for consistency + $data = $this->as_array(); + if ($tmp = $this->parent()) { + $data["parent"] = rest::url("item", $tmp); + } + unset($data["parent_id"]); + if ($tmp = $this->album_cover()) { + $data["album_cover"] = rest::url("item", $tmp); + } + unset($data["album_cover_item_id"]); + return $data; + } } diff --git a/modules/gallery/tests/Item_Model_Test.php b/modules/gallery/tests/Item_Model_Test.php index eb9ecc99..9f632fb5 100644 --- a/modules/gallery/tests/Item_Model_Test.php +++ b/modules/gallery/tests/Item_Model_Test.php @@ -346,4 +346,16 @@ class Item_Model_Test extends Gallery_Unit_Test_Case { } $this->assert_true(false, "Shouldn't get here"); } + + public function as_restful_array_test() { + $album = test::random_album(); + $photo = test::random_photo($album); + $album->reload(); + + $result = $album->as_restful_array(); + $this->assert_same(rest::url("item", item::root()), $result["parent"]); + $this->assert_same(rest::url("item", $photo), $result["album_cover"]); + $this->assert_true(!array_key_exists("parent_id", $result)); + $this->assert_true(!array_key_exists("album_cover_item_id", $result)); + } } -- cgit v1.2.3 From ee35b0a9fe16b862e6d1080fec45b539e1b41375 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Sun, 31 Jan 2010 13:10:34 -0800 Subject: Elide data that isn't useful from the REST array. --- modules/gallery/models/item.php | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'modules/gallery/models') diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index fd121a5a..083fd06b 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -929,6 +929,11 @@ class Item_Model extends ORM_MPTT { $data["album_cover"] = rest::url("item", $tmp); } unset($data["album_cover_item_id"]); + + // Elide some internal-only data that is going to cause confusion in the client. + foreach (array("relative_path_cache", "relative_url_cache", "left_ptr", "right_ptr") as $key) { + unset($data[$key]); + } return $data; } } -- cgit v1.2.3 From c050acf30a7351bf0ef5b8ee206704c073e881c7 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Sun, 31 Jan 2010 16:07:41 -0800 Subject: Fix lots of warnings that pop up when we're in E_STRICT mode. They're mostly issues around uninitialized variables, calling non-static functions in a static context, calling Session functions directly instead of on its singleton, passing non-variables by reference, and subclasses not using the same interface as the parent class. --- modules/comment/controllers/admin_comments.php | 1 + modules/comment/helpers/comment_rss.php | 1 + modules/comment/models/comment.php | 3 ++- modules/digibug/controllers/digibug.php | 2 +- modules/forge/libraries/Form_Group.php | 2 +- modules/g2_import/controllers/admin_g2_import.php | 5 +++++ modules/g2_import/helpers/g2_import.php | 11 +++++++++++ modules/g2_import/helpers/g2_import_task.php | 10 ++++++++-- modules/g2_import/views/admin_g2_import.html.php | 2 +- modules/gallery/controllers/admin_modules.php | 1 + modules/gallery/controllers/combined.php | 2 +- modules/gallery/controllers/file_proxy.php | 2 +- modules/gallery/helpers/gallery_block.php | 2 +- modules/gallery/helpers/gallery_rss.php | 1 + modules/gallery/helpers/gallery_task.php | 11 ++++++++--- modules/gallery/helpers/graphics.php | 3 +++ modules/gallery/helpers/l10n_client.php | 1 + modules/gallery/helpers/module.php | 3 ++- modules/gallery/libraries/Form_Script.php | 4 ++-- modules/gallery/libraries/MY_Database.php | 2 +- modules/gallery/libraries/MY_View.php | 2 +- modules/gallery/libraries/ORM_MPTT.php | 2 +- modules/gallery/models/item.php | 2 +- modules/gallery/models/task.php | 4 ++-- modules/gallery/tests/Database_Test.php | 8 ++++---- modules/gallery/tests/Item_Rest_Helper_Test.php | 17 +++++++++++++++++ .../gallery_unit_test/controllers/gallery_unit_test.php | 6 +++++- modules/rest/controllers/rest.php | 1 + modules/rest/helpers/rest.php | 2 +- modules/rest/tests/Rest_Controller_Test.php | 8 ++++---- modules/search/helpers/search.php | 3 ++- modules/tag/helpers/tag_rss.php | 2 ++ modules/tag/helpers/tags_rest.php | 1 - modules/tag/models/tag.php | 2 +- modules/tag/tests/Tag_Item_Rest_Helper_Test.php | 4 +++- modules/tag/tests/Tag_Rest_Helper_Test.php | 8 ++++++++ modules/tag/tests/Tags_Rest_Helper_Test.php | 4 ++++ modules/user/controllers/admin_users.php | 2 +- 38 files changed, 111 insertions(+), 36 deletions(-) (limited to 'modules/gallery/models') diff --git a/modules/comment/controllers/admin_comments.php b/modules/comment/controllers/admin_comments.php index b7dc5fb3..3dd45919 100644 --- a/modules/comment/controllers/admin_comments.php +++ b/modules/comment/controllers/admin_comments.php @@ -92,6 +92,7 @@ class Admin_Comments_Controller extends Admin_Controller { } private function _counts() { + $counts = new stdClass(); $counts->unpublished = 0; $counts->published = 0; $counts->spam = 0; diff --git a/modules/comment/helpers/comment_rss.php b/modules/comment/helpers/comment_rss.php index 77044884..79fa07df 100644 --- a/modules/comment/helpers/comment_rss.php +++ b/modules/comment/helpers/comment_rss.php @@ -42,6 +42,7 @@ class comment_rss_Core { $comments->where("item_id", "=", $id); } + $feed = new stdClass(); $feed->view = "comment.mrss"; $feed->children = array(); foreach ($comments->find_all($limit, $offset) as $comment) { diff --git a/modules/comment/models/comment.php b/modules/comment/models/comment.php index add15ce8..d9d05995 100644 --- a/modules/comment/models/comment.php +++ b/modules/comment/models/comment.php @@ -116,7 +116,8 @@ class Comment_Model extends ORM { // We only notify on the related items if we're making a visible change. if ($visible_change) { - module::event("item_related_update", $this->item()); + $item = $this->item(); + module::event("item_related_update", $item); } return $this; diff --git a/modules/digibug/controllers/digibug.php b/modules/digibug/controllers/digibug.php index e3b06196..c98ae20c 100644 --- a/modules/digibug/controllers/digibug.php +++ b/modules/digibug/controllers/digibug.php @@ -91,7 +91,7 @@ class Digibug_Controller extends Controller { } // We don't need to save the session for this request - Session::abort_save(); + Session::instance()->abort_save(); if (!TEST_MODE) { // Dump out the image diff --git a/modules/forge/libraries/Form_Group.php b/modules/forge/libraries/Form_Group.php index e0601321..0a04912b 100644 --- a/modules/forge/libraries/Form_Group.php +++ b/modules/forge/libraries/Form_Group.php @@ -80,7 +80,7 @@ class Form_Group_Core extends Forge { } } - public function render() + public function render($template = 'forge_template', $custom = FALSE) { // No Sir, we don't want any html today thank you return; diff --git a/modules/g2_import/controllers/admin_g2_import.php b/modules/g2_import/controllers/admin_g2_import.php index 1c65f482..6dd155b9 100644 --- a/modules/g2_import/controllers/admin_g2_import.php +++ b/modules/g2_import/controllers/admin_g2_import.php @@ -19,6 +19,7 @@ */ class Admin_g2_import_Controller extends Admin_Controller { public function index() { + g2_import::lower_error_reporting(); if (g2_import::is_configured()) { g2_import::init(); } @@ -31,6 +32,7 @@ class Admin_g2_import_Controller extends Admin_Controller { $view = new Admin_View("admin.html"); $view->content = new View("admin_g2_import.html"); $view->content->form = $this->_get_import_form(); + $view->content->version = g2_import::version(); if (g2_import::is_initialized()) { $view->content->g2_stats = $g2_stats; @@ -38,11 +40,13 @@ class Admin_g2_import_Controller extends Admin_Controller { $view->content->thumb_size = module::get_var("gallery", "thumb_size"); $view->content->resize_size = module::get_var("gallery", "resize_size"); } + g2_import::restore_error_reporting(); print $view; } public function save() { access::verify_csrf(); + g2_import::lower_error_reporting(); $form = $this->_get_import_form(); if ($form->validate()) { @@ -63,6 +67,7 @@ class Admin_g2_import_Controller extends Admin_Controller { $view = new Admin_View("admin.html"); $view->content = new View("admin_g2_import.html"); $view->content->form = $form; + g2_import::restore_error_reporting(); print $view; } diff --git a/modules/g2_import/helpers/g2_import.php b/modules/g2_import/helpers/g2_import.php index fa95e547..0fcc0539 100644 --- a/modules/g2_import/helpers/g2_import.php +++ b/modules/g2_import/helpers/g2_import.php @@ -24,6 +24,7 @@ class g2_import_Core { public static $g2_base_url = null; private static $current_g2_item = null; + private static $error_reporting = null; static function is_configured() { return module::get_var("g2_import", "embed_path"); @@ -931,6 +932,16 @@ class g2_import_Core { "useAuthToken" => false)); return str_replace(self::$g2_base_url, "", $url); } + + static function lower_error_reporting() { + // Gallery 2 was not designed to run in E_STRICT mode and will barf out errors. So dial down + // the error reporting when we make G2 calls. + self::$error_reporting = error_reporting(error_reporting() & ~E_STRICT); + } + + static function restore_error_reporting() { + error_reporting(self::$error_reporting); + } } /** diff --git a/modules/g2_import/helpers/g2_import_task.php b/modules/g2_import/helpers/g2_import_task.php index e80b88b9..21ba4c3a 100644 --- a/modules/g2_import/helpers/g2_import_task.php +++ b/modules/g2_import/helpers/g2_import_task.php @@ -19,17 +19,19 @@ */ class g2_import_task_Core { static function available_tasks() { + g2_import::lower_error_reporting(); if (g2_import::is_configured()) { g2_import::init(); } - + $version = g2_import::version(); + g2_import::restore_error_reporting(); if (class_exists("GalleryCoreApi")) { return array(Task_Definition::factory() ->callback("g2_import_task::import") ->name(t("Import from Gallery 2")) ->description( - t("Gallery %version detected", array("version" => g2_import::version()))) + t("Gallery %version detected", array("version" => $version))) ->severity(log::SUCCESS)); } @@ -37,6 +39,8 @@ class g2_import_task_Core { } static function import($task) { + g2_import::lower_error_reporting(); + $start = microtime(true); g2_import::init(); @@ -207,5 +211,7 @@ class g2_import_task_Core { $task->set("mode", $mode); $task->set("queue", $queue); $task->set("done", $done); + + g2_import::restore_error_reporting(); } } diff --git a/modules/g2_import/views/admin_g2_import.html.php b/modules/g2_import/views/admin_g2_import.html.php index 0875e7f7..6a5214a3 100644 --- a/modules/g2_import/views/admin_g2_import.html.php +++ b/modules/g2_import/views/admin_g2_import.html.php @@ -34,7 +34,7 @@

  • - g2_import::version())) ?> + $version)) ?>
  • diff --git a/modules/gallery/controllers/admin_modules.php b/modules/gallery/controllers/admin_modules.php index 84fee25d..081b3f12 100644 --- a/modules/gallery/controllers/admin_modules.php +++ b/modules/gallery/controllers/admin_modules.php @@ -67,6 +67,7 @@ class Admin_Modules_Controller extends Admin_Controller { } private function _do_save() { + $changes = new stdClass(); $changes->activate = array(); $changes->deactivate = array(); $activated_names = array(); diff --git a/modules/gallery/controllers/combined.php b/modules/gallery/controllers/combined.php index e90a2f1a..7f3a3c7d 100644 --- a/modules/gallery/controllers/combined.php +++ b/modules/gallery/controllers/combined.php @@ -41,7 +41,7 @@ class Combined_Controller extends Controller { $input = Input::instance(); // We don't need to save the session for this request - Session::abort_save(); + Session::instance()->abort_save(); // Our data is immutable, so if they already have a copy then it needs no updating. if ($input->server("HTTP_IF_MODIFIED_SINCE")) { diff --git a/modules/gallery/controllers/file_proxy.php b/modules/gallery/controllers/file_proxy.php index 646edf17..33952366 100644 --- a/modules/gallery/controllers/file_proxy.php +++ b/modules/gallery/controllers/file_proxy.php @@ -121,7 +121,7 @@ class File_Proxy_Controller extends Controller { expires::check(2592000, $item->updated); // We don't need to save the session for this request - Session::abort_save(); + Session::instance()->abort_save(); expires::set(2592000, $item->updated); // 30 days diff --git a/modules/gallery/helpers/gallery_block.php b/modules/gallery/helpers/gallery_block.php index 9d4e81b6..be0f11b8 100644 --- a/modules/gallery/helpers/gallery_block.php +++ b/modules/gallery/helpers/gallery_block.php @@ -72,7 +72,7 @@ class gallery_block_Core { $block->content = new View("admin_block_platform.html"); if (is_readable("/proc/loadavg")) { $block->content->load_average = - join(" ", array_slice(explode(" ", array_shift(file("/proc/loadavg"))), 0, 3)); + join(" ", array_slice(explode(" ", current(file("/proc/loadavg"))), 0, 3)); } else { $block->content->load_average = t("Unavailable"); } diff --git a/modules/gallery/helpers/gallery_rss.php b/modules/gallery/helpers/gallery_rss.php index d422636f..c1790d28 100644 --- a/modules/gallery/helpers/gallery_rss.php +++ b/modules/gallery/helpers/gallery_rss.php @@ -25,6 +25,7 @@ class gallery_rss_Core { } static function feed($feed_id, $offset, $limit, $id) { + $feed = new stdClass(); switch ($feed_id) { case "latest": $feed->children = ORM::factory("item") diff --git a/modules/gallery/helpers/gallery_task.php b/modules/gallery/helpers/gallery_task.php index c75e050a..b2f18d7c 100644 --- a/modules/gallery/helpers/gallery_task.php +++ b/modules/gallery/helpers/gallery_task.php @@ -111,6 +111,7 @@ class gallery_task_Core { site_status::clear("graphics_dirty"); } } catch (Exception $e) { + Kohana_Log::add("error",(string)$e); $task->done = true; $task->state = "error"; $task->status = $e->getMessage(); @@ -214,6 +215,7 @@ class gallery_task_Core { Cache::instance()->delete("update_l10n_cache:{$task->id}"); } } catch (Exception $e) { + Kohana_Log::add("error",(string)$e); $task->done = true; $task->state = "error"; $task->status = $e->getMessage(); @@ -233,10 +235,10 @@ class gallery_task_Core { try { $start = microtime(true); $data = Cache::instance()->get("file_cleanup_cache:{$task->id}"); - if ($data) { - $files = unserialize($data); - } + $files = $data ? unserialize($data) : array(); $i = 0; + $current = 0; + $total = 0; switch ($task->get("mode", "init")) { case "init": // 0% @@ -262,6 +264,7 @@ class gallery_task_Core { if (count($files) == 0) { break; } + case "delete_files": $current = $task->get("current"); $total = $task->get("total"); @@ -279,8 +282,10 @@ class gallery_task_Core { if ($total == $current) { $task->done = true; $task->state = "success"; + $task->percent_complete = 100; } } catch (Exception $e) { + Kohana_Log::add("error",(string)$e); $task->done = true; $task->state = "error"; $task->status = $e->getMessage(); diff --git a/modules/gallery/helpers/graphics.php b/modules/gallery/helpers/graphics.php index 5a290905..c85c7750 100644 --- a/modules/gallery/helpers/graphics.php +++ b/modules/gallery/helpers/graphics.php @@ -262,6 +262,9 @@ class graphics_Core { */ static function detect_toolkits() { $toolkits = new stdClass(); + $toolkits->gd = new stdClass(); + $toolkits->imagemagick = new stdClass(); + $toolkits->graphicsmagick = new stdClass(); // GD is special, it doesn't use exec() $gd = function_exists("gd_info") ? gd_info() : array(); diff --git a/modules/gallery/helpers/l10n_client.php b/modules/gallery/helpers/l10n_client.php index 086245e8..c27e4e5b 100644 --- a/modules/gallery/helpers/l10n_client.php +++ b/modules/gallery/helpers/l10n_client.php @@ -77,6 +77,7 @@ class l10n_client_Core { * translations for. */ static function fetch_updates(&$num_fetched) { + $request = new stdClass(); $request->locales = array(); $request->messages = new stdClass(); diff --git a/modules/gallery/helpers/module.php b/modules/gallery/helpers/module.php index 95e426c4..9523d1d2 100644 --- a/modules/gallery/helpers/module.php +++ b/modules/gallery/helpers/module.php @@ -430,7 +430,8 @@ class module_Core { // This could happen if there's a race condition continue; } - self::$var_cache->{$row->module_name}->{$row->name} = $row->value; + // Mute the "Creating default object from empty value" warning below + @self::$var_cache->{$row->module_name}->{$row->name} = $row->value; } $cache = ORM::factory("var"); $cache->module_name = "gallery"; diff --git a/modules/gallery/libraries/Form_Script.php b/modules/gallery/libraries/Form_Script.php index e841408d..1f965767 100644 --- a/modules/gallery/libraries/Form_Script.php +++ b/modules/gallery/libraries/Form_Script.php @@ -50,7 +50,7 @@ class Form_Script_Core extends Forge { return $this; } - public function render() { + public function render($template="forge_template", $custom=false) { $script = array(); if (!empty($this->data["url"])) { $script[] = html::script($this->data["url"]); @@ -63,4 +63,4 @@ class Form_Script_Core extends Forge { return implode("\n", $script); } -} // End Form Script \ No newline at end of file +} \ No newline at end of file diff --git a/modules/gallery/libraries/MY_Database.php b/modules/gallery/libraries/MY_Database.php index 61f23fb0..e2ef68cd 100644 --- a/modules/gallery/libraries/MY_Database.php +++ b/modules/gallery/libraries/MY_Database.php @@ -38,7 +38,7 @@ abstract class Database extends Database_Core { * Parse the query string and convert any strings of the form `\([a-zA-Z0-9_]*?)\] * table prefix . $1 */ - public function query($sql = '') { + public function query($sql) { if (!empty($sql)) { $sql = $this->add_table_prefixes($sql); } diff --git a/modules/gallery/libraries/MY_View.php b/modules/gallery/libraries/MY_View.php index cec59ec1..83e0d0be 100644 --- a/modules/gallery/libraries/MY_View.php +++ b/modules/gallery/libraries/MY_View.php @@ -27,7 +27,7 @@ class View extends View_Core { View::$global_data[$key] = $value; } - public function is_set($key) { + public function is_set($key=null) { return parent::is_set($key) ? true : array_key_exists($key, View::$global_data); } diff --git a/modules/gallery/libraries/ORM_MPTT.php b/modules/gallery/libraries/ORM_MPTT.php index 83f9b51e..3668d42d 100644 --- a/modules/gallery/libraries/ORM_MPTT.php +++ b/modules/gallery/libraries/ORM_MPTT.php @@ -85,7 +85,7 @@ class ORM_MPTT_Core extends ORM { /** * Delete this node and all of its children. */ - public function delete() { + public function delete($ignored_id=null) { $children = $this->children(); if ($children) { foreach ($this->children() as $item) { diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index 083fd06b..dbd56fa2 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -70,7 +70,7 @@ class Item_Model extends ORM_MPTT { return $this->type == 'movie'; } - public function delete() { + public function delete($ignored_id=null) { if ($this->id == 1) { $v = new Validation(array("id")); $v->add_error("id", "cant_delete_root_album"); diff --git a/modules/gallery/models/task.php b/modules/gallery/models/task.php index f40be492..24d909cb 100644 --- a/modules/gallery/models/task.php +++ b/modules/gallery/models/task.php @@ -27,7 +27,7 @@ class Task_Model extends ORM { } } - public function set($key, $value) { + public function set($key, $value=null) { $context = unserialize($this->context); $context[$key] = $value; $this->context = serialize($context); @@ -40,7 +40,7 @@ class Task_Model extends ORM { return parent::save(); } - public function delete() { + public function delete($ignored_id=null) { Cache::instance()->delete($this->_cache_key()); return parent::delete(); } diff --git a/modules/gallery/tests/Database_Test.php b/modules/gallery/tests/Database_Test.php index e58f73eb..861f7bba 100644 --- a/modules/gallery/tests/Database_Test.php +++ b/modules/gallery/tests/Database_Test.php @@ -168,12 +168,12 @@ class Database_Mock extends Database { return array("test"); } - public function quote_column($val) { - return "[$val]"; + public function quote_column($val, $alias=null) { + return $alias ? "[$val,$alias]" : "[$val]"; } - public function quote_table($val) { - return "[$val]"; + public function quote_table($val, $alias=null) { + return $alias ? "[$val,$alias]" : "[$val]"; } public function quote($val) { diff --git a/modules/gallery/tests/Item_Rest_Helper_Test.php b/modules/gallery/tests/Item_Rest_Helper_Test.php index 088b1cbd..6d1dd864 100644 --- a/modules/gallery/tests/Item_Rest_Helper_Test.php +++ b/modules/gallery/tests/Item_Rest_Helper_Test.php @@ -32,6 +32,7 @@ class Item_Rest_Helper_Test extends Gallery_Unit_Test_Case { $album1->reload(); // No scope is the same as "direct" + $request = new stdClass(); $request->url = rest::url("item", $album1); $request->params = new stdClass(); $this->assert_equal_array( @@ -84,7 +85,9 @@ class Item_Rest_Helper_Test extends Gallery_Unit_Test_Case { $photo2->save(); $album1->reload(); + $request = new stdClass(); $request->url = rest::url("item", $album1); + $request->params = new stdClass(); $request->params->name = "foo"; $this->assert_equal_array( array("url" => rest::url("item", $album1), @@ -104,7 +107,9 @@ class Item_Rest_Helper_Test extends Gallery_Unit_Test_Case { $album2 = test::random_album($album1); $album1->reload(); + $request = new stdClass(); $request->url = rest::url("item", $album1); + $request->params = new stdClass(); $request->params->type = "album"; $this->assert_equal_array( array("url" => rest::url("item", $album1), @@ -122,7 +127,9 @@ class Item_Rest_Helper_Test extends Gallery_Unit_Test_Case { $album1 = test::random_album(); access::allow(identity::everybody(), "edit", $album1); + $request = new stdClass(); $request->url = rest::url("item", $album1); + $request->params = new stdClass(); $request->params->title = "my new title"; item_rest::put($request); @@ -133,7 +140,9 @@ class Item_Rest_Helper_Test extends Gallery_Unit_Test_Case { $album1 = test::random_album(); access::allow(identity::everybody(), "edit", $album1); + $request = new stdClass(); $request->url = rest::url("item", $album1); + $request->params = new stdClass(); $request->params->title = "my new title"; $request->params->slug = "not url safe"; @@ -150,7 +159,9 @@ class Item_Rest_Helper_Test extends Gallery_Unit_Test_Case { $album1 = test::random_album(); access::allow(identity::everybody(), "edit", $album1); + $request = new stdClass(); $request->url = rest::url("item", $album1); + $request->params = new stdClass(); $request->params->type = "album"; $request->params->name = "my album"; $request->params->title = "my album"; @@ -165,7 +176,9 @@ class Item_Rest_Helper_Test extends Gallery_Unit_Test_Case { $album1 = test::random_album(); access::allow(identity::everybody(), "edit", $album1); + $request = new stdClass(); $request->url = rest::url("item", $album1); + $request->params = new stdClass(); $request->params->type = "album"; $request->params->name = "my album"; $request->params->title = "my album"; @@ -185,7 +198,9 @@ class Item_Rest_Helper_Test extends Gallery_Unit_Test_Case { $album1 = test::random_album(); access::allow(identity::everybody(), "edit", $album1); + $request = new stdClass(); $request->url = rest::url("item", $album1); + $request->params = new stdClass(); $request->params->type = "photo"; $request->params->name = "my photo.jpg"; $request->file = MODPATH . "gallery/tests/test.jpg"; @@ -200,6 +215,7 @@ class Item_Rest_Helper_Test extends Gallery_Unit_Test_Case { $album1 = test::random_album(); access::allow(identity::everybody(), "edit", $album1); + $request = new stdClass(); $request->url = rest::url("item", $album1); item_rest::delete($request); @@ -212,6 +228,7 @@ class Item_Rest_Helper_Test extends Gallery_Unit_Test_Case { access::deny(identity::everybody(), "edit", $album1); identity::set_active_user(identity::guest()); + $request = new stdClass(); $request->url = rest::url("item", $album1); try { item_rest::delete($request); diff --git a/modules/gallery_unit_test/controllers/gallery_unit_test.php b/modules/gallery_unit_test/controllers/gallery_unit_test.php index e05fcbaa..2690ad24 100644 --- a/modules/gallery_unit_test/controllers/gallery_unit_test.php +++ b/modules/gallery_unit_test/controllers/gallery_unit_test.php @@ -18,11 +18,15 @@ * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA. */ class Gallery_Unit_Test_Controller extends Controller { - function Index() { + function index() { if (!TEST_MODE) { throw new Kohana_404_Exception(); } + // Force strict behavior to flush out bugs early + ini_set("display_errors", true); + error_reporting(-1); + // Jump through some hoops to satisfy the way that we check for the site_domain in // config.php. We structure this such that the code in config will leave us with a // site_domain of "." (for historical reasons) diff --git a/modules/rest/controllers/rest.php b/modules/rest/controllers/rest.php index 9141d6d4..374ae0d2 100644 --- a/modules/rest/controllers/rest.php +++ b/modules/rest/controllers/rest.php @@ -40,6 +40,7 @@ class Rest_Controller extends Controller { public function __call($function, $args) { $input = Input::instance(); + $request = new stdClass(); switch ($method = strtolower($input->server("REQUEST_METHOD"))) { case "get": $request->params = (object) $input->get(); diff --git a/modules/rest/helpers/rest.php b/modules/rest/helpers/rest.php index b3f80a55..a61aba2f 100644 --- a/modules/rest/helpers/rest.php +++ b/modules/rest/helpers/rest.php @@ -19,7 +19,7 @@ */ class rest_Core { static function reply($data=array()) { - Session::abort_save(); + Session::instance()->abort_save(); if ($data) { if (Input::instance()->get("output") == "html") { diff --git a/modules/rest/tests/Rest_Controller_Test.php b/modules/rest/tests/Rest_Controller_Test.php index 5e624112..9f73bed9 100644 --- a/modules/rest/tests/Rest_Controller_Test.php +++ b/modules/rest/tests/Rest_Controller_Test.php @@ -138,8 +138,8 @@ class Rest_Controller_Test extends Gallery_Unit_Test_Case { } class mock_rest { - function get($request) { return $request; } - function post($request) { return $request; } - function put($request) { return $request; } - function delete($request) { return $request; } + static function get($request) { return $request; } + static function post($request) { return $request; } + static function put($request) { return $request; } + static function delete($request) { return $request; } } \ No newline at end of file diff --git a/modules/search/helpers/search.php b/modules/search/helpers/search.php index b2497eae..9018ffa2 100644 --- a/modules/search/helpers/search.php +++ b/modules/search/helpers/search.php @@ -65,7 +65,8 @@ class search_Core { $record->item_id = $item->id; } - module::event("item_index_data", $record->item(), $data); + $item = $record->item(); + module::event("item_index_data", $item, $data); $record->data = join(" ", (array)$data); $record->dirty = 0; $record->save(); diff --git a/modules/tag/helpers/tag_rss.php b/modules/tag/helpers/tag_rss.php index f09a4530..5d42caab 100644 --- a/modules/tag/helpers/tag_rss.php +++ b/modules/tag/helpers/tag_rss.php @@ -34,6 +34,8 @@ class tag_rss_Core { if (!$tag->loaded()) { throw new Kohana_404_Exception(); } + + $feed = new stdClass(); $feed->children = $tag->items($limit, $offset, "photo"); $feed->max_pages = ceil($tag->count / $limit); $feed->title = $tag->name; diff --git a/modules/tag/helpers/tags_rest.php b/modules/tag/helpers/tags_rest.php index ac0eb81d..f28be7b5 100644 --- a/modules/tag/helpers/tags_rest.php +++ b/modules/tag/helpers/tags_rest.php @@ -35,7 +35,6 @@ class tags_rest_Core { $query->or_where("edit_{$group->id}", "=", access::ALLOW); } $has_any_edit_perm = $query->close()->count_records(); - if (!$has_any_edit_perm) { access::forbidden(); } diff --git a/modules/tag/models/tag.php b/modules/tag/models/tag.php index 2b33c30d..38a8ed69 100644 --- a/modules/tag/models/tag.php +++ b/modules/tag/models/tag.php @@ -95,7 +95,7 @@ class Tag_Model extends ORM { * Overload ORM::delete() to trigger an item_related_update event for all items that are * related to this tag. */ - public function delete() { + public function delete($ignored_id=null) { $related_item_ids = array(); foreach (db::build() ->select("item_id") diff --git a/modules/tag/tests/Tag_Item_Rest_Helper_Test.php b/modules/tag/tests/Tag_Item_Rest_Helper_Test.php index 69c580f1..cb368790 100644 --- a/modules/tag/tests/Tag_Item_Rest_Helper_Test.php +++ b/modules/tag/tests/Tag_Item_Rest_Helper_Test.php @@ -28,6 +28,7 @@ class Tag_Item_Rest_Helper_Test extends Gallery_Unit_Test_Case { public function get_test() { $tag = tag::add(item::root(), "tag1")->reload(); + $request = new stdClass(); $request->url = rest::url("tag_item", $tag, item::root()); $this->assert_equal_array( array("url" => rest::url("tag_item", $tag, item::root()), @@ -38,6 +39,7 @@ class Tag_Item_Rest_Helper_Test extends Gallery_Unit_Test_Case { } public function get_with_invalid_url_test() { + $request = new stdClass(); $request->url = "bogus"; try { tag_item_rest::get($request); @@ -50,6 +52,7 @@ class Tag_Item_Rest_Helper_Test extends Gallery_Unit_Test_Case { public function delete_test() { $tag = tag::add(item::root(), "tag1")->reload(); + $request = new stdClass(); $request->url = rest::url("tag_item", $tag, item::root()); tag_item_rest::delete($request); @@ -60,7 +63,6 @@ class Tag_Item_Rest_Helper_Test extends Gallery_Unit_Test_Case { $album = test::random_album(); $tag = tag::add($album, "tag1")->reload(); - $tuple = rest::resolve(rest::url("tag_item", $tag, $album)); $this->assert_equal_array($tag->as_array(), $tuple[0]->as_array()); $this->assert_equal_array($album->as_array(), $tuple[1]->as_array()); diff --git a/modules/tag/tests/Tag_Rest_Helper_Test.php b/modules/tag/tests/Tag_Rest_Helper_Test.php index d3cae0fb..838de975 100644 --- a/modules/tag/tests/Tag_Rest_Helper_Test.php +++ b/modules/tag/tests/Tag_Rest_Helper_Test.php @@ -28,6 +28,7 @@ class Tag_Rest_Helper_Test extends Gallery_Unit_Test_Case { public function get_test() { $tag = tag::add(item::root(), "tag1")->reload(); + $request = new stdClass(); $request->url = rest::url("tag", $tag); $this->assert_equal_array( array("url" => rest::url("tag", $tag), @@ -41,6 +42,7 @@ class Tag_Rest_Helper_Test extends Gallery_Unit_Test_Case { } public function get_with_invalid_url_test() { + $request = new stdClass(); $request->url = "bogus"; try { tag_rest::get($request); @@ -53,6 +55,7 @@ class Tag_Rest_Helper_Test extends Gallery_Unit_Test_Case { public function get_with_no_relationships_test() { $tag = test::random_tag(); + $request = new stdClass(); $request->url = rest::url("tag", $tag); $this->assert_equal_array( array("url" => rest::url("tag", $tag), @@ -72,7 +75,9 @@ class Tag_Rest_Helper_Test extends Gallery_Unit_Test_Case { access::allow(identity::everybody(), "edit", $album); // Add the album to the tag + $request = new stdClass(); $request->url = rest::url("tag", $tag); + $request->params = new stdClass(); $request->params->url = rest::url("item", $album); $this->assert_equal_array( array("url" => rest::url("tag_item", $tag, $album)), @@ -93,7 +98,9 @@ class Tag_Rest_Helper_Test extends Gallery_Unit_Test_Case { public function put_test() { $tag = test::random_tag(); + $request = new stdClass(); $request->url = rest::url("tag", $tag); + $request->params = new stdClass(); $request->params->name = "new name"; tag_rest::put($request); @@ -102,6 +109,7 @@ class Tag_Rest_Helper_Test extends Gallery_Unit_Test_Case { public function delete_tag_test() { $tag = test::random_tag(); + $request = new stdClass(); $request->url = rest::url("tag", $tag); tag_rest::delete($request); diff --git a/modules/tag/tests/Tags_Rest_Helper_Test.php b/modules/tag/tests/Tags_Rest_Helper_Test.php index a1713811..cdf7bfdf 100644 --- a/modules/tag/tests/Tags_Rest_Helper_Test.php +++ b/modules/tag/tests/Tags_Rest_Helper_Test.php @@ -43,6 +43,8 @@ class Tags_Rest_Helper_Test extends Gallery_Unit_Test_Case { public function post_test() { access::allow(identity::everybody(), "edit", item::root()); + $request = new stdClass(); + $request->params = new stdClass(); $request->params->name = "test tag"; $this->assert_equal( array("url" => url::site("rest/tag/1")), @@ -55,6 +57,8 @@ class Tags_Rest_Helper_Test extends Gallery_Unit_Test_Case { identity::set_active_user(identity::guest()); try { + $request = new stdClass(); + $request->params = new stdClass(); $request->params->name = "test tag"; tags_rest::post($request); } catch (Exception $e) { diff --git a/modules/user/controllers/admin_users.php b/modules/user/controllers/admin_users.php index c11b0596..03d9858b 100644 --- a/modules/user/controllers/admin_users.php +++ b/modules/user/controllers/admin_users.php @@ -323,7 +323,7 @@ class Admin_Users_Controller extends Admin_Controller { return $form; } - private function _add_locale_dropdown(&$form, $user=null) { + private static function _add_locale_dropdown(&$form, $user=null) { $locales = locales::installed(); foreach ($locales as $locale => $display_name) { $locales[$locale] = SafeString::of_safe_html($display_name); -- cgit v1.2.3