From: Michael J. Rubinsky Date: Tue, 7 Dec 2010 15:08:32 +0000 (-0500) Subject: Clean up exceptions and logging. X-Git-Url: https://git.internetallee.de/?a=commitdiff_plain;h=14800f5a422cf05a2a1e01394488273b5da7232f;p=horde.git Clean up exceptions and logging. Inject Horde_Log_Logger, don't log exceptions from Horde_Db_Adapter when running queries, since those would be logged by the adapter, don't log queries in client code, it's logged by the adapter. Partially relates to Bug: 9430 --- diff --git a/framework/Core/lib/Horde/Core/Factory/ShareBase.php b/framework/Core/lib/Horde/Core/Factory/ShareBase.php index 1f21ef458..4f1558fde 100644 --- a/framework/Core/lib/Horde/Core/Factory/ShareBase.php +++ b/framework/Core/lib/Horde/Core/Factory/ShareBase.php @@ -48,7 +48,7 @@ class Horde_Core_Factory_ShareBase $listCache = $GLOBALS['session']->retrieve($cache_sig); $ob->setListCache($listCache); } - + $ob->setLogger($GLOBALS['injector']->getInstance('Horde_Log_Logger')); if (!empty($GLOBALS['conf']['share']['cache'])) { register_shutdown_function(array($this, 'shutdown'), $cache_sig, $ob); } diff --git a/framework/Share/lib/Horde/Share.php b/framework/Share/lib/Horde/Share.php index 28384ac3e..d5e349d3d 100644 --- a/framework/Share/lib/Horde/Share.php +++ b/framework/Share/lib/Horde/Share.php @@ -101,6 +101,13 @@ class Horde_Share protected $_shareCallback; /** + * Logger + * + * @var Horde_Log_Logger + */ + protected $_logger; + + /** * Configured callbacks. We currently support: *
      * add      - Called immediately before a new share is added. Receives the
@@ -135,6 +142,19 @@ class Horde_Share
         $this->_user = $user;
         $this->_permsObject = $perms;
         $this->_groups = $groups;
+        $this->_logger = new Horde_Support_Stub();
+    }
+
+    /**
+     * Set a logger object.
+     *
+     * @inject
+     *
+     * @var Horde_Log_Logger $logger  The logger object.
+     */
+    public function setLogger(Horde_Log_Logger $logger)
+    {
+        $this->_logger = $logger;
     }
 
     /**
@@ -554,4 +574,47 @@ class Horde_Share
         return count($aParts) > count($bParts);
     }
 
+    /**
+     * Logs a debug entry
+     *
+     * @param string $sql     The log statement.
+     * @param string $name    TODO
+     * @param float $runtime  Runtime interval.
+     */
+    protected function _logDebug($entry, $name, $runtime = null)
+    {
+        /*@TODO */
+        $name = (empty($name) ? '' : $name)
+              . (empty($runtime) ? '' : sprintf(" (%.4fs)", $runtime));
+        $this->_logger->debug($this->_formatLogEntry($name, $entry));
+    }
+
+    /**
+     * Logs an error entry.
+     *
+     * @param string $error   The error statement.
+     * @param string $name    TODO
+     * @param float $runtime  Runtime interval.
+     */
+    protected function _logError($error, $name, $runtime = null)
+    {
+        /*@TODO */
+        $name = (empty($name) ? '' : $name)
+              . (empty($runtime) ? '' : sprintf(" (%.4fs)", $runtime));
+        $this->_logger->err($this->_formatLogEntry($name, $error));
+    }
+
+    /**
+     * Formats the log entry.
+     *
+     * @param string $message  Message.
+     * @param string $sql      SQL statment.
+     *
+     * @return string  Formatted log entry.
+     */
+    protected function _formatLogEntry($message, $sql)
+    {
+        return "SQL $message  \n\t" . wordwrap(preg_replace("/\s+/", ' ', $sql), 70, "\n\t  ", 1);
+    }
+
 }
diff --git a/framework/Share/lib/Horde/Share/Sql.php b/framework/Share/lib/Horde/Share/Sql.php
index e7c836d61..236e12b35 100644
--- a/framework/Share/lib/Horde/Share/Sql.php
+++ b/framework/Share/lib/Horde/Share/Sql.php
@@ -70,7 +70,7 @@ class Horde_Share_Sql extends Horde_Share
 
     /**
      *
-     * @return Horde_Db_Adapter_Base
+     * @return Horde_Db_Adapter
      */
     public function getStorage()
     {
@@ -111,7 +111,6 @@ class Horde_Share_Sql extends Horde_Share
                     $share['perm']['users'][$row['user_uid']] = (int)$row['perm'];
                 }
             } catch (Horde_Db_Exception $e) {
-                Horde::logMessage($e->getMessage(), 'ERR');
                 throw new Horde_Share_Exception($e->getMessage());
             }
         }
@@ -133,7 +132,6 @@ class Horde_Share_Sql extends Horde_Share
                     $share['perm']['groups'][$row['group_uid']] = (int)$row['perm'];
                 }
             } catch (Horde_Db_Exception $e) {
-                Horde::logMessage($e->getMessage(), 'ERR');
                 throw new Horde_Share_Exception($e->getMessage());
             }
         }
@@ -145,7 +143,7 @@ class Horde_Share_Sql extends Horde_Share
      *
      * @param string $name  The name of the share to retrieve.
      *
-     * @return Horde_Share_Object_sql  The requested share.
+     * @return Horde_Share_Object  The requested share.
      * @throws Horde_Exception_NotFound
      * @throws Horde_Share_Exception
      */
@@ -154,10 +152,10 @@ class Horde_Share_Sql extends Horde_Share
         try {
             $results = $this->_db->selectOne('SELECT * FROM ' . $this->_table . ' WHERE share_name = ?', array($name));
         } catch (Horde_Db_Exception $e) {
-            Horde::logMessage($e, 'ERR');
             throw new Horde_Share_Exception($e->getMessage());
         }
         if (!$results) {
+            $this->_logError(sprintf("Share name %s not found", $name), 'NOT FOUND');
             throw new Horde_Exception_NotFound();
         }
         $data = $this->_fromDriverCharset($results);
@@ -166,7 +164,7 @@ class Horde_Share_Sql extends Horde_Share
         return $this->_createObject($data);
     }
 
-    protected function _createObject($data = array())
+    protected function _createObject(array $data = array())
     {
         $object = new $this->_shareObject($data);
         $this->initShareObject($object);
@@ -202,17 +200,17 @@ class Horde_Share_Sql extends Horde_Share
      * @param integer $id  The id of the share to retrieve.
      *
      * @return Horde_Share_Object_sql  The requested share.
-     * @throws Horde_Share_Exception
+     * @throws Horde_Share_Exception, Horde_Exception_NotFound
      */
     protected function _getShareById($id)
     {
         try {
             $results = $this->_db->selectOne('SELECT * FROM ' . $this->_table . ' WHERE share_id = ?', array($id));
         } catch (Horde_Db_Exception $e) {
-            Horde::logMessage($e, 'ERR');
             throw new Horde_Share_Exception($e->getMessage());
         }
         if (!$results) {
+            $this->_logError(sprintf("Share name %s not found", $name), 'NOT FOUND');
             throw new Horde_Exception_NotFound();
         }
         $data = $this->_fromDriverCharset($results);
@@ -229,14 +227,14 @@ class Horde_Share_Sql extends Horde_Share
      * @param array $ids  The array of ids to retrieve.
      *
      * @return array  The requested shares.
+     * @throws Horde_Share_Exception
      */
-    protected function _getShares($ids)
+    protected function _getShares(array $ids)
     {
         $shares = array();
         try {
             $rows = $this->_db->selectAll('SELECT * FROM ' . $this->_table . ' WHERE share_id IN (' . str_repeat('?', count($ids) - 1) . '?) ', $ids);
         } catch (Horde_Db_Exception $e) {
-            Horde::logMessage($e, 'ERR');
             throw new Horde_Share_Exception($e->getMessage());
         }
 
@@ -257,7 +255,6 @@ class Horde_Share_Sql extends Horde_Share
             try {
                 $rows = $this->_db->selectAll('SELECT share_id, user_uid, perm FROM ' . $this->_table . '_users WHERE share_id IN (' . str_repeat('?', count($users) - 1) . '?) ', $users);
             } catch (Horde_Db_Exception $e) {
-                Horde::logMessage($e, 'ERR');
                 throw new Horde_Share_Exception($e->getMessage());
             }
             foreach ($rows as $share) {
@@ -270,7 +267,6 @@ class Horde_Share_Sql extends Horde_Share
             try {
                 $rows = $this->_db->selectAll('SELECT share_id, group_uid, perm FROM ' . $this->_table . '_groups WHERE share_id IN (' .  str_repeat('?', count($groups) - 1) . '?) ', $groups);
             } catch (Horde_Db_Exception $e) {
-                Horde::logMessage($e, 'ERR');
                 throw new Horde_Share_Exception($e->getMessage());
             }
             foreach ($rows as $share) {
@@ -314,7 +310,6 @@ class Horde_Share_Sql extends Horde_Share
         try {
             $rows = $this->_db->selectAll('SELECT * FROM ' . $this->_table . ' ORDER BY share_name ASC');
         } catch (Horde_Db_Exception $e) {
-            Horde::logMessage($e, 'ERR');
             throw new Horde_Share_Exception($e->getMessage());
         }
 
@@ -326,7 +321,6 @@ class Horde_Share_Sql extends Horde_Share
         try {
             $rows = $this->_db->selectAll('SELECT share_id, user_uid, perm FROM ' . $this->_table . '_users');
         } catch (Horde_Db_Exception $e) {
-            Horde::logMessage($e->getMessage(), 'ERR');
             throw new Horde_Share_Exception($e);
         }
         foreach ($rows as $share) {
@@ -337,7 +331,6 @@ class Horde_Share_Sql extends Horde_Share
         try {
             $rows = $this->_db->selectAll('SELECT share_id, group_uid, perm FROM ' . $this->_table . '_groups');
         } catch (Horde_Db_Exception $e) {
-            Horde::logMessage($e->getMessage(), 'ERR');
             throw new Horde_Share_Exception($e->getMessage());
         }
         foreach ($rows as $share) {
@@ -368,8 +361,9 @@ class Horde_Share_Sql extends Horde_Share
      *
* * @return array The shares the user has access to. + * @throws Horde_Share_Exception */ - public function listShares($userid, $params = array()) + public function listShares($userid, array $params = array()) { $params = array_merge(array('perm' => Horde_Perms::SHOW, 'attributes' => null, @@ -398,11 +392,9 @@ class Horde_Share_Sql extends Horde_Share . (($params['direction'] == 0) ? ' ASC' : ' DESC'); $query = $this->_db->addLimitOffset($query, array('limit' => $params['count'], 'offset' => $params['from'])); - Horde::logMessage(sprintf("SQL Query by Horde_Share_sql::listShares: %s", $query), 'DEBUG'); try { $rows = $this->_db->selectAll($query); } catch (Horde_Db_Exception $e) { - Horde::logMessage($e->getMessage(), 'ERR'); throw new Horde_Share_Exception($e->getMessage()); } $users = array(); @@ -425,7 +417,6 @@ class Horde_Share_Sql extends Horde_Share try { $rows = $this->_db->selectAll($query); } catch (Horde_Db_Exception $e) { - Horde::logMessage($e->getMessage(), 'ERR'); throw new Horde_Share_Exception($e->getMessage()); } foreach ($rows as $share) { @@ -441,7 +432,6 @@ class Horde_Share_Sql extends Horde_Share try { $rows = $this->_db->selectAll($query); } catch (Horde_Db_Exception $e) { - Horde::logMessage($e, 'ERR'); throw new Horde_Share_Exception($e->getMessage()); } foreach ($rows as $share) { @@ -474,11 +464,9 @@ class Horde_Share_Sql extends Horde_Share public function listSystemShares() { $query = 'SELECT * FROM ' . $this->_table . ' WHERE share_owner IS NULL'; - Horde::logMessage('SQL Query by Horde_Share_sql::listSystemShares: ' . $query, 'DEBUG'); try { $rows = $this->_db->selectAll($query); } catch (Horde_Db_Exception $e) { - Horde::logMessage($e->getMessage(), 'ERR'); throw new Horde_Share_Exception($e->getMessage());; } @@ -503,15 +491,21 @@ class Horde_Share_Sql extends Horde_Share * username. * * @return integer The number of shares + * @throws Horde_Share_Exception */ protected function _countShares($userid, $perm = Horde_Perms::SHOW, $attributes = null) { - $query = $this->getShareCriteria($userid, $perm, $attributes); - $query = 'SELECT COUNT(DISTINCT s.share_id) ' . $query; - Horde::logMessage(sprintf("SQL Query by Horde_Share_sql::_countShares: %s", $query), 'DEBUG'); + $query = 'SELECT COUNT(DISTINCT s.share_id) ' + . $this->getShareCriteria($userid, $perm, $attributes); - return $this->_db->selectValue($query); + try { + $results = $this->_db->selectValue($query); + } catch (Horde_Db_Exception $e) { + throw new Horde_Share_Exception($e); + } + + return $results; } /** @@ -519,13 +513,15 @@ class Horde_Share_Sql extends Horde_Share * * @param string $name The share's name. * - * @return Horde_Share_Object_sql A new share object. + * @return Horde_Share_Object A new share object + * @throws InvalidArgumentException */ protected function _newShare($name) { if (empty($name)) { - throw new Horde_Share_Exception('Share names must be non-empty'); + throw new InvalidArgumentException('Share names must be non-empty'); } + return $this->_createObject(array('share_name' => $name)); } @@ -536,9 +532,9 @@ class Horde_Share_Sql extends Horde_Share * Horde_Share_sql::_newShare(), and have any initial details added * to it, before this function is called. * - * @param Horde_Share_Object_sql $share The new share object. + * @param Horde_Share_Object $share The new share object. */ - protected function _addShare($share) + protected function _addShare(Horde_Share_Object $share) { return $share->save(); } @@ -546,12 +542,12 @@ class Horde_Share_Sql extends Horde_Share /** * Removes a share from the shares system permanently. * - * @param Horde_Share_Object_sql $share The share to remove. + * @param Horde_Share_Object $share The share to remove. * * @return boolean * @throws Horde_Share_Exception */ - protected function _removeShare($share) + protected function _removeShare(Horde_Share_Object $share) { $params = array($share->getId()); $tables = array($this->_table, @@ -561,7 +557,6 @@ class Horde_Share_Sql extends Horde_Share try { $this->_db->delete('DELETE FROM ' . $table . ' WHERE share_id = ?', $params); } catch (Horde_Db_Exception $e) { - Horde::logMessage($e->getMessage(), 'ERR'); throw new Horde_Share_Exception($e->getMessage()); } } @@ -635,7 +630,7 @@ class Horde_Share_Sql extends Horde_Share . ' AND (' . Horde_SQL::buildClause($this->_db, 'g.perm', '&', $perm) . '))'; } } catch (Horde_Group_Exception $e) { - Horde::logMessage($e, 'ERR'); + $this->_logError($e, 'Horde_Share::getShareCriteria()'); } } else { $where = '(' . Horde_SQL::buildClause($this->_db, 's.perm_guest', '&', $perm) . ')'; diff --git a/framework/Share/lib/Horde/Share/Sql/Hierarchical.php b/framework/Share/lib/Horde/Share/Sql/Hierarchical.php index 0889d8198..7c35c785f 100644 --- a/framework/Share/lib/Horde/Share/Sql/Hierarchical.php +++ b/framework/Share/lib/Horde/Share/Sql/Hierarchical.php @@ -47,8 +47,9 @@ class Horde_Share_Sql_Hierarchical extends Horde_Share_Sql * * * @return array The shares the user has access to. + * @throws Horde_Share_Exception */ - public function listShares($userid, $params = array()) + public function listShares($userid, array $params = array()) { $params = array_merge(array('perm' => Horde_Perms::SHOW, 'attributes' => null, @@ -79,11 +80,9 @@ class Horde_Share_Sql_Hierarchical extends Horde_Share_Sql . (($params['direction'] == 0) ? ' ASC' : ' DESC'); $query = $this->_db->addLimitOffset($query, array('limit' => $params['count'], 'offset' => $params['from'])); - Horde::logMessage('Query By Horde_Share_sql_hierarchical: ' . $query, 'DEBUG'); try { $rows = $this->_db->selectAll($query); } catch (Horde_Db_Exception $e) { - Horde::logMessage($e, 'ERR'); throw new Horde_Share_Exception($e->getMessage()); } @@ -107,7 +106,6 @@ class Horde_Share_Sql_Hierarchical extends Horde_Share_Sql try { $rows = $this->_db->selectAll($query, $users); } catch (Horde_Db_Exception $e) { - Horde::logMessage($e, 'ERR'); throw new Horde_Share_Exception($e->getMessage()); } foreach ($rows as $share) { @@ -122,7 +120,6 @@ class Horde_Share_Sql_Hierarchical extends Horde_Share_Sql try { $rows = $this->_db->selectAll($query, $groups); } catch (Horde_Db_Exception $e) { - Horde::logMessage($e, 'ERR'); throw new Horde_Share_Exception($e->getMessage()); } foreach ($rows as $share) { @@ -152,7 +149,7 @@ class Horde_Share_Sql_Hierarchical extends Horde_Share_Sql * * @TODO: check method visisbility, * remove ignorePerms param, simply set perm to null for this - * + * * @param string $userid The userid of the user to check access for. * @param integer $perm The level of permissions required. * @param mixed $attributes Restrict the shares returned to those who @@ -217,7 +214,9 @@ class Horde_Share_Sql_Hierarchical extends Horde_Share_Sql $where .= ' OR (g.group_uid IN (' . implode(',', $group_ids) . ')' . ' AND (' . Horde_SQL::buildClause($this->_db, 'g.perm', '&', $perm) . '))'; } - } catch (Horde_Group_Exception $e) {} + } catch (Horde_Group_Exception $e) { + $this->_logError($e, 'Horde_Share_Sql_Hierarchical::getShareCriteria()'); + } } } @@ -291,6 +290,7 @@ class Horde_Share_Sql_Hierarchical extends Horde_Share_Sql * @param integer $count The number of users to return. * * @return array List of users. + * @throws Horde_Share_Exception */ public function listOwners($perm = Horde_Perms::SHOW, $parent = null, $allLevels = true, $from = 0, $count = 0) @@ -305,7 +305,6 @@ class Horde_Share_Sql_Hierarchical extends Horde_Share_Sql try { $allowners = $this->_db->selectValues($sql); } catch (Horde_Db_Exception $e) { - Horde::logMessage($e, 'ERR'); throw new Horde_Share_Exception($e); } @@ -330,23 +329,30 @@ class Horde_Share_Sql_Hierarchical extends Horde_Share_Sql * children of $parent? * * @return integer Number of users. + * @throws Horde_Share_Exception */ public function countOwners($perm = Horde_Perms::SHOW, $parent = null, $allLevels = true) { $sql = 'SELECT COUNT(DISTINCT(s.share_owner)) ' . $this->getShareCriteria($this->_user, $perm, null, $parent, $allLevels); - return $this->_db->selectValue($sql); + try { + $results = $this->_db->selectValue($sql); + } catch (Horde_Db_Exception $e) { + throw new Horde_Share_Exception($e); + } + + return $results; } /** * Returns a share's direct parent object. * - * @param Horde_Share_Object $share The share to get parent for. + * @param Horde_Share_Object $child The share to get parent for. * * @return Horde_Share_Object The parent share, if it exists. */ - public function getParent($child) + public function getParent(Horde_Share_Object $child) { $parents = $child->get('parents'); @@ -355,6 +361,7 @@ class Horde_Share_Sql_Hierarchical extends Horde_Share_Sql return null; } $parents = explode(':', $parents); + return $this->getShareById(array_pop($parents)); } @@ -383,9 +390,8 @@ class Horde_Share_Sql_Hierarchical extends Horde_Share_Sql * @param array $cids The array of ids to retrieve. * * @return array The requested shares keyed by share_id. - * @throws Horde_Share_Exception */ - public function getShares($cids) + public function getShares(array $cids) { $all_shares = array(); $missing_ids = array(); @@ -413,7 +419,8 @@ class Horde_Share_Sql_Hierarchical extends Horde_Share_Sql * delete all child shares as well. * * @param Horde_Share_Object $share The share to remove. - * @throws Horde_Exception + * + * @return boolean */ public function removeShare(Horde_Share_Object $share) { @@ -432,22 +439,22 @@ class Horde_Share_Sql_Hierarchical extends Horde_Share_Sql } /** - * Returns an array of Horde_Share_Object_sql objects corresponding + * Returns an array of Horde_Share_Object objects corresponding * to the given set of unique IDs, with the details retrieved * appropriately. * * @param array $cids The array of ids to retrieve. * * @return array The requested shares keyed by share_id. + * @throws Horde_Share_Exception */ - protected function _getShares($ids) + protected function _getShares(array $ids) { $shares = array(); $query = 'SELECT * FROM ' . $this->_table . ' WHERE share_id IN (' . str_repeat('?,', count($ids) - 1) . '?)'; try { $rows = $this->_db->selectAll($query, $ids); } catch (Horde_Db_Exception $e) { - Horde::logMessage($e, 'ERR'); throw new Horde_Share_Exception($e->getMessage()); } @@ -469,7 +476,6 @@ class Horde_Share_Sql_Hierarchical extends Horde_Share_Sql try { $rows = $this->_db->selectAll($query, $users); } catch (Horde_Db_Exception $e) { - Horde::logMessage($e, 'ERR'); throw new Horde_Share_Exception($e->getMessage()); } foreach ($rows as $share) { @@ -484,12 +490,11 @@ class Horde_Share_Sql_Hierarchical extends Horde_Share_Sql try { $rows = $this->_db->selectAll($query, $groups); } catch (Horde_Db_Exception $e) { - Horde::logMessage($e, 'ERR'); throw new Horde_Share_Exception($e->getMessage()); } foreach ($rows as $share) { $shares[$share['share_id']]['perm']['groups'][$share['group_uid']] = (int)$share['perm']; - } + } } $sharelist = array(); @@ -525,13 +530,20 @@ class Horde_Share_Sql_Hierarchical extends Horde_Share_Sql * children of $parent? * * @return integer Number of shares the user has access to. + * @throws Horde_Share_Exception */ public function countShares($userid, $perm = Horde_Perms::SHOW, $attributes = null, $parent = null, $allLevels = true) { $query = 'SELECT COUNT(DISTINCT s.share_id) ' - . $this->getShareCriteria($userid, $perm, $attributes, - $parent, $allLevels); + . $this->getShareCriteria($userid, $perm, $attributes, $parent, $allLevels); + + try { + $result = $this->_db->selectValue($query); + } catch (Horde_Db_Exception $e) { + throw new Horde_Share_Exception($e); + } + return $this->_db->selectValue($query); }