From e8a88a37ac66630663deff450b90d87fc9bf3101 Mon Sep 17 00:00:00 2001 From: Gunnar Wrobel Date: Tue, 29 Sep 2009 15:04:10 +0200 Subject: [PATCH] Fix and test error handling of the class. --- framework/History/lib/Horde/History.php | 19 +++ framework/History/lib/Horde/History/Sql.php | 62 ++++--- framework/History/lib/Horde/HistoryObject.php | 2 +- .../History/test/Horde/History/InterfaceTest.php | 184 ++++++++++++++++----- 4 files changed, 208 insertions(+), 59 deletions(-) diff --git a/framework/History/lib/Horde/History.php b/framework/History/lib/Horde/History.php index 595388926..aa246a08d 100644 --- a/framework/History/lib/Horde/History.php +++ b/framework/History/lib/Horde/History.php @@ -43,6 +43,13 @@ class Horde_History static protected $_instances; /** + * Our log handler. + * + * @var Horde_Log_Logger + */ + protected $_logger; + + /** * Attempts to return a reference to a concrete History instance. * It will only create a new instance if no History instance * currently exists. @@ -90,6 +97,18 @@ class Horde_History } /** + * Set the log handler. + * + * @param Horde_Log_Logger $logger The log handler. + * + * @return NULL + */ + public function setLogger(Horde_Log_Logger $logger) + { + $this->_logger = $logger; + } + + /** * Logs an event to an item's history log. The item must be uniquely * identified by $guid. Any other details about the event are passed in * $attributes. Standard suggested attributes are: diff --git a/framework/History/lib/Horde/History/Sql.php b/framework/History/lib/Horde/History/Sql.php index 734d9403d..e3cc548a2 100644 --- a/framework/History/lib/Horde/History/Sql.php +++ b/framework/History/lib/Horde/History/Sql.php @@ -31,7 +31,7 @@ class Horde_History_Sql extends Horde_History /** * Pointer to a DB instance to manage the history. * - * @var DB + * @var DB_common */ protected $_db; @@ -39,7 +39,7 @@ class Horde_History_Sql extends Horde_History * Handle for the current database connection, used for writing. Defaults * to the same handle as $_db if a separate write database is not required. * - * @var DB + * @var DB_common */ protected $_write_db; @@ -47,9 +47,12 @@ class Horde_History_Sql extends Horde_History * Constructor. * * @param DB_common $db The database connection. + * + * @throws Horde_Exception */ public function __construct(DB_common $db) { + $this->handleError($db); $this->_write_db = $db; $this->_db = $db; } @@ -61,9 +64,12 @@ class Horde_History_Sql extends Horde_History * @param DB_common $db The database connection. * * @return NULL + * + * @throws Horde_Exception */ public function setReadDb(DB_common $db) { + $this->handleError($db); $this->_db = $db; } @@ -101,8 +107,7 @@ class Horde_History_Sql extends Horde_History isset($attributes['desc']) ? $attributes['desc'] : null ); - unset($attributes['ts'], $attributes['who'], - $attributes['desc'], $attributes['action']); + unset($attributes['ts'], $attributes['who'], $attributes['desc'], $attributes['action']); $values[] = $attributes ? serialize($attributes) @@ -116,10 +121,7 @@ class Horde_History_Sql extends Horde_History ' history_extra = ? WHERE history_id = ?', $values ); - if ($r instanceof PEAR_Error) { - Horde::logMessage($r, __FILE__, __LINE__, PEAR_LOG_ERR); - throw new Horde_Exception($r->getMessage()); - } + $this->handleError($r); $done = true; break; } @@ -130,10 +132,7 @@ class Horde_History_Sql extends Horde_History * replace, insert a new row. */ if (!$done) { $history_id = $this->_write_db->nextId('horde_histories'); - if ($history_id instanceof PEAR_Error) { - Horde::logMessage($history_id, __FILE__, __LINE__, PEAR_LOG_ERR); - throw new Horde_Exception($history_id->getMessage()); - } + $this->handleError($history_id); $values = array( $history_id, @@ -144,8 +143,7 @@ class Horde_History_Sql extends Horde_History isset($attributes['action']) ? $attributes['action'] : null ); - unset($attributes['ts'], $attributes['who'], - $attributes['desc'], $attributes['action']); + unset($attributes['ts'], $attributes['who'], $attributes['desc'], $attributes['action']); $values[] = $attributes ? serialize($attributes) @@ -155,11 +153,7 @@ class Horde_History_Sql extends Horde_History 'INSERT INTO horde_histories (history_id, object_uid, history_ts, history_who, history_desc, history_action, history_extra)' . ' VALUES (?, ?, ?, ?, ?, ?, ?)', $values ); - - if ($r instanceof PEAR_Error) { - Horde::logMessage($r, __FILE__, __LINE__, PEAR_LOG_ERR); - throw new Horde_Exception($r->getMessage()); - } + $this->handleError($r); } return true; @@ -176,6 +170,7 @@ class Horde_History_Sql extends Horde_History public function getHistory($guid) { $rows = $this->_db->getAll('SELECT * FROM horde_histories WHERE object_uid = ?', array($guid), DB_FETCHMODE_ASSOC); + $this->handleError($rows); return new Horde_HistoryObject($guid, $rows); } @@ -220,7 +215,9 @@ class Horde_History_Sql extends Horde_History $where[] = 'object_uid LIKE ' . $this->_db->quote($parent . ':%'); } - return $this->_db->getAssoc('SELECT DISTINCT object_uid, history_id FROM horde_histories WHERE ' . implode(' AND ', $where)); + $result = $this->_db->getAssoc('SELECT DISTINCT object_uid, history_id FROM horde_histories WHERE ' . implode(' AND ', $where)); + $this->handleError($result); + return $result; } /** @@ -243,7 +240,30 @@ class Horde_History_Sql extends Horde_History $ids[] = $this->_write_db->quote($name); } - return $this->_write_db->query('DELETE FROM horde_histories WHERE object_uid IN (' . implode(',', $ids) . ')'); + $result = $this->_write_db->query('DELETE FROM horde_histories WHERE object_uid IN (' . implode(',', $ids) . ')'); + $this->handleError($result); + return $result; } + /** + * Determine if the given result is a PEAR error. If it is, log the event + * and throw an exception. + * + * @param mixed $result The result to check. + * + * @return NULL + * + * @throws Horde_Exception + */ + protected function handleError($result) + { + if ($result instanceof PEAR_Error) { + if (!empty($this->_logger)) { + $this->_logger->error($result->getMessage()); + } else { + Horde::logMessage($result, __FILE__, __LINE__, PEAR_LOG_ERR); + } + throw new Horde_Exception($result->getMessage()); + } + } } diff --git a/framework/History/lib/Horde/HistoryObject.php b/framework/History/lib/Horde/HistoryObject.php index da3b1643a..48743438c 100644 --- a/framework/History/lib/Horde/HistoryObject.php +++ b/framework/History/lib/Horde/HistoryObject.php @@ -31,7 +31,7 @@ class Horde_HistoryObject { $this->uid = $uid; - if (!$data || ($data instanceof PEAR_Error)) { + if (!$data) { return; } diff --git a/framework/History/test/Horde/History/InterfaceTest.php b/framework/History/test/Horde/History/InterfaceTest.php index b37e71d93..178a6acca 100644 --- a/framework/History/test/Horde/History/InterfaceTest.php +++ b/framework/History/test/Horde/History/InterfaceTest.php @@ -84,22 +84,21 @@ class Horde_History_InterfaceTest extends PHPUnit_Framework_TestCase } /** - * Return the history handler for the specified environment. + * Initialize the given environment. * * @param string $environment The selected environment. * - * @return History The history. + * @return Horde_Injector The environment. */ - public function &getHistory($environment) + public function initializeEnvironment($environment) { - if (!isset($this->_histories[$environment])) { - switch ($environment) { - case self::ENVIRONMENT_DB: - global $conf; + switch ($environment) { + case self::ENVIRONMENT_DB: + global $conf; - $this->_db_file = Horde::getTempFile('Horde_Test', false); - $this->_db = sqlite_open($this->_db_file, '0640'); - $table = <<_db_file = Horde::getTempFile('Horde_Test', false); + $this->_db = sqlite_open($this->_db_file, '0640'); + $table = <<_db, $table); - sqlite_exec($this->_db, 'CREATE INDEX history_action_idx ON horde_histories (history_action);'); - sqlite_exec($this->_db, 'CREATE INDEX history_ts_idx ON horde_histories (history_ts);'); - sqlite_exec($this->_db, 'CREATE INDEX history_uid_idx ON horde_histories (object_uid);'); - sqlite_close($this->_db); - - $conf['sql']['database'] = $this->_db_file; - $conf['sql']['mode'] = '0640'; - $conf['sql']['charset'] = 'utf-8'; - $conf['sql']['phptype'] = 'sqlite'; - - $injector = new Horde_Injector(new Horde_Injector_TopLevel()); - $injector->bindFactory( - 'Horde_History', - 'Horde_History_Factory', - 'getHistory' - ); - - $config = new stdClass; - $config->driver = 'Sql'; - $config->params = $conf['sql']; - $injector->setInstance('Horde_History_Config', $config); - - $history = $injector->getInstance('Horde_History'); - break; - } + sqlite_exec($this->_db, $table); + sqlite_exec($this->_db, 'CREATE INDEX history_action_idx ON horde_histories (history_action);'); + sqlite_exec($this->_db, 'CREATE INDEX history_ts_idx ON horde_histories (history_ts);'); + sqlite_exec($this->_db, 'CREATE INDEX history_uid_idx ON horde_histories (object_uid);'); + sqlite_close($this->_db); + + $conf['sql']['database'] = $this->_db_file; + $conf['sql']['mode'] = '0640'; + $conf['sql']['charset'] = 'utf-8'; + $conf['sql']['phptype'] = 'sqlite'; + + $injector = new Horde_Injector(new Horde_Injector_TopLevel()); + $injector->bindFactory( + 'Horde_History', + 'Horde_History_Factory', + 'getHistory' + ); + + $config = new stdClass; + $config->driver = 'Sql'; + $config->params = $conf['sql']; + $injector->setInstance('Horde_History_Config', $config); + break; + } + return $injector; + } + /** + * Return the history handler for the specified environment. + * + * @param string $environment The selected environment. + * + * @return Horde_History The history. + */ + public function getHistory($environment) + { + if (!isset($this->_histories[$environment])) { + $injector = $this->initializeEnvironment($environment); + $this->_histories[$environment] = $injector->getInstance('Horde_History'); } - $this->_histories[$environment] = &$history; return $this->_histories[$environment]; } @@ -392,4 +402,104 @@ EOL; } } + public function testBaseHordehistoryClassProvidesIncompleteImplementation() + { + global $conf; + + try { + $history = new Horde_History(); + $history->getHistory(''); + $this->fail('No exception was thrown!'); + } catch (Horde_Exception $e) { + } + try { + $history = new Horde_History(); + $history->getByTimestamp('=', 1); + $this->fail('No exception was thrown!'); + } catch (Horde_Exception $e) { + } + try { + $history = new Horde_History(); + $history->removeByNames(array()); + $this->fail('No exception was thrown!'); + } catch (Horde_Exception $e) { + } + + unset($conf['sql']); + + try { + $history = Horde_History::singleton(); + $this->fail('No exception was thrown!'); + } catch (Horde_Exception $e) { + } + } + + public function testHordehistorysqlConvertsPearErrorToHordeexceptions() + { + $injector = $this->initializeEnvironment(self::ENVIRONMENT_DB); + $mock = new Dummy_Db(); + $injector->setInstance('DB_common_write', $mock); + $history = $injector->getInstance('Horde_History'); + + try { + $history->log('test', array('who' => 'me', 'ts' => 1000, 'action' => 'test')); + $this->fail('No exception was thrown!'); + } catch (Horde_Exception $e) { + } + + try { + $history->getHistory('test'); + $this->fail('No exception was thrown!'); + } catch (Horde_Exception $e) { + } + + try { + $history->getByTimestamp('>', 1, array(array('field' => 'who', 'op' => '=', 'value' => 'you'))); + $this->fail('No exception was thrown!'); + } catch (Horde_Exception $e) { + } + + try { + $history->removeByNames(array('test')); + $this->fail('No exception was thrown!'); + } catch (Horde_Exception $e) { + } + } +} + +/** + * A dummy database connection producing nothing bot errors. + * + * Copyright 2009 The Horde Project (http://www.horde.org/) + * + * See the enclosed file COPYING for license information (LGPL). If you + * did not receive this file, see http://www.fsf.org/copyleft/lgpl.html. + * + * @category Horde + * @package History + * @author Gunnar Wrobel + * @license http://www.fsf.org/copyleft/lgpl.html LGPL + * @link http://pear.horde.org/index.php?package=History + */ +class Dummy_Db extends DB_common +{ + public function query($sql, $values = null) + { + return PEAR::raiseError('Error'); + } + + public function nextId($table) + { + return PEAR::raiseError('Error'); + } + + public function getAll($sql) + { + return PEAR::raiseError('Error'); + } + + public function getAssoc($sql) + { + return PEAR::raiseError('Error'); + } } \ No newline at end of file -- 2.11.0