From b90bcee1fcfe7f1f5d285834042152c8856d7ddb Mon Sep 17 00:00:00 2001 From: LogMANOriginal Date: Mon, 15 Oct 2018 17:21:43 +0200 Subject: [PATCH] Return exceptions in requested feed formats (#841) * [Exceptions] Don't return header for bridge exceptions * [Exceptions] Add link to list in exception message This is an alternative when the button is not rendered for some reason. * [index] Don't return bridge exception for formats * [index] Return feed item for bridge exceptions * [BridgeAbstract] Rename 'getCacheTime' to 'getModifiedTime' * [BridgeAbstract] Move caching to index.php to separate concerns index.php needs more control over caching behavior in order to cache exceptions. This cannot be done in a bridge, as the bridge might be broken, thus preventing caching from working. This also (and more importantly) separates concerns. The bridge should not even care if caching is involved or not. Its purpose is to collect and provide data. Response times should be faster, as more complex bridge functions like 'setDatas' (evaluates all input parameters to predict the current context) and 'collectData' (collects data from sites) can be skipped entirely. Notice: In its current form, index.php takes care of caching. This could, however, be moved into a separate class (i.e. CacheAbstract) in order to make implementation details cache specific. * [index] Add '_error_time' parameter to $item['uri'] This ensures that error messages are recognized by feed readers as new errors after 24 hours. During that time the same item is returned no matter how often the cache is cleared. References https://github.com/RSS-Bridge/rss-bridge/issues/814#issuecomment-420876162 * [index] Include '_error_time' in the title for errors This prevents feed readers from "updating" feeds based on the title * [index] Handle "HTTP_IF_MODIFIED_SINCE" client requests Implementation is based on `BridgeAbstract::dieIfNotModified()`, introduced in 422c125d8e25b9ac5209580b1976fb5d75dab8f8 and simplified based on https://stackoverflow.com/a/10847262 Basically, before returning cached data we check if the client send the "HTTP_IF_MODIFIED_SINCE" header. If the modification time is more recent or equal to the cache time, we reply with "HTTP/1.1 304 Not Modified" (same as before). Otherwise send the cached data. * [index] Don't encode exception message with `htmlspecialchars` * [Exceptions] Include error message in exception * [index] Show different error message for error code 0 --- index.php | 104 ++++++++++++++++++++++++++++++++-------- lib/BridgeAbstract.php | 99 ++------------------------------------ lib/BridgeInterface.php | 30 ------------ lib/Exceptions.php | 12 +++-- 4 files changed, 96 insertions(+), 149 deletions(-) diff --git a/index.php b/index.php index 0bab8a0a..2cb0286d 100644 --- a/index.php +++ b/index.php @@ -178,14 +178,18 @@ try { define('NOPROXY', true); } - // Custom cache timeout + // Cache timeout $cache_timeout = -1; if(array_key_exists('_cache_timeout', $params)) { + if(!CUSTOM_CACHE_TIMEOUT) { throw new \HttpException('This server doesn\'t support "_cache_timeout"!'); } $cache_timeout = filter_var($params['_cache_timeout'], FILTER_VALIDATE_INT); + + } else { + $cache_timeout = $bridge->getCacheTimeout(); } // Remove parameters that don't concern bridges @@ -219,28 +223,90 @@ try { $cache->purgeCache(86400); // 24 hours $cache->setParameters($cache_params); - // Load cache & data - try { - $bridge->setCache($cache); - $bridge->setCacheTimeout($cache_timeout); - $bridge->dieIfNotModified(); - $bridge->setDatas($bridge_params); - } catch(Error $e) { - http_response_code($e->getCode()); - header('Content-Type: text/html'); - die(buildBridgeException($e, $bridge)); - } catch(Exception $e) { - http_response_code($e->getCode()); - header('Content-Type: text/html'); - die(buildBridgeException($e, $bridge)); + $items = array(); + $infos = array(); + $mtime = $cache->getTime(); + + if($mtime !== false + && (time() - $cache_timeout < $mtime) + && (!defined('DEBUG') || DEBUG !== true)) { // Load cached data + + // Send "Not Modified" response if client supports it + // Implementation based on https://stackoverflow.com/a/10847262 + if(isset($_SERVER['HTTP_IF_MODIFIED_SINCE'])) { + $stime = strtotime($_SERVER['HTTP_IF_MODIFIED_SINCE']); + + if($mtime <= $stime) { // Cached data is older or same + header('HTTP/1.1 304 Not Modified'); + die(); + } + } + + $cached = $cache->loadData(); + + if(isset($cached['items']) && isset($cached['extraInfos'])) { + $items = $cached['items']; + $infos = $cached['extraInfos']; + } + + } else { // Collect new data + + try { + $bridge->setDatas($bridge_params); + $bridge->collectData(); + + $items = $bridge->getItems(); + $infos = array( + 'name' => $bridge->getName(), + 'uri' => $bridge->getURI(), + 'icon' => $bridge->getIcon() + ); + } catch(Error $e) { + $item = array(); + + // Create "new" error message every 24 hours + $params['_error_time'] = urlencode((int)(time() / 86400)); + + // Error 0 is a special case (i.e. "trying to get property of non-object") + if($e->getCode() === 0) { + $item['title'] = 'Bridge encountered an unexpected situation! (' . $params['_error_time'] . ')'; + } else { + $item['title'] = 'Bridge returned error ' . $e->getCode() . '! (' . $params['_error_time'] . ')'; + } + + $item['uri'] = parse_url($_SERVER['REQUEST_URI'], PHP_URL_PATH) . '?' . http_build_query($params); + $item['timestamp'] = time(); + $item['content'] = buildBridgeException($e, $bridge); + + $items[] = $item; + } catch(Exception $e) { + $item = array(); + + // Create "new" error message every 24 hours + $params['_error_time'] = urlencode((int)(time() / 86400)); + + $item['uri'] = parse_url($_SERVER['REQUEST_URI'], PHP_URL_PATH) . '?' . http_build_query($params); + $item['title'] = 'Bridge returned error ' . $e->getCode() . '! (' . $params['_error_time'] . ')'; + $item['timestamp'] = time(); + $item['content'] = buildBridgeException($e, $bridge); + + $items[] = $item; + } + + // Store data in cache + $cache->saveData(array( + 'items' => $items, + 'extraInfos' => $infos + )); + } // Data transformation try { $format = Format::create($format); - $format->setItems($bridge->getItems()); - $format->setExtraInfos($bridge->getExtraInfos()); - $format->setLastModified($bridge->getCacheTime()); + $format->setItems($items); + $format->setExtraInfos($infos); + $format->setLastModified($mtime); $format->display(); } catch(Error $e) { http_response_code($e->getCode()); @@ -249,7 +315,7 @@ try { } catch(Exception $e) { http_response_code($e->getCode()); header('Content-Type: text/html'); - die(buildBridgeException($e, $bridge)); + die(buildTransformException($e, $bridge)); } } else { echo BridgeList::create($whitelist_selection, $showInactive); diff --git a/lib/BridgeAbstract.php b/lib/BridgeAbstract.php index c44d95da..014201c8 100644 --- a/lib/BridgeAbstract.php +++ b/lib/BridgeAbstract.php @@ -9,23 +9,9 @@ abstract class BridgeAbstract implements BridgeInterface { const CACHE_TIMEOUT = 3600; const PARAMETERS = array(); - protected $cache; - protected $extraInfos; protected $items = array(); protected $inputs = array(); protected $queriedContext = ''; - protected $cacheTimeout; - - /** - * Return cachable datas (extrainfos and items) stored in the bridge - * @return mixed - */ - public function getCachable(){ - return array( - 'items' => $this->getItems(), - 'extraInfos' => $this->getExtraInfos() - ); - } /** * Return items stored in the bridge @@ -118,34 +104,18 @@ abstract class BridgeAbstract implements BridgeInterface { /** * Defined datas with parameters depending choose bridge - * Note : you can define a cache with "setCache" * @param array array with expected bridge paramters */ public function setDatas(array $inputs){ - if(!is_null($this->cache)) { - $time = $this->cache->getTime(); - if($time !== false - && (time() - $this->getCacheTimeout() < $time) - && (!defined('DEBUG') || DEBUG !== true)) { - $cached = $this->cache->loadData(); - if(isset($cached['items']) && isset($cached['extraInfos'])) { - $this->items = $cached['items']; - $this->extraInfos = $cached['extraInfos']; - return; - } - } - } if(empty(static::PARAMETERS)) { + if(!empty($inputs)) { returnClientError('Invalid parameters value(s)'); } - $this->collectData(); - if(!is_null($this->cache)) { - $this->cache->saveData($this->getCachable()); - } return; + } $validator = new ParameterValidator(); @@ -172,11 +142,6 @@ abstract class BridgeAbstract implements BridgeInterface { $this->setInputs($inputs, $this->queriedContext); - $this->collectData(); - - if(!is_null($this->cache)) { - $this->cache->saveData($this->getCachable()); - } } /** @@ -201,20 +166,10 @@ abstract class BridgeAbstract implements BridgeInterface { } public function getName(){ - // Return cached name when bridge is using cached data - if(isset($this->extraInfos)) { - return $this->extraInfos['name']; - } - return static::NAME; } public function getIcon(){ - // Return cached icon when bridge is using cached data - if(isset($this->extraInfos)) { - return $this->extraInfos['icon']; - } - return ''; } @@ -223,59 +178,11 @@ abstract class BridgeAbstract implements BridgeInterface { } public function getURI(){ - // Return cached uri when bridge is using cached data - if(isset($this->extraInfos)) { - return $this->extraInfos['uri']; - } - return static::URI; } - public function getExtraInfos(){ - return array( - 'name' => $this->getName(), - 'uri' => $this->getURI(), - 'icon' => $this->getIcon() - ); - } - - public function setCache(\CacheInterface $cache){ - $this->cache = $cache; - } - - public function setCacheTimeout($timeout){ - if(is_numeric($timeout) && ($timeout < 1 || $timeout > 86400)) { - $this->cacheTimeout = static::CACHE_TIMEOUT; - return; - } - - $this->cacheTimeout = $timeout; - } - public function getCacheTimeout(){ - return isset($this->cacheTimeout) ? $this->cacheTimeout : static::CACHE_TIMEOUT; + return static::CACHE_TIMEOUT; } - public function getCacheTime(){ - return !is_null($this->cache) ? $this->cache->getTime() : false; - } - - public function dieIfNotModified(){ - if ((defined('DEBUG') && DEBUG === true)) return; // disabled in debug mode - - $if_modified_since = isset($_SERVER['HTTP_IF_MODIFIED_SINCE']) ? $_SERVER['HTTP_IF_MODIFIED_SINCE'] : false; - if (!$if_modified_since) return; // If-Modified-Since value is required - - $last_modified = $this->getCacheTime(); - if (!$last_modified) return; // did not detect cache time - - if (time() - $this->getCacheTimeout() > $last_modified) return; // cache timeout - - $last_modified = (gmdate('D, d M Y H:i:s ', $last_modified) . 'GMT'); - - if ($if_modified_since == $last_modified) { - header('HTTP/1.1 304 Not Modified'); - die(); - } - } } diff --git a/lib/BridgeInterface.php b/lib/BridgeInterface.php index d67b99ed..f2ff11d5 100644 --- a/lib/BridgeInterface.php +++ b/lib/BridgeInterface.php @@ -6,13 +6,6 @@ interface BridgeInterface { */ public function collectData(); - /** - * Returns an array of cachable elements - * - * @return array Associative array of cachable elements - */ - public function getCachable(); - /** * Returns the description * @@ -20,13 +13,6 @@ interface BridgeInterface { */ public function getDescription(); - /** - * Return an array of extra information - * - * @return array Associative array of extra information - */ - public function getExtraInfos(); - /** * Returns an array of collected items * @@ -69,22 +55,6 @@ interface BridgeInterface { */ public function getURI(); - /** - * Sets the cache instance - * - * @param object CacheInterface The cache instance - */ - public function setCache(\CacheInterface $cache); - - /** - * Sets the timeout for clearing the cache files. The timeout must be - * specified between 1..86400 seconds (max. 24 hours). The default timeout - * (specified by the bridge maintainer) applies for invalid values. - * - * @param int $timeout The cache timeout in seconds - */ - public function setCacheTimeout($timeout); - /** * Returns the cache timeout * diff --git a/lib/Exceptions.php b/lib/Exceptions.php index 9b893208..24849bd4 100644 --- a/lib/Exceptions.php +++ b/lib/Exceptions.php @@ -69,14 +69,18 @@ function buildBridgeException($e, $bridge){ . Configuration::getVersion() . '`'; + $body_html = nl2br($body); $link = buildGitHubIssueQuery($title, $body, 'bug report', $bridge->getMaintainer()); $header = buildHeader($e, $bridge); - $message = "{$bridge->getName()} was -unable to receive or process the remote website's content!"; + $message = <<{$bridge->getName()} was unable to receive or process the +remote website's content!
+{$body_html} +EOD; $section = buildSection($e, $bridge, $message, $link); - return buildPage($title, $header, $section); + return $section; } /** @@ -127,7 +131,7 @@ function buildSection($e, $bridge, $message, $link){