prefer the feeds logo over the favicon

The logo of the feed is prefered if it is a square picture,
else the favicon is returned.

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Co-authored-by: Sean Molenaar <SMillerDev@users.noreply.github.com>

9 files changed, 109 insertions, 75 deletions

diff --git a/.github/workflows/api-integration-tests.yml b/.github/workflows/api-integration-tests.yml
index 6289b31c3..3c0d057b3 100644
--- a/.github/workflows/api-integration-tests.yml
+++ b/.github/workflows/api-integration-tests.yml
@@ -127,6 +127,7 @@ jobs:
         run: |
           ./occ news:feed:add 'admin' "https://nextcloud.com/blog/feed/"
           ./occ news:feed:list 'admin' | grep 'nextcloud\.com'
+          ./occ news:feed:list 'admin' | grep -F '"faviconLink": "https:\/\/nextcloud.com\/media\/screenshot-150x150.png"'
 
       - name: Functional tests opml
         working-directory: ../server
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 05d8cd52d..7bd59c9c4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,8 @@ The format is almost based on [Keep a Changelog](https://keepachangelog.com/en/1
 ### Changed
 - Remove outdated item DB code.
 - Stop returning all feeds after marking folder as read.
+- Always fetch favicon (#1164)
+- Use feed logo instead of favicon if it exists and is square
 
 ### Fixed
 
diff --git a/lib/Command/ShowFeed.php b/lib/Command/ShowFeed.php
index 4e3084322..180c6ca3a 100644
--- a/lib/Command/ShowFeed.php
+++ b/lib/Command/ShowFeed.php
@@ -70,7 +70,7 @@ class ShowFeed extends Command
         $fullTextEnabled = (bool) $input->getOption('full-text');
 
         try {
-            list($feed, $items) = $this->feedFetcher->fetch($url, true, null, $fullTextEnabled, $user, $password);
+            list($feed, $items) = $this->feedFetcher->fetch($url, null, $fullTextEnabled, $user, $password);
         } catch (\Exception $ex) {
             $output->writeln('<error>Failed to fetch feed info:</error>');
             $output->writeln($ex->getMessage());
diff --git a/lib/Fetcher/FeedFetcher.php b/lib/Fetcher/FeedFetcher.php
index b9526165d..a70e67114 100755
--- a/lib/Fetcher/FeedFetcher.php
+++ b/lib/Fetcher/FeedFetcher.php
@@ -21,6 +21,7 @@ use FeedIo\FeedIo;
 
 use Net_URL2;
 use OCP\IL10N;
+use OCP\ITempManager;
 
 use OCA\News\Db\Item;
 use OCA\News\Db\Feed;
@@ -53,6 +54,11 @@ class FeedFetcher implements IFeedFetcher
     private $l10n;
 
     /**
+     * @var ITempManager
+     */
+    private $ITempManager;
+
+    /**
      * @var Time
      */
     private $time;
@@ -67,6 +73,7 @@ class FeedFetcher implements IFeedFetcher
         Favicon $favicon,
         Scraper $scraper,
         IL10N $l10n,
+        ITempManager $ITempManager,
         Time $time,
         LoggerInterface $logger
     ) {
@@ -74,6 +81,7 @@ class FeedFetcher implements IFeedFetcher
         $this->faviconFactory = $favicon;
         $this->scraper        = $scraper;
         $this->l10n           = $l10n;
+        $this->ITempManager   = $ITempManager;
         $this->time           = $time;
         $this->logger         = $logger;
     }
@@ -99,7 +107,6 @@ class FeedFetcher implements IFeedFetcher
      */
     public function fetch(
         string $url,
-        bool $favicon,
         ?string $lastModified,
         bool $fullTextEnabled,
         ?string $user,
@@ -118,17 +125,19 @@ class FeedFetcher implements IFeedFetcher
         $feed = $this->buildFeed(
             $parsedFeed,
             $url,
-            $favicon,
             $location
         );
 
         $items = [];
         $RTL = $this->determineRtl($parsedFeed);
         $feedName = $parsedFeed->getTitle();
-        $this->logger->debug('Feed {url} was modified since last fetch. #{count} items', [
+        $this->logger->debug(
+            'Feed {url} was modified since last fetch. #{count} items',
+            [
             'url'   => $url,
             'count' => count($parsedFeed),
-        ]);
+            ]
+        );
 
         foreach ($parsedFeed as $item) {
             $body = null;
@@ -144,11 +153,14 @@ class FeedFetcher implements IFeedFetcher
             }
 
             $builtItem = $this->buildItem($item, $body, $currRTL);
-            $this->logger->debug('Added item {title} for feed {feed} publishdate: {datetime}', [
+            $this->logger->debug(
+                'Added item {title} for feed {feed} lastmodified: {datetime}',
+                [
                 'title' => $builtItem->getTitle(),
                 'feed'  => $feedName,
                 'datetime'  => $builtItem->getLastModified(),
-            ]);
+                ]
+            );
             $items[] = $builtItem;
         }
 
@@ -295,16 +307,54 @@ class FeedFetcher implements IFeedFetcher
     }
 
     /**
+     * Return the favicon for a given feed and url
+     *
+     * @param FeedInterface $feed     Feed to check for a logo
+     * @param string        $url      Original URL for the feed
+     *
+     * @return string|mixed|bool
+     */
+    protected function getFavicon(FeedInterface $feed, string $url)
+    {
+        $favicon = $feed->getLogo();
+
+        // check if feed has a logo
+        if (is_null($favicon)) {
+            return $this->faviconFactory->get($url);
+        }
+
+        $favicon_path = join("/", [$this->ITempManager->getTempBaseDir(), basename($favicon)]);
+        copy(
+            $favicon,
+            $favicon_path
+        );
+
+        $is_image = substr(mime_content_type($favicon_path), 0, 5) === "image";
+
+        // check if file is actually an image
+        if (!$is_image) {
+            return $this->faviconFactory->get($url);
+        }
+
+        list($width, $height, $type, $attr) = getimagesize($favicon_path);
+        // check if image is square else fall back to favicon
+        if ($width !== $height) {
+            return $this->faviconFactory->get($url);
+        }
+
+        return $favicon;
+    }
+
+    /**
      * Build a feed based on provided info
      *
-     * @param FeedInterface $feed       Feed to build from
-     * @param string        $url        URL to use
-     * @param boolean       $getFavicon To get the favicon
-     * @param string        $location   String base URL
+     * @param FeedInterface $feed     Feed to build from
+     * @param string        $url      URL to use
+     * @param string        $location String base URL
      *
      * @return Feed
      */
-    protected function buildFeed(FeedInterface $feed, string $url, bool $getFavicon, string $location): Feed
+    protected function buildFeed(FeedInterface $feed, string $url, string $location): Feed
     {
         $newFeed = new Feed();
 
@@ -321,10 +371,9 @@ class FeedFetcher implements IFeedFetcher
         }
         $newFeed->setAdded($this->time->getTime());
 
-        if (!$getFavicon) {
-            return $newFeed;
-        }
-        $favicon = $this->faviconFactory->get($url);
+
+
+        $favicon = $this->getFavicon($feed, $url);
         $newFeed->setFaviconLink($favicon);
 
         return $newFeed;
diff --git a/lib/Fetcher/Fetcher.php b/lib/Fetcher/Fetcher.php
index 883fa49e4..724c264bc 100644
--- a/lib/Fetcher/Fetcher.php
+++ b/lib/Fetcher/Fetcher.php
@@ -44,7 +44,6 @@ class Fetcher
      * Fetch a feed from remote
      *
      * @param  string      $url               remote url of the feed
-     * @param  boolean     $getFavicon        if the favicon should also be fetched, defaults to true
      * @param  string|null $lastModified      a last modified value from an http header defaults to false.
      *                                    If lastModified matches the http header from the feed no results are fetched
      * @param  bool        $fullTextEnabled   If true use a scraper to download the full article
@@ -57,7 +56,6 @@ class Fetcher
      */
     public function fetch(
         string $url,
-        bool $getFavicon = true,
         ?string $lastModified = null,
         bool $fullTextEnabled = false,
         ?string $user = null,
@@ -69,7 +67,6 @@ class Fetcher
             }
             return $fetcher->fetch(
                 $url,
-                $getFavicon,
                 $lastModified,
                 $fullTextEnabled,
                 $user,
diff --git a/lib/Fetcher/IFeedFetcher.php b/lib/Fetcher/IFeedFetcher.php
index ff9a89903..abb367889 100644
--- a/lib/Fetcher/IFeedFetcher.php
+++ b/lib/Fetcher/IFeedFetcher.php
@@ -24,7 +24,6 @@ interface IFeedFetcher
      * Fetch feed content.
      *
      * @param  string      $url           remote url of the feed
-     * @param  boolean     $favicon       if the favicon should also be fetched, defaults to true
      * @param  string|null $lastModified  a last modified value from an http header defaults to false.
      *                                    If lastModified matches the http header from the feed no results are fetched
      * @param  bool      $fullTextEnabled If true use a scraper to download the full article
@@ -38,7 +37,6 @@ interface IFeedFetcher
      */
     public function fetch(
         string $url,
-        bool $favicon,
         ?string $lastModified,
         bool $fullTextEnabled,
         ?string $user,
diff --git a/lib/Service/FeedServiceV2.php b/lib/Service/FeedServiceV2.php
index ccb7c047a..f340bfc8d 100644
--- a/lib/Service/FeedServiceV2.php
+++ b/lib/Service/FeedServiceV2.php
@@ -258,7 +258,6 @@ class FeedServiceV2 extends Service
              */
             list($fetchedFeed, $items) = $this->feedFetcher->fetch(
                 $location,
-                false,
                 $feed->getHttpLastModified(),
                 $feed->getFullTextEnabled(),
                 $feed->getBasicAuthUser(),
diff --git a/tests/Unit/Command/ShowFeedTest.php b/tests/Unit/Command/ShowFeedTest.php
index ecaf4b0a0..6383c3f19 100644
--- a/tests/Unit/Command/ShowFeedTest.php
+++ b/tests/Unit/Command/ShowFeedTest.php
@@ -72,7 +72,7 @@ class ShowFeedTest extends TestCase
 
         $this->fetcher->expects($this->exactly(1))
                            ->method('fetch')
-                           ->with('feed', true, null, true, 'user', 'user')
+                           ->with('feed', null, true, 'user', 'user')
                            ->willReturn([['feed'], [['items']]]);
 
         $this->consoleOutput->expects($this->exactly(2))
@@ -106,7 +106,7 @@ class ShowFeedTest extends TestCase
 
         $this->fetcher->expects($this->exactly(1))
                            ->method('fetch')
-                           ->with('feed', true, null, true, 'user', 'user')
+                           ->with('feed', null, true, 'user', 'user')
                            ->will($this->throwException(new ServiceNotFoundException('test')));
 
         $this->consoleOutput->expects($this->exactly(2))
diff --git a/tests/Unit/Fetcher/FeedFetcherTest.php b/tests/Unit/Fetcher/FeedFetcherTest.php
index acb8e939d..4d5f45f73 100644
--- a/tests/Unit/Fetcher/FeedFetcherTest.php
+++ b/tests/Unit/Fetcher/FeedFetcherTest.php
@@ -30,6 +30,7 @@ use OCA\News\Fetcher\FeedFetcher;
 
 use OCA\News\Utility\Time;
 use OCP\IL10N;
+use OCP\ITempManager;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
 use Psr\Log\LoggerInterface;
@@ -80,6 +81,11 @@ class FeedFetcherTest extends TestCase
     private $l10n;
 
     /**
+     * @var MockObject|ITempManager
+     */
+    private $ITempManager;
+
+    /**
      * @var MockObject|ItemInterface
      */
     private $item_mock;
@@ -145,6 +151,9 @@ class FeedFetcherTest extends TestCase
         $this->l10n = $this->getMockBuilder(IL10N::class)
             ->disableOriginalConstructor()
             ->getMock();
+        $this->ITempManager = $this->getMockBuilder(ITempManager::class)
+            ->disableOriginalConstructor()
+            ->getMock();
         $this->reader = $this->getMockBuilder(FeedIo::class)
             ->disableOriginalConstructor()
             ->getMock();
@@ -183,6 +192,7 @@ class FeedFetcherTest extends TestCase
             $this->favicon,
             $this->scraper,
             $this->l10n,
+            $this->ITempManager,
             $timeFactory,
             $this->logger
         );
@@ -231,13 +241,14 @@ class FeedFetcherTest extends TestCase
         $item = $this->createItem();
         $feed = $this->createFeed();
         $this->mockIterator($this->feed_mock, [$this->item_mock]);
-        $result = $this->fetcher->fetch($this->url, false, '0', false, null, null);
+        $result = $this->fetcher->fetch($this->url, '0', false, null, null);
 
         $this->assertEquals([$feed, [$item]], $result);
     }
 
     /**
      * Return body options
+     *
      * @return array
      */
     public function feedBodyProvider()
@@ -282,7 +293,7 @@ class FeedFetcherTest extends TestCase
         $item = $this->createItem();
         $feed = $this->createFeed();
         $this->mockIterator($this->feed_mock, [$this->item_mock]);
-        $result = $this->fetcher->fetch($this->url, false, '0', false, null, null);
+        $result = $this->fetcher->fetch($this->url, '0', false, null, null);
 
         $this->assertEquals([$feed, [$item]], $result);
 
@@ -299,7 +310,7 @@ class FeedFetcherTest extends TestCase
         $item = $this->createItem();
         $feed = $this->createFeed();
         $this->mockIterator($this->feed_mock, [$this->item_mock]);
-        $result = $this->fetcher->fetch($this->url, false, '@1553118393', false, null, null);
+        $result = $this->fetcher->fetch($this->url, '@1553118393', false, null, null);
 
         $this->assertEquals([$feed, [$item]], $result);
     }
@@ -311,11 +322,10 @@ class FeedFetcherTest extends TestCase
     {
         $this->setUpReader('http://account%40email.com:F9sEU%2ARt%25%3AKFK8HMHT%26@tests/');
         $item = $this->createItem();
-        $feed = $this->createFeed('de-DE', false, 'http://account%40email.com:F9sEU%2ARt%25%3AKFK8HMHT%26@tests/');
+        $feed = $this->createFeed('de-DE', 'http://account%40email.com:F9sEU%2ARt%25%3AKFK8HMHT%26@tests/');
         $this->mockIterator($this->feed_mock, [$this->item_mock]);
         $result = $this->fetcher->fetch(
             $this->url,
-            false,
             '@1553118393',
             false,
             'account@email.com',
@@ -334,7 +344,7 @@ class FeedFetcherTest extends TestCase
         $item = $this->createItem('audio/ogg');
         $feed = $this->createFeed();
         $this->mockIterator($this->feed_mock, [$this->item_mock]);
-        $result = $this->fetcher->fetch($this->url, false, '@1553118393', false, null, null);
+        $result = $this->fetcher->fetch($this->url, '@1553118393', false, null, null);
 
         $this->assertEquals([$feed, [$item]], $result);
     }
@@ -348,7 +358,7 @@ class FeedFetcherTest extends TestCase
         $item = $this->createItem('video/ogg');
         $feed = $this->createFeed();
         $this->mockIterator($this->feed_mock, [$this->item_mock]);
-        $result = $this->fetcher->fetch($this->url, false, '@1553118393', false, null, null);
+        $result = $this->fetcher->fetch($this->url, '@1553118393', false, null, null);
 
         $this->assertEquals([$feed, [$item]], $result);
     }
@@ -360,29 +370,10 @@ class FeedFetcherTest extends TestCase
     {
         $this->setUpReader($this->url);
 
-        $feed = $this->createFeed('de-DE', true);
-        $item = $this->createItem();
-        $this->mockIterator($this->feed_mock, [$this->item_mock]);
-        $result = $this->fetcher->fetch($this->url, true, '@1553118393', false, null, null);
-
-        $this->assertEquals([$feed, [$item]], $result);
-    }
-
-    /**
-     * Test if fetching a feed without a favicon works.
-     */
-    public function testNoFavicon()
-    {
-        $this->setUpReader($this->url);
-
-        $feed = $this->createFeed('de-DE', false);
-
-        $this->favicon->expects($this->never())
-            ->method('get');
-
+        $feed = $this->createFeed('de-DE');
         $item = $this->createItem();
         $this->mockIterator($this->feed_mock, [$this->item_mock]);
-        $result = $this->fetcher->fetch($this->url, false, '@1553118393', false, null, null);
+        $result = $this->fetcher->fetch($this->url, '@1553118393', false, null, null);
 
         $this->assertEquals([$feed, [$item]], $result);
     }
@@ -396,7 +387,7 @@ class FeedFetcherTest extends TestCase
         $this->createFeed('he-IL');
         $this->createItem();
         $this->mockIterator($this->feed_mock, [$this->item_mock]);
-        list($_, $items) = $this->fetcher->fetch($this->url, false, '@1553118393', false, null, null);
+        list($_, $items) = $this->fetcher->fetch($this->url, '@1553118393', false, null, null);
         $this->assertTrue($items[0]->getRtl());
     }
 
@@ -422,7 +413,7 @@ class FeedFetcherTest extends TestCase
 
 
         $this->mockIterator($this->feed_mock, [$this->item_mock]);
-        list($feed, $items) = $this->fetcher->fetch($this->url, false, '@1553118393', false, null, null);
+        list($feed, $items) = $this->fetcher->fetch($this->url, '@1553118393', false, null, null);
         $this->assertSame($items[0]->getPubDate(), 1522180229);
     }
 
@@ -448,7 +439,7 @@ class FeedFetcherTest extends TestCase
 
 
         $this->mockIterator($this->feed_mock, [$this->item_mock]);
-        list($feed, $items) = $this->fetcher->fetch($this->url, false, '@1553118393', false, null, null);
+        list($feed, $items) = $this->fetcher->fetch($this->url, '@1553118393', false, null, null);
         $this->assertSame($items[0]->getPubDate(), 1519761029);
     }
 
@@ -589,15 +580,15 @@ class FeedFetcherTest extends TestCase
         $item = new Item();
 
         $item->setUnread(true)
-             ->setUrl($this->permalink)
-             ->setTitle('my<\' title')
-             ->setGuid($this->guid)
-             ->setGuidHash($this->guid_hash)
-             ->setBody($this->parsed_body)
-             ->setRtl(false)
-             ->setLastModified(3)
-             ->setPubDate(3)
-             ->setAuthor(html_entity_decode($this->author->getName()));
+            ->setUrl($this->permalink)
+            ->setTitle('my<\' title')
+            ->setGuid($this->guid)
+            ->setGuidHash($this->guid_hash)
+            ->setBody($this->parsed_body)
+            ->setRtl(false)
+            ->setLastModified(3)
+            ->setPubDate(3)
+            ->setAuthor(html_entity_decode($this->author->getName()));
 
         if ($enclosureType === 'audio/ogg' || $enclosureType === 'video/ogg') {
             $media = $this->getMockbuilder(MediaInterface::class)->getMock();
@@ -637,7 +628,7 @@ class FeedFetcherTest extends TestCase
      *
      * @return Feed
      */
-    private function createFeed($lang = 'de-DE', $favicon = false, $url = null)
+    private function createFeed($lang = 'de-DE', $url = null)
     {
         $url = $url ?? $this->url;
         $this->feed_mock->expects($this->exactly(3))
@@ -661,16 +652,13 @@ class FeedFetcherTest extends TestCase
         $feed->setUrl($url);
         $feed->setHttpLastModified((new DateTime('@3'))->format(DateTime::RSS));
         $feed->setAdded($this->time);
-        if ($favicon) {
-            $feed->setFaviconLink('http://anon.google.com');
-            $this->favicon->expects($this->exactly(1))
-                ->method('get')
-                ->with($this->equalTo($this->feed_link))
-                ->will($this->returnValue($this->web_favicon));
-        } else {
-            $this->favicon->expects($this->never())
-                ->method('get');
-        }
+
+        $feed->setFaviconLink('http://anon.google.com');
+        $this->favicon->expects($this->exactly(1))
+            ->method('get')
+            ->with($this->equalTo($url))
+            ->will($this->returnValue($this->web_favicon));
+
 
         return $feed;
     }