From fbd056327a675e91fcb3970a7131840a1c48b2e7 Mon Sep 17 00:00:00 2001 From: Philipp Holzer Date: Sat, 13 Apr 2019 20:46:58 +0200 Subject: [PATCH 1/3] DBA-Logger fix --- src/Core/Installer.php | 3 +- src/Database/DBA.php | 74 ++++++++++++++++++++++++------- src/Factory/DBFactory.php | 3 +- src/Factory/DependencyFactory.php | 2 + tests/DatabaseTest.php | 2 + 5 files changed, 65 insertions(+), 19 deletions(-) diff --git a/src/Core/Installer.php b/src/Core/Installer.php index 65561b097e..b9f096eb5b 100644 --- a/src/Core/Installer.php +++ b/src/Core/Installer.php @@ -11,6 +11,7 @@ use Friendica\Core\Config\Cache\IConfigCache; use Friendica\Database\DBA; use Friendica\Database\DBStructure; use Friendica\Object\Image; +use Friendica\Util\Logger\VoidLogger; use Friendica\Util\Network; use Friendica\Util\Profiler; use Friendica\Util\Strings; @@ -601,7 +602,7 @@ class Installer $dbpass = $configCache->get('database', 'password'); $dbdata = $configCache->get('database', 'database'); - if (!DBA::connect($basePath, $configCache, $profiler, $dbhost, $dbuser, $dbpass, $dbdata)) { + if (!DBA::connect($basePath, $configCache, $profiler, new VoidLogger(), $dbhost, $dbuser, $dbpass, $dbdata)) { $this->addCheck(L10n::t('Could not connect to database.'), false, true, ''); return false; diff --git a/src/Database/DBA.php b/src/Database/DBA.php index 5211c09106..3b17ead387 100644 --- a/src/Database/DBA.php +++ b/src/Database/DBA.php @@ -3,7 +3,6 @@ namespace Friendica\Database; use Friendica\Core\Config\Cache\IConfigCache; -use Friendica\Core\Logger; use Friendica\Core\System; use Friendica\Util\DateTimeFormat; use Friendica\Util\Profiler; @@ -13,6 +12,7 @@ use mysqli_stmt; use PDO; use PDOException; use PDOStatement; +use Psr\Log\LoggerInterface; /** * @class MySQL database class @@ -40,6 +40,10 @@ class DBA * @var Profiler */ private static $profiler; + /** + * @var LoggerInterface + */ + private static $logger; /** * @var string */ @@ -59,7 +63,7 @@ class DBA private static $db_name = ''; private static $db_charset = ''; - public static function connect($basePath, IConfigCache $configCache, Profiler $profiler, $serveraddr, $user, $pass, $db, $charset = null) + public static function connect($basePath, IConfigCache $configCache, Profiler $profiler, LoggerInterface $logger, $serveraddr, $user, $pass, $db, $charset = null) { if (!is_null(self::$connection) && self::connected()) { return true; @@ -69,6 +73,7 @@ class DBA self::$basePath = $basePath; self::$configCache = $configCache; self::$profiler = $profiler; + self::$logger = $logger; self::$db_serveraddr = $serveraddr; self::$db_user = $user; self::$db_pass = $pass; @@ -143,6 +148,21 @@ class DBA return self::$connected; } + /** + * Sets the logger for DBA + * + * @note this is necessary because if we want to load the logger configuration + * from the DB, but there's an error, we would print out an exception. + * So the logger gets updated after the logger configuration can be retrieved + * from the database + * + * @param LoggerInterface $logger + */ + public static function setLogger(LoggerInterface $logger) + { + self::$logger = $logger; + } + /** * Disconnects the current database connection */ @@ -169,7 +189,7 @@ class DBA public static function reconnect() { self::disconnect(); - $ret = self::connect(self::$basePath, self::$configCache, self::$profiler, self::$db_serveraddr, self::$db_user, self::$db_pass, self::$db_name, self::$db_charset); + $ret = self::connect(self::$basePath, self::$configCache, self::$profiler, self::$logger, self::$db_serveraddr, self::$db_user, self::$db_pass, self::$db_name, self::$db_charset); return $ret; } @@ -425,7 +445,7 @@ class DBA if ((substr_count($sql, '?') != count($args)) && (count($args) > 0)) { // Question: Should we continue or stop the query here? - Logger::warning('Query parameters mismatch.', ['query' => $sql, 'args' => $args, 'callstack' => System::callstack()]); + self::$logger->warning('Query parameters mismatch.', ['query' => $sql, 'args' => $args, 'callstack' => System::callstack()]); } $sql = self::cleanQuery($sql); @@ -565,22 +585,35 @@ class DBA $error = self::$error; $errorno = self::$errorno; - Logger::log('DB Error '.self::$errorno.': '.self::$error."\n". - System::callstack(8)."\n".self::replaceParameters($sql, $args)); + self::$logger->error('DB Error', [ + 'code' => self::$errorno, + 'error ' => self::$error, + 'callstack' => System::callstack(8), + 'params' => self::replaceParameters($sql, $args), + ]); // On a lost connection we try to reconnect - but only once. if ($errorno == 2006) { if (self::$in_retrial || !self::reconnect()) { // It doesn't make sense to continue when the database connection was lost if (self::$in_retrial) { - Logger::log('Giving up retrial because of database error '.$errorno.': '.$error); + self::$logger->notice('Giving up retrial because of database error', [ + 'code' => self::$errorno, + 'error ' => self::$error, + ]); } else { - Logger::log("Couldn't reconnect after database error ".$errorno.': '.$error); + self::$logger->notice('Couldn\'t reconnect after database error', [ + 'code' => self::$errorno, + 'error ' => self::$error, + ]); } exit(1); } else { // We try it again - Logger::log('Reconnected after database error '.$errorno.': '.$error); + self::$logger->notice('Reconnected after database error', [ + 'code' => self::$errorno, + 'error ' => self::$error, + ]); self::$in_retrial = true; $ret = self::p($sql, $args); self::$in_retrial = false; @@ -649,13 +682,20 @@ class DBA $error = self::$error; $errorno = self::$errorno; - Logger::log('DB Error '.self::$errorno.': '.self::$error."\n". - System::callstack(8)."\n".self::replaceParameters($sql, $params)); + self::$logger->error('DB Error', [ + 'code' => self::$errorno, + 'error ' => self::$error, + 'callstack' => System::callstack(8), + 'params' => self::replaceParameters($sql, $params), + ]); // On a lost connection we simply quit. // A reconnect like in self::p could be dangerous with modifications if ($errorno == 2006) { - Logger::log('Giving up because of database error '.$errorno.': '.$error); + self::$logger->error('Giving up because of database error', [ + 'code' => self::$errorno, + 'error ' => self::$error, + ]); exit(1); } @@ -850,7 +890,7 @@ class DBA public static function insert($table, $param, $on_duplicate_update = false) { if (empty($table) || empty($param)) { - Logger::log('Table and fields have to be set'); + self::$logger->info('Table and fields have to be set'); return false; } @@ -1068,7 +1108,7 @@ class DBA public static function delete($table, array $conditions, array $options = [], array &$callstack = []) { if (empty($table) || empty($conditions)) { - Logger::log('Table and conditions have to be set'); + self::$logger->info('Table and conditions have to be set'); return false; } @@ -1154,7 +1194,7 @@ class DBA if ((count($command['conditions']) > 1) || is_int($first_key)) { $sql = "DELETE FROM `" . $command['table'] . "`" . $condition_string; - Logger::log(self::replaceParameters($sql, $conditions), Logger::DATA); + self::$logger->debug(self::replaceParameters($sql, $conditions)); if (!self::e($sql, $conditions)) { if ($do_transaction) { @@ -1184,7 +1224,7 @@ class DBA $sql = "DELETE FROM `" . $table . "` WHERE `" . $field . "` IN (" . substr(str_repeat("?, ", count($field_values)), 0, -2) . ");"; - Logger::log(self::replaceParameters($sql, $field_values), Logger::DATA); + self::$logger->debug(self::replaceParameters($sql, $field_values)); if (!self::e($sql, $field_values)) { if ($do_transaction) { @@ -1233,7 +1273,7 @@ class DBA public static function update($table, $fields, $condition, $old_fields = []) { if (empty($table) || empty($fields) || empty($condition)) { - Logger::log('Table, fields and condition have to be set'); + self::$logger->info('Table, fields and condition have to be set'); return false; } diff --git a/src/Factory/DBFactory.php b/src/Factory/DBFactory.php index b4f0c9e3c1..eeab555667 100644 --- a/src/Factory/DBFactory.php +++ b/src/Factory/DBFactory.php @@ -4,6 +4,7 @@ namespace Friendica\Factory; use Friendica\Core\Config\Cache; use Friendica\Database; +use Friendica\Util\Logger\VoidLogger; use Friendica\Util\Profiler; class DBFactory @@ -51,7 +52,7 @@ class DBFactory $db_data = $server['MYSQL_DATABASE']; } - if (Database\DBA::connect($basePath, $configCache, $profiler, $db_host, $db_user, $db_pass, $db_data, $charset)) { + if (Database\DBA::connect($basePath, $configCache, $profiler, new VoidLogger(), $db_host, $db_user, $db_pass, $db_data, $charset)) { // Loads DB_UPDATE_VERSION constant Database\DBStructure::definition($basePath, false); } diff --git a/src/Factory/DependencyFactory.php b/src/Factory/DependencyFactory.php index e55a52d14e..9512d8d948 100644 --- a/src/Factory/DependencyFactory.php +++ b/src/Factory/DependencyFactory.php @@ -3,6 +3,7 @@ namespace Friendica\Factory; use Friendica\App; +use Friendica\Database\DBA; use Friendica\Factory; use Friendica\Util\BasePath; use Friendica\Util\BaseURL; @@ -34,6 +35,7 @@ class DependencyFactory // needed to call PConfig::init() Factory\ConfigFactory::createPConfig($configCache); $logger = Factory\LoggerFactory::create($channel, $config, $profiler); + DBA::setLogger($logger); Factory\LoggerFactory::createDev($channel, $config, $profiler); $baseURL = new BaseURL($config, $_SERVER); diff --git a/tests/DatabaseTest.php b/tests/DatabaseTest.php index fec31b05af..0c2350e2de 100644 --- a/tests/DatabaseTest.php +++ b/tests/DatabaseTest.php @@ -10,6 +10,7 @@ use Friendica\Database\DBA; use Friendica\Factory; use Friendica\Util\BasePath; use Friendica\Util\Config\ConfigFileLoader; +use Friendica\Util\Logger\VoidLogger; use Friendica\Util\Profiler; use PHPUnit\DbUnit\DataSet\YamlDataSet; use PHPUnit\DbUnit\TestCaseTrait; @@ -52,6 +53,7 @@ abstract class DatabaseTest extends MockedTest $basePath, $config, $profiler, + new VoidLogger(), getenv('MYSQL_HOST'), getenv('MYSQL_USERNAME'), getenv('MYSQL_PASSWORD'), From e2f69a04b9048510849c2cb82de2359351b45cc2 Mon Sep 17 00:00:00 2001 From: Philipp Holzer Date: Sat, 13 Apr 2019 20:55:47 +0200 Subject: [PATCH 2/3] trim whitespaces --- src/Database/DBA.php | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/Database/DBA.php b/src/Database/DBA.php index 3b17ead387..d95548067f 100644 --- a/src/Database/DBA.php +++ b/src/Database/DBA.php @@ -586,10 +586,10 @@ class DBA $errorno = self::$errorno; self::$logger->error('DB Error', [ - 'code' => self::$errorno, - 'error ' => self::$error, + 'code' => self::$errorno, + 'error' => self::$error, 'callstack' => System::callstack(8), - 'params' => self::replaceParameters($sql, $args), + 'params' => self::replaceParameters($sql, $args), ]); // On a lost connection we try to reconnect - but only once. @@ -598,21 +598,21 @@ class DBA // It doesn't make sense to continue when the database connection was lost if (self::$in_retrial) { self::$logger->notice('Giving up retrial because of database error', [ - 'code' => self::$errorno, - 'error ' => self::$error, + 'code' => self::$errorno, + 'error' => self::$error, ]); } else { self::$logger->notice('Couldn\'t reconnect after database error', [ - 'code' => self::$errorno, - 'error ' => self::$error, + 'code' => self::$errorno, + 'error' => self::$error, ]); } exit(1); } else { // We try it again self::$logger->notice('Reconnected after database error', [ - 'code' => self::$errorno, - 'error ' => self::$error, + 'code' => self::$errorno, + 'error' => self::$error, ]); self::$in_retrial = true; $ret = self::p($sql, $args); @@ -683,18 +683,18 @@ class DBA $errorno = self::$errorno; self::$logger->error('DB Error', [ - 'code' => self::$errorno, - 'error ' => self::$error, + 'code' => self::$errorno, + 'error' => self::$error, 'callstack' => System::callstack(8), - 'params' => self::replaceParameters($sql, $params), + 'params' => self::replaceParameters($sql, $params), ]); // On a lost connection we simply quit. // A reconnect like in self::p could be dangerous with modifications if ($errorno == 2006) { self::$logger->error('Giving up because of database error', [ - 'code' => self::$errorno, - 'error ' => self::$error, + 'code' => self::$errorno, + 'error' => self::$error, ]); exit(1); } From f17d6e88fd71336d7e33a91017091312d9db523b Mon Sep 17 00:00:00 2001 From: Philipp Holzer Date: Sat, 13 Apr 2019 21:03:07 +0200 Subject: [PATCH 3/3] fix loglevel --- src/Database/DBA.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Database/DBA.php b/src/Database/DBA.php index d95548067f..c5ba523810 100644 --- a/src/Database/DBA.php +++ b/src/Database/DBA.php @@ -692,7 +692,7 @@ class DBA // On a lost connection we simply quit. // A reconnect like in self::p could be dangerous with modifications if ($errorno == 2006) { - self::$logger->error('Giving up because of database error', [ + self::$logger->notice('Giving up because of database error', [ 'code' => self::$errorno, 'error' => self::$error, ]);