Permalink
Browse files

Merge pull request #459 from shadlaws/fixes_20130716_1

Fixes 20130716 1
  • Loading branch information...
2 parents 4d6f0d1 + 0197194 commit ed54e8f42ad9bf80e8905ab2ab5602588fb676e3 @shadlaws shadlaws committed Jul 16, 2013
@@ -43,8 +43,7 @@ public function url($path, $absolute_url=false) {
* - "collection_query_callback" (reqd) - callback which returns an ORM query for the
* collection's objects without limit or offset applied.
* - "breadcrumbs_callback" - callback which returns an array of Breadcrumb objects without
- * set_first or set_last applied. We use this as a semaphore that we have an *item*
- * collection, and set the item display context accordingly.
+ * set_first or set_last applied.
* - "collection_order_by" - array of order_by's to apply to the collection after its objects are
* counted. If not given, this is omitted. This is used in Controller_Search::action_index().
* - "page" - the current page. If not given, this is set from query parameters.
@@ -70,19 +69,19 @@ public function init_collection() {
$this->set_global("page_size", Module::get_var("gallery", "page_size", 9));
}
- // Get "collection_query" from its callback.
- $this->set_global("collection_query", call_user_func_array(
+ // Get "children_query" from its callback.
+ $this->set_global("children_query", call_user_func_array(
$this->collection_query_callback[0], $this->collection_query_callback[1]));
// Redirect if "show" query parameter is set.
if ($show = Request::current()->query("show")) {
// Apply "collection_order_by" if set (required for search module).
if (isset($this->collection_order_by)) {
- $this->collection_query->merge_order_by($this->collection_order_by);
+ $this->children_query->merge_order_by($this->collection_order_by);
}
$position = null;
- foreach ($this->collection_query->find_all() as $key => $child) {
+ foreach ($this->children_query->find_all() as $key => $child) {
if ($child->id == $show) {
$position = $key + 1; // 1-indexed position
break;
@@ -105,13 +104,13 @@ public function init_collection() {
}
// Get "children_count" before applying any order_by calls.
- $this->set_global("children_count", $this->collection_query
+ $this->set_global("children_count", $this->children_query
->reset(false)
->count_all());
// Apply "collection_order_by" if set (required for search module).
if (isset($this->collection_order_by)) {
- $this->collection_query->merge_order_by($this->collection_order_by);
+ $this->children_query->merge_order_by($this->collection_order_by);
}
// Set "max_pages" using other params.
@@ -125,23 +124,26 @@ public function init_collection() {
}
// Get "children" for the page.
- $this->set_global("children", $this->collection_query
+ $this->set_global("children", $this->children_query
->limit($this->page_size)
->offset($this->page_size * ($this->page - 1))
->reset(false)
->find_all());
- // See if we have "breadcrumb_callback" set. If so, we assume this is a type of
- // item display - get "breadcrumbs" and set the item display context.
+ // Get "breadcrumbs" if "breadcrumb_callback" is set.
if (!empty($this->breadcrumbs_callback)) {
$this->set_global("breadcrumbs", Breadcrumb::set_first_and_last(call_user_func_array(
$this->breadcrumbs_callback[0],
array_merge(array(null), $this->breadcrumbs_callback[1]))));
+ }
+ // See if we have an item collection. If so, set the item display context. We default to
+ // empty arrays here, and reset them as needed in View_Gallery::init_item().
+ if ($this->collection_type() == "item") {
Item::set_display_context(
$this->collection_query_callback,
- $this->breadcrumbs_callback,
- (empty($this->collection_order_by) ? array() : $this->collection_order_by));
+ (!empty($this->breadcrumbs_callback) ? $this->breadcrumbs_callback : array()),
+ (!empty($this->collection_order_by) ? $this->collection_order_by : array()));
}
return $this;
@@ -153,9 +155,9 @@ public function init_collection() {
* As inputs, this uses one view variable:
* - "item" (reqd) - item to be displayed.
* and the item display context:
- * - "sibling_query_callback" - same as the collection's "collection_query_callback"
- * - "breadcrumbs_callback" - same as the collection's "breadcrumbs_callback"
- * - "collection_order_by" - same as the collection's "collection_order_by"
+ * - "collection_query_callback"
+ * - "breadcrumbs_callback"
+ * - "collection_order_by"
* If the context isn't found or is invalid, it will be reset to that of the item's parent album.
*
* From these, this:
@@ -174,19 +176,19 @@ public function init_item() {
// Get the current display context. We default to the item's album if not found.
$context = Item::get_display_context();
- $sibling_query_callback = Arr::get($context, 0);
- $breadcrumbs_callback = Arr::get($context, 1);
- $collection_order_by = Arr::get($context, 2, array());
- if (!$sibling_query_callback || !$breadcrumbs_callback) {
+ $collection_query_callback = Arr::get($context, 0);
+ $breadcrumbs_callback = Arr::get($context, 1);
+ $collection_order_by = Arr::get($context, 2, array());
+ if (!$collection_query_callback || !$breadcrumbs_callback) {
$album = $this->item->parent;
- $sibling_query_callback = array("Item::get_album_query", array($album));
- $breadcrumbs_callback = array("Item::get_breadcrumbs", array($album));
+ $collection_query_callback = array("Controller_Items::get_album_query", array($album));
+ $breadcrumbs_callback = array("Controller_Items::get_breadcrumbs", array($album));
}
// Get "sibling_query" from its callback.
$this->set_global("sibling_query", call_user_func_array(
- $sibling_query_callback[0],
- $sibling_query_callback[1]));
+ $collection_query_callback[0],
+ $collection_query_callback[1]));
// Apply "collection_order_by" (required for search module), restrict query to non-albums.
$this->sibling_query
@@ -282,6 +284,16 @@ protected function _paginator_url($page=1, $absolute=false) {
}
/**
+ * Return the collection type (e.g. "item", "user", "comment"), or null if not a collection view.
+ */
+ public function collection_type() {
+ return (($this->page_type == "collection") &&
+ !empty($this->children_query) &&
+ ($this->children_query instanceof ORM)) ?
+ $this->children_query->object_name() : null;
+ }
+
+ /**
* Begin gather up scripts or css files so that they can be combined into a single request.
*
* @param $types a comma separated list of types to combine, eg "script,css"
@@ -29,7 +29,7 @@ public function action_index() {
}
/**
- * Show a tag's items. This finds the tag by its URL and generates a view.
+ * Show a tag's items. This finds the tag(s) by its URL and generates a view.
*/
public function action_show() {
// See if we got here via "tags/show/<id>" - if so, fire a 301.
@@ -41,50 +41,62 @@ public function action_show() {
$this->redirect($tag->abs_url(), 301);
}
+ // Get the route and look for slashes commas and/or slashes.
$tag_url = $this->request->param("tag_url");
- if (empty($tag_url)) {
- // @todo: this is the future home of the album of all tags. For now, we 404.
- throw HTTP_Exception::factory(404);
- }
-
- // See if we have a slash in the URL, which might be a Gallery 3.0.x canonical URL with
- // the form "tag/<id>/<name>" - if so, fire a 301.
if (($slash_pos = strpos($tag_url, "/")) !== false) {
+ // We have a slash in the URL, which might be a Gallery 3.0.x canonical URL with
+ // the form "tag/<id>/<name>" - if so, fire a 301; if not, fire a 404.
$tag_id = substr($tag_url, 0, $slash_pos);
$tag = ORM::factory("Tag", $tag_id);
if (!$tag->loaded()) {
throw HTTP_Exception::factory(404);
}
$this->redirect($tag->abs_url(), 301);
- }
+ } else if (empty($tag_url)) {
+ // @todo: this is the future home of the album of all tags. For now, we 404.
+ throw HTTP_Exception::factory(404);
+ } else {
+ // Find the tags by their canonical URLs, which have the form "tag/<slug_1>(,<slug_2>(,...))".
+ $slugs = array_unique(array_filter(explode(",", $tag_url)));
+
+ // Check for extra commas and duplicate tags and redirect as needed.
+ // (e.g. "tag/,foo,,bar,bar," --> "tag/foo,bar")
+ $filtered_url = implode(",", $slugs);
+ if ($tag_url !== $filtered_url) {
+ $this->redirect("tag/$filtered_url", 301);
+ }
- // Find the tag by its canonical URL, which has the form "tag(/<slug>)".
- $tag = ORM::factory("Tag")
- ->where("slug", "=", $tag_url)
- ->find();
- if (!$tag->loaded()) {
- // See if we have a numeric URL, which might be a malformed Gallery 3.0.x URL
- // with the form "tag/<id>" - if so, fire a 301.
- if (!preg_match("/[^0-9]/", $tag_url)) {
- $tag = ORM::factory("Tag", $tag_url);
- if ($tag->loaded()) {
- $this->redirect($tag->abs_url(), 301);
+ $tags = array();
+ for ($i = 0; $i < count($slugs); $i++) {
+ $tags[$i] = ORM::factory("Tag", array("slug" => $slugs[$i]));
+ if (!$tags[$i]->loaded()) {
+ if ($i == 0) {
+ // See if we have a numeric URL, which might be a malformed Gallery 3.0.x URL
+ // with the form "tag/<id>" - if so, fire a 301; if not, fire a 404.
+ if (!preg_match("/[^0-9]/", $slugs[$i])) {
+ $tag = ORM::factory("Tag", $slugs[$i]);
+ if ($tag->loaded()) {
+ $this->redirect($tag->abs_url(), 301);
+ }
+ }
+ }
+
+ throw HTTP_Exception::factory(404);
}
}
- throw HTTP_Exception::factory(404);
- }
-
- $root = Item::root();
- $template = new View_Theme("required/page.html", "collection", "tag");
- $template->set_global(array(
- "tag" => $tag,
- "collection_query_callback" => array("Controller_Tags::get_tag_query", array($tag)),
- "breadcrumbs_callback" => array("Controller_Tags::get_breadcrumbs", array($tag)),
- ));
- $template->init_collection();
- $template->content = new View("required/dynamic.html");
- $template->content->title = t("Tag: %tag_name", array("tag_name" => $tag->name));
+ $template = new View_Theme("required/page.html", "collection", "tag");
+ $template->set_global(array(
+ "tag" => $tags[0], // backward compatibility - @todo: remove the need for this.
+ "tags" => $tags,
+ "collection_query_callback" => array("Controller_Tags::get_tag_query", array($tags)),
+ "breadcrumbs_callback" => array("Controller_Tags::get_breadcrumbs", array($tags)),
+ ));
+ $template->init_collection();
+
+ $template->content = new View("required/dynamic.html");
+ $template->content->title = Tag::title($tags);
+ }
$this->response->body($template);
}
@@ -162,7 +174,7 @@ public function action_autocomplete() {
*/
public function action_find_by_name() {
$tag_name = $this->request->arg(0);
- $tag = ORM::factory("Tag")->where("name", "=", $tag_name)->find();
+ $tag = ORM::factory("Tag", array("name" => $tag_name));
if (!$tag->loaded()) {
throw HTTP_Exception::factory(404);
}
@@ -173,20 +185,51 @@ public function action_find_by_name() {
* Get the tag query for its collection view.
* @see Controller_Tags::action_show()
*/
- static function get_tag_query($tag) {
- return $tag->items->viewable();
+ static function get_tag_query($tags) {
+ if (count($tags) == 1) {
+ return $tags[0]->items->viewable();
+ }
+
+ // ORM doesn't handle multiple "has many through" relationships very easily,
+ // so we query the pivot table manually.
+ // @todo: try to do this in 1 query instead of 2 without tripping up count_all() and find_all().
+
+ $tag_ids = array();
+ foreach ($tags as $tag) {
+ $tag_ids[] = $tag->id;
+ }
+
+ // Find the item ids that are found in the pivot table with *every* tag id. We do a
+ // "where in...", which finds the "OR" set, then require that the count is the total number
+ // of tags, effectively making it an "AND" set.
+ $rows = DB::select("item_id")
+ ->distinct(true)
+ ->select(array(DB::expr("COUNT(\"*\")"), "C"))
+ ->from("items_tags")
+ ->where("tag_id", "IN", $tag_ids)
+ ->having("C", "=", count($tags))
+ ->group_by("item_id")
+ ->as_object()
+ ->execute();
+
+ $item_ids = array();
+ foreach ($rows as $row) {
+ $item_ids[] = $row->item_id;
+ }
+
+ // Return the ORM query.
+ return ORM::factory("Item")->where("item.id", "IN", $item_ids)->viewable();
}
/**
* Get the breadcrumbs for a tag.
* @see Controller_Tags::action_show()
*/
- static function get_breadcrumbs($item=null, $tag) {
+ static function get_breadcrumbs($item=null, $tags) {
$params = $item ? "show={$item->id}" : null;
$breadcrumbs = Breadcrumb::array_from_item_parents(Item::root());
- $breadcrumbs[] = Breadcrumb::factory(t("Tag: %tag_name", array("tag_name" => $tag->name)),
- $tag->url($params));
+ $breadcrumbs[] = Breadcrumb::factory(Tag::title($tags), Tag::url($tags, $params));
if ($item) {
$breadcrumbs[] = Breadcrumb::factory($item->title, $item->url());
}
@@ -22,7 +22,7 @@ class Tag_Hook_TagEvent {
* Initialization. This sets the tag routes.
*/
static function gallery_ready() {
- Route::set("tag", "tag(/<tag_url>)", array("tag_url" => "[A-Za-z0-9-_/]++"))
+ Route::set("tag", "tag(/<tag_url>)", array("tag_url" => "[A-Za-z0-9-_/,]++"))
->defaults(array(
"controller" => "tags",
"action" => "show"
@@ -130,4 +130,45 @@ static function add_from_metadata($item) {
}
}
}
+
+ /**
+ * Return the absolute url for an array of tags.
+ * @param array array of Model_Tag objects
+ * @param string (optional) query string (e.g. "page=3")
+ */
+ static function abs_url($tags, $query=null) {
+ $url = array_shift($tags)->abs_url();
+ foreach ($tags as $tag) {
+ $url .= ",{$tag->slug}";
+ }
+
+ return $url . ($query ? "?$query" : "");
+ }
+
+ /**
+ * Return the url for an array of tags.
+ * @param array array of Model_Tag objects
+ * @param string (optional) query string (e.g. "page=3")
+ */
+ static function url($tags, $query=null) {
+ $url = array_shift($tags)->url();
+ foreach ($tags as $tag) {
+ $url .= ",{$tag->slug}";
+ }
+
+ return $url . ($query ? "?$query" : "");
+ }
+
+ /**
+ * Return the title for an array of tags.
+ * @param array array of Model_Tag objects
+ */
+ static function title($tags) {
+ $name = array_shift($tags)->name;
+ foreach ($tags as $tag) {
+ $name .= ", {$tag->name}";
+ }
+
+ return t2("Tag: %tag_name", "Tags: %tag_name", count($tags) + 1, array("tag_name" => $name));
+ }
}
@@ -38,4 +38,28 @@ public function test_redirect_30x_url() {
$this->assertEquals($canonical_url, $redirected_url);
}
}
+
+ public function test_redirect_malformed_multitag_url() {
+ $name = Test::random_name();
+ $album = Test::random_album();
+ Tag::add($album, "{$name}1");
+ Tag::add($album, "{$name}2");
+ $tag1 = ORM::factory("Tag")->where("name", "=", "{$name}1")->find();
+ $tag2 = ORM::factory("Tag")->where("name", "=", "{$name}2")->find();
+
+ $urls = array(
+ "tag/,{$tag1->slug},{$tag2->slug}", // Leading comma
+ "tag/{$tag1->slug},{$tag2->slug},", // Trailing comma
+ "tag/{$tag1->slug},,{$tag2->slug},,", // Duplicate commas
+ "tag/{$tag1->slug},{$tag2->slug},{$tag1->slug}", // Duplicate tag
+ );
+
+ $canonical_url = Tag::abs_url(array($tag1, $tag2)); // Correct URL
+
+ foreach ($urls as $url) {
+ // Check that URL is redirected to canonical URL.
+ $redirected_url = Request::factory($url)->execute()->headers("Location");
+ $this->assertEquals($canonical_url, $redirected_url);
+ }
+ }
}

0 comments on commit ed54e8f

Please sign in to comment.