Change all access API methods to take ORMs instead of ids. This will

minimize reloading objects from the database.

8 files changed, 126 insertions, 114 deletions

diff --git a/core/controllers/albums.php b/core/controllers/albums.php
index bba7fd6e..1977003b 100644
--- a/core/controllers/albums.php
+++ b/core/controllers/albums.php
@@ -23,7 +23,7 @@ class Albums_Controller extends Items_Controller {
    *  @see Rest_Controller::_show($resource)
    */
   public function _show($item) {
-    if (!access::can("view", $item->id)) {
+    if (!access::can("view", $item)) {
       return Kohana::show_404();
     }
 
diff --git a/core/controllers/photos.php b/core/controllers/photos.php
index 8b3e81fc..c72fae12 100644
--- a/core/controllers/photos.php
+++ b/core/controllers/photos.php
@@ -23,7 +23,7 @@ class Photos_Controller extends Items_Controller {
    *  @see Rest_Controller::_show($resource)
    */
   public function _show($item) {
-    if (!access::can("view", $item->id)) {
+    if (!access::can("view", $item)) {
       return Kohana::show_404();
     }
 
diff --git a/core/controllers/welcome.php b/core/controllers/welcome.php
index 2839b2f1..dacaa87f 100644
--- a/core/controllers/welcome.php
+++ b/core/controllers/welcome.php
@@ -22,13 +22,11 @@ class Welcome_Controller extends Template_Controller {
 
   function index() {
     Session::instance();
-
     $this->template->syscheck = new View("welcome_syscheck.html");
     $this->template->syscheck->errors = $this->_get_config_errors();
     $this->template->syscheck->modules = array();
 
     $old_handler = set_error_handler(array("Welcome_Controller", "_error_handler"));
-
     try {
       $this->template->syscheck->modules = $this->_read_modules();
       $this->template->album_count = ORM::factory("item")->where("type", "album")->count_all();
@@ -101,20 +99,18 @@ class Welcome_Controller extends Template_Controller {
             try {
               call_user_func(array("{$module->name}_installer", "uninstall"));
             } catch (Exception $e) {
-              $clean = false;
             }
           }
         }
       } catch (Exception $e) { }
-      set_error_handler($old_handler);
-      if (!$clean) {
-        // Since we're in a state of flux, it's possible that other stuff went wrong with the
-        // uninstall, so back off and nuke things from orbit.
-        $db = Database::instance();
-        foreach ($db->list_tables() as $table) {
-          $db->query("DROP TABLE `$table`");
-        }
+
+      // Since we're in a state of flux, it's possible that other stuff went wrong with the
+      // uninstall, so back off and nuke it from orbit.  It's the only way to be sure.
+      $db = Database::instance();
+      foreach ($db->list_tables() as $table) {
+        $db->query("DROP TABLE `$table`");
       }
+      set_error_handler($old_handler);
     }
     call_user_func(array("{$module_name}_installer", "uninstall"));
     url::redirect("welcome");
@@ -488,18 +484,20 @@ class Welcome_Controller extends Template_Controller {
   }
 
   public function add_perm($group_id, $perm, $item_id) {
-    access::allow($group_id, $perm, $item_id);
+    access::allow(ORM::factory("group", $group_id), $perm, ORM::factory("item", $item_id));
     url::redirect("welcome");
   }
 
   public function deny_perm($group_id, $perm, $item_id) {
-    access::deny($group_id, $perm, $item_id);
+    access::deny(ORM::factory("group", $group_id), $perm, ORM::factory("item", $item_id));
     url::redirect("welcome");
   }
 
   public function reset_all_perms($group_id, $item_id) {
+    $group = ORM::factory("group", $group_id);
+    $item = ORM::factory("item", $item_id);
     foreach (ORM::factory("permission")->find_all() as $perm) {
-      access::reset($group_id, $perm->name, $item_id);
+      access::reset($group, $perm->name, $item);
     }
     url::redirect("welcome");
   }
diff --git a/core/helpers/access.php b/core/helpers/access.php
index c21583a8..57fad3c0 100644
--- a/core/helpers/access.php
+++ b/core/helpers/access.php
@@ -73,33 +73,34 @@ class access_Core {
   /**
    * Does this group have this permission on this item?
    *
-   * @param  integer $group_id
-   * @param  string  $perm_name
-   * @param  integer $item_id
+   * @param  Group_Model $group
+   * @param  string      $perm_name
+   * @param  Item_Model  $item
    * @return boolean
    */
-  public static function group_can($group_id, $perm_name, $item_id) {
-    $access = ORM::factory("access_cache")->where("item_id", $item_id)->find();
+  public static function group_can($group, $perm_name, $item) {
+    $access = ORM::factory("access_cache")->where("item_id", $item->id)->find();
     if (!$access) {
-      throw new Exception("@todo MISSING_ACCESS for $item_id");
+      throw new Exception("@todo MISSING_ACCESS for $item->id");
     }
 
-    return $access->__get("{$perm_name}_{$group_id}") === self::ALLOW;
+    $group_id = $group ? $group->id : 0;
+    return $access->__get("{$perm_name}_$group_id") === self::ALLOW;
   }
 
   /**
    * Does the active user have this permission on this item?
    *
-   * @param  string  $perm_name
-   * @param  integer $item_id
+   * @param  string     $perm_name
+   * @param  Item_Model $item
    * @return boolean
    */
-  public static function can($perm_name, $item_id) {
+  public static function can($perm_name, $item) {
     $user = Session::instance()->get("user", null);
     if ($user) {
-      $access = ORM::factory("access_cache")->where("item_id", $item_id)->find();
+      $access = ORM::factory("access_cache")->where("item_id", $item->id)->find();
       if (!$access) {
-        throw new Exception("@todo MISSING_ACCESS for $item_id");
+        throw new Exception("@todo MISSING_ACCESS for $item->id");
       }
 
       if ($access->view_0 == self::ALLOW) {
@@ -112,64 +113,69 @@ class access_Core {
       }
       return false;
     } else {
-      return self::group_can(group::EVERYBODY, $perm_name, $item_id);
+      return self::group_can(group::EVERYBODY, $perm_name, $item->id);
     }
   }
 
   /**
    * Internal method to set a permission
    *
-   * @param  integer $group_id
+   * @param  Group_Model $group
    * @param  string  $perm_name
-   * @param  integer $item_id
+   * @param  Item_Model $item
    * @param  boolean $value
    * @return boolean
    */
-  private static function _set($group_id, $perm_name, $item_id, $value) {
-    $access = ORM::factory("access_intent")->where("item_id", $item_id)->find();
+  private static function _set($group, $perm_name, $item, $value) {
+    $access = ORM::factory("access_intent")->where("item_id", $item->id)->find();
     if (!$access->loaded) {
-      throw new Exception("@todo MISSING_ACCESS for $item_id");
+      throw new Exception("@todo MISSING_ACCESS for $item->id");
     }
 
-    $access->__set("{$perm_name}_{$group_id}", $value);
+    $group_id = $group ? $group->id : 0;
+    $access->__set("{$perm_name}_$group_id", $value);
     $access->save();
 
-    self::_update_access_cache($group_id, $perm_name, $item_id);
+    self::_update_access_cache($group, $perm_name, $item);
   }
 
   /**
    * Allow a group to have a permission on an item.
    *
-   * @param  integer $group_id
+   * @param  Group_Model $group
    * @param  string  $perm_name
-   * @param  integer $item_id
+   * @param  Item_Model $item
    * @return boolean
    */
-  public static function allow($group_id, $perm_name, $item_id) {
-    self::_set($group_id, $perm_name, $item_id, self::ALLOW);
+  public static function allow($group, $perm_name, $item) {
+    self::_set($group, $perm_name, $item, self::ALLOW);
   }
 
   /**
    * Deny a group the given permission on an item.
    *
-   * @param  integer $group_id
+   * @param  Group_Model $group
    * @param  string  $perm_name
-   * @param  integer $item_id
+   * @param  Item_Model $item
    * @return boolean
    */
-  public static function deny($group_id, $perm_name, $item_id) {
-    self::_set($group_id, $perm_name, $item_id, self::DENY);
+  public static function deny($group, $perm_name, $item) {
+    self::_set($group, $perm_name, $item, self::DENY);
   }
 
   /**
    * Unset the given permission for this item and use inherited values
    *
+   * @param  Group_Model $group
+   * @param  string  $perm_name
+   * @param  Item_Model $item
+   * @return boolean
    */
-  public static function reset($group_id, $perm_name, $item_id) {
-    if ($item_id == 1) {
+  public static function reset($group, $perm_name, $item) {
+    if ($item->id == 1) {
       throw new Exception("@todo CANT_RESET_ROOT_PERMISSION");
     }
-    self::_set($group_id, $perm_name, $item_id, null);
+    self::_set($group, $perm_name, $item, null);
   }
 
   /**
@@ -187,9 +193,9 @@ class access_Core {
     $permission->save();
 
     foreach (self::_get_all_groups() as $group) {
-      self::_add_columns($perm_name, $group->id);
+      self::_add_columns($perm_name, $group);
     }
-    self::_add_columns($perm_name, 0);
+    self::_add_columns($perm_name, null);
   }
 
   /**
@@ -200,9 +206,9 @@ class access_Core {
    */
   public static function delete_permission($name) {
     foreach (self::_get_all_groups() as $group) {
-      self::_drop_columns($name, $group->id);
+      self::_drop_columns($name, $group);
     }
-    self::_drop_columns($name, 0);
+    self::_drop_columns($name, null);
     $permission = ORM::factory("permission")->where("name", $name)->find();
     if ($permission->loaded) {
       $permission->delete();
@@ -217,7 +223,7 @@ class access_Core {
    */
   public static function add_group($group) {
     foreach (ORM::factory("permission")->find_all() as $perm) {
-      self::_add_columns($perm->name, $group->id);
+      self::_add_columns($perm->name, $group);
     }
   }
 
@@ -229,7 +235,7 @@ class access_Core {
    */
   public static function delete_group($group) {
     foreach (ORM::factory("permission")->find_all() as $perm) {
-      self::_drop_columns($perm->name, $group->id);
+      self::_drop_columns($perm->name, $group);
     }
   }
 
@@ -287,13 +293,14 @@ class access_Core {
   /**
    * Internal method to  remove Permission/Group columns
    *
-   * @param  integer $group_id
-   * @param  string  $perm_name
+   * @param  Group_Model $group
+   * @param  string      $perm_name
    * @return void
    */
-  private static function _drop_columns($perm_name, $group_id) {
+  private static function _drop_columns($perm_name, $group) {
+    $group_id = $group ? $group->id : 0;
     $db = Database::instance();
-    $field = "{$perm_name}_{$group_id}";
+    $field = "{$perm_name}_$group_id";
     $db->query("ALTER TABLE `access_caches` DROP `$field`");
     $db->query("ALTER TABLE `access_intents` DROP `$field`");
   }
@@ -301,13 +308,14 @@ class access_Core {
   /**
    * Internal method to add Permission/Group columns
    *
-   * @param  integer $group_id
+   * @param  Group_Model $group
    * @param  string  $perm_name
    * @return void
    */
-  private static function _add_columns($perm_name, $group_id) {
+  private static function _add_columns($perm_name, $group) {
+    $group_id = $group ? $group->id : 0;
     $db = Database::instance();
-    $field = "{$perm_name}_{$group_id}";
+    $field = "{$perm_name}_$group_id";
     $db->query("ALTER TABLE `access_caches` ADD `$field` TINYINT(2) NOT NULL DEFAULT 0");
     $db->query("ALTER TABLE `access_intents` ADD `$field` BOOLEAN DEFAULT NULL");
   }
@@ -319,20 +327,17 @@ class access_Core {
    *
    * @todo: use database locking
    *
-   * @param  integer $group_id
+   * @param  Group_Model $group
    * @param  string  $perm_name
-   * @param  integer $item_id
+   * @param  Item_Model $item
    * @return void
    */
-  public static function _update_access_cache($group_id, $perm_name, $item_id) {
-    $item = ORM::factory("item", $item_id);
-    if (!$item->loaded) {
-      throw new Exception("@todo MISSING_ITEM for $item_id");
-    }
-    $access = ORM::factory("access_intent")->where("item_id", $item_id)->find();
+  public static function _update_access_cache($group, $perm_name, $item) {
+    $access = ORM::factory("access_intent")->where("item_id", $item->id)->find();
 
+    $group_id = $group ? $group->id : 0;
     $db = Database::instance();
-    $field = "{$perm_name}_{$group_id}";
+    $field = "{$perm_name}_$group_id";
 
     if ($perm_name == "view") {
       // With view permissions, deny values in the parent can override allow values in the child,
diff --git a/core/helpers/core_block.php b/core/helpers/core_block.php
index 5bc469b5..45cc2026 100644
--- a/core/helpers/core_block.php
+++ b/core/helpers/core_block.php
@@ -29,7 +29,7 @@ class core_block_Core {
       $profiler->render();
     }
 
-    if (access::can("edit", $theme->item()->id)) {
+    if (access::can("edit", $theme->item())) {
       return new View("in_place_edit.html");
     }
   }
diff --git a/core/helpers/core_installer.php b/core/helpers/core_installer.php
index e8aa0d24..052b9cb5 100644
--- a/core/helpers/core_installer.php
+++ b/core/helpers/core_installer.php
@@ -101,8 +101,8 @@ class core_installer {
         ->save();
 
       access::add_item($root);
-      access::allow(0, "view", $root->id);
-      access::deny(0, "edit", $root->id);
+      access::allow(0, "view", $root);
+      access::deny(0, "edit", $root);
 
       module::set_version("core", 1);
     }
diff --git a/core/tests/Access_Helper_Test.php b/core/tests/Access_Helper_Test.php
index 41f2770e..066b0a08 100644
--- a/core/tests/Access_Helper_Test.php
+++ b/core/tests/Access_Helper_Test.php
@@ -81,36 +81,36 @@ class Access_Helper_Test extends Unit_Test_Case {
     $root = ORM::factory("item", 1);
     $item = ORM::factory("item")->add_to_parent($root);
     access::add_item($item);
-    $intent = ORM::factory("access_intent")->where("item_id", $item->id)->find();
+    $intent = ORM::factory("access_intent")->where("item_id", $item)->find();
 
     // Allow
-    access::allow(0, "view", $item->id);
+    access::allow(0, "view", $item);
     $this->assert_same(access::ALLOW, $intent->reload()->view_0);
 
     // Deny
-    access::deny(0, "view", $item->id);
+    access::deny(0, "view", $item);
     $this->assert_same(
       access::DENY,
-      ORM::factory("access_intent")->where("item_id", $item->id)->find()->view_0);
+      ORM::factory("access_intent")->where("item_id", $item)->find()->view_0);
 
     // Allow again.  If the initial value was allow, then the first Allow clause above may not
     // have actually changed any values.
-    access::allow(0, "view", $item->id);
+    access::allow(0, "view", $item);
     $this->assert_same(
       access::ALLOW,
-      ORM::factory("access_intent")->where("item_id", $item->id)->find()->view_0);
+      ORM::factory("access_intent")->where("item_id", $item)->find()->view_0);
 
-    access::reset(0, "view", $item->id);
+    access::reset(0, "view", $item);
     $this->assert_same(
       null,
-      ORM::factory("access_intent")->where("item_id", $item->id)->find()->view_0);
+      ORM::factory("access_intent")->where("item_id", $item)->find()->view_0);
 
     $item->delete();
   }
 
   public function cant_reset_root_item_test() {
     try {
-      access::reset(0, "view", 1);
+      access::reset(0, "view", ORM::factory("item", 1));
     } catch (Exception $e) {
       return;
     }
@@ -120,8 +120,8 @@ class Access_Helper_Test extends Unit_Test_Case {
 
   public function can_view_item_test() {
     $root = ORM::factory("item", 1);
-    access::allow(0, "view", $root->id);
-    $this->assert_true(access::group_can(0, "view", $root->id));
+    access::allow(0, "view", $root);
+    $this->assert_true(access::group_can(0, "view", $root));
   }
 
   public function cant_view_child_of_hidden_parent_test() {
@@ -129,9 +129,9 @@ class Access_Helper_Test extends Unit_Test_Case {
     $album = ORM::factory("item")->add_to_parent($root);
     access::add_item($album);
 
-    access::deny(0, "view", $root->id);
-    access::reset(0, "view", $album->id);
-    $this->assert_false(access::group_can(0, "view", $album->id));
+    access::deny(0, "view", $root);
+    access::reset(0, "view", $album);
+    $this->assert_false(access::group_can(0, "view", $album));
   }
 
   public function view_permissions_propagate_down_test() {
@@ -139,9 +139,9 @@ class Access_Helper_Test extends Unit_Test_Case {
     $album = ORM::factory("item")->add_to_parent($root);
     access::add_item($album);
 
-    access::allow(0, "view", $root->id);
-    access::reset(0, "view", $album->id);
-    $this->assert_true(access::group_can(0, "view", $album->id));
+    access::allow(0, "view", $root);
+    access::reset(0, "view", $album);
+    $this->assert_true(access::group_can(0, "view", $album));
   }
 
   public function can_toggle_view_permissions_propagate_down_test() {
@@ -166,15 +166,20 @@ class Access_Helper_Test extends Unit_Test_Case {
     $album4->add_to_parent($album3);
     access::add_item($album4);
 
-    access::allow(0, "view", $root->id);
-    access::deny(0, "view", $album1->id);
-    access::reset(0, "view", $album2->id);
-    access::reset(0, "view", $album3->id);
-    access::reset(0, "view", $album4->id);
-    $this->assert_false(access::group_can(0, "view", $album4->id));
+    $album1->reload();
+    $album2->reload();
+    $album3->reload();
+    $album4->reload();
 
-    access::allow(0, "view", $album1->id);
-    $this->assert_true(access::group_can(0, "view", $album4->id));
+    access::allow(0, "view", $root);
+    access::deny(0, "view", $album1);
+    access::reset(0, "view", $album2);
+    access::reset(0, "view", $album3);
+    access::reset(0, "view", $album4);
+    $this->assert_false(access::group_can(0, "view", $album4));
+
+    access::allow(0, "view", $album1);
+    $this->assert_true(access::group_can(0, "view", $album4));
   }
 
   public function revoked_view_permissions_cant_be_allowed_lower_down_test() {
@@ -182,15 +187,15 @@ class Access_Helper_Test extends Unit_Test_Case {
     $album = ORM::factory("item")->add_to_parent($root);
     access::add_item($album);
 
-    access::deny(0, "view", $root->id);
-    access::allow(0, "view", $album->id);
-    $this->assert_false(access::group_can(0, "view", $album->id));
+    access::deny(0, "view", $root);
+    access::allow(0, "view", $album);
+    $this->assert_false(access::group_can(0, "view", $album));
   }
 
   public function can_edit_item_test() {
     $root = ORM::factory("item", 1);
-    access::allow(0, "edit", $root->id);
-    $this->assert_true(access::group_can(0, "edit", $root->id));
+    access::allow(0, "edit", $root);
+    $this->assert_true(access::group_can(0, "edit", $root));
   }
 
   public function non_view_permissions_propagate_down_test() {
@@ -198,9 +203,9 @@ class Access_Helper_Test extends Unit_Test_Case {
     $album = ORM::factory("item")->add_to_parent($root);
     access::add_item($album);
 
-    access::allow(0, "edit", $root->id);
-    access::reset(0, "edit", $album->id);
-    $this->assert_true(access::group_can(0, "edit", $album->id));
+    access::allow(0, "edit", $root);
+    access::reset(0, "edit", $album);
+    $this->assert_true(access::group_can(0, "edit", $album));
   }
 
   public function non_view_permissions_can_be_revoked_lower_down_test() {
@@ -220,13 +225,16 @@ class Access_Helper_Test extends Unit_Test_Case {
     $inner_photo = ORM::factory("item")->add_to_parent($inner);
     access::add_item($inner_photo);
 
-    access::allow(0, "edit", $root->id);
-    access::deny(0, "edit", $outer->id);
-    access::allow(0, "edit", $inner->id);
+    $outer->reload();
+    $inner->reload();
+
+    access::allow(0, "edit", $root);
+    access::deny(0, "edit", $outer);
+    access::allow(0, "edit", $inner);
 
     // Outer album is not editable, inner one is.
-    $this->assert_false(access::group_can(0, "edit", $outer_photo->id));
-    $this->assert_true(access::group_can(0, "edit", $inner_photo->id));
+    $this->assert_false(access::group_can(0, "edit", $outer_photo));
+    $this->assert_true(access::group_can(0, "edit", $inner_photo));
   }
 
   public function i_can_edit_test() {
@@ -238,15 +246,16 @@ class Access_Helper_Test extends Unit_Test_Case {
     Session::instance()->set("user", $user);
 
     // This user can't edit anything
-    $this->assert_false(access::can("edit", 1));
+    $root = ORM::factory("item", 1);
+    $this->assert_false(access::can("edit", $root));
 
     // Now add them to a group that has edit permission
     $group = group::create("access_test");
     $group->add($user);
-    access::allow($group->id, "edit", 1);
+    access::allow($group, "edit", $root);
     Session::instance()->set("user", $user->reload());
 
     // And verify that the user can edit.
-    $this->assert_true(access::can("edit", 1));
+    $this->assert_true(access::can("edit", $root));
   }
 }
diff --git a/core/views/welcome.html.php b/core/views/welcome.html.php
index a0c4500b..47d9bd84 100644
--- a/core/views/welcome.html.php
+++ b/core/views/welcome.html.php
@@ -354,7 +354,7 @@
                   <?= html::anchor("albums/{$current->album->id}", $current->album->title) ?>
                   &raquo;
                   <? foreach (array("view", "edit") as $perm): ?>
-                  <? if (access::group_can(group::EVERYBODY, $perm, $current->album->id)): ?>
+                  <? if (access::group_can(group::EVERYBODY, $perm, $current->album)): ?>
                   <?= html::anchor("welcome/deny_perm/0/$perm/{$current->album->id}", strtoupper($perm), array("class" => "allowed")) ?>
                   <? else: ?>
                   <?= html::anchor("welcome/add_perm/0/$perm/{$current->album->id}", strtolower($perm), array("class" => "denied")) ?>