feat: improve error handling (#2902)

10 files changed, 180 insertions, 254 deletions

diff --git a/actions/DisplayAction.php b/actions/DisplayAction.php
index efcddb69..b345bddc 100644
--- a/actions/DisplayAction.php
+++ b/actions/DisplayAction.php
@@ -16,16 +16,6 @@ class DisplayAction implements ActionInterface
 {
     public $userData = [];
 
-    private function getReturnCode($error)
-    {
-        $returnCode = $error->getCode();
-        if ($returnCode === 301 || $returnCode === 302) {
-            # Don't pass redirect codes to the exterior
-            $returnCode = 508;
-        }
-        return $returnCode;
-    }
-
     public function execute()
     {
         $bridgeFactory = new \BridgeFactory();
@@ -188,19 +178,31 @@ class DisplayAction implements ActionInterface
                         );
 
                         $item->setTimestamp(time());
-                        $item->setContent(buildBridgeException($e, $bridge));
+
+                        $message = sprintf(
+                            'Uncaught Exception %s: %s at %s line %s',
+                            get_class($e),
+                            $e->getMessage(),
+                            trim_path_prefix($e->getFile()),
+                            $e->getLine()
+                        );
+
+                        $content = render_template('bridge-error.html.php', [
+                            'message' => $message,
+                            'stacktrace' => create_sane_stacktrace($e),
+                            'searchUrl' => self::createGithubSearchUrl($bridge),
+                            'issueUrl' => self::createGithubIssueUrl($bridge, $e, $message),
+                            'bridge' => $bridge,
+                        ]);
+                        $item->setContent($content);
 
                         $items[] = $item;
                     } elseif (Configuration::getConfig('error', 'output') === 'http') {
-                        header('Content-Type: text/html', true, $this->getReturnCode($e));
-                        $response = buildTransformException($e, $bridge);
-                        print $response;
-                        exit;
+                        throw $e;
                     }
                 }
             }
 
-            // Store data in cache
             $cache->saveData([
                 'items' => array_map(function ($i) {
                     return $i->toArray();
@@ -209,26 +211,40 @@ class DisplayAction implements ActionInterface
             ]);
         }
 
-        // Data transformation
-        try {
-            $formatFactory = new FormatFactory();
-            $format = $formatFactory->create($format);
-            $format->setItems($items);
-            $format->setExtraInfos($infos);
-            $lastModified = $cache->getTime();
-            $format->setLastModified($lastModified);
-            if ($lastModified) {
-                header('Last-Modified: ' . gmdate('D, d M Y H:i:s ', $lastModified) . 'GMT');
-            }
-            header('Content-Type: ' . $format->getMimeType() . '; charset=' . $format->getCharset());
-
-            echo $format->stringify();
-        } catch (\Throwable $e) {
-            error_log($e);
-            header('Content-Type: text/html', true, $e->getCode());
-            $response = buildTransformException($e, $bridge);
-            print $response;
-            exit;
+        $formatFactory = new FormatFactory();
+        $format = $formatFactory->create($format);
+        $format->setItems($items);
+        $format->setExtraInfos($infos);
+        $lastModified = $cache->getTime();
+        $format->setLastModified($lastModified);
+        if ($lastModified) {
+            header('Last-Modified: ' . gmdate('D, d M Y H:i:s ', $lastModified) . 'GMT');
         }
+        header('Content-Type: ' . $format->getMimeType() . '; charset=' . $format->getCharset());
+        print $format->stringify();
+    }
+
+    private static function createGithubIssueUrl($bridge, $e, string $message): string
+    {
+        return sprintf('https://github.com/RSS-Bridge/rss-bridge/issues/new?%s', http_build_query([
+            'title' => sprintf('%s failed with error %s', $bridge->getName(), $e->getCode()),
+            'body' => sprintf(
+                "```\n%s\n\n%s\n\nQuery string:%s\nVersion:%s\n```",
+                $message,
+                implode("\n", create_sane_stacktrace($e)),
+                $_SERVER['QUERY_STRING'] ?? '',
+                Configuration::getVersion(),
+            ),
+            'labels' => 'Bridge-Broken',
+            'assignee' => $bridge->getMaintainer(),
+        ]));
+    }
+
+    private static function createGithubSearchUrl($bridge): string
+    {
+        return sprintf(
+            'https://github.com/RSS-Bridge/rss-bridge/issues?q=%s',
+            urlencode('is:issue is:open ' . $bridge->getName())
+        );
     }
 }
diff --git a/index.php b/index.php
index 2fc5771f..39e970c5 100644
--- a/index.php
+++ b/index.php
@@ -26,7 +26,18 @@ try {
     }
 } catch (\Throwable $e) {
     error_log($e);
+
+    $message = sprintf(
+        'Uncaught Exception %s: %s at %s line %s',
+        get_class($e),
+        $e->getMessage(),
+        trim_path_prefix($e->getFile()),
+        $e->getLine()
+    );
+
+    http_response_code(500);
     print render('error.html.php', [
-        'message' => sprintf("Uncaught Exception %s: '%s'\n", get_class($e), $e->getMessage()),
+        'message' => $message,
+        'stacktrace' => create_sane_stacktrace($e),
     ]);
 }
diff --git a/lib/Exceptions.php b/lib/Exceptions.php
deleted file mode 100644
index 489cf56a..00000000
--- a/lib/Exceptions.php
+++ /dev/null
@@ -1,201 +0,0 @@
-<?php
-
-/**
- * This file is part of RSS-Bridge, a PHP project capable of generating RSS and
- * Atom feeds for websites that don't have one.
- *
- * For the full license information, please view the UNLICENSE file distributed
- * with this source code.
- *
- * @package Core
- * @license http://unlicense.org/ UNLICENSE
- * @link    https://github.com/rss-bridge/rss-bridge
- */
-
-/**
- * Builds a GitHub search query to find open bugs for the current bridge
- */
-function buildGitHubSearchQuery($bridgeName)
-{
-    return REPOSITORY
-    . 'issues?q='
-    . urlencode('is:issue is:open ' . $bridgeName);
-}
-
-/**
- * Returns an URL that automatically populates a new issue on GitHub based
- * on the information provided
- *
- * @param string $title string Sets the title of the issue
- * @param string $body string Sets the body of the issue (GitHub markdown applies)
- * @param string $labels mixed (optional) Specifies labels to add to the issue
- * @param string $maintainer string (optional) Specifies the maintainer for the issue.
- * The maintainer only applies if part of the development team!
- * @return string|null A qualified URL to a new issue with populated conent or null.
- *
- * @todo This function belongs inside a class
- */
-function buildGitHubIssueQuery($title, $body, $labels = null, $maintainer = null)
-{
-    if (!isset($title) || !isset($body) || empty($title) || empty($body)) {
-        return null;
-    }
-
-    // Add title and body
-    $uri = REPOSITORY
-        . 'issues/new?title='
-        . urlencode($title)
-        . '&body='
-        . urlencode($body);
-
-    // Add labels
-    if (!is_null($labels) && is_array($labels) && count($labels) > 0) {
-        if (count($lables) === 1) {
-            $uri .= '&labels=' . urlencode($labels[0]);
-        } else {
-            foreach ($labels as $label) {
-                $uri .= '&labels[]=' . urlencode($label);
-            }
-        }
-    } elseif (!is_null($labels) && is_string($labels)) {
-        $uri .= '&labels=' . urlencode($labels);
-    }
-
-    // Add maintainer
-    if (!empty($maintainer)) {
-        $uri .= '&assignee=' . urlencode($maintainer);
-    }
-
-    return $uri;
-}
-
-function buildBridgeException(\Throwable $e, BridgeInterface $bridge): string
-{
-    $title = $bridge->getName() . ' failed with error ' . $e->getCode();
-
-    // Build a GitHub compatible message
-    $body = 'Error message: `'
-    . $e->getMessage()
-    . "`\nQuery string: `"
-    . ($_SERVER['QUERY_STRING'] ?? '')
-    . "`\nVersion: `"
-    . Configuration::getVersion()
-    . '`';
-
-    $body_html = nl2br($body);
-    $link = buildGitHubIssueQuery($title, $body, 'Bridge-Broken', $bridge->getMaintainer());
-    $searchQuery = buildGitHubSearchQuery($bridge::NAME);
-
-    $header = buildHeader($e, $bridge);
-    $message = <<<EOD
-<strong>{$bridge->getName()}</strong> was unable to receive or process the
-remote website's content!<br>
-{$body_html}
-EOD;
-    $section = buildSection($e, $bridge, $message, $link, $searchQuery);
-
-    return $section;
-}
-
-function buildTransformException(\Throwable $e, BridgeInterface $bridge): string
-{
-    $title = $bridge->getName() . ' failed with error ' . $e->getCode();
-
-    // Build a GitHub compatible message
-    $body = 'Error message: `'
-    . $e->getMessage()
-    . "`\nQuery string: `"
-    . ($_SERVER['QUERY_STRING'] ?? '')
-    . '`';
-
-    $link = buildGitHubIssueQuery($title, $body, 'Bridge-Broken', $bridge->getMaintainer());
-    $searchQuery = buildGitHubSearchQuery($bridge::NAME);
-    $header = buildHeader($e, $bridge);
-    $message = "RSS-Bridge was unable to transform the contents returned by
-<strong>{$bridge->getName()}</strong>!";
-    $section = buildSection($e, $bridge, $message, $link, $searchQuery);
-
-    return buildPage($title, $header, $section);
-}
-
-/**
- * Builds a new HTML header with data from a exception an a bridge
- *
- * @param object $e The exception object
- * @param object $bridge The bridge object
- * @return string The HTML header
- *
- * @todo This function belongs inside a class
- */
-function buildHeader($e, $bridge)
-{
-    return <<<EOD
-<header>
-	<h1>Error {$e->getCode()}</h1>
-	<h2>{$e->getMessage()}</h2>
-	<p class="status">{$bridge->getName()}</p>
-</header>
-EOD;
-}
-
-/**
- * Builds a new HTML section
- *
- * @param object $e The exception object
- * @param object $bridge The bridge object
- * @param string $message The message to display
- * @param string $link The link to include in the anchor
- * @param string $searchQuery A GitHub search query for the current bridge
- * @return string The HTML section
- *
- * @todo This function belongs inside a class
- */
-function buildSection($e, $bridge, $message, $link, $searchQuery)
-{
-    return <<<EOD
-<section>
-	<p class="exception-message">{$message}</p>
-	<div class="advice">
-		<ul class="advice">
-			<li>Press Return to check your input parameters</li>
-			<li>Press F5 to retry</li>
-			<li>Check if this issue was already reported on <a href="{$searchQuery}">GitHub</a> (give it a thumbs-up)</li>
-			<li>Open a <a href="{$link}">GitHub Issue</a> if this error persists</li>
-		</ul>
-	</div>
-	<a href="{$searchQuery}" title="Opens GitHub to search for similar issues">
-		<button>Search GitHub Issues</button>
-	</a>
-	<a href="{$link}" title="After clicking this button you can review
-	the issue before submitting it"><button>Open GitHub Issue</button></a>
-	<p class="maintainer">{$bridge->getMaintainer()}</p>
-</section>
-EOD;
-}
-
-/**
- * Builds a new HTML page
- *
- * @param string $title The HTML title
- * @param string $header The HTML header
- * @param string $section The HTML section
- * @return string The HTML page
- *
- * @todo This function belongs inside a class
- */
-function buildPage($title, $header, $section)
-{
-    return <<<EOD
-<!DOCTYPE html>
-<html lang="en">
-<head>
-	<title>{$title}</title>
-	<link href="static/style.css" rel="stylesheet">
-</head>
-<body>
-	{$header}
-	{$section}
-</body>
-</html>
-EOD;
-}
diff --git a/lib/error.php b/lib/error.php
index b8b2f9af..1a3d294d 100644
--- a/lib/error.php
+++ b/lib/error.php
@@ -79,3 +79,34 @@ function logBridgeError($bridgeName, $code)
 
     return $report['count'];
 }
+
+function create_sane_stacktrace(\Throwable $e): array
+{
+    $frames = array_reverse($e->getTrace());
+    $frames[] = [
+        'file' => $e->getFile(),
+        'line' => $e->getLine(),
+    ];
+    $stackTrace = [];
+    foreach ($frames as $i => $frame) {
+        $file = $frame['file'] ?? '(no file)';
+        $line = $frame['line'] ?? '(no line)';
+        $stackTrace[] = sprintf(
+            '#%s %s:%s',
+            $i,
+            trim_path_prefix($file),
+            $line,
+        );
+    }
+    return $stackTrace;
+}
+
+/**
+ * Trim path prefix for privacy/security reasons
+ *
+ * Example: "/var/www/rss-bridge/index.php" => "index.php"
+ */
+function trim_path_prefix(string $filePath): string
+{
+    return mb_substr($filePath, mb_strlen(dirname(__DIR__)) + 1);
+}
diff --git a/lib/rssbridge.php b/lib/rssbridge.php
index 560c0fe4..33f9bf06 100644
--- a/lib/rssbridge.php
+++ b/lib/rssbridge.php
@@ -51,8 +51,7 @@ define('FILE_CONFIG_DEFAULT', PATH_ROOT . 'config.default.ini.php');
 /** URL to the RSS-Bridge repository */
 define('REPOSITORY', 'https://github.com/RSS-Bridge/rss-bridge/');
 
-// Functions
-require_once PATH_LIB . 'Exceptions.php';
+// Files
 require_once PATH_LIB . 'html.php';
 require_once PATH_LIB . 'error.php';
 require_once PATH_LIB . 'contents.php';
diff --git a/phpcs.xml b/phpcs.xml
index 3f4450d2..599fc9ca 100644
--- a/phpcs.xml
+++ b/phpcs.xml
@@ -3,6 +3,7 @@
   <description>Created with the PHP Coding Standard Generator. http://edorian.github.com/php-coding-standard-generator/</description>
   <exclude-pattern>./static</exclude-pattern>
   <exclude-pattern>./vendor</exclude-pattern>
+  <exclude-pattern>./templates</exclude-pattern>
   <exclude-pattern>./config.default.ini.php</exclude-pattern>
   <exclude-pattern>./config.ini.php</exclude-pattern>
 
diff --git a/static/style.css b/static/style.css
index d8e954d2..26bd74a0 100644
--- a/static/style.css
+++ b/static/style.css
@@ -1,13 +1,28 @@
-html, body, div, span, applet, object, iframe, h1, h2, h3, h4, h5, h6, p, blockquote, pre, a, abbr, acronym, address, big, cite, code, del, dfn, em, img, ins, kbd, q, s, samp, small, strike, strong, sub, sup, tt, var, b, u, i, center, dl, dt, dd, ol, ul, li, fieldset, form, label, legend, table, caption, tbody, tfoot, thead, tr, th, td, article, aside, canvas, details, figcaption, figure, footer, header, hgroup, menu, nav, section, summary, time, mark, audio, video {
+html {
+	box-sizing: border-box;
+	font-size: 16px;
+}
+
+*, *:before, *:after {
+	box-sizing: inherit;
+}
+
+body, h1, h2, h3, h4, h5, h6, p, ol, ul {
 	margin: 0;
 	padding: 0;
-	border: 0;
-	outline: 0;
-	font-size: 100%;
-	font: inherit;
-	vertical-align: baseline;
+	font-weight: normal;
 }
 
+ol, ul {
+	list-style: none;
+}
+
+img {
+	max-width: 100%;
+	height: auto;
+}
+
+
 /* HTML5 display-role reset for older browsers */
 article, aside, details, figcaption, figure, footer, header, hgroup, menu, nav, section {
 	display: block;
@@ -23,7 +38,7 @@ article, aside, details, figcaption, figure, footer, header, hgroup, menu, nav,
 /* Let's go for the actual style */
 body {
 	background-color: #f0f0f0;
-	font-family: -apple-system,BlinkMacSystemFont,"Segoe UI",Roboto,Helvetica,Arial,sans-serif,"Apple Color Emoji","Segoe UI Emoji","Segoe UI Symbol";
+	font-family: sans-serif;
 }
 
 a, a:link, a:visited {
@@ -434,4 +449,4 @@ h5 {
 	.showmore:hover, .showless:hover {
 		color: #d8d3cb;
 	}
-}
\ No newline at end of file
+}
diff --git a/templates/base.html.php b/templates/base.html.php
index 9e0cc7ca..39442706 100644
--- a/templates/base.html.php
+++ b/templates/base.html.php
@@ -15,3 +15,4 @@
 
     <?= raw($page) ?>
 </html>
+
diff --git a/templates/bridge-error.html.php b/templates/bridge-error.html.php
new file mode 100644
index 00000000..51711c9f
--- /dev/null
+++ b/templates/bridge-error.html.php
@@ -0,0 +1,43 @@
+<section>
+    <p class="exception-message">
+        <?= e($message ?? '') ?>
+    </p>
+
+    <?php if (isset($stacktrace)): ?>
+        <?php foreach ($stacktrace as $frame): ?>
+            <code>
+                <?= e($frame) ?>
+            </code>
+            <br>
+        <?php endforeach; ?>
+    <?php endif; ?>
+
+    <br>
+
+    <p>
+        Query string: <?= e($_SERVER['QUERY_STRING'] ?? '') ?>
+    </p>
+    <p>
+        Version: <?= e(Configuration::getVersion()) ?>
+    </p>
+
+    <div class="advice">
+        <ul class="advice">
+            <li>Press Return to check your input parameters</li>
+            <li>Press F5 to retry</li>
+            <li>Check if this issue was already reported on <a href="<?= raw($searchUrl) ?>">GitHub</a> (give it a thumbs-up)</li>
+            <li>Open a <a href="<?= raw($issueUrl) ?>">GitHub Issue</a> if this error persists</li>
+        </ul>
+    </div>
+
+    <a href="<?= raw($searchUrl) ?>" title="Opens GitHub to search for similar issues">
+        <button>Search GitHub Issues</button>
+    </a>
+
+    <a href="<?= raw($issueUrl) ?>" title="After clicking this button you can review the issue before submitting it">
+        <button>Open GitHub Issue</button>
+    </a>
+
+    <p class="maintainer"><?= e($bridge->getMaintainer()) ?></p>
+</section>
+
diff --git a/templates/error.html.php b/templates/error.html.php
index 4664b51d..db2f233f 100644
--- a/templates/error.html.php
+++ b/templates/error.html.php
@@ -1,9 +1,19 @@
-<section>
-    <h2>Something went wrong</h2>
+<div style="width: 60%; margin: 30px auto">
+
+    <h1>Something went wrong</h1>
+    <br>
+    <?= e($message) ?>
     <br>
+    <br>
+
+    <?php if (isset($stacktrace)): ?>
+        <?php foreach ($stacktrace as $frame) : ?>
+            <code>
+                <?= e($frame) ?>
+            </code>
+            <br>
+        <?php endforeach; ?>
+    <?php endif; ?>
 
-    <p>
-        <?= e($message) ?>
-    </p>
+</div>
 
-</section>