From 0e14be8b15875436f0b30930add5ec91ff0bd551 Mon Sep 17 00:00:00 2001 From: Pierre Goiffon Date: Wed, 20 Oct 2021 16:01:08 +0200 Subject: [PATCH] =?UTF-8?q?N=C2=B04261=20Refactor=20ExceptionLog=20(#239)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Doing a code review with Bruno, we agreed to do some little refactoring : * Level per exception class - Before the whole ExceptionLog::Log method was a total rewrite of its parent, with some code duplicates... not a good idea : we should better improve LogAPI to make other similar uses possible in the future ! - The logic to get level from config must be in a GetMinLogLevel override * Write to DB - Pull up this functionnality in LogAPI - Add a sCode parameter in GetLevelDefault Doing this refactoring, I also improved : * Test the attributes set when creating the EventIssue object : during my dev I had crashes because I didn't filled all the mandatory fields... Having a PHPUnit test checking this will prevent future bugs to happen if attributes are modified in the class or in the object creation method * Use Throwable instead of Exception : this was added in PHP 7.0 and will allow to catch both Exception and Error * Because we need to have 2 statements on the same line in \Combodo\iTop\Test\UnitTest\Core\Log\ExceptionLogTest::testLogInFile, I modified the editorConfig file to allow disabling the formatter using comments. --- .editorconfig | 14 +- core/log.class.inc.php | 426 +++++++++++++----- test/core/Log/ExceptionLogTest.php | 96 +++- test/core/Log/LogAPITest.php | 186 ++++++++ .../core/{ => Log}/LogFileNameBuilderTest.php | 7 +- test/core/LogAPITest.php | 147 ------ 6 files changed, 587 insertions(+), 289 deletions(-) create mode 100644 test/core/Log/LogAPITest.php rename test/core/{ => Log}/LogFileNameBuilderTest.php (97%) delete mode 100644 test/core/LogAPITest.php diff --git a/.editorconfig b/.editorconfig index 4eb414385..b1daa6d05 100644 --- a/.editorconfig +++ b/.editorconfig @@ -11,7 +11,7 @@ tab_width = 4 ij_continuation_indent_size = 8 ij_formatter_off_tag = @formatter:off ij_formatter_on_tag = @formatter:on -ij_formatter_tags_enabled = false +ij_formatter_tags_enabled = true ij_smart_tabs = false ij_visual_guides = 300 ij_wrap_on_typing = true @@ -78,7 +78,7 @@ ij_editorconfig_space_before_colon = false ij_editorconfig_space_before_comma = false ij_editorconfig_spaces_around_assignment_operators = true -[{*.ant, *.fxml, *.jhm, *.jnlp, *.jrxml, *.rng, *.tld, *.wsdl, *.xml, *.xsd, *.xsl, *.xslt, *.xul, phpunit.xml.dist}] +[{*.ant,*.fxml,*.jhm,*.jnlp,*.jrxml,*.rng,*.tld,*.wsdl,*.xml,*.xsd,*.xsl,*.xslt,*.xul,phpunit.xml.dist}] indent_size = 2 tab_width = 2 ij_smart_tabs = true @@ -280,16 +280,17 @@ ij_javascript_while_brace_force = always ij_javascript_while_on_new_line = false ij_javascript_wrap_comments = false -[{*.ctp, *.hphp, *.inc, *.module, *.php, *.php4, *.php5, *.phtml}] +[{*.ctp,*.hphp,*.inc,*.module,*.php,*.php4,*.php5,*.phtml}] indent_style = tab ij_continuation_indent_size = 4 ij_smart_tabs = true ij_wrap_on_typing = false ij_php_align_assignments = false -ij_php_align_class_constants = false +ij_php_align_class_constants = true ij_php_align_group_field_declarations = false ij_php_align_inline_comments = false ij_php_align_key_value_pairs = true +ij_php_align_match_arm_bodies = false ij_php_align_multiline_array_initializer_expression = true ij_php_align_multiline_binary_operation = false ij_php_align_multiline_chained_methods = false @@ -298,6 +299,7 @@ ij_php_align_multiline_for = true ij_php_align_multiline_parameters = false ij_php_align_multiline_parameters_in_calls = false ij_php_align_multiline_ternary_operation = false +ij_php_align_named_arguments = false ij_php_align_phpdoc_comments = false ij_php_align_phpdoc_param_names = false ij_php_anonymous_brace_style = end_of_line @@ -417,6 +419,7 @@ ij_php_see_weight = 3 ij_php_since_weight = 28 ij_php_sort_phpdoc_elements = true ij_php_space_after_colon = true +ij_php_space_after_colon_in_enum_backed_type = true ij_php_space_after_colon_in_named_argument = true ij_php_space_after_colon_in_return_type = true ij_php_space_after_comma = true @@ -431,6 +434,7 @@ ij_php_space_before_catch_parentheses = true ij_php_space_before_class_left_brace = true ij_php_space_before_closure_left_parenthesis = true ij_php_space_before_colon = true +ij_php_space_before_colon_in_enum_backed_type = false ij_php_space_before_colon_in_named_argument = false ij_php_space_before_colon_in_return_type = false ij_php_space_before_comma = false @@ -466,6 +470,7 @@ ij_php_spaces_around_equality_operators = true ij_php_spaces_around_logical_operators = true ij_php_spaces_around_multiplicative_operators = true ij_php_spaces_around_null_coalesce_operator = true +ij_php_spaces_around_pipe_in_union_type = false ij_php_spaces_around_relational_operators = true ij_php_spaces_around_shift_operators = true ij_php_spaces_around_unary_operator = false @@ -540,7 +545,6 @@ ij_html_space_after_tag_name = false ij_html_space_around_equality_in_attribute = false ij_html_space_inside_empty_tag = false ij_html_text_wrap = normal -ij_html_uniform_ident = false [{*.markdown,*.md}] ij_visual_guides = none diff --git a/core/log.class.inc.php b/core/log.class.inc.php index db439e429..e50bf0f1d 100644 --- a/core/log.class.inc.php +++ b/core/log.class.inc.php @@ -504,7 +504,7 @@ class FileLog { $sTextPrefix = empty($sLevel) ? '' : (str_pad($sLevel, 7)); $sTextPrefix .= ' | '; - $sTextPrefix .= str_pad(UserRights::GetUserId(), 5)." | "; + $sTextPrefix .= str_pad(LogAPI::GetUserInfo(), 5)." | "; $sTextSuffix = ' | '.(empty($sChannel) ? '' : $sChannel); $sTextSuffix .= ' |||'; @@ -542,12 +542,13 @@ class FileLog */ class LogChannels { - public const CLI = 'CLI'; - public const CONSOLE = 'console'; - public const DEADLOCK = 'DeadLock'; + public const CLI = 'CLI'; + public const CONSOLE = 'console'; + public const DEADLOCK = 'DeadLock'; public const INLINE_IMAGE = 'InlineImage'; - public const PORTAL = 'portal'; - public const CMDB_SOURCE = 'cmdbsource'; + public const PORTAL = 'portal'; + public const CMDB_SOURCE = 'cmdbsource'; + public const CORE = 'core'; } @@ -562,27 +563,39 @@ abstract class LogAPI public const LEVEL_DEBUG = 'Debug'; public const LEVEL_TRACE = 'Trace'; /** - * @var string default log level - * @see GetMinLogLevel + * @see GetMinLogLevel * @used-by GetLevelDefault + * @var string default log level * @since 2.7.1 N°2977 */ public const LEVEL_DEFAULT = self::LEVEL_OK; + /** + * @see GetMinLogLevel + * @used-by GetLevelDefault + * @var string default log level when writing to DB + * @since 3.0.0 N°4261 + */ + public const LEVEL_DEFAULT_DB = self::LEVEL_ERROR; protected static $aLevelsPriority = array( - self::LEVEL_ERROR => 400, + self::LEVEL_ERROR => 400, self::LEVEL_WARNING => 300, - self::LEVEL_INFO => 200, - self::LEVEL_OK => 200, - self::LEVEL_DEBUG => 100, - self::LEVEL_TRACE => 50, + self::LEVEL_INFO => 200, + self::LEVEL_OK => 200, + self::LEVEL_DEBUG => 100, + self::LEVEL_TRACE => 50, ); + public const ENUM_CONFIG_PARAM_FILE = 'log_level_min'; + public const ENUM_CONFIG_PARAM_DB = 'log_level_min.write_in_db'; + /** * @var \Config attribute allowing to mock config in the tests */ protected static $m_oMockMetaModelConfig = null; + protected static $oLastEventIssue = null; + public static function Enable($sTargetFile) { // m_oFileLog is not defined as a class attribute so that each impl will have its own @@ -633,10 +646,6 @@ abstract class LogAPI */ public static function Log($sLevel, $sMessage, $sChannel = null, $aContext = array()) { - if (!static::$m_oFileLog) { - return; - } - if (!isset(self::$aLevelsPriority[$sLevel])) { IssueLog::Error("invalid log level '{$sLevel}'"); @@ -647,22 +656,46 @@ abstract class LogAPI $sChannel = static::CHANNEL_DEFAULT; } - if (!static::IsLogLevelEnabled($sLevel, $sChannel)) { - return; + static::WriteLog($sLevel, $sMessage, $sChannel, $aContext); + } + + /** + * @throws \ConfigException + */ + protected static function WriteLog(string $sLevel, string $sMessage, ?string $sChannel = null, ?array $aContext = array()): void + { + if ( + (null !== static::$m_oFileLog) + && static::IsLogLevelEnabled($sLevel, $sChannel, static::ENUM_CONFIG_PARAM_FILE) + ) { + static::$m_oFileLog->$sLevel($sMessage, $sChannel, $aContext); } - static::$m_oFileLog->$sLevel($sMessage, $sChannel, $aContext); + if (static::IsLogLevelEnabled($sLevel, $sChannel, static::ENUM_CONFIG_PARAM_DB)) { + self::WriteToDb($sMessage, $sChannel, $aContext); + } + } + + public static function GetUserInfo(): ?string + { + $oConnectedUser = UserRights::GetUserObject(); + if (is_null($oConnectedUser)) { + return 'null'; + } + + return $oConnectedUser->GetKey(); } /** * @throws \ConfigException if log wrongly configured * @uses GetMinLogLevel */ - final public static function IsLogLevelEnabled(string $sLevel, string $sChannel, string $sCode = 'log_level_min'): bool + final public static function IsLogLevelEnabled(string $sLevel, string $sChannel, string $sConfigKey = self::ENUM_CONFIG_PARAM_FILE): bool { - $sMinLogLevel = self::GetMinLogLevel($sChannel, $sCode); + $sMinLogLevel = self::GetMinLogLevel($sChannel, $sConfigKey); - if ($sMinLogLevel === false || $sMinLogLevel === 'false') { + // the is_bool call is to remove a IDE O:) warning as $sMinLogLevel is typed as string + if ((is_bool($sMinLogLevel) && ($sMinLogLevel === false)) || $sMinLogLevel === 'false') { return false; } if (!is_string($sMinLogLevel)) { @@ -680,7 +713,7 @@ abstract class LogAPI /** * @param string $sChannel - * @param string $sCode + * @param string $sConfigKey * * @return string one of the LEVEL_* const value : the one configured it if exists, otherwise default log level for this channel * Config can be set : @@ -696,21 +729,44 @@ abstract class LogAPI * * @uses \LogAPI::GetConfig() * @uses `log_level_min` config parameter + * @uses `log_level_min.write_to_db` config parameter * @uses \LogAPI::GetLevelDefault * * @link https://www.itophub.io/wiki/page?id=3_0_0%3Aadmin%3Alog iTop log reference */ - protected static function GetMinLogLevel($sChannel, $sCode = 'log_level_min') + protected static function GetMinLogLevel($sChannel, $sConfigKey = self::ENUM_CONFIG_PARAM_FILE) + { + $sLogLevelMin = static::GetLogConfig($sConfigKey); + + $sConfiguredLevelForChannel = static::GetMinLogLevelFromChannel($sLogLevelMin, $sChannel, $sConfigKey); + if (!is_null($sConfiguredLevelForChannel)) { + return $sConfiguredLevelForChannel; + } + + return static::GetMinLogLevelFromDefault($sLogLevelMin, $sChannel, $sConfigKey); + } + + final protected static function GetLogConfig($sConfigKey) { $oConfig = static::GetConfig(); if (!$oConfig instanceof Config) { - return static::GetLevelDefault(); + return static::GetLevelDefault($sConfigKey); } - $sLogLevelMin = $oConfig->Get($sCode); + return $oConfig->Get($sConfigKey); + } + /** + * @param string|array $sLogLevelMin log config parameter value + * @param string $sChannel + * @param string $sConfigKey config option key + * + * @return string|null null if not defined + */ + protected static function GetMinLogLevelFromChannel($sLogLevelMin, $sChannel, $sConfigKey) + { if (empty($sLogLevelMin)) { - return static::GetLevelDefault(); + return static::GetLevelDefault($sConfigKey); } if (!is_array($sLogLevelMin)) { @@ -721,16 +777,75 @@ abstract class LogAPI return $sLogLevelMin[$sChannel]; } + return null; + } + + protected static function GetMinLogLevelFromDefault($sLogLevelMin, $sChannel, $sConfigKey) + { if (isset($sLogLevelMin[static::CHANNEL_DEFAULT])) { - return $sLogLevelMin[static::CHANNEL_DEFAULT]; + return $sLogLevelMin[static::CHANNEL_DEFAULT]; } - // Even though the *self*::CHANNEL_DEFAULT is set to '' in the current class (LogAPI), the test below is necessary as the CHANNEL_DEFAULT constant can be (and is!) overloaded in derivated classes, don't remove this test to factorize it with the previous one. + // Even though the *self*::CHANNEL_DEFAULT is set to '' in the current class (LogAPI), the test below is necessary as the CHANNEL_DEFAULT constant can be (and is!) overloaded in children classes, don't remove this test to factorize it with the previous one. if (isset($sLogLevelMin[''])) { - return $sLogLevelMin['']; + return $sLogLevelMin['']; } - return static::GetLevelDefault(); + return static::GetLevelDefault($sConfigKey); + } + + protected static function WriteToDb(string $sMessage, string $sChannel, array $aContext): void + { + if (false === MetaModel::IsLogEnabledIssue()) { + return; + } + if (false === MetaModel::IsValidClass('EventIssue')) { + return; + } + + // Protect against reentrance + static $bWriteToDbReentrance; + if ($bWriteToDbReentrance === true) { + return; + } + $bWriteToDbReentrance = true; + + try { + self::$oLastEventIssue = static::GetEventIssue($sMessage, $sChannel, $aContext); + self::$oLastEventIssue->DBInsertNoReload(); + } + catch (Exception $e) { + // calling low level methods : if we would call Error() for example we would try to write to DB again... + static::$m_oFileLog->Error('Failed to log issue into the DB', LogChannels::CORE, [ + 'exception message' => $e->getMessage(), + 'exception stack' => $e->getTraceAsString(), + ]); + } + finally { + $bWriteToDbReentrance = false; + } + } + + /** + * @throws \CoreException + * @throws \CoreUnexpectedValue + * @throws \OQLException + */ + protected static function GetEventIssue(string $sMessage, string $sChannel, array $aContext): EventIssue + { + $sDate = date('Y-m-d H:i:s'); + $aStack = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 5); + $sCurrentCallStack = var_export($aStack, true); + + $oEventIssue = new EventIssue(); + $oEventIssue->Set('issue', $sMessage); + $oEventIssue->Set('message', $sMessage); + $oEventIssue->Set('date', $sDate); + $oEventIssue->Set('userinfo', static::GetUserInfo()); + $oEventIssue->Set('callstack', $sCurrentCallStack); + $oEventIssue->Set('data', $aContext); + + return $oEventIssue; } /** @@ -746,16 +861,29 @@ abstract class LogAPI } /** - * A method to override if default log level needs to be computed. Otherwise simply override the {@see LEVEL_DEFAULT} constant + * A method to override if default log level needs to be computed. Otherwise, simply override the corresponding constants * * @used-by GetMinLogLevel + * + * @param string $sConfigKey config key used for log + * + * @return string + * * @uses \LogAPI::LEVEL_DEFAULT + * @uses \LogAPI::LEVEL_DEFAULT_DB * * @since 3.0.0 N°3731 + * @since 3.0.0 N°4261 add specific default level for DB write */ - protected static function GetLevelDefault(): string + protected static function GetLevelDefault(string $sConfigKey): string { - return static::LEVEL_DEFAULT; + switch ($sConfigKey) { + case static::ENUM_CONFIG_PARAM_DB: + return static::LEVEL_DEFAULT_DB; + case static::ENUM_CONFIG_PARAM_FILE: + default: + return static::LEVEL_DEFAULT; + } } } @@ -770,6 +898,16 @@ class SetupLog extends LogAPI const LEVEL_DEFAULT = self::LEVEL_INFO; protected static $m_oFileLog = null; + + /** + * In the setup there is no user logged... + * + * @return string|null + */ + public static function GetUserInfo(): ?string + { + return 'SETUP'; + } } class IssueLog extends LogAPI @@ -808,6 +946,7 @@ class DeadLockLog extends LogAPI parent::Enable($sTargetFile); } + /** @noinspection PhpUnreachableStatementInspection we want to keep the break statements to keep clarity and avoid errors */ private static function GetChannelFromMysqlErrorNo($iMysqlErrorNo) { switch ($iMysqlErrorNo) @@ -967,13 +1106,28 @@ class DeprecatedCallsLog extends LogAPI return true; } - protected static function GetLevelDefault(): string + /** + * Override so that : + * - if we are in dev mode ({@see \utils::IsDevelopmentEnvironment()}), the level for file will be DEBUG + * - else call parent method + * + * In other words, when in dev mode all deprecated calls will be logged to file + * + * @param string $sConfigKey + * + * @return string + */ + protected static function GetLevelDefault(string $sConfigKey): string { + if ($sConfigKey === self::ENUM_CONFIG_PARAM_DB) { + return parent::GetLevelDefault($sConfigKey); + } + if (utils::IsDevelopmentEnvironment()) { return static::LEVEL_DEBUG; } - return static::LEVEL_DEFAULT; + return parent::GetLevelDefault($sConfigKey); } /** @@ -1135,14 +1289,12 @@ class LogFileRotationProcess implements iScheduledProcess * * Please use {@see ExceptionLog::LogException()} to log exceptions * - * @since 3.0.0 + * @since 3.0.0 N°4261 class creation to ease logging when an exception occurs */ class ExceptionLog extends LogAPI { - const CHANNEL_DEFAULT = 'Exception'; - const CONTEXT_EXCEPTION = '__exception'; - - private static $oLastEventIssue = null; + public const CHANNEL_DEFAULT = 'Exception'; + public const CONTEXT_EXCEPTION = '__exception'; protected static $m_oFileLog = null; @@ -1150,16 +1302,24 @@ class ExceptionLog extends LogAPI * This method should be used to write logs. * * As it encapsulate the operations performed using the Exception, you should prefer it to the standard API inherited from LogApi `ExceptionLog::Error($oException->getMessage(), get_class($oException), ['__exception' => $oException]);` - * The parameter order is not standard, but in our use case, the resulting API is way more convenient this way. + * The parameter order is not standard, but in our use case, the resulting API is way more convenient this way ! */ - public static function LogException(Exception $oException, $aContext = array(), $sLevel = self::LEVEL_WARNING) + public static function LogException(Throwable $oException, $aContext = array(), $sLevel = self::LEVEL_WARNING): void { + if (!isset(self::$aLevelsPriority[$sLevel])) { + IssueLog::Error("invalid log level '{$sLevel}'"); + + return; + } + + $sExceptionClass = get_class($oException); + if (empty($aContext[self::CONTEXT_EXCEPTION])) { $aContext[self::CONTEXT_EXCEPTION] = $oException; } if (empty($aContext['exception class'])) { - $aContext['exception class'] = get_class($oException); + $aContext['exception class'] = $sExceptionClass; } if (empty($aContext['file'])) { @@ -1167,73 +1327,124 @@ class ExceptionLog extends LogAPI } if (empty($aContext['line'])) { - $aContext['line'] =$oException->getLine(); + $aContext['line'] = $oException->getLine(); } - self::Log($sLevel, $oException->getMessage(), get_class($oException), $aContext); + parent::Log($sLevel, $oException->getMessage(), $sExceptionClass, $aContext); + } + + /** @noinspection PhpUnhandledExceptionInspection */ + public static function Log($sLevel, $sMessage, $sChannel = null, $aContext = array()) + { + throw new ApplicationException('Do not call this directly, prefer using ExceptionLog::LogException() instead'); + } + + /** @noinspection PhpParameterNameChangedDuringInheritanceInspection */ + protected static function WriteLog(string $sLevel, string $sMessage, ?string $sExceptionClass = null, ?array $aContext = array()): void + { + if ( + (null !== static::$m_oFileLog) + && static::IsLogLevelEnabled($sLevel, $sExceptionClass, static::ENUM_CONFIG_PARAM_FILE) + ) { + $sExceptionClassConfiguredForFile = static::ExceptionClassFromHierarchy($sExceptionClass, static::ENUM_CONFIG_PARAM_FILE); + if (null === $sExceptionClassConfiguredForFile) { + $sExceptionClassConfiguredForFile = $sExceptionClass; + } + + // clearing the Exception object as it is too verbose to write to a file ! + $aContextForFile = array_diff_key($aContext, [self::CONTEXT_EXCEPTION => null]); + + static::$m_oFileLog->$sLevel($sMessage, $sExceptionClassConfiguredForFile, $aContextForFile); + } + + if (static::IsLogLevelEnabled($sLevel, $sExceptionClass, static::ENUM_CONFIG_PARAM_DB)) { + $sExceptionClassConfiguredForDb = static::ExceptionClassFromHierarchy($sExceptionClass, static::ENUM_CONFIG_PARAM_DB); + if (null === $sExceptionClassConfiguredForDb) { + $sExceptionClassConfiguredForDb = $sExceptionClass; + } + self::WriteToDb($sMessage, $sExceptionClassConfiguredForDb, $aContext); + } } /** - * @inheritDoc - * @throws \ConfigException if log wrongly configured + * Will seek for the configuration based on the exception class, using {@see \ExceptionLog::ExceptionClassFromHierarchy()} + * + * @param string $sExceptionClass + * @param string $sConfigKey + * + * @return string + * @noinspection PhpParameterNameChangedDuringInheritanceInspection */ - public static function Log($sLevel, $sMessage, $sClass = null, $aContext = array()) + protected static function GetMinLogLevel($sExceptionClass, $sConfigKey = self::ENUM_CONFIG_PARAM_FILE) { - if (!static::$m_oFileLog) { - return; - } - - if (!isset(self::$aLevelsPriority[$sLevel])) { - IssueLog::Error("invalid log level '{$sLevel}'"); - - return; - } - - - $sChannel = self::FindClassChannel($sClass); - if (static::IsLogLevelEnabled($sLevel, $sChannel)) { - static::$m_oFileLog->$sLevel($sMessage, $sChannel, array_diff_key($aContext, [self::CONTEXT_EXCEPTION => null])); //The exception should not be included in the error.log because of its verbosity. - } - - $sDbChannel = self::FindClassChannel($sClass, 'log_level_min.write_in_db'); - if (static::IsLogLevelEnabled($sLevel, $sDbChannel, 'log_level_min.write_in_db')) { - self::WriteToDb($aContext); + $sLogLevelMin = static::GetLogConfig($sConfigKey); + $sExceptionClassInConfig = static::ExceptionClassFromHierarchy($sExceptionClass, $sConfigKey); + + if (null !== $sExceptionClassInConfig) { + return $sConfigKey[$sExceptionClassInConfig]; } + return static::GetMinLogLevelFromDefault($sLogLevelMin, $sExceptionClass, $sConfigKey); } - protected static function FindClassChannel($sClass, $sCode = 'log_level_min') + /** + * Searching config first for the current exception class + * If not found we are seeking for config for all the parent classes + * + * That means if we are logging a UnknownClassOqlException, we will seek log config all the way the class hierarchy : + * 1. UnknownClassOqlException + * 2. OqlNormalizeException + * 3. OQLException + * 4. CoreException + * 5. Exception + * + * @param string $sExceptionClass + * @param string $sConfigKey + * + * @return string|null the current or parent class name defined in the config, otherwise null if no class of the hierarchy found in the config + */ + protected static function ExceptionClassFromHierarchy($sExceptionClass, $sConfigKey = self::ENUM_CONFIG_PARAM_FILE) { - $oConfig = static::GetConfig(); - if (!$oConfig instanceof Config) { - return static::GetLevelDefault(); + $sLogLevelMin = static::GetLogConfig($sConfigKey); + + if (false === is_array($sLogLevelMin)) { + return null; } - $sLogLevelMin = $oConfig->Get($sCode); + $sExceptionClassInHierarchy = $sExceptionClass; + while ($sExceptionClassInHierarchy !== false) { + $sConfiguredLevelForExceptionClass = static::GetMinLogLevelFromChannel($sLogLevelMin, $sExceptionClassInHierarchy, $sConfigKey); + if (!is_null($sConfiguredLevelForExceptionClass)) { + break; + } - if (empty($sLogLevelMin)) { - return $sClass; + $sExceptionClassInHierarchy = get_parent_class($sExceptionClassInHierarchy); } - if (!is_array($sLogLevelMin)) { - return $sClass; + if ($sExceptionClassInHierarchy === false) { + return null; } - $sParentClass = $sClass; - while ( - (!isset($sLogLevelMin[$sParentClass])) - && - ($sParentClass !== false) - ) - { - $sParentClass = get_parent_class($sParentClass); - } + return $sExceptionClassInHierarchy; + } - if (isset($sLogLevelMin[$sParentClass])) { - return $sParentClass; - } + protected static function GetEventIssue(string $sMessage, string $sChannel, array $aContext): EventIssue + { + $oEventIssue = parent::GetEventIssue($sMessage, $sChannel, $aContext); - return $sClass; + $oContextException = $aContext[self::CONTEXT_EXCEPTION]; + unset($aContext[self::CONTEXT_EXCEPTION]); + + $sIssue = ($oContextException instanceof CoreException) ? $oContextException->GetIssue() : 'PHP Exception'; + $sErrorStackTrace = ($oContextException instanceof CoreException) ? $oContextException->getFullStackTraceAsString() : $oContextException->getTraceAsString(); + $aContextData = ($oContextException instanceof CoreException) ? $oContextException->getContextData() : []; + + $oEventIssue->Set('issue', $sIssue); + $oEventIssue->Set('message', $oContextException->getMessage()); + $oEventIssue->Set('callstack', $sErrorStackTrace); + $oEventIssue->Set('data', array_merge($aContextData, $aContext)); + + return $oEventIssue; } /** @@ -1241,43 +1452,12 @@ class ExceptionLog extends LogAPI */ public static function Enable($sTargetFile = null) { - if (empty($sTargetFile)) - { + if (empty($sTargetFile)) { $sTargetFile = APPROOT.'log/error.log'; } parent::Enable($sTargetFile); } - private static function WriteToDb(array $aContext): void - { - $oContextException = $aContext[self::CONTEXT_EXCEPTION]; - unset($aContext[self::CONTEXT_EXCEPTION]); - - if (MetaModel::IsLogEnabledIssue()) { - if (MetaModel::IsValidClass('EventIssue')) { - try { - self::$oLastEventIssue = new EventIssue(); - - $sIssue = ($oContextException instanceof CoreException) ? $oContextException->GetIssue() : 'PHP Exception'; - $sErrorStackTrace = ($oContextException instanceof CoreException) ? $oContextException->getFullStackTraceAsString() : $oContextException->getTraceAsString(); - $aContextData = ($oContextException instanceof CoreException) ? $oContextException->getContextData() : []; - - - self::$oLastEventIssue->Set('message', $oContextException->getMessage()); - self::$oLastEventIssue->Set('userinfo', ''); - self::$oLastEventIssue->Set('issue', $sIssue); - self::$oLastEventIssue->Set('impact', ''); - self::$oLastEventIssue->Set('callstack', $sErrorStackTrace); - self::$oLastEventIssue->Set('data', array_merge($aContextData, $aContext)); - self::$oLastEventIssue->DBInsertNoReload(); - } - catch (Exception $e) { - IssueLog::Error("Failed to log issue into the DB"); - } - } - } - } - /** * @internal Used by the tests */ diff --git a/test/core/Log/ExceptionLogTest.php b/test/core/Log/ExceptionLogTest.php index c5b6da325..20ffa3e7d 100644 --- a/test/core/Log/ExceptionLogTest.php +++ b/test/core/Log/ExceptionLogTest.php @@ -72,17 +72,22 @@ class ExceptionLogTest extends ItopDataTestCase $aContext = ['contextKey1' => 'value']; foreach ($aLevels as $i => $sLevel) { - $sExpectedFile = __FILE__; + // @formatter:off $oException = new $aExceptions[$i]("Iteration number $i"); $sExpectedLine = __LINE__; //Both should remain on the same line + // @formatter:on $iExpectedWriteNumber = $aExpectedWriteNumber[$i]; $iExpectedDbWriteNumber = $aExpectedDbWriteNumber[$i]; - $aExpectedFileContext = array_merge($aContext, ['exception class' => get_class($oException), 'file' => $sExpectedFile, 'line' => $sExpectedLine]); //The context is preserved, and, if the key 'exception class' is not yet in the array, it is added + $aExpectedFileContext = array_merge($aContext, [ + 'exception class' => get_class($oException), + 'file' => $sExpectedFile, + 'line' => $sExpectedLine, + ] + ); //The context is preserved, and, if the key 'exception class' is not yet in the array, it is added $mockFileLog->expects($this->exactly($iExpectedWriteNumber)) ->method($sLevel) - ->with($oException->GetMessage(), $sChannel, $aExpectedFileContext) - ; + ->with($oException->GetMessage(), $sChannel, $aExpectedFileContext); ExceptionLog::MockStaticObjects($mockFileLog, $oMockConfig); @@ -165,7 +170,7 @@ class ExceptionLogTest extends ItopDataTestCase 'iExpectedDbWriteNumber' => [0], 'logLevelMinWriteInDb' => ['Exception' => 'Error'], ], - 'default channel, default conf' => [ + 'default channel, default conf' => [ 'aLevels' => ['Warning'], 'aExceptions' => [\Exception::class], 'sChannel' => 'Exception', @@ -174,7 +179,7 @@ class ExceptionLogTest extends ItopDataTestCase 'iExpectedDbWriteNumber' => [0], 'logLevelMinWriteInDb' => null, ], - 'enabled' => [ + 'enabled' => [ 'aLevels' => ['Debug'], 'aExceptions' => [\Exception::class], 'sChannel' => 'Exception', @@ -184,17 +189,84 @@ class ExceptionLogTest extends ItopDataTestCase 'logLevelMinWriteInDb' => ['Exception' => 'Debug'], ], 'file: 2 enabled, 2 filtered, db: 1 enabled, 3 filtered' => [ - 'aLevels' => ['Debug', 'Trace', 'Warning', 'Error'], - 'aExceptions' => [\Exception::class, \Exception::class, \Exception::class, \Exception::class], - 'sChannel' => 'Exception', - 'aExpectedWriteNumber' => [0, 0, 1, 1], - 'logLevelMin' => null, + 'aLevels' => ['Debug', 'Trace', 'Warning', 'Error'], + 'aExceptions' => [\Exception::class, \Exception::class, \Exception::class, \Exception::class], + 'sChannel' => 'Exception', + 'aExpectedWriteNumber' => [0, 0, 1, 1], + 'logLevelMin' => null, 'iExpectedDbWriteNumber' => [0, 0, 0, 1], - 'logLevelMinWriteInDb' => null, + 'logLevelMinWriteInDb' => null, + ], + 'Simple Error (testing Throwable signature)' => [ + 'aLevels' => ['Debug'], + 'aExceptions' => [\Error::class], + 'sChannel' => 'Error', + 'aExpectedWriteNumber' => [1], + 'logLevelMin' => 'Debug', + 'iExpectedDbWriteNumber' => [1], + 'logLevelMinWriteInDb' => 'Debug', ], ]; } + /** + * @dataProvider exceptionClassProvider + */ + public function testExceptionClassFromHierarchy($aLogConfig, $sActualExceptionClass, $sExpectedExceptionClass) + { + $oMockConfig = $this->createMock('Config'); + + $oMockConfig + ->method('Get') + ->willReturn($aLogConfig); + + ExceptionLog::MockStaticObjects(null, $oMockConfig); + $sReturnedExceptionClass = $this->InvokeNonPublicStaticMethod(ExceptionLog::class, 'ExceptionClassFromHierarchy', [$sActualExceptionClass]); + + static::assertEquals($sExpectedExceptionClass, $sReturnedExceptionClass, 'Not getting correct exception in hierarchy !'); + } + + public function exceptionClassProvider() + { + // WARNING : cannot use Exception::class or LogAPI constants for levels :/ + return [ + 'Exception, defined in config' => [ + 'aLogConfig' => ['Exception' => 'Debug'], + 'sActualExceptionClass' => 'Exception', + 'sExpectedExceptionClass' => 'Exception', + ], + 'Child of Exception, Exception defined in config' => [ + 'aLogConfig' => ['Exception' => 'Debug'], + 'sActualExceptionClass' => 'ChildException', + 'sExpectedExceptionClass' => 'Exception', + ], + 'Grand child of Exception, Exception defined in config' => [ + 'aLogConfig' => ['Exception' => 'Debug'], + 'sActualExceptionClass' => 'GrandChildException', + 'sExpectedExceptionClass' => 'Exception', + ], + 'Exception, just a default level defined in config' => [ + 'aLogConfig' => 'Debug', + 'sActualExceptionClass' => 'Exception', + 'sExpectedExceptionClass' => null, + ], + 'Exception, no exception class defined in config' => [ + 'aLogConfig' => ['IssueLog' => 'Debug'], + 'sActualExceptionClass' => 'Exception', + 'sExpectedExceptionClass' => null, + ], + 'Exception, just the child defined in config' => [ + 'aLogConfig' => ['ChildException' => 'Debug'], + 'sActualExceptionClass' => 'Exception', + 'sExpectedExceptionClass' => null, + ], + 'Exception, Exception and the child defined in config' => [ + 'aLogConfig' => ['Exception' => 'Debug', 'ChildException' => 'Debug'], + 'sActualExceptionClass' => 'Exception', + 'sExpectedExceptionClass' => 'Exception', + ], + ]; + } } diff --git a/test/core/Log/LogAPITest.php b/test/core/Log/LogAPITest.php new file mode 100644 index 000000000..5d75549fa --- /dev/null +++ b/test/core/Log/LogAPITest.php @@ -0,0 +1,186 @@ +Set('developer_mode.enabled', false); + + $this->mockFileLog = $this->createMock('FileLog'); + $this->oMetaModelConfig = $this->createMock('Config'); + } + + + /** + * @dataProvider LogApiProvider + * @test + * @backupGlobals disabled + */ + public function TestLogApi($oConfigObject, $sMessage, $Channel, $sExpectedLevel, $sExpectedMessage, $sExpectedChannel = '') + { + \IssueLog::MockStaticObjects($this->mockFileLog, $oConfigObject); + + $this->mockFileLog->expects($this->exactly(1)) + ->method($sExpectedLevel) + ->with($sExpectedMessage, $sExpectedChannel); + + \IssueLog::Error($sMessage, $Channel); + } + + public function LogApiProvider() + { + return [ + [$this->oMetaModelConfig, "log msg", '', "Error", "log msg"], + [$this->oMetaModelConfig, "log msg", 'PoudlardChannel', "Error", "log msg", 'PoudlardChannel'], + [null, "log msg", '', "Error", "log msg"], + ]; + } + + /** + * @dataProvider LogWarningWithASpecificChannelProvider + * @test + * @backupGlobals disabled + */ + public function TestLogWarningWithASpecificChannel($expectedCallNb, $sExpectedLevel, $ConfigReturnedObject, $bExceptionRaised=false) + { + $this->oMetaModelConfig + ->method("Get") + ->willReturnMap([ + [\LogAPI::ENUM_CONFIG_PARAM_FILE, $ConfigReturnedObject], + [\LogAPI::ENUM_CONFIG_PARAM_DB, $ConfigReturnedObject], + ]); + + \IssueLog::MockStaticObjects($this->mockFileLog, $this->oMetaModelConfig); + + $this->mockFileLog->expects($this->exactly($expectedCallNb)) + ->method($sExpectedLevel) + ->with("log msg", "GaBuZoMeuChannel"); + + try{ + \IssueLog::Warning("log msg", "GaBuZoMeuChannel"); + if ($bExceptionRaised) { + $this->fail("raised should have been raised"); + } + } + catch(\Exception $e) { + if (!$bExceptionRaised) { + $this->fail("raised should NOT have been raised"); + } + } + } + + public function LogWarningWithASpecificChannelProvider() + { + return [ + "empty config" => [ 0, "Ok", ''], + "Default Unknown Level" => [ 0, "Ok", 'TotoLevel', true], + "Info as Default Level" => [ 1 , "Warning", 'Info'], + "Error as Default Level" => [ 0, "Warning", 'Error'], + "Empty array" => [ 0, "Ok", array()], + "Channel configured on an undefined level" => [ 0, "Ok", ["GaBuZoMeuChannel" => "TotoLevel"], true], + "Channel defined with Error" => [ 0, "Warning", ["GaBuZoMeuChannel" => "Error"]], + "Channel defined with Info" => [ 1, "Warning", ["GaBuZoMeuChannel" => "Info"]], + ]; + } + + /** + * @dataProvider LogOkWithASpecificChannel + * @test + * @backupGlobals disabled + */ + public function TestLogOkWithASpecificChannel($expectedCallNb, $sExpectedLevel, $ConfigReturnedObject, $bExceptionRaised=false) + { + $this->oMetaModelConfig + ->method("Get") + ->willReturnMap([ + [\LogAPI::ENUM_CONFIG_PARAM_FILE, $ConfigReturnedObject], + [\LogAPI::ENUM_CONFIG_PARAM_DB, $ConfigReturnedObject], + ]); + + \IssueLog::MockStaticObjects($this->mockFileLog, $this->oMetaModelConfig); + + $this->mockFileLog->expects($this->exactly($expectedCallNb)) + ->method($sExpectedLevel) + ->with("log msg", "GaBuZoMeuChannel"); + + try { + \IssueLog::Ok("log msg", "GaBuZoMeuChannel"); + if ($bExceptionRaised) { + $this->fail("raised should have been raised"); + } + } + catch (\Exception $e) { + if (!$bExceptionRaised) { + $this->fail("raised should NOT have been raised"); + } + } + } + + public function LogOkWithASpecificChannel() + { + return [ + "empty config" => [1, "Ok", ''], + "Empty array" => [1, "Ok", array()], + ]; + } + + /** + * Tests that we are creating a valid object, with all its mandatory fields set ! + * + * @throws \CoreException + */ + public function testGetEventIssue(): void + { + $oEventIssue = $this->InvokeNonPublicStaticMethod(\LogAPI::class, 'GetEventIssue', [ + 'My message', + \LogChannels::CORE, + ['context' => 'hop'], + ]); + + // Finding mandatory fields in EventIssue class + $aEventIssueAllAttributes = \MetaModel::ListAttributeDefs(\EventIssue::class); + $aEventIssueMandatoryAttributes = array_filter($aEventIssueAllAttributes, static function ($oAttDef, $sAttCode) { + if (false === $oAttDef->IsNullAllowed()) { + return $oAttDef; + } + }, ARRAY_FILTER_USE_BOTH); + + // remove fields set in the OnInsert method + unset($aEventIssueMandatoryAttributes['page']); + + foreach ($aEventIssueMandatoryAttributes as $sAttCode => $oAttDef) { + $this->assertNotEmpty($oEventIssue->Get($sAttCode), "In the EventIssue instance returned by LogAPI the '$sAttCode' mandatory attr is empty :("); + } + } +} diff --git a/test/core/LogFileNameBuilderTest.php b/test/core/Log/LogFileNameBuilderTest.php similarity index 97% rename from test/core/LogFileNameBuilderTest.php rename to test/core/Log/LogFileNameBuilderTest.php index 049e0f04d..c3e6ee9fa 100644 --- a/test/core/LogFileNameBuilderTest.php +++ b/test/core/Log/LogFileNameBuilderTest.php @@ -1,7 +1,10 @@ mockFileLog = $this->createMock('FileLog'); - $this->oMetaModelConfig = $this->createMock('Config'); - } - - - /** - * @dataProvider LogApiProvider - * @test - * @backupGlobals disabled - */ - public function TestLogApi($oConfigObject, $sMessage, $Channel, $sExpectedLevel, $sExpectedMessage, $sExpectedChannel = '') - { - \IssueLog::MockStaticObjects($this->mockFileLog, $oConfigObject); - - $this->mockFileLog->expects($this->exactly(1)) - ->method($sExpectedLevel) - ->with($sExpectedMessage, $sExpectedChannel); - - \IssueLog::Error($sMessage, $Channel); - } - - public function LogApiProvider() - { - return [ - [ $this->oMetaModelConfig, "log msg", '' , "Error", "log msg"], - [ $this->oMetaModelConfig, "log msg", 'PoudlardChannel' , "Error", "log msg", 'PoudlardChannel'], - ]; - } - - /** - * @dataProvider LogWarningWithASpecificChannelProvider - * @test - * @backupGlobals disabled - */ - public function TestLogWarningWithASpecificChannel($expectedCallNb, $sExpectedLevel, $ConfigReturnedObject, $bExceptionRaised=false) - { - $this->oMetaModelConfig - ->method("Get") - ->with('log_level_min') - ->willReturn($ConfigReturnedObject); - - \IssueLog::MockStaticObjects($this->mockFileLog, $this->oMetaModelConfig); - - $this->mockFileLog->expects($this->exactly($expectedCallNb)) - ->method($sExpectedLevel) - ->with("log msg", "GaBuZoMeuChannel"); - - try{ - \IssueLog::Warning("log msg", "GaBuZoMeuChannel"); - if ($bExceptionRaised) - { - $this->fail("raised should have been raised"); - } - } - catch(\Exception $e) - { - if (!$bExceptionRaised) - { - $this->fail("raised should NOT have been raised"); - } - } - } - - public function LogWarningWithASpecificChannelProvider() - { - return [ - "empty config" => [ 0, "Ok", ''], - "Default Unknown Level" => [ 0, "Ok", 'TotoLevel', true], - "Info as Default Level" => [ 1 , "Warning", 'Info'], - "Error as Default Level" => [ 0, "Warning", 'Error'], - "Empty array" => [ 0, "Ok", array()], - "Channel configured on an undefined level" => [ 0, "Ok", ["GaBuZoMeuChannel" => "TotoLevel"], true], - "Channel defined with Error" => [ 0, "Warning", ["GaBuZoMeuChannel" => "Error"]], - "Channel defined with Info" => [ 1, "Warning", ["GaBuZoMeuChannel" => "Info"]], - ]; - } - - /** - * @dataProvider LogOkWithASpecificChannel - * @test - * @backupGlobals disabled - */ - public function TestLogOkWithASpecificChannel($expectedCallNb, $sExpectedLevel, $ConfigReturnedObject, $bExceptionRaised=false) - { - $this->oMetaModelConfig - ->method("Get") - ->with('log_level_min') - ->willReturn($ConfigReturnedObject); - - \IssueLog::MockStaticObjects($this->mockFileLog, $this->oMetaModelConfig); - - $this->mockFileLog->expects($this->exactly($expectedCallNb)) - ->method($sExpectedLevel) - ->with("log msg", "GaBuZoMeuChannel"); - - try{ - \IssueLog::Ok("log msg", "GaBuZoMeuChannel"); - if ($bExceptionRaised) - { - $this->fail("raised should have been raised"); - } - } - catch(\Exception $e) - { - if (!$bExceptionRaised) - { - $this->fail("raised should NOT have been raised"); - } - } - } - - public function LogOkWithASpecificChannel() - { - return [ - "empty config" => [ 1, "Ok", ''], - "Empty array" => [ 1, "Ok", array()], - ]; - } - -}