Improve logging and error handling (#3059)

* refactor: logging and errror handling

lib/AuthenticationMiddleware.php

@@ -20,7 +20,7 @@ final class AuthenticationMiddleware
         $password = $_SERVER['PHP_AUTH_PW'] ?? null;
 
         if ($user === null || $password === null) {
-            $this->renderAuthenticationDialog();
+            print $this->renderAuthenticationDialog();
             exit;
         }
         if (
@@ -29,14 +29,14 @@ final class AuthenticationMiddleware
         ) {
             return;
         }
-        $this->renderAuthenticationDialog();
+        print $this->renderAuthenticationDialog();
         exit;
     }
 
-    private function renderAuthenticationDialog(): void
+    private function renderAuthenticationDialog(): string
     {
         http_response_code(401);
         header('WWW-Authenticate: Basic realm="RSS-Bridge"');
-        print render('error.html.php', ['message' => 'Please authenticate in order to access this instance !']);
+        return render('access-denied.html.php');
     }
 }

lib/Debug.php

@@ -101,13 +101,12 @@ class Debug
         if (!self::isEnabled()) {
             return;
         }
-        $backtrace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 3);
-        $lastFrame = end($backtrace);
-        $file = trim_path_prefix($lastFrame['file']);
-        $line = $lastFrame['line'];
-        $class = $lastFrame['class'] ?? '';
-        $function = $lastFrame['function'];
-        $text = sprintf('%s:%s %s->%s() %s', $file, $line, $class, $function, $message);
-        Logger::info($text);
+        $e = new \Exception();
+        $trace = trace_from_exception($e);
+        // Drop the current frame
+        array_pop($trace);
+        $lastFrame = $trace[array_key_last($trace)];
+        $text = sprintf('%s(%s): %s', $lastFrame['file'], $lastFrame['line'], $message);
+        Logger::debug($text);
     }
 }

lib/FormatFactory.php

@@ -38,11 +38,11 @@ class FormatFactory
         if (! preg_match('/^[a-zA-Z0-9-]*$/', $name)) {
             throw new \InvalidArgumentException('Format name invalid!');
         }
-        $name = $this->sanitizeFormatName($name);
-        if ($name === null) {
-            throw new \InvalidArgumentException('Unknown format given!');
+        $sanitizedName = $this->sanitizeFormatName($name);
+        if ($sanitizedName === null) {
+            throw new \InvalidArgumentException(sprintf('Unknown format given `%s`', $name));
         }
-        $className = '\\' . $name . 'Format';
+        $className = '\\' . $sanitizedName . 'Format';
         return new $className();
     }
 

lib/Logger.php

@@ -4,6 +4,11 @@ declare(strict_types=1);
 
 final class Logger
 {
+    public static function debug(string $message, array $context = [])
+    {
+        self::log('DEBUG', $message, $context);
+    }
+
     public static function info(string $message, array $context = []): void
     {
         self::log('INFO', $message, $context);
@@ -23,22 +28,20 @@ final class Logger
     {
         if (isset($context['e'])) {
             $context['message'] = create_sane_exception_message($context['e']);
-            $context['file'] = trim_path_prefix($context['e']->getFile());
-            $context['line'] = $context['e']->getLine();
             $context['code'] = $context['e']->getCode();
             $context['url'] = get_current_url();
-            $context['trace'] = create_sane_stacktrace($context['e']);
+            $context['trace'] = trace_to_call_points(trace_from_exception($context['e']));
             unset($context['e']);
+            // Don't log these records
             $ignoredExceptions = [
-                'Exception Exception: You must specify a format!',
-                'Exception InvalidArgumentException: Format name invalid!',
-                'Exception InvalidArgumentException: Unknown format given!',
-                'Exception InvalidArgumentException: Bridge name invalid!',
+                'Exception Exception: You must specify a format',
+                'Exception InvalidArgumentException: Format name invalid',
+                'Exception InvalidArgumentException: Unknown format given',
+                'Exception InvalidArgumentException: Bridge name invalid',
                 'Exception Exception: twitter: No results for this query',
             ];
             foreach ($ignoredExceptions as $ignoredException) {
                 if (str_starts_with($context['message'], $ignoredException)) {
-                    // Don't log this record because it's usually a bot
                     return;
                 }
             }

lib/RssBridge.php

@@ -16,9 +16,9 @@ final class RssBridge
         } catch (\Throwable $e) {
             Logger::error('Exception in main', ['e' => $e]);
             http_response_code(500);
-            print render('error.html.php', [
-                'message' => create_sane_exception_message($e),
-                'stacktrace' => create_sane_stacktrace($e),
+            print render(__DIR__ . '/../templates/error.html.php', [
+                'message'   => create_sane_exception_message($e),
+                'trace'     => trace_from_exception($e),
             ]);
         }
     }
@@ -38,6 +38,7 @@ final class RssBridge
                 return false;
             }
             $text = sprintf('%s at %s line %s', $message, trim_path_prefix($file), $line);
+            // Drop the current frame
             Logger::warning($text);
             if (Debug::isEnabled()) {
                 print sprintf('<pre>%s</pre>', $text);

lib/html.php

@@ -14,6 +14,9 @@
 
 function render(string $template, array $context = []): string
 {
+    if ($template === 'base.html.php') {
+        throw new \Exception('Do not render base.html.php into itself');
+    }
     $context['page'] = render_template($template, $context);
     return render_template('base.html.php', $context);
 }

lib/utils.php

@@ -48,7 +48,7 @@ function get_current_url(): string
 function create_sane_exception_message(\Throwable $e): string
 {
     return sprintf(
-        'Exception %s: %s at %s line %s',
+        'Exception %s: %s in %s line %s',
         get_class($e),
         $e->getMessage(),
         trim_path_prefix($e->getFile()),
@@ -56,7 +56,15 @@ function create_sane_exception_message(\Throwable $e): string
     );
 }
 
-function create_sane_stacktrace(\Throwable $e): array
+/**
+ * Returns e.g. https://github.com/RSS-Bridge/rss-bridge/blob/master/bridges/AO3Bridge.php#L8
+ */
+function render_github_url(string $file, int $line, string $revision = 'master'): string
+{
+    return sprintf('https://github.com/RSS-Bridge/rss-bridge/blob/%s/%s#L%s', $revision, $file, $line);
+}
+
+function trace_from_exception(\Throwable $e): array
 {
     $frames = array_reverse($e->getTrace());
     $frames[] = [
@@ -64,19 +72,50 @@ function create_sane_stacktrace(\Throwable $e): array
         'line' => $e->getLine(),
     ];
     $trace = [];
-    foreach ($frames as $i => $frame) {
-        $file = $frame['file'] ?? '(no file)';
-        $line = $frame['line'] ?? '(no line)';
-        $trace[] = sprintf(
-            '#%s %s:%s',
-            $i,
-            trim_path_prefix($file),
-            $line,
-        );
+    foreach ($frames as $frame) {
+        $trace[] = [
+            'file'      => trim_path_prefix($frame['file'] ?? ''),
+            'line'      => $frame['line'] ?? null,
+            'class'     => $frame['class'] ?? null,
+            'type'      => $frame['type'] ?? null,
+            'function'  => $frame['function'] ?? null,
+        ];
     }
     return $trace;
 }
 
+function trace_to_call_points(array $trace): array
+{
+    return array_map(fn($frame) => frame_to_call_point($frame), $trace);
+}
+
+function frame_to_call_point(array $frame): string
+{
+    if ($frame['class']) {
+        return sprintf(
+            '%s(%s): %s%s%s()',
+            $frame['file'],
+            $frame['line'],
+            $frame['class'],
+            $frame['type'],
+            $frame['function'],
+        );
+    } elseif ($frame['function']) {
+        return sprintf(
+            '%s(%s): %s()',
+            $frame['file'],
+            $frame['line'],
+            $frame['function'],
+        );
+    } else {
+        return sprintf(
+            '%s(%s)',
+            $frame['file'],
+            $frame['line'],
+        );
+    }
+}
+
 /**
  * Trim path prefix for privacy/security reasons
  *