From 0e3327bca70623175791ee41085d55d0cb13fe5b Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Sun, 3 Jan 2010 20:30:35 -0800 Subject: Simplify the REST API code. Here's what I did: 1) Simplify gallery_rest to return flat models, no children and do no validation for now. 2) Flatten the REST replies and use HTTP codes to indicate success/failure instead of additional status messages. 3) Use the message and error code support in the base Exception class, instead of brewing our own in Rest_Exception. 4) Get rid of rest::success() and rest::fail() -- we only need rest::reply() since all failures are covered by throwing an exception. 5) Get rid of /rest/access_key and just use /rest for authentication. 6) Inline and simplify rest::normalize_request since we only use it once 7) Change rest::set_active_user to succeed or throw an exception 8) Extract Rest_Exception::sendHeaders into rest::send_headers() Here's what's currently broken: 1) Data validation. There currently is none 2) Logging. That's gone too 3) image block and tag code is broken 4) Tests are broken 5) No movie support --- modules/gallery/helpers/gallery_rest.php | 263 +++++---------------- modules/gallery/tests/Gallery_Rest_Helper_Test.php | 3 +- modules/image_block/helpers/image_block_rest.php | 4 +- modules/rest/controllers/rest.php | 60 +++-- modules/rest/helpers/rest.php | 90 ++----- modules/rest/libraries/Rest_Exception.php | 17 +- modules/rest/tests/Rest_Controller_Test.php | 20 +- modules/tag/helpers/tag_rest.php | 14 +- modules/tag/tests/Tag_Rest_Helper_Test.php | 12 +- 9 files changed, 146 insertions(+), 337 deletions(-) diff --git a/modules/gallery/helpers/gallery_rest.php b/modules/gallery/helpers/gallery_rest.php index a87ebb4e..f1c8d825 100644 --- a/modules/gallery/helpers/gallery_rest.php +++ b/modules/gallery/helpers/gallery_rest.php @@ -17,232 +17,83 @@ * along with this program; if not, write to the Free Software * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA. */ -class gallery_rest_Core { - static function get($request) { - $path = implode("/", $request->arguments); - $item = gallery_rest::_get_item($path); +// @todo Add logging +// @todo VALIDATION + +// Validation questions +// +// We need to be able to properly validate anything we want to enter here. But all of our +// validation currently happens at the controller / form level, and we're not using the same +// controllers or forms. +// +// Possible solutions: +// 1) Move validation into the model and use it both here and in the regular controllers. But +// if we do that, how do we translate validation failures into a user-consumable output which +// we need so that we can return proper error responses to form submissions? +// +// 2) Create some kind of validation helper that can validate every field. Wait, isn't this +// just like #1 except in a helper instead of in the model? - $parent = $item->parent(); - $response_data = array("type" => $item->type, - "name" => $item->name, - "path" => $item->relative_url(), - "parent_path" => empty($parent) ? null : $parent->relative_url(), - "title" => $item->title, - "thumb_url" => $item->thumb_url(true), - "thumb_size" => array("height" => $item->thumb_height, - "width" => $item->thumb_width), - "resize_url" => $item->resize_url(true), - "resize_size" => array("height" => (int)$item->resize_height, - "width" => (int)$item->resize_width), - "url" => $item->file_url(true), - "size" => array("height" => $item->height, - "width" => $item->width), - "description" => $item->description, - "slug" => $item->slug); +class gallery_rest_Core { + static function get($request) { + $item = url::get_item_from_uri($request->path); + access::required("view", $item); - $children = self::_get_children($item, $request); - if (!empty($children) || $item->is_album()) { - $response_data["children"] = $children; - } - return rest::success(array("resource" => $response_data)); + return json_encode($item->as_array()); } static function put($request) { - if (empty($request->arguments)) { - throw new Rest_Exception(400, "Bad request"); - } - $path = implode("/", $request->arguments); - $item = gallery_rest::_get_item($path, "edit"); - - // Validate the request data - $new_values = gallery_rest::_validate($request, $item->parent_id, $item->id); - $errors = $new_values->errors(); - if (empty($errors)) { - $item->title = $new_values->title; - $item->description = $new_values->description; - if ($item->id != 1) { - $item->rename($new_values->name); + $item = url::get_item_from_uri($request->path); + access::required("edit", $item); + + $params = $request->params; + foreach (array("captured", "description", "slug", "sort_column", "sort_order", + "title", "view_count", "weight") as $key) { + if (isset($params->$key)) { + $item->$key = $params->$key; } - $item->slug = $new_values->slug; - $item->save(); - - log::success("content", "Updated $item->type", - "type}s/$item->id\">view"); - - return rest::success(); - } else { - return rest::validation_error($errors); } + $item->save(); + + return rest::reply(array("url" => url::abs_site("/rest/gallery/" . $item->relative_url()))); } static function post($request) { - if (empty($request->arguments)) { - throw new Rest_Exception(400, "Bad request"); - } + $parent = url::get_item_from_uri($request->path); + access::required("edit", $parent); - $components = $request->arguments; - $name = urldecode(array_pop($components)); - - $parent = gallery_rest::_get_item(implode("/", $components), "edit"); - - // Validate the request data - $request->name = $name; - $new_values = gallery_rest::_validate($request, $parent->id); - $errors = $new_values->errors(); - if (!empty($errors)) { - return rest::validation_error($errors); - } + $params = $request->params; + switch ($params->type) { + case "album": + $item = album::create( + $parent, + $params->name, + isset($params->title) ? $params->title : $name, + isset($params->description) ? $params->description : null); + break; - if (empty($new_values["image"])) { - $new_item = album::create( + case "photo": + $item = photo::create( $parent, - $name, - empty($new_values["title"]) ? $name : $new_values["title"], - empty($new_values["description"]) ? null : $new_values["description"], - identity::active_user()->id, - empty($new_values["slug"]) ? $name : $new_values["slug"]); - $log_message = t("Added an album"); - } else { - $temp_filename = upload::save("image"); - $path_info = @pathinfo($temp_filename); - if (array_key_exists("extension", $path_info) && - in_array(strtolower($path_info["extension"]), array("flv", "mp4"))) { - $new_item = - movie::create($parent, $temp_filename, $new_values["name"], $new_values["title"]); - $log_message = t("Added a movie"); - } else { - $new_item = - photo::create($parent, $temp_filename, $new_values["name"], $new_values["title"]); - $log_message = t("Added a photo"); - } - } + $request->file, + $params->name, + isset($params->title) ? $params->title : $name, + isset($params->description) ? $params->description : null); + break; - log::success("content", $log_message, "type}s/$new_item->id\">view"); + default: + throw new Rest_Exception("Invalid type: $args->type", 400); + } - return rest::success(array("path" => $new_item->relative_url())); + return rest::reply(array("url" => url::abs_site("/rest/gallery/" . $item->relative_url()))); } static function delete($request) { - if (empty($request->arguments)) { - throw new Rest_Exception(400, "Bad request"); - } - $path = implode("/", $request->arguments); - - $item = gallery_rest::_get_item($path, "edit"); - - if ($item->id == 1) { - throw new Rest_Exception(400, "Bad request"); - } + $item = url::get_item_from_uri($request->path); + access::required("edit", $item); - $parent = $item->parent(); $item->delete(); - - if ($item->is_album()) { - $msg = t("Deleted album %title", array("title" => html::purify($item->title))); - } else { - $msg = t("Deleted photo %title", array("title" => html::purify($item->title))); - } - log::success("content", $msg); - - return rest::success(array("resource" => array("parent_path" => $parent->relative_url()))); - } - - private static function _get_item($path, $permission="view") { - $item = url::get_item_from_uri($path); - - if (!$item->loaded()) { - throw new Kohana_404_Exception(); - } - - if (!access::can($permission, $item)) { - throw new Kohana_404_Exception(); - } - - return $item; - } - - private static function _get_children($item, $request) { - $children = array(); - $limit = empty($request->limit) ? null : $request->limit; - $offset = empty($request->offset) ? null : $request->offset; - $where = empty($request->filter) ? array() : array("type" => $request->filter); - foreach ($item->viewable()->children($limit, $offset, $where) as $child) { - $children[] = array("type" => $child->type, - "has_children" => $child->children_count() > 0, - "path" => $child->relative_url(), - "thumb_url" => $child->thumb_url(true), - "thumb_dimensions" => array("width" => $child->thumb_width, - "height" => $child->thumb_height), - "has_thumb" => $child->has_thumb(), - "title" => $child->title); - } - - return $children; - } - - private static function _validate($request, $parent_id, $item_id=0) { - $item = ORM::factory("item", $item_id); - - // Normalize the inputs so all fields have a value - $new_values = Validation::factory(array()); - foreach ($item->form_rules as $field => $rule_set) { - if (isset($request->$field)) { - $new_values[$field] = $request->$field; - } else if (isset($item->$field)) { - $new_values[$field] = $item->$field; - } - foreach (explode("|", $rule_set) as $rule) { - $new_values->add_rules($field, $rule); - } - } - $name = $new_values["name"]; - $new_values["title"] = empty($new_values["title"]) ? $name : $new_values["title"]; - $new_values["description"] = - empty($new_values["description"]) ? null : $new_values["description"]; - $new_values["slug"] = empty($new_values["slug"]) ? $name : $new_values["slug"]; - - if (!empty($request->image)) { - $new_values["image"] = $request->image; - $new_values->add_rules( - "image", "upload::valid", "upload::required", "upload::type[gif,jpg,jpeg,png,flv,mp4]"); - } - - if ($new_values->validate() && $item_id != 1) { - $errors = gallery_rest::_check_for_conflicts($parent_id, $item_id, - $new_values["name"], $new_values["slug"]); - if (!empty($errors)) { - !empty($errors["name_conflict"]) OR $new_values->add_error("name", "Duplicate name"); - !empty($errors["slug_conflict"]) OR - $new_values->add_error("slug", "Duplicate Internet address"); - } - } - - return $new_values; - } - - private static function _check_for_conflicts($parent_id, $item_id, $new_name, $new_slug) { - $errors = array(); - - if ($row = db::build() - ->select(array("name", "slug")) - ->from("items") - ->where("parent_id", "=", $parent_id) - ->where("id", "<>", $item_id) - ->and_open() - ->where("name", "=", $new_name) - ->or_where("slug", "=", $new_slug) - ->close() - ->execute() - ->current()) { - if ($row->name == $new_name) { - $errors["name_conflict"] = 1; - } - if ($row->slug == $new_slug) { - $errors["slug_conflict"] = 1; - } - } - - return $errors; + return rest::reply(); } } diff --git a/modules/gallery/tests/Gallery_Rest_Helper_Test.php b/modules/gallery/tests/Gallery_Rest_Helper_Test.php index cd0aabae..c5c8a890 100644 --- a/modules/gallery/tests/Gallery_Rest_Helper_Test.php +++ b/modules/gallery/tests/Gallery_Rest_Helper_Test.php @@ -136,7 +136,8 @@ class Gallery_Rest_Helper_Test extends Unit_Test_Case { try { gallery_rest::put($request); } catch (Rest_Exception $e) { - $this->assert_equal("400 Bad request", $e->getMessage()); + $this->assert_equal("Bad request", $e->getMessage()); + $this->assert_equal(400, $e->getCode()); } catch (Exception $e) { $this->assert_false(true, $e->__toString()); } diff --git a/modules/image_block/helpers/image_block_rest.php b/modules/image_block/helpers/image_block_rest.php index 7afd974c..65eefb21 100644 --- a/modules/image_block/helpers/image_block_rest.php +++ b/modules/image_block/helpers/image_block_rest.php @@ -54,9 +54,9 @@ class image_block_rest_Core { "thumb_size" => array("height" => $item->thumb_height, "width" => $item->thumb_width)); - return rest::success(array("resource" => $response_data)); + return rest::reply(array("resource" => $response_data)); } else { - return rest::fail("No Image found"); + return rest::reply(); } } } diff --git a/modules/rest/controllers/rest.php b/modules/rest/controllers/rest.php index 26e5b31a..0332e5fc 100644 --- a/modules/rest/controllers/rest.php +++ b/modules/rest/controllers/rest.php @@ -18,20 +18,14 @@ * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA. */ class Rest_Controller extends Controller { - public function access_key() { + public function index() { try { - $request = (object)Input::instance()->get(); - if (empty($request->user) || empty($request->password)) { - throw new Rest_Exception(403, "Forbidden"); - } - - $user = identity::lookup_user_by_name($request->user); - if (empty($user)) { - throw new Rest_Exception(403, "Forbidden"); - } + $username = Input::instance()->post("user"); + $password = Input::instance()->post("password"); - if (!identity::is_correct_password($user, $request->password)) { - throw new Rest_Exception(403, "Forbidden"); + $user = identity::lookup_user_by_name($username); + if (empty($user) || !identity::is_correct_password($user, $password)) { + throw new Rest_Exception("Forbidden", 403); } $key = ORM::factory("user_access_token") @@ -42,27 +36,45 @@ class Rest_Controller extends Controller { $key->access_key = md5($user->name . rand()); $key->save(); } - print rest::success(array("token" => $key->access_key)); - } catch (Rest_Exception $e) { - $e->sendHeaders(); + + rest::reply($key->access_key); + } catch (Exception $e) { + rest::send_headers($e); } } public function __call($function, $args) { - $request = rest::normalize_request($args); + $input = Input::instance(); + switch ($method = strtolower($input->server("REQUEST_METHOD"))) { + case "get": + $request->params = (object) Input::instance()->get(); + break; + + case "post": + $request->params = (object) Input::instance()->post(); + if (isset($_FILES["file"])) { + $request->file = upload::save("file"); + } + break; + } + + $request->method = strtolower($input->server("HTTP_X_GALLERY_REQUEST_METHOD", $method)); + $request->access_token = $input->server("HTTP_X_GALLERY_REQUEST_KEY"); + $request->path = implode("/", $args); + try { - if (rest::set_active_user($request->access_token)) { - $handler_class = "{$function}_rest"; - $handler_method = $request->method; + rest::set_active_user($request->access_token); - if (!method_exists($handler_class, $handler_method)) { - throw new Rest_Exception(403, "Forbidden"); - } + $handler_class = "{$function}_rest"; + $handler_method = $request->method; - print call_user_func(array($handler_class, $handler_method), $request); + if (!method_exists($handler_class, $handler_method)) { + throw new Rest_Exception("Forbidden", 403); } + + print call_user_func(array($handler_class, $handler_method), $request); } catch (Rest_Exception $e) { - $e->sendHeaders(); + rest::send_headers($e); } } } \ No newline at end of file diff --git a/modules/rest/helpers/rest.php b/modules/rest/helpers/rest.php index be0644f2..f7f3f9fd 100644 --- a/modules/rest/helpers/rest.php +++ b/modules/rest/helpers/rest.php @@ -18,87 +18,37 @@ * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA. */ class rest_Core { - /** - * Request failed - */ - static function fail($log_message=null) { - if (!empty($log_message)) { - Kohana_Log::add("info", $log_message); - } - // We don't need to save the session for this request + static function reply($data=array()) { Session::abort_save(); - return json_encode(array("status" => "ERROR", "message" => (string)$message)); - } - /** - * Success - */ - static function success($response_data=array(), $message=null) { - $response = array("status" => "OK"); - if (!empty($message)) { - $response["message"] = (string)$message; + if ($data) { + print json_encode($data); } - $response = array_merge($response, $response_data); - - // We don't need to save the session for this request - Session::abort_save(); - return json_encode($response); } - /** - * Validation Error - */ - static function validation_error($error_data) { - $response = array("status" => "VALIDATE_ERROR"); - $response = array_merge($response, array("fields" => $error_data)); - - // We don't need to save the session for this request - Session::abort_save(); - return json_encode($response); - } + static function set_active_user($access_token) { + if (empty($access_token)) { + identity::set_active_user(identity::guest()); + return; + } + $key = ORM::factory("user_access_token") + ->where("access_key", "=", $access_token) + ->find(); - static function normalize_request($args=array()) { - $input = Input::instance(); - $method = strtolower($input->server("REQUEST_METHOD")); - $request = new stdClass(); - foreach (array_keys($input->get()) as $key) { - $request->$key = $input->get($key); - } - if ($method != "get") { - foreach (array_keys($input->post()) as $key) { - $request->$key = $input->post($key); - } - foreach (array_keys($_FILES) as $key) { - $request->$key = $_FILES[$key]; - } + if (!$key->loaded()) { + throw new Rest_Exception("Forbidden", 403); } - $request->method = strtolower($input->server("HTTP_X_GALLERY_REQUEST_METHOD", $method)); - $request->access_token = $input->server("HTTP_X_GALLERY_REQUEST_KEY"); - $request->arguments = $args; // Let the rest handler figure out what the arguments mean + $user = identity::lookup_user($key->user_id); + if (empty($user)) { + throw new Rest_Exception("Forbidden", 403); + } - return $request; + identity::set_active_user($user); } - static function set_active_user($access_token) { - if (empty($access_token)) { - $user = identity::guest(); - } else { - $key = ORM::factory("user_access_token") - ->where("access_key", "=", $access_token) - ->find(); - - if ($key->loaded()) { - $user = identity::lookup_user($key->user_id); - if (empty($user)) { - throw new Rest_Exception(403, "Forbidden"); - } - } else { - throw new Rest_Exception(403, "Forbidden"); - } - } - identity::set_active_user($user); - return true; + static function send_headers($exception) { + header("HTTP/1.1 " . $exception->getCode() . " " . $exception->getMessage()); } } diff --git a/modules/rest/libraries/Rest_Exception.php b/modules/rest/libraries/Rest_Exception.php index 905b94a0..596b3712 100644 --- a/modules/rest/libraries/Rest_Exception.php +++ b/modules/rest/libraries/Rest_Exception.php @@ -18,19 +18,4 @@ * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA. */ class Rest_Exception_Core extends Exception { - /** - * Set internal properties. - */ - public function __construct($code, $text) { - parent::__construct("$code $text"); - } - - /** - * Sends the headers, to emulate server behavior. - * - * @return void - */ - public function sendHeaders() { - header('HTTP/1.1 {$this->getMessage()}'); - } -} // End Rest Exception \ No newline at end of file +} \ No newline at end of file diff --git a/modules/rest/tests/Rest_Controller_Test.php b/modules/rest/tests/Rest_Controller_Test.php index 83bd9db6..c881583c 100644 --- a/modules/rest/tests/Rest_Controller_Test.php +++ b/modules/rest/tests/Rest_Controller_Test.php @@ -84,7 +84,8 @@ class Rest_Controller_Test extends Unit_Test_Case { try { $this->_call_controller(); } catch (Rest_Exception $e) { - $this->assert_equal("403 Forbidden", $e->getMessage()); + $this->assert_equal(403, $e->getCode()); + $this->assert_equal("Forbidden", $e->getMessage()); } catch (Exception $e) { $this->assert_false(true, $e->__toString()); } @@ -97,7 +98,8 @@ class Rest_Controller_Test extends Unit_Test_Case { try { $this->_call_controller(); } catch (Rest_Exception $e) { - $this->assert_equal("403 Forbidden", $e->getMessage()); + $this->assert_equal(403, $e->getCode()); + $this->assert_equal("Forbidden", $e->getMessage()); } catch (Exception $e) { $this->assert_false(true, $e->__toString()); } @@ -109,7 +111,8 @@ class Rest_Controller_Test extends Unit_Test_Case { try { $this->_call_controller(); } catch (Rest_Exception $e) { - $this->assert_equal("403 Forbidden", $e->getMessage()); + $this->assert_equal(403, $e->getCode()); + $this->assert_equal("Forbidden", $e->getMessage()); } catch (Exception $e) { $this->assert_false(true, $e->__toString()); } @@ -137,7 +140,8 @@ class Rest_Controller_Test extends Unit_Test_Case { try { $this->_call_controller(); } catch (Rest_Exception $e) { - $this->assert_equal("403 Forbidden", $e->getMessage()); + $this->assert_equal(403, $e->getCode()); + $this->assert_equal("Forbidden", $e->getMessage()); } catch (Exception $e) { $this->assert_false(true, $e->__toString()); } @@ -155,7 +159,8 @@ class Rest_Controller_Test extends Unit_Test_Case { try { $this->_call_controller("rest", explode("/", $photo->relative_url())); } catch (Rest_Exception $e) { - $this->assert_equal("403 Forbidden", $e->getMessage()); + $this->assert_equal(403, $e->getCode()); + $this->assert_equal("Forbidden", $e->getMessage()); } catch (Exception $e) { $this->assert_false(true, $e->__toString()); } @@ -171,7 +176,8 @@ class Rest_Controller_Test extends Unit_Test_Case { try { $this->_call_controller("rest", explode("/", $photo->relative_url())); } catch (Rest_Exception $e) { - $this->assert_equal("501 Not Implemented", $e->getMessage()); + $this->assert_equal(501, $e->getCode()); + $this->assert_equal("Not Implemented", $e->getMessage()); } catch (Exception $e) { $this->assert_false(true, $e->__toString()); } @@ -218,7 +224,7 @@ class rest_rest { $response["thumb_url"] = $item->thumb_url(); $response["description"] = $item->description; $response["internet_address"] = $item->slug; - return rest::success(array($item->type => $response), t("Processed")); + return rest::reply(array($item->type => $response)); } } diff --git a/modules/tag/helpers/tag_rest.php b/modules/tag/helpers/tag_rest.php index cd1ca6c6..0c06587b 100644 --- a/modules/tag/helpers/tag_rest.php +++ b/modules/tag/helpers/tag_rest.php @@ -55,12 +55,12 @@ class tag_rest_Core { } } - return rest::success($resources); + return rest::reply($resources); } static function post($request) { if (empty($request->arguments) || count($request->arguments) != 1 || empty($request->path)) { - throw new Rest_Exception(400, "Bad request"); + throw new Rest_Exception("Bad request", 400); } $path = $request->path; $tags = explode(",", $request->arguments[0]); @@ -80,12 +80,12 @@ class tag_rest_Core { foreach ($tags as $tag) { tag::add($item, $tag); } - return rest::success(); + return rest::reply(); } static function put($request) { if (empty($request->arguments[0]) || empty($request->new_name)) { - throw new Rest_Exception(400, "Bad request"); + throw new Rest_Exception("Bad request", 400); } $name = $request->arguments[0]; @@ -100,12 +100,12 @@ class tag_rest_Core { $tag->name = $request->new_name; $tag->save(); - return rest::success(); + return rest::reply(); } static function delete($request) { if (empty($request->arguments[0])) { - throw new Rest_Exception(400, "Bad request"); + throw new Rest_Exception("Bad request", 400); } $tags = explode(",", $request->arguments[0]); if (!empty($request->path)) { @@ -127,7 +127,7 @@ class tag_rest_Core { }; tag::compact(); - return rest::success(); + return rest::reply(); } private static function _get_items($request) { diff --git a/modules/tag/tests/Tag_Rest_Helper_Test.php b/modules/tag/tests/Tag_Rest_Helper_Test.php index 4e8dd527..185953ab 100644 --- a/modules/tag/tests/Tag_Rest_Helper_Test.php +++ b/modules/tag/tests/Tag_Rest_Helper_Test.php @@ -133,7 +133,8 @@ class Tag_Rest_Helper_Test extends Unit_Test_Case { try { tag_rest::post($request); } catch (Rest_Exception $e) { - $this->assert_equal("400 Bad request", $e->getMessage()); + $this->assert_equal(400, $e->getCode()); + $this->assert_equal("Bad request", $e->getMessage()); } catch (Exception $e) { $this->assert_false(true, $e->__toString()); } @@ -191,7 +192,8 @@ class Tag_Rest_Helper_Test extends Unit_Test_Case { try { tag_rest::put($request); } catch (Rest_Exception $e) { - $this->assert_equal("400 Bad request", $e->getMessage()); + $this->assert_equal(400, $e->getCode()); + $this->assert_equal("Bad request", $e->getMessage()); } catch (Exception $e) { $this->assert_false(true, $e->__toString()); } @@ -202,7 +204,8 @@ class Tag_Rest_Helper_Test extends Unit_Test_Case { try { tag_rest::put($request); } catch (Rest_Exception $e) { - $this->assert_equal("400 Bad request", $e->getMessage()); + $this->assert_equal(400, $e->getCode()); + $this->assert_equal("Bad request", $e->getMessage()); } catch (Exception $e) { $this->assert_false(true, $e->__toString()); } @@ -211,7 +214,8 @@ class Tag_Rest_Helper_Test extends Unit_Test_Case { try { tag_rest::put($request); } catch (Rest_Exception $e) { - $this->assert_equal("400 Bad request", $e->getMessage()); + $this->assert_equal(400, $e->getCode()); + $this->assert_equal("Bad request", $e->getMessage()); } catch (Exception $e) { $this->assert_false(true, $e->__toString()); } -- cgit v1.2.3 From 3fffa18e650189e7f846592c9d4c3e7bbfe71c62 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 4 Jan 2010 21:48:21 -0800 Subject: Further progress on refining the REST server side code. 1) Deal in fully qualified URL resources through the rest interface. All rest methods are now passed the complete url in request->url. 2) Create rest::resolve() which lets individual resource definition code convert a full url into the appropriate matching resource. Implement gallery_rest::resolve() and tag_rest::resolve() 3) Reimplement tag_rest's get() and post() methods. They're much simpler now. 4) Implement the tags_rest helper which supports working with the entire tags collection. --- modules/gallery/helpers/gallery_rest.php | 14 ++++--- modules/rest/controllers/rest.php | 2 +- modules/rest/helpers/rest.php | 20 +++++++++ modules/tag/helpers/tag.php | 2 +- modules/tag/helpers/tag_rest.php | 69 ++++++++------------------------ modules/tag/helpers/tags_rest.php | 48 ++++++++++++++++++++++ 6 files changed, 96 insertions(+), 59 deletions(-) create mode 100644 modules/tag/helpers/tags_rest.php diff --git a/modules/gallery/helpers/gallery_rest.php b/modules/gallery/helpers/gallery_rest.php index f1c8d825..858721d0 100644 --- a/modules/gallery/helpers/gallery_rest.php +++ b/modules/gallery/helpers/gallery_rest.php @@ -37,14 +37,14 @@ class gallery_rest_Core { static function get($request) { - $item = url::get_item_from_uri($request->path); + $item = rest::resolve($request->url); access::required("view", $item); - return json_encode($item->as_array()); + return rest::reply($item->as_array()); } static function put($request) { - $item = url::get_item_from_uri($request->path); + $item = rest::resolve($request->url); access::required("edit", $item); $params = $request->params; @@ -60,7 +60,7 @@ class gallery_rest_Core { } static function post($request) { - $parent = url::get_item_from_uri($request->path); + $parent = rest::resolve($request->url); access::required("edit", $parent); $params = $request->params; @@ -90,10 +90,14 @@ class gallery_rest_Core { } static function delete($request) { - $item = url::get_item_from_uri($request->path); + $item = rest::resolve($request->url); access::required("edit", $item); $item->delete(); return rest::reply(); } + + static function resolve($path) { + return url::get_item_from_uri($path); + } } diff --git a/modules/rest/controllers/rest.php b/modules/rest/controllers/rest.php index 0332e5fc..5ef9eb84 100644 --- a/modules/rest/controllers/rest.php +++ b/modules/rest/controllers/rest.php @@ -60,7 +60,7 @@ class Rest_Controller extends Controller { $request->method = strtolower($input->server("HTTP_X_GALLERY_REQUEST_METHOD", $method)); $request->access_token = $input->server("HTTP_X_GALLERY_REQUEST_KEY"); - $request->path = implode("/", $args); + $request->url = url::abs_current(true); try { rest::set_active_user($request->access_token); diff --git a/modules/rest/helpers/rest.php b/modules/rest/helpers/rest.php index f7f3f9fd..b1b83e1b 100644 --- a/modules/rest/helpers/rest.php +++ b/modules/rest/helpers/rest.php @@ -51,4 +51,24 @@ class rest_Core { static function send_headers($exception) { header("HTTP/1.1 " . $exception->getCode() . " " . $exception->getMessage()); } + + /** + * Convert a REST url into an object. + * Eg: "http://example.com/gallery3/index.php/rest/gallery/Family/Wedding" -> Item_Model + * + * @param string the fully qualified REST url + * @return mixed the corresponding object (usually a model of some kind) + */ + static function resolve($url) { + $components = explode("/", substr($url, strlen(url::abs_site("rest"))), 3); + + // The first component will be empty because of the slash between "rest" and the + // resource type. + $class = "$components[1]_rest"; + if (!method_exists($class, "resolve")) { + throw new Kohana_404_Exception($url); + } + + return call_user_func(array($class, "resolve"), !empty($components[2]) ? $components[2] : null); + } } diff --git a/modules/tag/helpers/tag.php b/modules/tag/helpers/tag.php index 8075afe4..d895e08f 100644 --- a/modules/tag/helpers/tag.php +++ b/modules/tag/helpers/tag.php @@ -41,7 +41,7 @@ class tag_Core { } if (!$tag->has($item)) { - if (!$tag->add($item, $tag)) { + if (!$tag->add($item)) { throw new Exception("@todo {$tag->name} WAS_NOT_ADDED_TO {$item->id}"); } $tag->count++; diff --git a/modules/tag/helpers/tag_rest.php b/modules/tag/helpers/tag_rest.php index 0c06587b..4b5103ef 100644 --- a/modules/tag/helpers/tag_rest.php +++ b/modules/tag/helpers/tag_rest.php @@ -18,71 +18,36 @@ * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA. */ class tag_rest_Core { - // If no arguments just return all the tags. If 2 or more then it is a path then - // return the tags for that item. But if its only 1, then is it a path or a tag? - // Assume a tag first, if nothing is found then try finding the item. static function get($request) { - $resources = array(); - switch (count($request->arguments)) { - case 0: - $tags = ORM::factory("tag") - ->select("name", "count") - ->order_by("count", "DESC"); - if (!empty($request->limit)) { - $tags->limit($request->limit); - } - if (!empty($request->offset)) { - $tags->offset($request->offset); - } - $resources = array("tags" => array()); - foreach ($tags->find_all() as $row) { - $resources["tags"][] = array("name" => $row->name, "count" => $row->count); - } - break; - case 1: - $resources = tag_rest::_get_items($request); - if (!empty($resources)) { - $resources = array("resources" => $resources); - break; - } - default: - $item = ORM::factory("item") - ->where("relative_url_cache", "=", implode("/", $request->arguments)) - ->viewable() - ->find(); - if ($item->loaded()) { - $resources = array("tags" => tag::item_tags($item)); - } - } - - return rest::reply($resources); + return rest::reply(rest::resolve($request->url)->as_array()); } static function post($request) { - if (empty($request->arguments) || count($request->arguments) != 1 || empty($request->path)) { + $tag = rest::resolve($request->url); + + if (empty($request->params->url)) { throw new Rest_Exception("Bad request", 400); } - $path = $request->path; - $tags = explode(",", $request->arguments[0]); - $item = ORM::factory("item") - ->where("relative_url_cache", "=", $path) - ->viewable() - ->find(); - if (!$item->loaded()) { - throw new Kohana_404_Exception(); - } + $item = rest::resolve($request->params->url); - if (!access::can("edit", $item)) { + access::required("edit", $item); + tag::add($item, $tag->name); + + return rest::reply(); + } + + static function resolve($tag_name) { + $tag = ORM::factory("tag")->where("name", "=", $tag_name)->find(); + if (!$tag->loaded()) { throw new Kohana_404_Exception(); } - foreach ($tags as $tag) { - tag::add($item, $tag); - } - return rest::reply(); + return $tag; } + // ------------------------------------------------------------ + static function put($request) { if (empty($request->arguments[0]) || empty($request->new_name)) { throw new Rest_Exception("Bad request", 400); diff --git a/modules/tag/helpers/tags_rest.php b/modules/tag/helpers/tags_rest.php new file mode 100644 index 00000000..d2bd28b0 --- /dev/null +++ b/modules/tag/helpers/tags_rest.php @@ -0,0 +1,48 @@ +find_all() as $tag) { + $data[$tag->name] = url::abs_site("rest/tags/" . rawurlencode($tag->name)); + } + return rest::reply($data); + } + + static function post($request) { + // @todo: what permission should be required to create a tag here? + // for now, require edit at the top level. Perhaps later, just require any edit perms, + // anywhere in the gallery? + access::required("edit", item::root()); + + if (empty($request->params->name)) { + throw new Rest_Exception("Bad Request", 400); + } + + $tag = ORM::factory("tag")->where("name", "=", $request->params->name)->find(); + if (!$tag->loaded()) { + $tag->name = $request->params->name; + $tag->count = 0; + $tag->save(); + } + + return rest::reply(array("url" => url::abs_site("rest/tag/" . rawurlencode($tag->name)))); + } +} -- cgit v1.2.3 From d18be7fe550e1233e7a19184529f1b87a6249343 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Tue, 5 Jan 2010 12:05:22 -0800 Subject: $offset should be null by default, not 0. Hold over bug from the K24 migration. --- modules/tag/models/tag.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/tag/models/tag.php b/modules/tag/models/tag.php index d0d2117c..2b33c30d 100644 --- a/modules/tag/models/tag.php +++ b/modules/tag/models/tag.php @@ -27,7 +27,7 @@ class Tag_Model extends ORM { * @param string $type the type of item (album, photo) * @return ORM_Iterator */ - public function items($limit=null, $offset=0, $type=null) { + public function items($limit=null, $offset=null, $type=null) { $model = ORM::factory("item") ->viewable() ->join("items_tags", "items.id", "items_tags.item_id") -- cgit v1.2.3 From 31454d37b3ea02104925f1976609576c5f09c0c6 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Tue, 5 Jan 2010 13:41:06 -0800 Subject: Improve REST tag support. - Add support for retrieving a list of members from a collection - Implement put(), post() and delete() for tags. - Use tag_rest::delete() as a way to remove members from the tag collection --- modules/gallery/helpers/gallery_rest.php | 7 +- modules/tag/helpers/tag_rest.php | 118 ++++++++++++------------------- modules/tag/helpers/tags_rest.php | 6 +- 3 files changed, 54 insertions(+), 77 deletions(-) diff --git a/modules/gallery/helpers/gallery_rest.php b/modules/gallery/helpers/gallery_rest.php index 858721d0..c7b32d8c 100644 --- a/modules/gallery/helpers/gallery_rest.php +++ b/modules/gallery/helpers/gallery_rest.php @@ -40,7 +40,12 @@ class gallery_rest_Core { $item = rest::resolve($request->url); access::required("view", $item); - return rest::reply($item->as_array()); + $children = array(); + foreach ($item->children() as $child) { + $children[] = url::abs_site("rest/gallery/" . $child->relative_url()); + } + + return rest::reply(array("resource" => $item->as_array(), "members" => $children)); } static function put($request) { diff --git a/modules/tag/helpers/tag_rest.php b/modules/tag/helpers/tag_rest.php index 4b5103ef..c1bbf4fb 100644 --- a/modules/tag/helpers/tag_rest.php +++ b/modules/tag/helpers/tag_rest.php @@ -19,107 +19,79 @@ */ class tag_rest_Core { static function get($request) { - return rest::reply(rest::resolve($request->url)->as_array()); + $tag = rest::resolve($request->url); + $items = array(); + foreach ($tag->items() as $item) { + $items[] = url::abs_site("rest/gallery/" . $item->relative_url()); + } + + return rest::reply(array("resource" => $tag->as_array(), "members" => $items)); } static function post($request) { - $tag = rest::resolve($request->url); - if (empty($request->params->url)) { throw new Rest_Exception("Bad request", 400); } + $tag = rest::resolve($request->url); $item = rest::resolve($request->params->url); - access::required("edit", $item); - tag::add($item, $tag->name); - return rest::reply(); + tag::add($item, $tag->name); + return rest::reply(array("url" => url::abs_site("rest/tag/" . rawurlencode($tag->name)))); } - static function resolve($tag_name) { - $tag = ORM::factory("tag")->where("name", "=", $tag_name)->find(); - if (!$tag->loaded()) { - throw new Kohana_404_Exception(); - } + static function put($request) { + $tag = rest::resolve($request->url); - return $tag; - } + // @todo: what permission should be required to edit a tag? + // for now, require edit at the top level. Perhaps later, just require any edit perms, + // anywhere in the gallery? - // ------------------------------------------------------------ + if (isset($request->params->remove)) { + if (!is_array($request->params->remove)) { + throw new Exception("Bad request", 400); + } - static function put($request) { - if (empty($request->arguments[0]) || empty($request->new_name)) { - throw new Rest_Exception("Bad request", 400); + foreach ($request->params->remove as $item_url) { + $item = rest::resolve($item_url); + access::required("edit", $item); + $tag->remove($item); + } } - $name = $request->arguments[0]; - - $tag = ORM::factory("tag") - ->where("name", "=", $name) - ->find(); - if (!$tag->loaded()) { - throw new Kohana_404_Exception(); + if (isset($request->params->name)) { + $tag->name = $request->params->name; } - $tag->name = $request->new_name; $tag->save(); - - return rest::reply(); + return rest::reply(array("url" => url::abs_site("rest/tag/" . rawurlencode($tag->name)))); } static function delete($request) { - if (empty($request->arguments[0])) { - throw new Rest_Exception("Bad request", 400); - } - $tags = explode(",", $request->arguments[0]); - if (!empty($request->path)) { - $tag_list = ORM::factory("tag") - ->join("items_tags", "tags.id", "items_tags.tag_id") - ->join("items", "items.id", "items_tags.item_id") - ->where("tags.name", "IN", $tags) - ->where("relative_url_cache", "=", $request->path) - ->viewable() - ->find_all(); - } else { - $tag_list = ORM::factory("tag") - ->where("name", "IN", $tags) - ->find_all(); - } + $tag = rest::resolve($request->url); - foreach ($tag_list as $row) { - $row->delete(); - }; + if (empty($request->params->url)) { + // Delete the tag + $tag->delete(); + return rest::reply(); + } else { + // Remove an item from the tag + $item = rest::resolve($request->params->url); + $tag->remove($item); + $tag->save(); - tag::compact(); - return rest::reply(); + tag::compact(); + return rest::reply(array("url" => url::abs_site("rest/tag/" . rawurlencode($tag->name)))); + } } - private static function _get_items($request) { - $tags = explode(",", $request->arguments[0]); - $items = ORM::factory("item") - ->select_distinct("*") - ->join("items_tags", "items.id", "items_tags.item_id") - ->join("tags", "tags.id", "items_tags.tag_id") - ->where("tags.name", "IN", $tags); - if (!empty($request->limit)) { - $items->limit($request->limit); - } - if (!empty($request->offset)) { - $items->offset($request->offset); - } - $resources = array(); - foreach ($items->find_all() as $item) { - $resources[] = array("type" => $item->type, - "has_children" => $item->children_count() > 0, - "path" => $item->relative_url(), - "thumb_url" => $item->thumb_url(true), - "thumb_dimensions" => array("width" => $item->thumb_width, - "height" => $item->thumb_height), - "has_thumb" => $item->has_thumb(), - "title" => $item->title); + static function resolve($tag_name) { + $tag = ORM::factory("tag")->where("name", "=", $tag_name)->find(); + if (!$tag->loaded()) { + throw new Kohana_404_Exception(); } - return $resources; + return $tag; } } diff --git a/modules/tag/helpers/tags_rest.php b/modules/tag/helpers/tags_rest.php index d2bd28b0..3ef897fd 100644 --- a/modules/tag/helpers/tags_rest.php +++ b/modules/tag/helpers/tags_rest.php @@ -19,11 +19,11 @@ */ class tags_rest_Core { static function get($request) { - $data = array(); + $tags = array(); foreach (ORM::factory("tag")->find_all() as $tag) { - $data[$tag->name] = url::abs_site("rest/tags/" . rawurlencode($tag->name)); + $tags[$tag->name] = url::abs_site("rest/tags/" . rawurlencode($tag->name)); } - return rest::reply($data); + return rest::reply(array("members" => $tags)); } static function post($request) { -- cgit v1.2.3 From d43badb4ecac092c897cb52cc54696d1489bc2e0 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 8 Jan 2010 11:11:38 -0800 Subject: Change url parsing in resolve() to ignore the query string. --- modules/rest/helpers/rest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/rest/helpers/rest.php b/modules/rest/helpers/rest.php index b1b83e1b..121191f2 100644 --- a/modules/rest/helpers/rest.php +++ b/modules/rest/helpers/rest.php @@ -60,10 +60,10 @@ class rest_Core { * @return mixed the corresponding object (usually a model of some kind) */ static function resolve($url) { - $components = explode("/", substr($url, strlen(url::abs_site("rest"))), 3); + $relative_url = substr($url, strlen(url::abs_site("rest"))); + $path = parse_url($relative_url, PHP_URL_PATH); + $components = explode("/", $path, 3); - // The first component will be empty because of the slash between "rest" and the - // resource type. $class = "$components[1]_rest"; if (!method_exists($class, "resolve")) { throw new Kohana_404_Exception($url); -- cgit v1.2.3 From 14f6e5f6d3933b958fa61b83c627412282610dee Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 8 Jan 2010 11:12:02 -0800 Subject: Allow the "name" param in get() so that you can restrict the query to children with a given name. --- modules/gallery/helpers/gallery_rest.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/modules/gallery/helpers/gallery_rest.php b/modules/gallery/helpers/gallery_rest.php index c7b32d8c..fd18d59a 100644 --- a/modules/gallery/helpers/gallery_rest.php +++ b/modules/gallery/helpers/gallery_rest.php @@ -40,8 +40,14 @@ class gallery_rest_Core { $item = rest::resolve($request->url); access::required("view", $item); + if (isset($request->params->name)) { + $where[] = array("name", "=", $request->params->name); + } else { + $where = array(); + } + $children = array(); - foreach ($item->children() as $child) { + foreach ($item->children($where) as $child) { $children[] = url::abs_site("rest/gallery/" . $child->relative_url()); } -- cgit v1.2.3 From a2f5ace4931d4c4d812cd9af1eb3a104bc112b96 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 8 Jan 2010 11:47:00 -0800 Subject: Change merge_where() and merge_or_where() to ignore empty tuples so that chaining is easier. --- modules/kohana23_compat/libraries/MY_Database_Builder.php | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/modules/kohana23_compat/libraries/MY_Database_Builder.php b/modules/kohana23_compat/libraries/MY_Database_Builder.php index c82b6ac4..b2a5c92a 100644 --- a/modules/kohana23_compat/libraries/MY_Database_Builder.php +++ b/modules/kohana23_compat/libraries/MY_Database_Builder.php @@ -23,8 +23,10 @@ class Database_Builder extends Database_Builder_Core { * @chainable */ public function merge_where($tuples) { - foreach ($tuples as $tuple) { - $this->where($tuple[0], $tuple[1], $tuple[2]); + if ($tuples) { + foreach ($tuples as $tuple) { + $this->where($tuple[0], $tuple[1], $tuple[2]); + } } return $this; } @@ -34,8 +36,10 @@ class Database_Builder extends Database_Builder_Core { * @chainable */ public function merge_or_where($tuples) { - foreach ($tuples as $tuple) { - $this->or_where($tuple[0], $tuple[1], $tuple[2]); + if ($tuples) { + foreach ($tuples as $tuple) { + $this->or_where($tuple[0], $tuple[1], $tuple[2]); + } } return $this; } -- cgit v1.2.3 From 2d8843ba4d780b8a9c3662f78d17ca29ebb1c8df Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 8 Jan 2010 11:48:44 -0800 Subject: use item::random() to get the random item. --- modules/image_block/helpers/image_block_block.php | 24 ++--------------------- 1 file changed, 2 insertions(+), 22 deletions(-) diff --git a/modules/image_block/helpers/image_block_block.php b/modules/image_block/helpers/image_block_block.php index f591e8d1..5f2bbcb7 100644 --- a/modules/image_block/helpers/image_block_block.php +++ b/modules/image_block/helpers/image_block_block.php @@ -30,29 +30,9 @@ class image_block_block_Core { $block->css_id = "g-image-block"; $block->title = t("Random image"); $block->content = new View("image_block_block.html"); + $block->content->items = item::random(array(array("type", "!=", "album"))); - $random = ((float)mt_rand()) / (float)mt_getrandmax(); - - $items = ORM::factory("item") - ->viewable() - ->where("type", "!=", "album") - ->where("rand_key", "<", $random) - ->order_by(array("rand_key" => "DESC")) - ->find_all(1); - - if ($items->count() == 0) { - // Try once more. If this fails, just ditch the block altogether - $items = ORM::factory("item") - ->viewable() - ->where("type", "!=", "album") - ->where("rand_key", ">=", $random) - ->order_by(array("rand_key" => "DESC")) - ->find_all(1); - } - - if ($items->count() > 0) { - $block->content->item = $items->current(); - } else { + if ($block->content->items->count() == 0) { $block = ""; } break; -- cgit v1.2.3 From 3fc6dab7acf8d6bacbc557a8554f92d251c0ed6b Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 8 Jan 2010 11:49:01 -0800 Subject: Expect merge_where and merge_or_where to handle empty tuples. --- modules/gallery/libraries/ORM_MPTT.php | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/modules/gallery/libraries/ORM_MPTT.php b/modules/gallery/libraries/ORM_MPTT.php index 0ea519c9..c660b119 100644 --- a/modules/gallery/libraries/ORM_MPTT.php +++ b/modules/gallery/libraries/ORM_MPTT.php @@ -165,11 +165,8 @@ class ORM_MPTT_Core extends ORM { * @return array ORM */ function children($limit=null, $offset=null, $where=null, $order_by=array("id" => "ASC")) { - if ($where) { - $this->merge_where($where); - } - return $this + ->merge_where($where) ->where("parent_id", "=", $this->id) ->order_by($order_by) ->find_all($limit, $offset); @@ -183,11 +180,8 @@ class ORM_MPTT_Core extends ORM { * @return array ORM */ function children_count($where=null) { - if ($where) { - $this->merge_where($where); - } - return $this + ->merge_where($where) ->where("parent_id", "=", $this->id) ->count_all(); } @@ -202,11 +196,8 @@ class ORM_MPTT_Core extends ORM { * @return object ORM_Iterator */ function descendants($limit=null, $offset=null, $where=null, $order_by=array("id" => "ASC")) { - if ($where) { - $this->merge_where($where); - } - return $this + ->merge_where($where); ->where("left_ptr", ">", $this->left_ptr) ->where("right_ptr", "<=", $this->right_ptr) ->order_by($order_by) @@ -220,11 +211,8 @@ class ORM_MPTT_Core extends ORM { * @return integer child count */ function descendants_count($where=null) { - if ($where) { - $this->merge_where($where); - } - return $this + ->merge_where($where); ->where("left_ptr", ">", $this->left_ptr) ->where("right_ptr", "<=", $this->right_ptr) ->count_all(); -- cgit v1.2.3 From 895ac72e706daf8aead624c4cb8d556a2299f73f Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 8 Jan 2010 11:49:18 -0800 Subject: Add item::random() to return a random Item_Model. --- modules/gallery/helpers/item.php | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/modules/gallery/helpers/item.php b/modules/gallery/helpers/item.php index f6181f8a..8098d1cd 100644 --- a/modules/gallery/helpers/item.php +++ b/modules/gallery/helpers/item.php @@ -173,4 +173,34 @@ class item_Core { static function root() { return model_cache::get("item", 1); } + + /** + * Return a random Item_Model, with optional filters + * + * @param array (optional) where tuple + */ + static function random($where=null) { + $random = ((float)mt_rand()) / (float)mt_getrandmax(); + + // Pick a random number and find the item that's got nearest smaller number. In the rare case + // that we chose the smallest number in the system, choose the item with the smallest number. + // This approach works best when the random numbers in the system are roughly evenly + // distributed so this is going to be more efficient with larger data sets. + $random = 0.0; + $items = ORM::factory("item") + ->viewable() + ->where("rand_key", "<", $random) + ->merge_where($where) + ->order_by("rand_key", "DESC") + ->find_all(1); + + if ($items->count() == 0) { + $items = ORM::factory("item") + ->viewable() + ->merge_where($where) + ->order_by("rand_key", "ASC") + ->find_all(1); + } + return $items; + } } \ No newline at end of file -- cgit v1.2.3 From ad3e003e487de09769ca3ba40f1d8b3397658ed6 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 8 Jan 2010 14:04:41 -0800 Subject: Remove stray semicolons. --- modules/gallery/libraries/ORM_MPTT.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/gallery/libraries/ORM_MPTT.php b/modules/gallery/libraries/ORM_MPTT.php index c660b119..ed77cac9 100644 --- a/modules/gallery/libraries/ORM_MPTT.php +++ b/modules/gallery/libraries/ORM_MPTT.php @@ -197,7 +197,7 @@ class ORM_MPTT_Core extends ORM { */ function descendants($limit=null, $offset=null, $where=null, $order_by=array("id" => "ASC")) { return $this - ->merge_where($where); + ->merge_where($where) ->where("left_ptr", ">", $this->left_ptr) ->where("right_ptr", "<=", $this->right_ptr) ->order_by($order_by) @@ -212,7 +212,7 @@ class ORM_MPTT_Core extends ORM { */ function descendants_count($where=null) { return $this - ->merge_where($where); + ->merge_where($where) ->where("left_ptr", ">", $this->left_ptr) ->where("right_ptr", "<=", $this->right_ptr) ->count_all(); -- cgit v1.2.3 From fb65a0a5854812a9837c770dfbb27a23bee49e3d Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 8 Jan 2010 14:20:15 -0800 Subject: Remove debug code. --- modules/gallery/helpers/item.php | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/gallery/helpers/item.php b/modules/gallery/helpers/item.php index 8098d1cd..eb528f8f 100644 --- a/modules/gallery/helpers/item.php +++ b/modules/gallery/helpers/item.php @@ -186,7 +186,6 @@ class item_Core { // that we chose the smallest number in the system, choose the item with the smallest number. // This approach works best when the random numbers in the system are roughly evenly // distributed so this is going to be more efficient with larger data sets. - $random = 0.0; $items = ORM::factory("item") ->viewable() ->where("rand_key", "<", $random) -- cgit v1.2.3 From 9864ab4b2708ec54c39092a21828403cbbd25e2e Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 8 Jan 2010 14:56:08 -0800 Subject: Move the random image functionality into the gallery REST helper since choosing a random image is essentially a function on an item collection. Also implemented a bunch of other query filters for item collections. Created item::random_query() as a way of generating a reasonable starting point for random queries. --- modules/gallery/helpers/gallery_rest.php | 55 +++++++++++++++++--- modules/gallery/helpers/item.php | 25 +++------ modules/image_block/helpers/image_block_block.php | 2 +- modules/image_block/helpers/image_block_rest.php | 62 ----------------------- 4 files changed, 55 insertions(+), 89 deletions(-) delete mode 100644 modules/image_block/helpers/image_block_rest.php diff --git a/modules/gallery/helpers/gallery_rest.php b/modules/gallery/helpers/gallery_rest.php index fd18d59a..0de5da2b 100644 --- a/modules/gallery/helpers/gallery_rest.php +++ b/modules/gallery/helpers/gallery_rest.php @@ -36,22 +36,63 @@ // just like #1 except in a helper instead of in the model? class gallery_rest_Core { + + /** + * For items that are collections, you can specify the following additional query parameters to + * query the collection. You can specify them in any combination. + * + * scope=direct + * only return items that are immediately under this one + * scope=all + * return items anywhere under this one + * + * name= + * only return items where the name contains this substring + * + * random=true + * return a single random item + * + * type= + * limit the type to types in this list. eg, "type=photo,movie" + */ static function get($request) { $item = rest::resolve($request->url); access::required("view", $item); - if (isset($request->params->name)) { - $where[] = array("name", "=", $request->params->name); + $p = $request->params; + if (isset($p->random)) { + $orm = item::random_query()->offset(0)->limit(1); } else { - $where = array(); + $orm = ORM::factory("item")->viewable(); + } + + if (!empty($p->scope) && !in_array($p->scope, array("direct", "all"))) { + throw new Exception("Bad Request", 400); + } + if (!empty($p->scope)) { + if ($p->scope == "direct") { + $orm->where("parent_id", "=", $item->id); + } else { + $orm->where("left_ptr", ">=", $item->left_ptr); + $orm->where("right_ptr", "<=", $item->left_ptr); + $orm->where("id", "<>", $item->id); + } + } + + if (isset($p->name)) { + $orm->where("name", "LIKE", "%{$p->name}%"); + } + + if (isset($p->type)) { + $orm->where("type", "IN", explode(",", $p->type)); } - $children = array(); - foreach ($item->children($where) as $child) { - $children[] = url::abs_site("rest/gallery/" . $child->relative_url()); + $members = array(); + foreach ($orm->find_all() as $child) { + $members[] = url::abs_site("rest/gallery/" . $child->relative_url()); } - return rest::reply(array("resource" => $item->as_array(), "members" => $children)); + return rest::reply(array("resource" => $item->as_array(), "members" => $members)); } static function put($request) { diff --git a/modules/gallery/helpers/item.php b/modules/gallery/helpers/item.php index eb528f8f..1fd9ef16 100644 --- a/modules/gallery/helpers/item.php +++ b/modules/gallery/helpers/item.php @@ -175,31 +175,18 @@ class item_Core { } /** - * Return a random Item_Model, with optional filters + * Return a query to get a random Item_Model, with optional filters * * @param array (optional) where tuple */ - static function random($where=null) { - $random = ((float)mt_rand()) / (float)mt_getrandmax(); - - // Pick a random number and find the item that's got nearest smaller number. In the rare case - // that we chose the smallest number in the system, choose the item with the smallest number. + static function random_query($where=null) { + // Pick a random number and find the item that's got nearest smaller number. // This approach works best when the random numbers in the system are roughly evenly // distributed so this is going to be more efficient with larger data sets. - $items = ORM::factory("item") + return ORM::factory("item") ->viewable() - ->where("rand_key", "<", $random) + ->where("rand_key", "<", ((float)mt_rand()) / (float)mt_getrandmax()) ->merge_where($where) - ->order_by("rand_key", "DESC") - ->find_all(1); - - if ($items->count() == 0) { - $items = ORM::factory("item") - ->viewable() - ->merge_where($where) - ->order_by("rand_key", "ASC") - ->find_all(1); - } - return $items; + ->order_by("rand_key", "DESC"); } } \ No newline at end of file diff --git a/modules/image_block/helpers/image_block_block.php b/modules/image_block/helpers/image_block_block.php index 5f2bbcb7..f28e775f 100644 --- a/modules/image_block/helpers/image_block_block.php +++ b/modules/image_block/helpers/image_block_block.php @@ -30,7 +30,7 @@ class image_block_block_Core { $block->css_id = "g-image-block"; $block->title = t("Random image"); $block->content = new View("image_block_block.html"); - $block->content->items = item::random(array(array("type", "!=", "album"))); + $block->content->items = item::random_query(array(array("type", "!=", "album")))->find_all(1); if ($block->content->items->count() == 0) { $block = ""; diff --git a/modules/image_block/helpers/image_block_rest.php b/modules/image_block/helpers/image_block_rest.php deleted file mode 100644 index 65eefb21..00000000 --- a/modules/image_block/helpers/image_block_rest.php +++ /dev/null @@ -1,62 +0,0 @@ -type) ? "random" : $request->type; - switch ($type) { - case "random": - $random = ((float)mt_rand()) / (float)mt_getrandmax(); - - $items = ORM::factory("item") - ->viewable() - ->where("type", "!=", "album") - ->where("rand_key", "<", $random) - ->order_by(array("rand_key" => "DESC")) - ->find_all(1); - - if ($items->count() == 0) { - // Try once more. If this fails, just ditch the block altogether - $items = ORM::factory("item") - ->viewable() - ->where("type", "!=", "album") - ->where("rand_key", ">= ", $random) - ->order_by(array("rand_key" => "DESC")) - ->find_all(1); - } - break; - default: - return rest::fail("Unsupported image block type: '{$type}'"); - } - - if ($items->count() > 0) { - $item = $items->current(); - $response_data = array("name" => $item->name, - "path" => $item->relative_url(), - "title" => $item->title, - "thumb_url" => $item->thumb_url(true), - "thumb_size" => array("height" => $item->thumb_height, - "width" => $item->thumb_width)); - - return rest::reply(array("resource" => $response_data)); - } else { - return rest::reply(); - } - } -} -- cgit v1.2.3 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/controllers/albums.php | 51 ++++-------- modules/gallery/helpers/album.php | 9 +-- modules/gallery/helpers/item.php | 18 ----- modules/gallery/models/item.php | 139 +++++++++++++++++++++------------ 4 files changed, 106 insertions(+), 111 deletions(-) diff --git a/modules/gallery/controllers/albums.php b/modules/gallery/controllers/albums.php index 2eeefdf1..8ad3ff72 100644 --- a/modules/gallery/controllers/albums.php +++ b/modules/gallery/controllers/albums.php @@ -129,42 +129,27 @@ class Albums_Controller extends Items_Controller { access::required("edit", $album); $form = album::get_edit_form($album); - if ($valid = $form->validate()) { - if ($album->id != 1 && - $form->edit_item->dirname->value != $album->name || - $form->edit_item->slug->value != $album->slug) { - // Make sure that there's not a conflict - if ($row = db::build() - ->select(array("name", "slug")) - ->from("items") - ->where("parent_id", "=", $album->parent_id) - ->where("id", "<>", $album->id) - ->and_open() - ->where("name", "=", $form->edit_item->dirname->value) - ->or_where("slug", "=", $form->edit_item->slug->value) - ->close() - ->execute() - ->current()) { - if ($row->name == $form->edit_item->dirname->value) { - $form->edit_item->dirname->add_error("name_conflict", 1); - } - if ($row->slug == $form->edit_item->slug->value) { - $form->edit_item->slug->add_error("slug_conflict", 1); - } - $valid = false; - } - } - } - - if ($valid) { + try { + $valid = $form->validate(); $album->title = $form->edit_item->title->value; $album->description = $form->edit_item->description->value; $album->sort_column = $form->edit_item->sort_order->column->value; $album->sort_order = $form->edit_item->sort_order->direction->value; - if ($album->id != 1) { - $album->rename($form->edit_item->dirname->value); - } + $album->name = $form->edit_item->dirname->value; $album->slug = $form->edit_item->slug->value; + $album->validate(); + } catch (ORM_Validation_Exception $e) { + // Translate ORM validation errors into form error messages + foreach ($e->validation->errors() as $key => $error) { + if ($key == "name") { + $key = "dirname"; + } + $form->edit_item->inputs[$key]->add_error($error, 1); + } + $valid = false; + } + + if ($valid) { $album->save(); module::event("item_edit_form_completed", $album, $form); @@ -180,9 +165,7 @@ class Albums_Controller extends Items_Controller { print json_encode(array("result" => "success")); } } 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 feaf74cc..477f1945 100644 --- a/modules/gallery/helpers/album.php +++ b/modules/gallery/helpers/album.php @@ -107,7 +107,6 @@ class album_Core { t("The internet address should contain only letters, numbers, hyphens and underscores")); $group->hidden("type")->value("album"); $group->submit("")->value(t("Create")); - $form->add_rules_from(ORM::factory("item")); $form->script("") ->url(url::abs_file("modules/gallery/js/albums_form_add.js")); return $form; @@ -124,15 +123,12 @@ class album_Core { $group->input("dirname")->label(t("Directory Name"))->value($parent->name) ->rules("required") ->error_messages( - "name_conflict", t("There is already a movie, photo or album with this name")) - ->callback("item::validate_no_slashes") + "conflict", t("There is already a movie, photo or album with this name")) ->error_messages("no_slashes", t("The directory name can't contain a \"/\"")) - ->callback("item::validate_no_trailing_period") ->error_messages("no_trailing_period", t("The directory name can't end in \".\"")); $group->input("slug")->label(t("Internet Address"))->value($parent->slug) ->error_messages( - "slug_conflict", t("There is already a movie, photo or album with this internet address")) - ->callback("item::validate_url_safe") + "conflict", t("There is already a movie, photo or album with this internet address")) ->error_messages( "not_url_safe", t("The internet address should contain only letters, numbers, hyphens and underscores")); @@ -159,7 +155,6 @@ class album_Core { $group = $form->group("buttons")->label(""); $group->hidden("type")->value("album"); $group->submit("")->value(t("Modify")); - $form->add_rules_from(ORM::factory("item")); return $form; } diff --git a/modules/gallery/helpers/item.php b/modules/gallery/helpers/item.php index 1fd9ef16..53291ccc 100644 --- a/modules/gallery/helpers/item.php +++ b/modules/gallery/helpers/item.php @@ -78,24 +78,6 @@ class item_Core { graphics::generate($album); } - static function validate_no_slashes($input) { - if (strpos($input->value, "/") !== false) { - $input->add_error("no_slashes", 1); - } - } - - static function validate_no_trailing_period($input) { - if (rtrim($input->value, ".") !== $input->value) { - $input->add_error("no_trailing_period", 1); - } - } - - static function validate_url_safe($input) { - if (preg_match("/[^A-Za-z0-9-_]/", $input->value)) { - $input->add_error("not_url_safe", 1); - } - } - /** * Sanitize a filename into something presentable as an item title * @param string $filename 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(-) 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(-) 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 5809949ae8ff87cd5acf56c528e6dc2af6619513 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 15 Jan 2010 11:28:05 -0800 Subject: Don't use Input directly to get album names, etc. Use the form fields. --- modules/gallery/controllers/albums.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/modules/gallery/controllers/albums.php b/modules/gallery/controllers/albums.php index 8ad3ff72..9f8c16ea 100644 --- a/modules/gallery/controllers/albums.php +++ b/modules/gallery/controllers/albums.php @@ -95,16 +95,16 @@ class Albums_Controller extends Items_Controller { access::required("view", $album); access::required("add", $album); - $input = Input::instance(); $form = album::get_add_form($album); if ($form->validate()) { $new_album = album::create( $album, - $input->post("name"), - $input->post("title", $input->post("name")), - $input->post("description"), + $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, - $input->post("slug")); + $form->add_album->slug->value); log::success("content", "Created an album", html::anchor("albums/$new_album->id", "view album")); -- 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(-) 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 1066e64354ff44f88c7dd0de3bb3e50411458523 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 15 Jan 2010 12:41:22 -0800 Subject: Call parent::save() before releasing the lock to make creating the hole and filling it an atomic operation. --- modules/gallery/libraries/ORM_MPTT.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/modules/gallery/libraries/ORM_MPTT.php b/modules/gallery/libraries/ORM_MPTT.php index 46ae0af8..404d61ff 100644 --- a/modules/gallery/libraries/ORM_MPTT.php +++ b/modules/gallery/libraries/ORM_MPTT.php @@ -73,10 +73,13 @@ class ORM_MPTT_Core extends ORM { $this->unlock(); throw $e; } + parent::save(); $this->unlock(); + } else { + parent::save(); } - return parent::save(); + return $this; } /** -- 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(-) 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(-) 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(-) 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 5a8449f16d3c0db8fb47acf515d319d6eb9e87f4 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Sat, 16 Jan 2010 11:12:27 -0800 Subject: Convert Photos_Controller::update() to use model based validation. --- modules/gallery/controllers/photos.php | 55 ++++++++++------------------------ modules/gallery/helpers/photo.php | 10 ++----- 2 files changed, 17 insertions(+), 48 deletions(-) diff --git a/modules/gallery/controllers/photos.php b/modules/gallery/controllers/photos.php index 56b454ce..98f2126d 100644 --- a/modules/gallery/controllers/photos.php +++ b/modules/gallery/controllers/photos.php @@ -61,48 +61,25 @@ class Photos_Controller extends Items_Controller { access::required("edit", $photo); $form = photo::get_edit_form($photo); - $valid = $form->validate(); - - if ($valid) { - $new_ext = pathinfo($form->edit_item->filename->value, PATHINFO_EXTENSION); - $old_ext = pathinfo($photo->name, PATHINFO_EXTENSION); - if (strcasecmp($new_ext, $old_ext)) { - $form->edit_item->filename->add_error("illegal_extension", 1); - $valid = false; - } - } - - if ($valid) { - if ($form->edit_item->filename->value != $photo->name || - $form->edit_item->slug->value != $photo->slug) { - // Make sure that there's not a name or slug conflict - if ($row = db::build() - ->select(array("name", "slug")) - ->from("items") - ->where("parent_id", "=", $photo->parent_id) - ->where("id", "<>", $photo->id) - ->and_open() - ->where("name", "=", $form->edit_item->filename->value) - ->or_where("slug", "=", $form->edit_item->slug->value) - ->close() - ->execute() - ->current()) { - if ($row->name == $form->edit_item->filename->value) { - $form->edit_item->filename->add_error("name_conflict", 1); - } - if ($row->slug == $form->edit_item->slug->value) { - $form->edit_item->slug->add_error("slug_conflict", 1); - } - $valid = false; + try { + $valid = $form->validate(); + $photo->title = $form->edit_item->title->value; + $photo->description = $form->edit_item->description->value; + $photo->slug = $form->edit_item->slug->value; + $photo->name = $form->edit_item->filename->value; + $photo->validate(); + } catch (ORM_Validation_Exception $e) { + // Translate ORM validation errors into form error messages + foreach ($e->validation->errors() as $key => $error) { + if ($key == "name") { + $key = "filename"; } + $form->edit_item->inputs[$key]->add_error($error, 1); } + $valid = false; } if ($valid) { - $photo->title = $form->edit_item->title->value; - $photo->description = $form->edit_item->description->value; - $photo->slug = $form->edit_item->slug->value; - $photo->rename($form->edit_item->filename->value); $photo->save(); module::event("item_edit_form_completed", $photo, $form); @@ -118,9 +95,7 @@ class Photos_Controller extends Items_Controller { print json_encode(array("result" => "success")); } } 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/photo.php b/modules/gallery/helpers/photo.php index 74e30409..cb94772e 100644 --- a/modules/gallery/helpers/photo.php +++ b/modules/gallery/helpers/photo.php @@ -31,18 +31,13 @@ class photo_Core { $group->input("title")->label(t("Title"))->value($photo->title); $group->textarea("description")->label(t("Description"))->value($photo->description); $group->input("filename")->label(t("Filename"))->value($photo->name) - ->rules("required") - ->error_messages( - "name_conflict", t("There is already a movie, photo or album with this name")) - ->callback("item::validate_no_slashes") + ->error_messages("conflict", t("There is already a movie, photo or album with this name")) ->error_messages("no_slashes", t("The photo name can't contain a \"/\"")) - ->callback("item::validate_no_trailing_period") ->error_messages("no_trailing_period", t("The photo name can't end in \".\"")) ->error_messages("illegal_extension", t("You cannot change the filename extension")); $group->input("slug")->label(t("Internet Address"))->value($photo->slug) - ->callback("item::validate_url_safe") ->error_messages( - "slug_conflict", t("There is already a movie, photo or album with this internet address")) + "conflict", t("There is already a movie, photo or album with this internet address")) ->error_messages( "not_url_safe", t("The internet address should contain only letters, numbers, hyphens and underscores")); @@ -51,7 +46,6 @@ class photo_Core { $group = $form->group("buttons")->label(""); $group->submit("")->value(t("Modify")); - $form->add_rules_from(ORM::factory("item")); return $form; } -- cgit v1.2.3 From b5a6a6a5d5741f592d504b8a444899964101b6b6 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Sat, 16 Jan 2010 11:44:21 -0800 Subject: Oops, log::failure() doesn't exist. Use log::error(). --- modules/gallery/helpers/gallery_event.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/modules/gallery/helpers/gallery_event.php b/modules/gallery/helpers/gallery_event.php index 9452e855..db3b34fe 100644 --- a/modules/gallery/helpers/gallery_event.php +++ b/modules/gallery/helpers/gallery_event.php @@ -79,7 +79,9 @@ class gallery_event_Core { try { graphics::generate($item); } catch (Exception $e) { - log::failure("Unable to create a thumbnail for item id {$item->id}"); + log::error("graphics", t("Couldn't create a thumbnail or resize for %item_title", + array("item_title" => $item->title)), + html::anchor($item->abs_url(), t("details"))); Kohana_Log::add("error", $e->getMessage() . "\n" . $e->getTraceAsString()); } -- 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(-) 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 8ce11ac97062f63eb6d0c5ef261bdf9ff6727ed2 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Sat, 16 Jan 2010 12:07:36 -0800 Subject: Convert Movies_Controller::update() over to model based validation. --- modules/gallery/controllers/movies.php | 55 ++++++++++------------------------ modules/gallery/helpers/movie.php | 10 ++----- 2 files changed, 18 insertions(+), 47 deletions(-) diff --git a/modules/gallery/controllers/movies.php b/modules/gallery/controllers/movies.php index 7a8e4d2a..0908e281 100644 --- a/modules/gallery/controllers/movies.php +++ b/modules/gallery/controllers/movies.php @@ -61,48 +61,25 @@ class Movies_Controller extends Items_Controller { access::required("edit", $movie); $form = movie::get_edit_form($movie); - $valid = $form->validate(); - - if ($valid) { - $new_ext = pathinfo($form->edit_item->filename->value, PATHINFO_EXTENSION); - $old_ext = pathinfo($movie->name, PATHINFO_EXTENSION); - if (strcasecmp($new_ext, $old_ext)) { - $form->edit_item->filename->add_error("illegal_extension", 1); - $valid = false; - } - } - - if ($valid) { - if ($form->edit_item->filename->value != $movie->name || - $form->edit_item->slug->value != $movie->slug) { - // Make sure that there's not a name or slug conflict - if ($row = db::build() - ->select(array("name", "slug")) - ->from("items") - ->where("parent_id", "=", $movie->parent_id) - ->where("id", "<>", $movie->id) - ->and_open() - ->where("name", "=", $form->edit_item->filename->value) - ->or_where("slug", "=", $form->edit_item->slug->value) - ->close() - ->execute() - ->current()) { - if ($row->name == $form->edit_item->filename->value) { - $form->edit_item->filename->add_error("name_conflict", 1); - } - if ($row->slug == $form->edit_item->slug->value) { - $form->edit_item->slug->add_error("slug_conflict", 1); - } - $valid = false; + try { + $valid = $form->validate(); + $movie->title = $form->edit_item->title->value; + $movie->description = $form->edit_item->description->value; + $movie->slug = $form->edit_item->slug->value; + $movie->name = $form->edit_item->filename->value; + $movie->validate(); + } catch (ORM_Validation_Exception $e) { + // Translate ORM validation errors into form error messages + foreach ($e->validation->errors() as $key => $error) { + if ($key == "name") { + $key = "filename"; } + $form->edit_item->inputs[$key]->add_error($error, 1); } + $valid = false; } if ($valid) { - $movie->title = $form->edit_item->title->value; - $movie->description = $form->edit_item->description->value; - $movie->slug = $form->edit_item->slug->value; - $movie->rename($form->edit_item->filename->value); $movie->save(); module::event("item_edit_form_completed", $movie, $form); @@ -118,9 +95,7 @@ class Movies_Controller extends Items_Controller { print json_encode(array("result" => "success")); } } 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/movie.php b/modules/gallery/helpers/movie.php index 0a27ac94..b2e846d3 100644 --- a/modules/gallery/helpers/movie.php +++ b/modules/gallery/helpers/movie.php @@ -31,18 +31,14 @@ class movie_Core { $group->input("title")->label(t("Title"))->value($movie->title); $group->textarea("description")->label(t("Description"))->value($movie->description); $group->input("filename")->label(t("Filename"))->value($movie->name) - ->rules("required") ->error_messages( - "name_conflict", t("There is already a movie, photo or album with this name")) - ->callback("item::validate_no_slashes") + "conflict", t("There is already a movie, photo or album with this name")) ->error_messages("no_slashes", t("The movie name can't contain a \"/\"")) - ->callback("item::validate_no_trailing_period") ->error_messages("no_trailing_period", t("The movie name can't end in \".\"")) ->error_messages("illegal_extension", t("You cannot change the filename extension")); $group->input("slug")->label(t("Internet Address"))->value($movie->slug) - ->callback("item::validate_url_safe") ->error_messages( - "slug_conflict", t("There is already a movie, photo or album with this internet address")) + "conflict", t("There is already a movie, photo or album with this internet address")) ->error_messages( "not_url_safe", t("The internet address should contain only letters, numbers, hyphens and underscores")); @@ -51,7 +47,7 @@ class movie_Core { $group = $form->group("buttons")->label(""); $group->submit("")->value(t("Modify")); - $form->add_rules_from(ORM::factory("item")); + return $form; } -- cgit v1.2.3 From ff728b3ccd329e988220ca2857b3dcd989ebad37 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Sat, 16 Jan 2010 17:51:57 -0800 Subject: Whitespace. --- modules/user/controllers/admin_users.php | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/modules/user/controllers/admin_users.php b/modules/user/controllers/admin_users.php index 96b86fff..ab747528 100644 --- a/modules/user/controllers/admin_users.php +++ b/modules/user/controllers/admin_users.php @@ -21,12 +21,8 @@ class Admin_Users_Controller extends Admin_Controller { public function index() { $view = new Admin_View("admin.html"); $view->content = new View("admin_users.html"); - $view->content->users = ORM::factory("user") - ->order_by("name", "ASC") - ->find_all(); - $view->content->groups = ORM::factory("group") - ->order_by("name", "ASC") - ->find_all(); + $view->content->users = ORM::factory("user")->order_by("name", "ASC")->find_all(); + $view->content->groups = ORM::factory("group")->order_by("name", "ASC")->find_all(); print $view; } -- cgit v1.2.3 From 7f20f660791a8276e6f74232e6b941cf20c8c1a7 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Sat, 16 Jan 2010 18:00:02 -0800 Subject: Whitespace. --- modules/user/views/user_form.html.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/user/views/user_form.html.php b/modules/user/views/user_form.html.php index 039ae8a5..4ce2b532 100644 --- a/modules/user/views/user_form.html.php +++ b/modules/user/views/user_form.html.php @@ -1,5 +1,5 @@ - -- cgit v1.2.3 From 5c527513c688571adcff45f513efff54b9c55e61 Mon Sep 17 00:00:00 2001 From: Andy Staudacher Date: Thu, 28 Jan 2010 19:46:53 -0800 Subject: Fix language preference block / language cookie reading. The preference block must have been broken by a jquery update, and the cookie reading by a Kohana update. --- modules/gallery/helpers/locales.php | 4 +++- modules/gallery/views/user_languages_block.html.php | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/modules/gallery/helpers/locales.php b/modules/gallery/helpers/locales.php index 5c8c227a..dc32b12f 100644 --- a/modules/gallery/helpers/locales.php +++ b/modules/gallery/helpers/locales.php @@ -238,7 +238,9 @@ class locales_Core { } static function cookie_locale() { - $cookie_data = Input::instance()->cookie("g_locale"); + // Can't use Input framework for client side cookies since + // they're not signed. + $cookie_data = isset($_COOKIE["g_locale"]) ? $_COOKIE["g_locale"] : null; $locale = null; if ($cookie_data) { if (preg_match("/^([a-z]{2,3}(?:_[A-Z]{2})?)$/", trim($cookie_data), $matches)) { diff --git a/modules/gallery/views/user_languages_block.html.php b/modules/gallery/views/user_languages_block.html.php index 89185967..3776ca13 100644 --- a/modules/gallery/views/user_languages_block.html.php +++ b/modules/gallery/views/user_languages_block.html.php @@ -1,7 +1,7 @@ diff --git a/modules/gallery/libraries/InPlaceEdit.php b/modules/gallery/libraries/InPlaceEdit.php index 67ab3805..04a2e9a5 100644 --- a/modules/gallery/libraries/InPlaceEdit.php +++ b/modules/gallery/libraries/InPlaceEdit.php @@ -70,7 +70,6 @@ class InPlaceEdit_Core { public function render() { $v = new View("in_place_edit.html"); - $v->hidden = array("csrf" => access::csrf_token()); $v->action = url::site($this->action); $v->form = $this->form; $v->errors = $this->errors; diff --git a/modules/gallery/views/in_place_edit.html.php b/modules/gallery/views/in_place_edit.html.php index 45cf1d8c..b556829c 100644 --- a/modules/gallery/views/in_place_edit.html.php +++ b/modules/gallery/views/in_place_edit.html.php @@ -1,5 +1,6 @@ - "post", "id" => "g-in-place-edit-form", "class" => "g-short-form"), $hidden) ?> + "post", "id" => "g-in-place-edit-form", "class" => "g-short-form")) ?> +
    class="g-error"> @@ -9,7 +10,7 @@
- +
-- cgit v1.2.3 From c214dfd0948c1878dbd034694c2d8d6f8c8e1180 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 29 Jan 2010 10:54:59 -0800 Subject: Clean up form validation code. --- modules/akismet/controllers/admin_akismet.php | 11 +---------- modules/akismet/helpers/akismet.php | 17 +++++++++++------ 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/modules/akismet/controllers/admin_akismet.php b/modules/akismet/controllers/admin_akismet.php index ca3a1473..4847db53 100644 --- a/modules/akismet/controllers/admin_akismet.php +++ b/modules/akismet/controllers/admin_akismet.php @@ -25,17 +25,8 @@ class Admin_Akismet_Controller extends Admin_Controller { // @todo move the "post" handler part of this code into a separate function access::verify_csrf(); - $valid = $form->validate(); - - if ($valid) { + if ($form->validate()) { $new_key = $form->configure_akismet->api_key->value; - if ($new_key && !akismet::validate_key($new_key)) { - $form->configure_akismet->api_key->add_error("invalid", 1); - $valid = false; - } - } - - if ($valid) { $old_key = module::get_var("akismet", "api_key"); if ($old_key && !$new_key) { message::success(t("Your Akismet key has been cleared.")); diff --git a/modules/akismet/helpers/akismet.php b/modules/akismet/helpers/akismet.php index 46a305b2..b4405de5 100644 --- a/modules/akismet/helpers/akismet.php +++ b/modules/akismet/helpers/akismet.php @@ -23,8 +23,9 @@ class akismet_Core { static function get_configure_form() { $form = new Forge("admin/akismet", "", "post", array("id" => "g-configure-akismet-form")); $group = $form->group("configure_akismet")->label(t("Configure Akismet")); - $group->input("api_key")->label(t("API Key"))->value(module::get_var("akismet", "api_key")); - $group->api_key->error_messages("invalid", t("The API key you provided is invalid.")); + $group->input("api_key")->label(t("API Key"))->value(module::get_var("akismet", "api_key")) + ->callback("akismet::validate_key") + ->error_messages("invalid", t("The API key you provided is invalid.")); $group->submit("")->value(t("Save")); return $form; } @@ -82,10 +83,14 @@ class akismet_Core { * @param string $api_key the API key * @return boolean */ - static function validate_key($api_key) { - $request = self::_build_verify_request($api_key); - $response = self::_http_post($request, "rest.akismet.com"); - return "valid" == $response->body[0]; + static function validate_key($api_key_input) { + if ($api_key_input->value) { + $request = self::_build_verify_request($api_key_input->value); + $response = self::_http_post($request, "rest.akismet.com"); + if ("valid" != $response->body[0]) { + $api_key_input->add_error("invalid", 1); + } + } } -- cgit v1.2.3 From 3f5ad7d77a4b877220cd410afbb286dce5b15b75 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 29 Jan 2010 11:20:35 -0800 Subject: Clean up form validation code. --- modules/recaptcha/controllers/admin_recaptcha.php | 2 +- modules/recaptcha/helpers/recaptcha.php | 40 ++++++++++++++--------- 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/modules/recaptcha/controllers/admin_recaptcha.php b/modules/recaptcha/controllers/admin_recaptcha.php index 6874fce9..0ddb98c1 100644 --- a/modules/recaptcha/controllers/admin_recaptcha.php +++ b/modules/recaptcha/controllers/admin_recaptcha.php @@ -42,7 +42,7 @@ class Admin_Recaptcha_Controller extends Admin_Controller { } else { module::set_var("recaptcha", "public_key", ""); module::set_var("recaptcha", "private_key", ""); - message::success(t("reCAPTCHA disabled!")); + message::success(t("No keys provided. reCAPTCHA is disabled!")); log::success("recaptcha", t("reCAPTCHA public and private keys cleared")); url::redirect("admin/recaptcha"); } diff --git a/modules/recaptcha/helpers/recaptcha.php b/modules/recaptcha/helpers/recaptcha.php index 789bae85..2b3b869c 100644 --- a/modules/recaptcha/helpers/recaptcha.php +++ b/modules/recaptcha/helpers/recaptcha.php @@ -24,12 +24,16 @@ class recaptcha_Core { ->label(t("Configure reCAPTCHA")); $group->input("public_key") ->label(t("Public Key")) - ->value(module::get_var("recaptcha", "public_key")); - $group->public_key->error_messages("invalid", t("The public key you provided is invalid.")); + ->value(module::get_var("recaptcha", "public_key")) + ->rules("required") + ->error_messages("required", t("You must enter a public key")) + ->error_messages("invalid", t("This public key is invalid")); $group->input("private_key") ->label(t("Private Key")) - ->value(module::get_var("recaptcha", "private_key")); - $group->private_key->error_messages("invalid", t("The private key you provided is invalid.")); + ->value(module::get_var("recaptcha", "private_key")) + ->callback("recaptcha::verify_key") + ->error_messages("required", t("You must enter a private key")) + ->error_messages("invalid", t("This private key is invalid")); $group->submit("")->value(t("Save")); $site_domain = urlencode(stripslashes($_SERVER["HTTP_HOST"])); @@ -55,19 +59,23 @@ class recaptcha_Core { * @param string $private_key * @return boolean */ - static function verify_key($private_key) { + static function verify_key($private_key_input) { + if (empty($private_key_input->value)) { + $private_key_input->add_error("required", 1); + return; + } + $remote_ip = Input::instance()->server("REMOTE_ADDR"); $response = self::_http_post("api-verify.recaptcha.net", "/verify", - array("privatekey" => $private_key, + array("privatekey" => $private_key_input->value, "remoteip" => $remote_ip, "challenge" => "right", "response" => "wrong")); - $answers = explode("\n", $response[1]); - if (trim($answers[0]) == "true") { - return null; - } else { - return $answers[1]; + if ($response[1] == "false\ninvalid-site-private-key") { + // This is the only thing I can figure out how to verify. + // See http://recaptcha.net/apidocs/captcha for possible return values + $private_key_input->add_error("invalid", 1); } } @@ -80,16 +88,16 @@ class recaptcha_Core { $input = Input::instance(); $remote_ip = $input->server("REMOTE_ADDR"); - //discard spam submissions + // discard spam submissions if (empty($challenge) || empty($response)) { return "incorrect-captcha-sol"; } $response = self::_http_post("api-verify.recaptcha.net", "/verify", - array ("privatekey" => $private_key, - "remoteip" => $remote_ip, - "challenge" => $challenge, - "response" => $response)); + array("privatekey" => $private_key, + "remoteip" => $remote_ip, + "challenge" => $challenge, + "response" => $response)); $answers = explode ("\n", $response [1]); if (trim ($answers [0]) == "true") { -- cgit v1.2.3 From 660130cf1ab9fd6cb051712b57966b191064a6a6 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 29 Jan 2010 11:23:28 -0800 Subject: Work around a weirdness where empty() doesn't work on input values. --- modules/recaptcha/helpers/recaptcha.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/recaptcha/helpers/recaptcha.php b/modules/recaptcha/helpers/recaptcha.php index 2b3b869c..5df22cbc 100644 --- a/modules/recaptcha/helpers/recaptcha.php +++ b/modules/recaptcha/helpers/recaptcha.php @@ -60,7 +60,7 @@ class recaptcha_Core { * @return boolean */ static function verify_key($private_key_input) { - if (empty($private_key_input->value)) { + if (!$private_key_input->value) { $private_key_input->add_error("required", 1); return; } -- cgit v1.2.3 From 1bc0d05760df7bff5cee0a330b5b7181b3c49835 Mon Sep 17 00:00:00 2001 From: Tim Almdal Date: Fri, 29 Jan 2010 11:36:35 -0800 Subject: Replace with . Also add a call to access::csrf_form_field in the form template. Fixes ticket #996. --- modules/gallery/views/in_place_edit.html.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/gallery/views/in_place_edit.html.php b/modules/gallery/views/in_place_edit.html.php index 45cf1d8c..ad9ea845 100644 --- a/modules/gallery/views/in_place_edit.html.php +++ b/modules/gallery/views/in_place_edit.html.php @@ -1,5 +1,6 @@ "post", "id" => "g-in-place-edit-form", "class" => "g-short-form"), $hidden) ?> +
    class="g-error"> @@ -9,7 +10,7 @@
- +
-- 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(-) 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 45cdac973d35de083875ed966ac687b805d60e7f Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 29 Jan 2010 14:06:36 -0800 Subject: Oops, somebody (me?) forgot to update the gallery module version number in gallery_installer::install() so the install.sql was out of sync. --- installer/install.sql | 2 +- modules/gallery/helpers/gallery_installer.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/installer/install.sql b/installer/install.sql index 39637fb3..a5eec229 100644 --- a/installer/install.sql +++ b/installer/install.sql @@ -228,7 +228,7 @@ CREATE TABLE {modules} ( UNIQUE KEY `name` (`name`) ) AUTO_INCREMENT=10 DEFAULT CHARSET=utf8; SET character_set_client = @saved_cs_client; -INSERT INTO {modules} VALUES (1,1,'gallery',21); +INSERT INTO {modules} VALUES (1,1,'gallery',22); INSERT INTO {modules} VALUES (2,1,'user',2); INSERT INTO {modules} VALUES (3,1,'comment',2); INSERT INTO {modules} VALUES (4,1,'organize',1); diff --git a/modules/gallery/helpers/gallery_installer.php b/modules/gallery/helpers/gallery_installer.php index bfab4645..93948045 100644 --- a/modules/gallery/helpers/gallery_installer.php +++ b/modules/gallery/helpers/gallery_installer.php @@ -276,7 +276,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", 21); + module::set_version("gallery", 22); } static function upgrade($version) { -- cgit v1.2.3 From 844d40a759db80108d20684303680d2e489af9d0 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 29 Jan 2010 14:12:07 -0800 Subject: Oops, forgot to bump the version to 2 in install(). --- modules/g2_import/helpers/g2_import_installer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/g2_import/helpers/g2_import_installer.php b/modules/g2_import/helpers/g2_import_installer.php index 77b61d3e..1ec8afa9 100644 --- a/modules/g2_import/helpers/g2_import_installer.php +++ b/modules/g2_import/helpers/g2_import_installer.php @@ -31,7 +31,7 @@ class g2_import_installer { KEY `g2_id` (`g2_id`)) DEFAULT CHARSET=utf8;"); - module::set_version("g2_import", 1); + module::set_version("g2_import", 2); mkdir(VARPATH . "modules/g2_import"); } -- cgit v1.2.3 From 98bcb95b1065a044909c4c4ff15d93fafc793df7 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 29 Jan 2010 14:20:34 -0800 Subject: Go through all slugs and make them legal values. Upgrade gallery3 module to version 23 --- modules/gallery/helpers/gallery_installer.php | 23 ++++++++++++++++++++++- modules/gallery/module.info | 2 +- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/modules/gallery/helpers/gallery_installer.php b/modules/gallery/helpers/gallery_installer.php index 93948045..8227fdc9 100644 --- a/modules/gallery/helpers/gallery_installer.php +++ b/modules/gallery/helpers/gallery_installer.php @@ -450,7 +450,7 @@ class gallery_installer { // Update the graphics rules table so that the maximum height for resizes is 640 not 480. // Fixes ticket #671 - if ( $version == 21) { + if ($version == 21) { $resize_rule = ORM::factory("graphics_rule") ->where("id", "=", "2") ->find(); @@ -463,6 +463,27 @@ class gallery_installer { } module::set_version("gallery", $version = 22); } + + // Update slug values to be legal. We should have done this in the 11->12 upgrader, but I was + // lazy. Mea culpa! + if ($version == 22) { + foreach (db::build() + ->from("items") + ->select("id", "slug") + ->where(new Database_Expression("`slug` REGEXP '[^_A-Za-z0-9-]'"), "=", 1) + ->execute() as $row) { + $new_slug = item::convert_filename_to_slug($row->slug); + if (empty($new_slug)) { + $new_slug = rand(); + } + db::build() + ->update("items") + ->set("slug", $new_slug) + ->where("id", "=", $row->id) + ->execute(); + } + module::set_version("gallery", $version = 23); + } } static function uninstall() { diff --git a/modules/gallery/module.info b/modules/gallery/module.info index 107d9a12..ee169cf1 100644 --- a/modules/gallery/module.info +++ b/modules/gallery/module.info @@ -1,4 +1,4 @@ name = "Gallery 3" description = "Gallery core application" -version = 22 +version = 23 -- cgit v1.2.3 From d4998e37d859e4702407b9a85af6a8ac2fabe686 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 29 Jan 2010 14:25:57 -0800 Subject: Don't forget to flush the relative_url_cache when updating the slug. --- modules/gallery/helpers/gallery_installer.php | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/gallery/helpers/gallery_installer.php b/modules/gallery/helpers/gallery_installer.php index 8227fdc9..d2378d64 100644 --- a/modules/gallery/helpers/gallery_installer.php +++ b/modules/gallery/helpers/gallery_installer.php @@ -479,6 +479,7 @@ class gallery_installer { db::build() ->update("items") ->set("slug", $new_slug) + ->set("relative_url_cache", null) ->where("id", "=", $row->id) ->execute(); } -- cgit v1.2.3 From a95609849e42656b35f0110a08cc4436ad56b916 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 29 Jan 2010 14:53:40 -0800 Subject: Use var_export instead of print_r for better clarity. --- modules/rest/helpers/rest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/rest/helpers/rest.php b/modules/rest/helpers/rest.php index 0d2ec9d4..1f95a7e7 100644 --- a/modules/rest/helpers/rest.php +++ b/modules/rest/helpers/rest.php @@ -25,8 +25,8 @@ class rest_Core { if (Input::instance()->get("output") == "html") { header("Content-type: text/html"); $html = preg_replace( - "#(^|[\n ])([\w]+?://[\w]+[^ \"\n\r\t<]*)#ise", "'\\1\\2'", - print_r($data, 1)); + "#([\w]+?://[\w]+[^ \'\"\n\r\t<]*)#ise", "'\\1'", + var_export($data, 1)); print "
$html
"; } else { header("Content-type: application/json"); -- cgit v1.2.3 From a04d0d278964c93b4829ec2e77f5f315abcba392 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 29 Jan 2010 19:42:38 -0800 Subject: Add missing permission checks. Make the tag relationship an associative array. --- modules/tag/helpers/tag_item_rest.php | 6 +++--- modules/tag/helpers/tag_items_rest.php | 8 ++++++-- modules/tag/tests/Tag_Item_Rest_Helper_Test.php | 4 ++-- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/modules/tag/helpers/tag_item_rest.php b/modules/tag/helpers/tag_item_rest.php index 60d37437..672cec53 100644 --- a/modules/tag/helpers/tag_item_rest.php +++ b/modules/tag/helpers/tag_item_rest.php @@ -23,8 +23,8 @@ class tag_item_rest_Core { return array( "url" => $request->url, "members" => array( - rest::url("tag", $tag), - rest::url("item", $item))); + "tag" => rest::url("tag", $tag), + "item" => rest::url("item", $item))); } static function delete($request) { @@ -37,7 +37,7 @@ class tag_item_rest_Core { list ($tag_id, $item_id) = split(",", $tuple); $tag = ORM::factory("tag", $tag_id); $item = ORM::factory("item", $item_id); - if (!$tag->loaded() || !$item->loaded() || !$tag->has($item)) { + if (!$tag->loaded() || !$item->loaded() || !$tag->has($item) || !access::can("view", $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 ef563ac6..18973ebb 100644 --- a/modules/tag/helpers/tag_items_rest.php +++ b/modules/tag/helpers/tag_items_rest.php @@ -37,12 +37,16 @@ class tag_items_rest_Core { $item = rest::resolve($request->params->item); access::required("view", $item); + if (!$tag->loaded()) { + throw new Kohana_404_Exception(); + } + tag::add($item, $tag->name); return array( "url" => rest::url("tag_item", $tag, $item), "members" => array( - rest::url("tag", $tag), - rest::url("item", $item))); + "tag" => rest::url("tag", $tag), + "item" => rest::url("item", $item))); } static function delete($request) { diff --git a/modules/tag/tests/Tag_Item_Rest_Helper_Test.php b/modules/tag/tests/Tag_Item_Rest_Helper_Test.php index 6c49ad67..69c580f1 100644 --- a/modules/tag/tests/Tag_Item_Rest_Helper_Test.php +++ b/modules/tag/tests/Tag_Item_Rest_Helper_Test.php @@ -32,8 +32,8 @@ class Tag_Item_Rest_Helper_Test extends Gallery_Unit_Test_Case { $this->assert_equal_array( array("url" => rest::url("tag_item", $tag, item::root()), "members" => array( - rest::url("tag", $tag), - rest::url("item", item::root()))), + "tag" => rest::url("tag", $tag), + "item" => rest::url("item", item::root()))), tag_item_rest::get($request)); } -- cgit v1.2.3 From dcba664f74439e37cc269df0cf549a2fee552aeb Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 29 Jan 2010 20:37:48 -0800 Subject: Use ? or & as appropriate when appending output=html. --- modules/rest/helpers/rest.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/modules/rest/helpers/rest.php b/modules/rest/helpers/rest.php index 1f95a7e7..3883794a 100644 --- a/modules/rest/helpers/rest.php +++ b/modules/rest/helpers/rest.php @@ -113,7 +113,11 @@ class rest_Core { $url = call_user_func_array(array($class, "url"), $args); if (Input::instance()->get("output") == "html") { - $url .= "?output=html"; + if (strpos($url, "?") === false) { + $url .= "?output=html"; + } else { + $url .= "&output=html"; + } } return $url; } -- cgit v1.2.3 From 43cb6d9b56f802a5952d16b8412f8407dd8cf3c4 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Sat, 30 Jan 2010 11:38:40 -0800 Subject: Make the error page more robust in the case where there's a failure early on in the framework code before we can load Gallery_I18n.php --- modules/gallery/views/kohana/error.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/gallery/views/kohana/error.php b/modules/gallery/views/kohana/error.php index 7271db14..26628cf2 100644 --- a/modules/gallery/views/kohana/error.php +++ b/modules/gallery/views/kohana/error.php @@ -1,5 +1,6 @@ +