From c09b4d8d3341780022dfdd3edbe5352a1d4a4abd Mon Sep 17 00:00:00 2001 From: Benjamin Brahmer Date: Thu, 11 Feb 2021 09:15:49 +0100 Subject: 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 Co-authored-by: Sean Molenaar --- .github/workflows/api-integration-tests.yml | 1 + CHANGELOG.md | 2 + lib/Command/ShowFeed.php | 2 +- lib/Fetcher/FeedFetcher.php | 79 ++++++++++++++++++++----- lib/Fetcher/Fetcher.php | 3 - lib/Fetcher/IFeedFetcher.php | 2 - lib/Service/FeedServiceV2.php | 1 - tests/Unit/Command/ShowFeedTest.php | 4 +- tests/Unit/Fetcher/FeedFetcherTest.php | 90 +++++++++++++---------------- 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('Failed to fetch feed info:'); $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; @@ -52,6 +53,11 @@ class FeedFetcher implements IFeedFetcher */ private $l10n; + /** + * @var ITempManager + */ + private $ITempManager; + /** * @var 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; } @@ -294,17 +306,55 @@ class FeedFetcher implements IFeedFetcher return $item; } + /** + * 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; @@ -79,6 +80,11 @@ class FeedFetcherTest extends TestCase */ private $l10n; + /** + * @var MockObject|ITempManager + */ + private $ITempManager; + /** * @var MockObject|ItemInterface */ @@ -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; } -- cgit v1.2.3