Download feed logos via guzzle to have better error handling

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>

AUTHORS.md

@@ -6,9 +6,9 @@
 * [Robin Appelman](mailto:icewind@owncloud.com)
 * [Sean Molenaar](mailto:sean@seanmolenaar.eu)
 * [Gregor Tätzner](mailto:gregor@freenet.de)
+* [anoy](mailto:anoymouserver+github@mailbox.org)
 * [Sean Molenaar](mailto:SMillerDev@users.noreply.github.com)
 * [Morris Jobke](mailto:hey@morrisjobke.de)
-* [anoy](mailto:anoymouserver+github@mailbox.org)
 * [Jimmy Huynh](mailto:jimmy.huynh@etu.unistra.fr)
 * [Aurélien](mailto:dav.aurelien@gmail.com)
 * [Jan-Christoph Borchardt](mailto:hey@jancborchardt.net)
@@ -29,6 +29,7 @@
 * [IBBoard](mailto:dev@ibboard.co.uk)
 * [Koen Martens](mailto:gmc@sonologic.nl)
 * [Lukas Reschke](mailto:lukas@owncloud.com)
+* [Tucker McKnight](mailto:tucker.mcknight@gmail.com)
 * [Bart Visscher](mailto:bartv@thisnet.nl)
 * [Christian Elmer](mailto:christian@keinkurt.de)
 * [Nicolas Wendling](mailto:nicolas.wendling1011@gmail.com)
@@ -36,6 +37,7 @@
 * [Volkan Gezer](mailto:volkangezer@gmail.com)
 * [Xéfir Destiny](mailto:xefir@crystalyx.net)
 * [Daniel Opitz](mailto:danopz@users.noreply.github.com)
+* [Daniel Rheinbay](mailto:danielrheinbay@gmail.com)
 * [Lars Bensmann](mailto:lars@almosthappy.de)
 * [Robin Appelman](mailto:robin@icewind.nl)
 * [bluehaze](mailto:francesco.sportolari@gmail.com)
@@ -73,6 +75,7 @@
 * [davidak](mailto:git@davidak.de)
 * [lsmooth](mailto:ls@lsmooth.de)
 * [s17t.net](mailto:mail+github@s17t.net)
+* [Alec Kojaev](mailto:alec@kojaev.name)
 * [Alessandro](mailto:cosenal@gmail.com)
 * [Alexander Grüßung](mailto:alexander@gruessung-online.de)
 * [Allan Nordhøy](mailto:epost@anotheragency.no)
@@ -95,6 +98,7 @@
 * [Colin W](mailto:cwmke@users.noreply.github.com)
 * [Daniel Aleksandersen](mailto:code@daniel.priv.no)
 * [Daniel S](mailto:daniel@while-true-do.org)
+* [David Baucum](mailto:david@baucum.me)
 * [David Engster](mailto:deng@randomsample.de)
 * [Detlev Zundel](mailto:dzu@member.fsf.org)
 * [Doron Behar](mailto:doron.behar@gmail.com)
@@ -128,6 +132,7 @@
 * [Pierre Ozoux](mailto:pierre@ozoux.net)
 * [Piotr Dobrowolski](mailto:admin@tastycode.pl)
 * [Raspbeguy](mailto:raspbeguy@users.noreply.github.com)
+* [René Henrich](mailto:contact@rene-henrich.de)
 * [Rodrigo Aguilera](mailto:rodrigo.aguilera@amazee.com)
 * [Roeland Jago Douma](mailto:roeland@famdouma.nl)
 * [Simon](mailto:sschubert89@gmail.com)
@@ -140,13 +145,13 @@
 * [Thomas Wouters](mailto:twouters@users.noreply.github.com)
 * [Tilo Spannagel](mailto:development@tilosp.de)
 * [Timo Schmidt](mailto:timo@xinterchange.net)
-* [Tucker McKnight](mailto:tucker.mcknight@gmail.com)
 * [WENDLING NICOLAS](mailto:nicolas.wendling1011@gmail.com)
 * [Welling Guzmán](mailto:WellingGuzman@users.noreply.github.com)
 * [Xaver Maierhofer](mailto:xaver.maierhofer@xwissen.info)
 * [Xemle](mailto:xemle@phtagr.org)
 * [YMHuang](mailto:ymhuang@fmbase.tw)
 * [Zach DeCook](mailto:zachdecook@gmail.com)
+* [Zach DeCook](mailto:zachdecook@librem.one)
 * [amittel](mailto:arnulf.mittelstaedt@gmail.com)
 * [b_b](mailto:brunobergot@gmail.com)
 * [bjoerns1983](mailto:bjoern@sengotta.net)
@@ -161,6 +166,7 @@
 * [kondou](mailto:kondou@ts.unde.re)
 * [markusj](mailto:markusj@users.noreply.github.com)
 * [mnassabain](mailto:34754819+mnassabain@users.noreply.github.com)
+* [mormegil](mailto:mormegil@centrum.cz)
 * [nexus-uw](mailto:you@example.com)
 * [repat](mailto:repat@repat.de)
 * [ritchiewilson](mailto:rawilson52@gmail.com)

CHANGELOG.md

@@ -7,9 +7,11 @@ The format is almost based on [Keep a Changelog](https://keepachangelog.com/en/1
 ## [16.x.x]
 ### Changed
 - Add changelog and DCO notice to CONTRIBUTING.md (#1521)
+- Download feed logos via guzzle to have better error handling (#1533)
 
 ### Fixed
 
+
 # Releases
 ## [16.1.0] - 2021-10-07
 ### Changed

composer.json

@@ -55,15 +55,15 @@
   },
   "require-dev": {
     "phpunit/phpunit": "9.5.*",
-    "squizlabs/php_codesniffer": "^3.5.6",
+    "squizlabs/php_codesniffer": "^3.6.1",
-    "phpstan/phpstan": "^0.12.43",
+    "phpstan/phpstan": "^0.12.99",
-    "phpstan/phpstan-doctrine": "^0.12.22",
+    "phpstan/phpstan-doctrine": "^0.12.44",
-    "phpstan/phpstan-strict-rules": "^0.12.5",
+    "phpstan/phpstan-strict-rules": "^0.12.11",
-    "phpstan/phpstan-phpunit": "^0.12.16",
+    "phpstan/phpstan-phpunit": "^0.12.22",
-    "phpstan/extension-installer": "^1.0",
+    "phpstan/extension-installer": "^1.1.0",
-    "guzzlehttp/guzzle": "^7.2",
+    "guzzlehttp/guzzle": "^7.3.0",
-    "doctrine/dbal": "^3.0",
+    "doctrine/dbal": "^3.1.3",
-    "symfony/console": "^4.4.18",
+    "symfony/console": "^4.4.19",
     "psr/log": "^1.1.0"
   },
   "replace": {

composer.lock

@@ -4,7 +4,7 @@
         "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies",
         "This file is @generated automatically"
     ],
-    "content-hash": "1932196eb576f7eb70b3846487613b5a",
+    "content-hash": "6487fb576db846a56aaf0d45f2469a8f",
     "packages": [
         {
             "name": "arthurhoaro/favicon",
@@ -2864,6 +2864,7 @@
                     "type": "github"
                 }
             ],
+            "abandoned": true,
             "time": "2020-09-28T06:45:17+00:00"
         },
         {

lib/Fetcher/FeedFetcher.php

@@ -18,6 +18,8 @@ use Favicon\Favicon;
 use FeedIo\Feed\ItemInterface;
 use FeedIo\FeedInterface;
 use FeedIo\FeedIo;
+use GuzzleHttp\Client;
+use GuzzleHttp\Exception\RequestException;
 
 use Net_URL2;
 use OCP\IL10N;
@@ -68,6 +70,11 @@ class FeedFetcher implements IFeedFetcher
      */
     private $logger;
 
+    /**
+     * @var Client
+     */
+    private $client;
+
     public function __construct(
         FeedIo $fetcher,
         Favicon $favicon,
@@ -75,7 +82,8 @@ class FeedFetcher implements IFeedFetcher
         IL10N $l10n,
         ITempManager $ITempManager,
         Time $time,
-        LoggerInterface $logger
+        LoggerInterface $logger,
+        Client $client
     ) {
         $this->reader         = $fetcher;
         $this->faviconFactory = $favicon;
@@ -84,6 +92,7 @@ class FeedFetcher implements IFeedFetcher
         $this->ITempManager   = $ITempManager;
         $this->time           = $time;
         $this->logger         = $logger;
+        $this->client         = $client;
     }
 
 
@@ -318,14 +327,15 @@ class FeedFetcher implements IFeedFetcher
     /**
      * Return the favicon for a given feed and url
      *
-     * @param FeedInterface $feed     Feed to check for a logo
+     * @param FeedInterface $feed Feed to check for a logo
-     * @param string        $url      Original URL for the feed
+     * @param string        $url  Original URL for the feed
      *
      * @return string|mixed|bool
      */
     protected function getFavicon(FeedInterface $feed, string $url)
     {
-        $favicon = $feed->getLogo();
+        // trim the string because authors do funny things
+        $favicon = trim($feed->getLogo());
 
         ini_set('user_agent', 'NextCloud-News/1.0');
 
@@ -334,17 +344,61 @@ class FeedFetcher implements IFeedFetcher
         $base_url = $base_url->getNormalizedURL();
 
         // check if feed has a logo entry
-        if (is_null($favicon) || trim($favicon) === '') {
+        if (is_null($favicon) || $favicon === '') {
             return $this->faviconFactory->get($base_url);
         }
 
-        $favicon_path = join("/", [$this->ITempManager->getTempBaseDir(), basename($favicon)]);
+        // logo will be saved in the tmp folder provided by Nextcloud, file is named as md5 of the url
+        $favicon_path = join("/", [$this->ITempManager->getTempBaseDir(), md5($favicon)]);
+        $downloaded = false;
 
-        $downloaded = copy(
+        if (file_exists($favicon_path)) {
-            $favicon,
+            $last_modified = filemtime($favicon_path);
-            $favicon_path,
+        } else {
-            stream_context_create([ 'http' => [ 'ignore_errors' => true ] ])
+            $last_modified = 0;
-        );
+        }
+
+        try {
+            $response = $this->client->request(
+                'GET',
+                $favicon,
+                [
+                    'sink' => $favicon_path,
+                    'headers' => [
+                        'User-Agent'        => 'NextCloud-News/1.0',
+                        'Accept'            => 'image/*',
+                        'If-Modified-Since' => date(DateTime::RFC7231, $last_modified)
+                    ]
+                ]
+            );
+            $downloaded = true;
+
+            $this->logger->debug(
+                "Feed:{url} Logo:{logo} Status:{status}",
+                [
+                'status' => $response->getStatusCode(),
+                'url'    => $favicon_path,
+                'logo'   => $favicon
+                ]
+            );
+        } catch (RequestException $e) {
+            if ($e->hasResponse()) {
+                $this->logger->info(
+                    'An error occurred while trying to download the feed logo of {url}: {error}',
+                    [
+                    'url'   => $url,
+                    'error' => $e->getResponse()
+                    ]
+                );
+            } else {
+                $this->logger->info(
+                    'An unknown error occurred while trying to download the feed logo of {url}.',
+                    [
+                    'url' => $url,
+                    ]
+                );
+            }
+        }
 
         $is_image = $downloaded && substr(mime_content_type($favicon_path), 0, 5) === "image";
 

tests/Unit/Fetcher/FeedFetcherTest.php

@@ -28,6 +28,7 @@ use \OCA\News\Db\Feed;
 use \OCA\News\Db\Item;
 use OCA\News\Scraper\Scraper;
 use OCA\News\Fetcher\FeedFetcher;
+use GuzzleHttp\Client;
 
 use OCA\News\Utility\Time;
 use OCP\IL10N;
@@ -106,6 +107,11 @@ class FeedFetcherTest extends TestCase
      */
     private $scraper;
 
+    /**
+     * @var MockObject|Client
+     */
+    private $client;
+
     //metadata
     /**
      * @var integer
@@ -188,6 +194,9 @@ class FeedFetcherTest extends TestCase
         $this->scraper = $this->getMockBuilder(Scraper::class)
             ->disableOriginalConstructor()
             ->getMock();
+        $this->client = $this->getMockBuilder(Client::class)
+            ->disableOriginalConstructor()
+            ->getMock();
         $this->fetcher = new FeedFetcher(
             $this->reader,
             $this->favicon,
@@ -195,7 +204,8 @@ class FeedFetcherTest extends TestCase
             $this->l10n,
             $this->ITempManager,
             $timeFactory,
-            $this->logger
+            $this->logger,
+            $this->client
         );
         $this->url = 'http://tests/';