Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error log improvements: Provide severity level and use Civi::log() #14222

Merged
merged 1 commit into from Aug 8, 2019

Conversation

@mfb
Copy link
Contributor

commented May 10, 2019

Overview

I'm not sure if we want to deprecate CRM_Core_Error::debug_var() and stop using it, or make some improvements to it.

If the latter, then we could provide a $level argument so that we can assign a severity level to fatal errors and other usages of CRM_Core_Error::debug_var(), and in addition, various CRM_Core_Error methods could use Civi::log() internally rather than CRM_Core_Error::debug_log_message().

Note: I would say that CRM_Core_Error::debug_log_message() should also use Civi::log(), but this isn't doable as long as CRM_Core_Error_Log::log() calls CRM_Core_Error::debug_log_message().. This could be worked on in a future PR if desired.

Before

A call like CRM_Core_Error::debug_var('Fatal Error Details', ...) logs at the default severity level, and doesn't use Civi::log().

After

  • CRM_Core_Error::debug_var() has an optional $level arg;
  • Various CRM_Core_Error methods use Civi::log() internally; and
  • Civi::log() supports a civi.prefix context param to specify the log file prefix.
CRM_Core_Error::debug_var() should have a level arg; CRM_Core_Error m…
…ethods could use Civi::log() internally; prefix param support for Civi::log().
@civibot

This comment has been minimized.

Copy link

commented May 10, 2019

(Standard links)

@civibot civibot bot added the master label May 10, 2019

@mattwire

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

@totten I think this is the right direction - could use your thoughts here?

@@ -39,6 +39,8 @@
require_once 'Log.php';
use Psr\Log\LogLevel;

This comment has been minimized.

Copy link
@eileenmcnaughton
@@ -232,7 +234,7 @@ public static function handle($pearError) {
$errorDetails = CRM_Core_Error::debug('', $error, FALSE);
$template->assign_by_ref('errorDetails', $errorDetails);
CRM_Core_Error::debug_var('Fatal Error Details', $error);
CRM_Core_Error::debug_var('Fatal Error Details', $error, TRUE, TRUE, '', LogLevel::ERROR);

This comment has been minimized.

Copy link
@eileenmcnaughton
@@ -339,7 +341,7 @@ public static function fatal($message = NULL, $code = NULL, $email = NULL) {
if (self::$modeException) {
// CRM-11043
CRM_Core_Error::debug_var('Fatal Error Details', $vars);
CRM_Core_Error::debug_var('Fatal Error Details', $vars, TRUE, TRUE, '', LogLevel::ERROR);

This comment has been minimized.

Copy link
@eileenmcnaughton
@@ -381,7 +383,7 @@ function_exists($config->fatalErrorHandler)
self::backtrace();
}
CRM_Core_Error::debug_var('Fatal Error Details', $vars);
CRM_Core_Error::debug_var('Fatal Error Details', $vars, TRUE, TRUE, '', LogLevel::ERROR);

This comment has been minimized.

Copy link
@eileenmcnaughton
@@ -421,7 +423,7 @@ public static function handleUnhandledException($exception) {
}
catch (Exception $other) {
// if the exception-handler generates an exception, then that sucks! oh, well. carry on.
CRM_Core_Error::debug_var('handleUnhandledException_nestedException', self::formatTextException($other));
CRM_Core_Error::debug_var('handleUnhandledException_nestedException', self::formatTextException($other), TRUE, TRUE, '', LogLevel::ERROR);

This comment has been minimized.

Copy link
@eileenmcnaughton
@@ -459,7 +461,7 @@ function_exists($config->fatalErrorHandler)
// Case C: Default error handler
// log to file
CRM_Core_Error::debug_var('Fatal Error Details', $vars, FALSE);
CRM_Core_Error::debug_var('Fatal Error Details', $vars, FALSE, TRUE, '', LogLevel::ERROR);

This comment has been minimized.

Copy link
@eileenmcnaughton
*
* @return string
* The generated output
*
* @see CRM_Core_Error::debug()
* @see CRM_Core_Error::debug_log_message()
*/
public static function debug_var($variable_name, $variable, $print = TRUE, $log = TRUE, $prefix = '') {
public static function debug_var($variable_name, $variable, $print = TRUE, $log = TRUE, $prefix = '', $level = LogLevel::INFO) {

This comment has been minimized.

Copy link
@eileenmcnaughton
@@ -574,7 +578,15 @@ public static function debug_var($variable_name, $variable, $print = TRUE, $log
reset($variable);
}
}
return self::debug_log_message($out, FALSE, $prefix);
Civi::log()->log($level, $out, ['civi.prefix' => $prefix]);

This comment has been minimized.

Copy link
@eileenmcnaughton

eileenmcnaughton May 24, 2019

Contributor

So this is the biggest change in here - however it seems that Civi::log winds up calling CRM_Core_Error_Log::log

which itself calls CRM_Core_Error::debug_log_message($message, FALSE, '', $this->map[$level]);

So it would be functionally equivalent except that the param $out is not passed through or echo'd. I think this function might need to echo $out itself to retain that behaviour

  /**
   * Logs with an arbitrary level.
   *
   * @param mixed $level
   * @param string $message
   * @param array $context
   */
  public function log($level, $message, array $context = []) {
    // FIXME: This flattens a $context a bit prematurely. When integrating
    // with external/CMS logs, we should pass through $context.
    if (!empty($context)) {
      if (isset($context['exception'])) {
        $context['exception'] = CRM_Core_Error::formatTextException($context['exception']);
      }
      $message .= "\n" . print_r($context, 1);

      if (CRM_Utils_System::isDevelopment() && CRM_Utils_Array::value('civi.tag', $context) === 'deprecated') {
        trigger_error($message, E_USER_DEPRECATED);
      }
    }
    CRM_Core_Error::debug_log_message($message, FALSE, '', $this->map[$level]);
  }
/**
* Generates debug HTML output.
*/
public static function debugOutput($message) {

This comment has been minimized.

Copy link
@eileenmcnaughton
@@ -600,7 +612,7 @@ public static function debug_log_message($message, $out = FALSE, $prefix = '', $
$file_log = self::createDebugLogger($prefix);
$file_log->log("$message\n", $priority);
$str = '<p/><code>' . htmlspecialchars($message) . '</code>';
$str = self::debugOutput($message);

This comment has been minimized.

Copy link
@eileenmcnaughton
@@ -962,7 +974,7 @@ public static function exceptionHandler($pearError) {
* $obj
*/
public static function nullHandler($obj) {
CRM_Core_Error::debug_log_message("Ignoring exception thrown by nullHandler: {$obj->code}, {$obj->message}");
Civi::log()->error("Ignoring exception thrown by nullHandler: {$obj->code}, {$obj->message}");

This comment has been minimized.

Copy link
@eileenmcnaughton

eileenmcnaughton May 24, 2019

Contributor

ok - in the absense of $out - this is the same

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

This looks good to me - but I have a concern with might be losing the output (not passing $out & not echoing to compensate)

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

@mfb did you have a response on ^^

@mfb

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

This looks good to me - but I have a concern with might be losing the output (not passing $out & not echoing to compensate)

debug_var() echos the output itself, and then when calling debug_log_message() always sets $out (the second parameter) to FALSE so no echoing happens there; just logging.

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

Looks like there is an answer to the question I raised - this is fairly low risk / easily fixable if we do lose a bit of 'noise' - merging

@eileenmcnaughton eileenmcnaughton merged commit 79ccdcd into civicrm:master Aug 8, 2019

1 check passed

default Build finished.
Details
mfb added a commit to mfb/civicrm-core that referenced this pull request Aug 8, 2019
eileenmcnaughton added a commit that referenced this pull request Aug 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.