From cd48b89f3166e7fa732b5cb06d33fba018af9127 Mon Sep 17 00:00:00 2001
From: Bharat Mediratta <bharat@menalto.com>
Date: Wed, 15 Dec 2010 14:57:00 -0800
Subject: Consolidate all the random code into a random helper that offers:  
 random::hash()   random::string()   random::percent()   random::int()

So that we don't have lots of different ways to get random values all
over the code.  Follow-on to #1527.
---
 modules/gallery/helpers/item.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(limited to 'modules/gallery/helpers/item.php')

diff --git a/modules/gallery/helpers/item.php b/modules/gallery/helpers/item.php
index 052b1c8e..664da812 100644
--- a/modules/gallery/helpers/item.php
+++ b/modules/gallery/helpers/item.php
@@ -232,7 +232,7 @@ class item_Core {
     // distributed so this is going to be more efficient with larger data sets.
     return ORM::factory("item")
       ->viewable()
-      ->where("rand_key", "<", ((float)mt_rand()) / (float)mt_getrandmax())
+      ->where("rand_key", "<", random::percent())
       ->order_by("rand_key", "DESC");
   }
 }
\ No newline at end of file
-- 
cgit v1.2.3


From 48640005a4edac955d9087f62fed1ab5f756b686 Mon Sep 17 00:00:00 2001
From: Kriss Andsten <kriss@sverok.se>
Date: Tue, 21 Dec 2010 09:03:46 +0800
Subject: Packaging + tests of Bharat's find_by_path routine.

---
 modules/gallery/helpers/item.php           | 25 +++++++++++++++-
 modules/gallery/tests/Item_Helper_Test.php | 48 ++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 1 deletion(-)

(limited to 'modules/gallery/helpers/item.php')

diff --git a/modules/gallery/helpers/item.php b/modules/gallery/helpers/item.php
index 664da812..dbad59b9 100644
--- a/modules/gallery/helpers/item.php
+++ b/modules/gallery/helpers/item.php
@@ -208,7 +208,30 @@ class item_Core {
 
     return $model;
   }
-
+  
+  static function find_by_path($path) {
+    $path = trim($path, '/');
+    
+    // The root path name is NULL, not '', hence this workaround.
+    if ($path == '') {
+      return ORM::factory("item", 1);
+    }
+    
+    $paths = explode("/", $path);
+    $count = count($paths);
+    foreach (ORM::factory("item")
+             ->where('name', '=', $paths[$count - 1])
+             ->where('level', '=', $count + 1)
+             ->find_all() as $item) {
+      if (urldecode($item->relative_path()) == $path) {
+        return $item;
+      }
+    }
+    
+    return false;
+  }
+  
+  
   /**
    * Return the root Item_Model
    * @return Item_Model
diff --git a/modules/gallery/tests/Item_Helper_Test.php b/modules/gallery/tests/Item_Helper_Test.php
index 26db5a63..1fced654 100644
--- a/modules/gallery/tests/Item_Helper_Test.php
+++ b/modules/gallery/tests/Item_Helper_Test.php
@@ -125,4 +125,52 @@ class Item_Helper_Test extends Gallery_Unit_Test_Case {
     $this->assert_same($photo2->id, $album->album_cover_item_id);
     $this->assert_same($photo2->id, $parent->album_cover_item_id);
   }
+
+  public function find_by_path_does_the_right_thing_test() {
+    $level1 = test::random_album();
+    $level2 = test::random_album($level1);    
+    $level3 = test::random_photo($level2);
+    $level3->name = 'same.jpg';
+    $level3->save();
+    
+    $level2b = test::random_album($level1);
+    $level3b = test::random_photo($level2b);
+    $level3b->name = 'same.jpg';
+    $level3b->save();
+    
+    // Item in album
+    $this->assert_same(
+      item::find_by_path('/' . $level1->name . '/' . $level2->name . '/' . $level3->name)->id,
+      $level3->id);
+    
+    // Album, ends with a slash
+    $this->assert_same(
+      item::find_by_path($level1->name . '/' . $level2->name . '/')->id,
+      $level2->id);
+    
+    // Album, ends without a slash
+    $this->assert_same(
+      item::find_by_path('/' . $level1->name . '/' . $level2->name)->id,
+      $level2->id);
+    
+    // Return root if '' is passed
+    $this->assert_same(
+      item::find_by_path('')->id,
+      "1");
+
+    // Verify that we don't get confused by the part names
+    $this->assert_same(
+      item::find_by_path($level1->name . '/' . $level2->name . '/' . $level3->name)->id,
+      $level3->id);
+
+    $this->assert_same(
+      item::find_by_path($level1->name . '/' . $level2b->name . '/' . $level3b->name)->id,
+      $level3b->id);
+    
+    // Verify that we don't get false positives
+    $this->assert_same(
+      item::find_by_path('foo/bar/baz'),
+      false);
+
+  }
 }
-- 
cgit v1.2.3


From addd384bbdca6a9f066403c1d2919f3e863e072e Mon Sep 17 00:00:00 2001
From: Kriss Andsten <kriss@sverok.se>
Date: Wed, 22 Dec 2010 07:55:26 +0800
Subject: Minor changes to satisfy the G3 code standards.

---
 modules/gallery/helpers/item.php           | 15 ++++++++++-----
 modules/gallery/tests/Item_Helper_Test.php | 18 +++++++++---------
 2 files changed, 19 insertions(+), 14 deletions(-)

(limited to 'modules/gallery/helpers/item.php')

diff --git a/modules/gallery/helpers/item.php b/modules/gallery/helpers/item.php
index dbad59b9..f38d9888 100644
--- a/modules/gallery/helpers/item.php
+++ b/modules/gallery/helpers/item.php
@@ -208,20 +208,25 @@ class item_Core {
 
     return $model;
   }
-  
+
+  /**
+   * Return an item by path.
+   * @param string $path
+   * @return object item
+   */
   static function find_by_path($path) {
-    $path = trim($path, '/');
+    $path = trim($path, "/");
     
     // The root path name is NULL, not '', hence this workaround.
     if ($path == '') {
-      return ORM::factory("item", 1);
+      return ORM::factory("item", item::root());
     }
     
     $paths = explode("/", $path);
     $count = count($paths);
     foreach (ORM::factory("item")
-             ->where('name', '=', $paths[$count - 1])
-             ->where('level', '=', $count + 1)
+             ->where("name", "=", $paths[$count - 1])
+             ->where("level", "=", $count + 1)
              ->find_all() as $item) {
       if (urldecode($item->relative_path()) == $path) {
         return $item;
diff --git a/modules/gallery/tests/Item_Helper_Test.php b/modules/gallery/tests/Item_Helper_Test.php
index 1fced654..4bc64ff0 100644
--- a/modules/gallery/tests/Item_Helper_Test.php
+++ b/modules/gallery/tests/Item_Helper_Test.php
@@ -130,46 +130,46 @@ class Item_Helper_Test extends Gallery_Unit_Test_Case {
     $level1 = test::random_album();
     $level2 = test::random_album($level1);    
     $level3 = test::random_photo($level2);
-    $level3->name = 'same.jpg';
+    $level3->name = "same.jpg";
     $level3->save();
     
     $level2b = test::random_album($level1);
     $level3b = test::random_photo($level2b);
-    $level3b->name = 'same.jpg';
+    $level3b->name = "same.jpg";
     $level3b->save();
     
     // Item in album
     $this->assert_same(
-      item::find_by_path('/' . $level1->name . '/' . $level2->name . '/' . $level3->name)->id,
+      item::find_by_path("/" . $level1->name . "/" . $level2->name . "/" . $level3->name)->id,
       $level3->id);
     
     // Album, ends with a slash
     $this->assert_same(
-      item::find_by_path($level1->name . '/' . $level2->name . '/')->id,
+      item::find_by_path($level1->name . "/" . $level2->name . "/")->id,
       $level2->id);
     
     // Album, ends without a slash
     $this->assert_same(
-      item::find_by_path('/' . $level1->name . '/' . $level2->name)->id,
+      item::find_by_path("/" . $level1->name . "/" . $level2->name)->id,
       $level2->id);
     
     // Return root if '' is passed
     $this->assert_same(
-      item::find_by_path('')->id,
+      item::find_by_path("")->id,
       "1");
 
     // Verify that we don't get confused by the part names
     $this->assert_same(
-      item::find_by_path($level1->name . '/' . $level2->name . '/' . $level3->name)->id,
+      item::find_by_path($level1->name . "/" . $level2->name . "/" . $level3->name)->id,
       $level3->id);
 
     $this->assert_same(
-      item::find_by_path($level1->name . '/' . $level2b->name . '/' . $level3b->name)->id,
+      item::find_by_path($level1->name . "/" . $level2b->name . "/" . $level3b->name)->id,
       $level3b->id);
     
     // Verify that we don't get false positives
     $this->assert_same(
-      item::find_by_path('foo/bar/baz'),
+      item::find_by_path("foo/bar/baz"),
       false);
 
   }
-- 
cgit v1.2.3


From f493130e59f26d41f090c5ca40e95b416b9b154b Mon Sep 17 00:00:00 2001
From: Bharat Mediratta <bharat@menalto.com>
Date: Tue, 21 Dec 2010 16:55:01 -0800
Subject: Tighten up item::find_by_path slightly.  Augment the tests to cover
 special characters in the file name ("+" is an edge case differentiator
 between rawurlencode and urlencode).

---
 modules/gallery/helpers/item.php           | 25 ++++++++++++-------------
 modules/gallery/tests/Item_Helper_Test.php |  2 +-
 2 files changed, 13 insertions(+), 14 deletions(-)

(limited to 'modules/gallery/helpers/item.php')

diff --git a/modules/gallery/helpers/item.php b/modules/gallery/helpers/item.php
index f38d9888..3596a2bf 100644
--- a/modules/gallery/helpers/item.php
+++ b/modules/gallery/helpers/item.php
@@ -210,33 +210,32 @@ class item_Core {
   }
 
   /**
-   * Return an item by path.
+   * Find an item by its path.  If there's no match, return an empty Item_Model.
    * @param string $path
-   * @return object item
+   * @return object Item_Model
    */
   static function find_by_path($path) {
     $path = trim($path, "/");
-    
-    // The root path name is NULL, not '', hence this workaround.
-    if ($path == '') {
-      return ORM::factory("item", item::root());
+
+    // The root path name is NULL not "", hence this workaround.
+    if ($path == "") {
+      return item::root();
     }
-    
+
     $paths = explode("/", $path);
-    $count = count($paths);
     foreach (ORM::factory("item")
-             ->where("name", "=", $paths[$count - 1])
-             ->where("level", "=", $count + 1)
+             ->where("name", "=", end($paths))
+             ->where("level", "=", count($paths) + 1)
              ->find_all() as $item) {
       if (urldecode($item->relative_path()) == $path) {
         return $item;
       }
     }
-    
+
     return false;
   }
-  
-  
+
+
   /**
    * Return the root Item_Model
    * @return Item_Model
diff --git a/modules/gallery/tests/Item_Helper_Test.php b/modules/gallery/tests/Item_Helper_Test.php
index d60380f0..4124e453 100644
--- a/modules/gallery/tests/Item_Helper_Test.php
+++ b/modules/gallery/tests/Item_Helper_Test.php
@@ -135,7 +135,7 @@ class Item_Helper_Test extends Gallery_Unit_Test_Case {
 
     $level2b = test::random_album($level1);
     $level3b = test::random_photo($level2b);
-    $level3b->name = "same.jpg";
+    $level3b->name = "has spaces+plusses.jpg";
     $level3b->save();
 
     // Item in album
-- 
cgit v1.2.3


From 2a08cbf76da0f9984c0e182e6c448b516d8d7db3 Mon Sep 17 00:00:00 2001
From: Bharat Mediratta <bharat@menalto.com>
Date: Tue, 21 Dec 2010 16:58:54 -0800
Subject: Return an empty Item_Model when item::find_by_path fails

---
 modules/gallery/helpers/item.php           | 2 +-
 modules/gallery/tests/Item_Helper_Test.php | 5 ++---
 2 files changed, 3 insertions(+), 4 deletions(-)

(limited to 'modules/gallery/helpers/item.php')

diff --git a/modules/gallery/helpers/item.php b/modules/gallery/helpers/item.php
index 3596a2bf..08a04ad0 100644
--- a/modules/gallery/helpers/item.php
+++ b/modules/gallery/helpers/item.php
@@ -232,7 +232,7 @@ class item_Core {
       }
     }
 
-    return false;
+    return new Item_Model();
   }
 
 
diff --git a/modules/gallery/tests/Item_Helper_Test.php b/modules/gallery/tests/Item_Helper_Test.php
index 4124e453..0aa7504e 100644
--- a/modules/gallery/tests/Item_Helper_Test.php
+++ b/modules/gallery/tests/Item_Helper_Test.php
@@ -166,8 +166,7 @@ class Item_Helper_Test extends Gallery_Unit_Test_Case {
       item::find_by_path("{$level1->name}/{$level2b->name}/{$level3b->name}")->id);
 
     // Verify that we don't get false positives
-    $this->assert_same(
-      false,
-      item::find_by_path("foo/bar/baz"));
+    $this->assert_false(
+      item::find_by_path("foo/bar/baz")->loaded());
   }
 }
-- 
cgit v1.2.3


From d9299f3b3f4b1a52f5b68399cfcaa96d5b367899 Mon Sep 17 00:00:00 2001
From: Bharat Mediratta <bharat@menalto.com>
Date: Tue, 21 Dec 2010 19:33:47 -0800
Subject: Change item::find_by_path() to check the relative_path_cache first,
 and only fall back the name/level comparison if there's no cached entry. 
 Update tests accordingly.

---
 modules/gallery/helpers/item.php           | 16 ++++++++++++++++
 modules/gallery/tests/Item_Helper_Test.php | 20 ++++++++++++++++----
 2 files changed, 32 insertions(+), 4 deletions(-)

(limited to 'modules/gallery/helpers/item.php')

diff --git a/modules/gallery/helpers/item.php b/modules/gallery/helpers/item.php
index 08a04ad0..bac189f4 100644
--- a/modules/gallery/helpers/item.php
+++ b/modules/gallery/helpers/item.php
@@ -211,6 +211,7 @@ class item_Core {
 
   /**
    * Find an item by its path.  If there's no match, return an empty Item_Model.
+   * NOTE: the caller is responsible for performing security checks on the resulting item.
    * @param string $path
    * @return object Item_Model
    */
@@ -222,6 +223,21 @@ class item_Core {
       return item::root();
     }
 
+    // Check to see if there's an item in the database with a matching relative_path_cache value.
+    // Since that field is urlencoded, we must urlencoded the components of the path.
+    foreach (explode("/", $path) as $part) {
+      $encoded_array[] = rawurlencode($part);
+    }
+    $encoded_path = join("/", $encoded_array);
+    $item = ORM::factory("item")
+      ->where("relative_path_cache", "=", $encoded_path)
+      ->find();
+    if ($item->loaded()) {
+      return $item;
+    }
+
+    // Since the relative_path_cache field is a cache, it can be unavailable.  If we don't find
+    // anything, fall back to checking the path the hard way.
     $paths = explode("/", $path);
     foreach (ORM::factory("item")
              ->where("name", "=", end($paths))
diff --git a/modules/gallery/tests/Item_Helper_Test.php b/modules/gallery/tests/Item_Helper_Test.php
index 0aa7504e..13ecec2b 100644
--- a/modules/gallery/tests/Item_Helper_Test.php
+++ b/modules/gallery/tests/Item_Helper_Test.php
@@ -129,14 +129,21 @@ class Item_Helper_Test extends Gallery_Unit_Test_Case {
   public function find_by_path_test() {
     $level1 = test::random_album();
     $level2 = test::random_album($level1);
-    $level3 = test::random_photo($level2);
+    $level3 = test::random_photo_unsaved($level2);
     $level3->name = "same.jpg";
-    $level3->save();
+    $level3->save()->reload();
 
     $level2b = test::random_album($level1);
-    $level3b = test::random_photo($level2b);
+    $level3b = test::random_photo_unsaved($level2b);
     $level3b->name = "has spaces+plusses.jpg";
-    $level3b->save();
+    $level3b->save()->reload();
+
+    // Make sure that some of the calls below use the fallback code.
+    db::build()
+      ->update("items")
+      ->set(array("relative_url_cache" => null, "relative_path_cache" => null))
+      ->where("id", "IN", array($level3->id, $level3b->id))
+      ->execute();
 
     // Item in album
     $this->assert_same(
@@ -168,5 +175,10 @@ class Item_Helper_Test extends Gallery_Unit_Test_Case {
     // Verify that we don't get false positives
     $this->assert_false(
       item::find_by_path("foo/bar/baz")->loaded());
+
+    // Verify that the fallback code works
+    $this->assert_same(
+      $level3b->id,
+      item::find_by_path("{$level1->name}/{$level2b->name}/{$level3b->name}")->id);
   }
 }
-- 
cgit v1.2.3


From 98fd1e9957ff0d65d1bbb0eaa2df6c1e59487b25 Mon Sep 17 00:00:00 2001
From: Bharat Mediratta <bharat@menalto.com>
Date: Tue, 21 Dec 2010 20:47:07 -0800
Subject: Implement item::find_by_relative_url with tests.

---
 modules/gallery/helpers/item.php           | 26 +++++++++++
 modules/gallery/tests/Item_Helper_Test.php | 70 +++++++++++++++++++++++++-----
 2 files changed, 86 insertions(+), 10 deletions(-)

(limited to 'modules/gallery/helpers/item.php')

diff --git a/modules/gallery/helpers/item.php b/modules/gallery/helpers/item.php
index bac189f4..29dd8603 100644
--- a/modules/gallery/helpers/item.php
+++ b/modules/gallery/helpers/item.php
@@ -252,6 +252,32 @@ class item_Core {
   }
 
 
+  /**
+   * Locate an item using the URL.  We assume that the url is in the form /a/b/c where each
+   * component matches up with an item slug.  If there's no match, return an empty Item_Model
+   * NOTE: the caller is responsible for performing security checks on the resulting item.
+   * @param string $url the relative url fragment
+   * @return Item_Model
+   */
+  static function find_by_relative_url($relative_url) {
+    // In most cases, we'll have an exact match in the relative_url_cache item field.
+    // but failing that, walk down the tree until we find it.  The fallback code will fix caches
+    // as it goes, so it'll never be run frequently.
+    $item = ORM::factory("item")->where("relative_url_cache", "=", $relative_url)->find();
+    if (!$item->loaded()) {
+      $segments = explode("/", $relative_url);
+      foreach (ORM::factory("item")
+               ->where("slug", "=", end($segments))
+               ->where("level", "=", count($segments) + 1)
+               ->find_all() as $match) {
+        if ($match->relative_url() == $relative_url) {
+          $item = $match;
+        }
+      }
+    }
+    return $item;
+  }
+
   /**
    * Return the root Item_Model
    * @return Item_Model
diff --git a/modules/gallery/tests/Item_Helper_Test.php b/modules/gallery/tests/Item_Helper_Test.php
index 13ecec2b..42acfb18 100644
--- a/modules/gallery/tests/Item_Helper_Test.php
+++ b/modules/gallery/tests/Item_Helper_Test.php
@@ -128,23 +128,19 @@ class Item_Helper_Test extends Gallery_Unit_Test_Case {
 
   public function find_by_path_test() {
     $level1 = test::random_album();
-    $level2 = test::random_album($level1);
+    $level2 = test::random_album_unsaved($level1);
+    $level2->name = "plus + space";
+    $level2->save()->reload();
+
     $level3 = test::random_photo_unsaved($level2);
     $level3->name = "same.jpg";
     $level3->save()->reload();
 
     $level2b = test::random_album($level1);
     $level3b = test::random_photo_unsaved($level2b);
-    $level3b->name = "has spaces+plusses.jpg";
+    $level3b->name = "same.jpg";
     $level3b->save()->reload();
 
-    // Make sure that some of the calls below use the fallback code.
-    db::build()
-      ->update("items")
-      ->set(array("relative_url_cache" => null, "relative_path_cache" => null))
-      ->where("id", "IN", array($level3->id, $level3b->id))
-      ->execute();
-
     // Item in album
     $this->assert_same(
       $level3->id,
@@ -163,7 +159,12 @@ class Item_Helper_Test extends Gallery_Unit_Test_Case {
     // Return root if "" is passed
     $this->assert_same(item::root()->id, item::find_by_path("")->id);
 
-    // Verify that we don't get confused by the part names
+    // Verify that we don't get confused by the part names, using the fallback code.
+    db::build()
+      ->update("items")
+      ->set(array("relative_path_cache" => null))
+      ->where("id", "IN", array($level3->id, $level3b->id))
+      ->execute();
     $this->assert_same(
       $level3->id,
       item::find_by_path("{$level1->name}/{$level2->name}/{$level3->name}")->id);
@@ -181,4 +182,53 @@ class Item_Helper_Test extends Gallery_Unit_Test_Case {
       $level3b->id,
       item::find_by_path("{$level1->name}/{$level2b->name}/{$level3b->name}")->id);
   }
+
+  public function find_by_relative_url_test() {
+    $level1 = test::random_album();
+    $level2 = test::random_album($level1);
+    $level3 = test::random_photo_unsaved($level2);
+    $level3->slug = "same";
+    $level3->save()->reload();
+
+    $level2b = test::random_album($level1);
+    $level3b = test::random_photo_unsaved($level2b);
+    $level3b->slug = "same";
+    $level3b->save()->reload();
+
+    // Item in album
+    $this->assert_same(
+      $level3->id,
+      item::find_by_relative_url("{$level1->slug}/{$level2->slug}/{$level3->slug}")->id);
+
+    // Album, ends without a slash
+    $this->assert_same(
+      $level2->id,
+      item::find_by_relative_url("{$level1->slug}/{$level2->slug}")->id);
+
+    // Return root if "" is passed
+    $this->assert_same(item::root()->id, item::find_by_relative_url("")->id);
+
+    // Verify that we don't get confused by the part slugs, using the fallback code.
+    db::build()
+      ->update("items")
+      ->set(array("relative_url_cache" => null))
+      ->where("id", "IN", array($level3->id, $level3b->id))
+      ->execute();
+    $this->assert_same(
+      $level3->id,
+      item::find_by_relative_url("{$level1->slug}/{$level2->slug}/{$level3->slug}")->id);
+
+    $this->assert_same(
+      $level3b->id,
+      item::find_by_relative_url("{$level1->slug}/{$level2b->slug}/{$level3b->slug}")->id);
+
+    // Verify that we don't get false positives
+    $this->assert_false(
+      item::find_by_relative_url("foo/bar/baz")->loaded());
+
+    // Verify that the fallback code works
+    $this->assert_same(
+      $level3b->id,
+      item::find_by_relative_url("{$level1->slug}/{$level2b->slug}/{$level3b->slug}")->id);
+  }
 }
-- 
cgit v1.2.3