Improve favicons handling

- Make sure that deleted favicons on the filesystem are downloaded again
- Purge orphan icons
- Add unit tests

app/handlers/feed.php

@@ -7,6 +7,7 @@ use Miniflux\Model;
 use PicoFeed;
 use PicoFeed\Config\Config as ReaderConfig;
 use PicoFeed\Logging\Logger;
+use PicoFeed\Reader\Favicon;
 use PicoFeed\Reader\Reader;
 
 function fetch_feed($url, $download_content = false, $etag = '', $last_modified = '')
@@ -74,7 +75,7 @@ function create_feed($user_id, $url, $download_content = false, $rtl = false, $c
         } else if ($feed_id === false) {
             $error_message = t('Unable to save this subscription in the database.');
         } else {
-            Model\Favicon\create_feed_favicon($feed_id, $feed->getSiteUrl(), $feed->getIcon());
+            fetch_favicon($feed_id, $feed->getSiteUrl(), $feed->getIcon());
 
             if (! empty($feed_group_ids)) {
                 Model\Group\update_feed_groups($user_id, $feed_id, $feed_group_ids, $group_name);
@@ -115,7 +116,7 @@ function update_feed($user_id, $feed_id)
 
     if ($feed !== null) {
         Model\Item\update_feed_items($user_id, $feed_id, $feed->getItems(), $subscription['rtl']);
-        Model\Favicon\create_feed_favicon($feed_id, $feed->getSiteUrl(), $feed->getIcon());
+        fetch_favicon($feed_id, $feed->getSiteUrl(), $feed->getIcon());
     }
 
     return true;
@@ -128,6 +129,17 @@ function update_feeds($user_id, $limit = null)
     }
 }
 
+function fetch_favicon($feed_id, $site_url, $icon_link)
+{
+    if (Helper\bool_config('favicons') && ! Model\Favicon\has_favicon($feed_id)) {
+        $favicon = new Favicon();
+
+        if ($favicon->find($site_url, $icon_link)) {
+            Model\Favicon\create_feed_favicon($feed_id, $favicon->getType(), $favicon->getContent());
+        }
+    }
+}
+
 function get_reader_config()
 {
     $config = new ReaderConfig;

app/models/favicon.php

@@ -5,42 +5,25 @@ namespace Miniflux\Model\Favicon;
 use Miniflux\Helper;
 use Miniflux\Model;
 use PicoDb\Database;
-use PicoFeed\Reader\Favicon;
 
 const TABLE      = 'favicons';
 const JOIN_TABLE = 'favicons_feeds';
 
-function create_feed_favicon($feed_id, $site_url, $icon_link)
+function create_feed_favicon($feed_id, $mime_type, $blob)
 {
-    $favicon = fetch_favicon($feed_id, $site_url, $icon_link);
+    $favicon_id = store_favicon($mime_type, $blob);
-    if ($favicon === false) {
-        return false;
-    }
-
-    $favicon_id = store_favicon($favicon->getType(), $favicon->getContent());
     if ($favicon_id === false) {
         return false;
     }
 
     return Database::getInstance('db')
         ->table(JOIN_TABLE)
-        ->save(array(
+        ->insert(array(
             'feed_id'    => $feed_id,
             'favicon_id' => $favicon_id
         ));
 }
 
-function fetch_favicon($feed_id, $site_url, $icon_link)
-{
-    if (Helper\bool_config('favicons') && ! has_favicon($feed_id)) {
-        $favicon = new Favicon();
-        $favicon->find($site_url, $icon_link);
-        return $favicon;
-    }
-
-    return false;
-}
-
 function store_favicon($mime_type, $blob)
 {
     if (empty($blob)) {
@@ -48,14 +31,7 @@ function store_favicon($mime_type, $blob)
     }
 
     $hash = sha1($blob);
-    $favicon_id = get_favicon_id($hash);
+    if (file_put_contents(get_favicon_filename($hash, $mime_type), $blob) === false) {
-
-    if ($favicon_id) {
-        return $favicon_id;
-    }
-
-    $file = $hash.Helper\favicon_extension($mime_type);
-    if (file_put_contents(FAVICON_DIRECTORY.DIRECTORY_SEPARATOR.$file, $blob) === false) {
         return false;
     }
 
@@ -67,46 +43,52 @@ function store_favicon($mime_type, $blob)
         ));
 }
 
-function get_favicon_data_url($filename, $mime_type)
+function purge_favicons()
 {
-    $blob = base64_encode(file_get_contents(FAVICON_DIRECTORY.DIRECTORY_SEPARATOR.$filename));
+    $favicons = Database::getInstance('db')
-    return sprintf('data:%s;base64,%s', $mime_type, $blob);
-}
-
-function get_favicon_id($hash)
-{
-    return Database::getInstance('db')
         ->table(TABLE)
-        ->eq('hash', $hash)
+        ->join(JOIN_TABLE, 'favicon_id', 'id')
-        ->findOneColumn('id');
+        ->isNull('feed_id')
-}
+        ->findAll();
 
-function delete_favicon(array $favicon)
+    foreach ($favicons as $favicon) {
-{
+        $filename = get_favicon_filename($favicon['hash'], $favicon['type']);
-    unlink(FAVICON_DIRECTORY.DIRECTORY_SEPARATOR.$favicon['hash'].Helper\favicon_extension($favicon['type']));
+        Database::getInstance('db')
+            ->table(TABLE)
+            ->eq('id', $favicon['id'])
+            ->remove();
 
-    Database::getInstance('db')
+        if (file_exists($filename)) {
-        ->table(TABLE)
+            unlink($filename);
-        ->eq('hash', $favicon['hash'])
+        }
-        ->remove();
+    }
 }
 
 function has_favicon($feed_id)
 {
-    return Database::getInstance('db')
+    $favicon = Database::getInstance('db')
         ->table(JOIN_TABLE)
         ->eq('feed_id', $feed_id)
-        ->exists();
+        ->join(TABLE, 'id', 'favicon_id')
+        ->findOne();
+
+    $has_favicon = ! empty($favicon);
+
+    if ($has_favicon && ! file_exists(get_favicon_filename($favicon['hash'], $favicon['type']))) {
+        Database::getInstance('db')
+            ->table(TABLE)
+            ->eq('id', $favicon['id'])
+            ->remove();
+
+        return false;
+    }
+
+    return $has_favicon;
 }
 
 function get_favicons_by_feed_ids(array $feed_ids)
 {
     $result = array();
-
-    if (! Helper\bool_config('favicons')) {
-        return $result;
-    }
-
     $favicons = Database::getInstance('db')
         ->table(TABLE)
         ->columns(
@@ -151,15 +133,27 @@ function get_favicons_with_data_url($user_id)
 {
     $favicons = Database::getInstance('db')
         ->table(TABLE)
-        ->columns(JOIN_TABLE.'.feed_id', TABLE.'.file', TABLE.'.type')
+        ->columns('feed_id', 'hash', 'type')
-        ->join(JOIN_TABLE, 'favicon_id', 'id', TABLE)
+        ->join(JOIN_TABLE, 'favicon_id', 'id')
-        ->join(Model\Feed\TABLE, 'id', 'feed_id')
+        ->join(Model\Feed\TABLE, 'id', 'feed_id', JOIN_TABLE)
         ->eq(Model\Feed\TABLE.'.user_id', $user_id)
+        ->asc(TABLE.'.id')
         ->findAll();
 
     foreach ($favicons as &$favicon) {
-        $favicon['url'] = get_favicon_data_url($favicon['file'], $favicon['mime_type']);
+        $favicon['url'] = get_favicon_data_url($favicon['hash'], $favicon['type']);
     }
 
     return $favicons;
 }
+
+function get_favicon_filename($hash, $mime_type)
+{
+    return FAVICON_DIRECTORY.DIRECTORY_SEPARATOR.$hash.Helper\favicon_extension($mime_type);
+}
+
+function get_favicon_data_url($hash, $mime_type)
+{
+    $blob = base64_encode(file_get_contents(get_favicon_filename($hash, $mime_type)));
+    return sprintf('data:%s;base64,%s', $mime_type, $blob);
+}

app/models/feed.php

@@ -2,6 +2,7 @@
 
 namespace Miniflux\Model\Feed;
 
+use Miniflux\Model\Favicon;
 use Miniflux\Model\Item;
 use Miniflux\Model\Group;
 use PicoDb\Database;
@@ -151,11 +152,17 @@ function change_feed_status($user_id, $feed_id, $status = STATUS_ACTIVE)
 
 function remove_feed($user_id, $feed_id)
 {
-    return Database::getInstance('db')
+    $result = Database::getInstance('db')
         ->table(TABLE)
         ->eq('user_id', $user_id)
         ->eq('id', $feed_id)
         ->remove();
+
+    if ($result) {
+        Favicon\purge_favicons();
+    }
+
+    return $result;
 }
 
 function count_failed_feeds($user_id)

tests/unit/BaseTest.php

@@ -42,7 +42,7 @@ abstract class BaseTest extends PHPUnit_Framework_TestCase
         return $item;
     }
 
-    public function buildFeed()
+    public function buildFeed($feedUrl = 'feed url')
     {
         $items = array();
 
@@ -63,7 +63,7 @@ abstract class BaseTest extends PHPUnit_Framework_TestCase
 
         $feed = new Feed();
         $feed->setTitle('My feed');
-        $feed->setFeedUrl('feed url');
+        $feed->setFeedUrl($feedUrl);
         $feed->setSiteUrl('site url');
         $feed->setItems($items);
 
@@ -72,6 +72,6 @@ abstract class BaseTest extends PHPUnit_Framework_TestCase
 
     public function assertCreateFeed(Feed $feed)
     {
-        $this->assertEquals(1, Model\Feed\create(1, $feed, 'etag', 'last modified'));
+        $this->assertNotFalse(Model\Feed\create(1, $feed, 'etag', 'last modified'));
     }
 }

tests/unit/FaviconModelTest.php

@@ -0,0 +1,264 @@
+<?php
+
+namespace Miniflux\Model\Favicon;
+
+use BaseTest;
+use Miniflux\Model;
+
+function file_put_contents($filename, $data)
+{
+    return FaviconModelTest::$functions->file_put_contents($filename, $data);
+}
+
+function file_get_contents($filename)
+{
+    return FaviconModelTest::$functions->file_get_contents($filename);
+}
+
+function file_exists($filename)
+{
+    return FaviconModelTest::$functions->file_exists($filename);
+}
+
+function unlink($filename)
+{
+    return FaviconModelTest::$functions->unlink($filename);
+}
+
+class FaviconModelTest extends BaseTest
+{
+    /**
+     * @var \PHPUnit_Framework_MockObject_MockObject
+     */
+    public static $functions;
+
+    public function setUp()
+    {
+        parent::setUp();
+
+        self::$functions = $this
+            ->getMockBuilder('stdClass')
+            ->setMethods(array(
+                'file_put_contents',
+                'file_get_contents',
+                'file_exists',
+                'unlink'
+            ))
+            ->getMock();
+    }
+
+    public function testCreateFeedFavicon()
+    {
+        $this->assertCreateFeed($this->buildFeed());
+
+        self::$functions
+            ->expects($this->once())
+            ->method('file_put_contents')
+            ->with(
+                $this->stringEndsWith('data/favicons/57978a20204f7af6967571041c79d907a8a8072c.png'),
+                $this->equalTo('binary data')
+            )
+            ->will($this->returnValue(true));
+
+        $this->assertEquals(1, create_feed_favicon(1, 'image/png', 'binary data'));
+    }
+
+    public function testCreateEmptyFavicon()
+    {
+        $this->assertCreateFeed($this->buildFeed());
+        $this->assertFalse(create_feed_favicon(1, 'image/png', ''));
+    }
+
+    public function testCreateFeedFaviconWithUnableToWriteOnDisk()
+    {
+        $this->assertCreateFeed($this->buildFeed());
+
+        self::$functions
+            ->expects($this->once())
+            ->method('file_put_contents')
+            ->with(
+                $this->stringEndsWith('data/favicons/57978a20204f7af6967571041c79d907a8a8072c.png'),
+                $this->equalTo('binary data')
+            )
+            ->will($this->returnValue(false));
+
+        $this->assertFalse(create_feed_favicon(1, 'image/png', 'binary data'));
+    }
+
+    public function testCreateFeedFaviconAlreadyExists()
+    {
+        $this->assertCreateFeed($this->buildFeed());
+
+        self::$functions
+            ->expects($this->any())
+            ->method('file_put_contents')
+            ->with(
+                $this->stringEndsWith('data/favicons/57978a20204f7af6967571041c79d907a8a8072c.png'),
+                $this->equalTo('binary data')
+            )
+            ->will($this->returnValue(true));
+
+        $this->assertEquals(1, create_feed_favicon(1, 'image/png', 'binary data'));
+        $this->assertFalse(create_feed_favicon(1, 'image/png', 'binary data'));
+    }
+
+    public function testGetFaviconsWithDataUrl()
+    {
+        $this->assertCreateFeed($this->buildFeed());
+        $this->assertCreateFeed($this->buildFeed('another feed url'));
+        $this->assertEquals(1, create_feed_favicon(1, 'image/png', 'binary data'));
+        $this->assertEquals(2, create_feed_favicon(2, 'image/gif', 'some binary data'));
+
+        self::$functions
+            ->expects($this->at(0))
+            ->method('file_get_contents')
+            ->with(
+                $this->stringEndsWith('57978a20204f7af6967571041c79d907a8a8072c.png')
+            )
+            ->will($this->returnValue('binary data'));
+
+        self::$functions
+            ->expects($this->at(1))
+            ->method('file_get_contents')
+            ->with(
+                $this->stringEndsWith('36242b50974c41478569d66616346ee5f2ad7b6e.gif')
+            )
+            ->will($this->returnValue('some binary data'));
+
+        $favicons = get_favicons_with_data_url(1);
+        $this->assertCount(2, $favicons);
+
+        $this->assertEquals(1, $favicons[0]['feed_id']);
+        $this->assertEquals('57978a20204f7af6967571041c79d907a8a8072c', $favicons[0]['hash']);
+        $this->assertEquals('image/png', $favicons[0]['type']);
+        $this->assertEquals('data:image/png;base64,YmluYXJ5IGRhdGE=', $favicons[0]['url']);
+
+        $this->assertEquals(2, $favicons[1]['feed_id']);
+        $this->assertEquals('36242b50974c41478569d66616346ee5f2ad7b6e', $favicons[1]['hash']);
+        $this->assertEquals('image/gif', $favicons[1]['type']);
+        $this->assertEquals('data:image/gif;base64,c29tZSBiaW5hcnkgZGF0YQ==', $favicons[1]['url']);
+    }
+
+    public function testGetItemsFavicons()
+    {
+        $this->assertCreateFeed($this->buildFeed());
+        $this->assertCreateFeed($this->buildFeed('another feed url'));
+
+        $this->assertEquals(1, create_feed_favicon(1, 'image/png', 'binary data'));
+        $this->assertEquals(2, create_feed_favicon(2, 'image/gif', 'some binary data'));
+
+        $items = Model\Item\get_items(1);
+        $favicons = get_items_favicons($items);
+        $this->assertCount(2, $favicons);
+
+        $this->assertEquals(1, $favicons[1]['feed_id']);
+        $this->assertEquals('57978a20204f7af6967571041c79d907a8a8072c', $favicons[1]['hash']);
+        $this->assertEquals('image/png', $favicons[1]['type']);
+
+        $this->assertEquals(2, $favicons[2]['feed_id']);
+        $this->assertEquals('36242b50974c41478569d66616346ee5f2ad7b6e', $favicons[2]['hash']);
+        $this->assertEquals('image/gif', $favicons[2]['type']);
+    }
+
+    public function testGetFeedsFavicons()
+    {
+        $this->assertCreateFeed($this->buildFeed());
+        $this->assertCreateFeed($this->buildFeed('another feed url'));
+
+        $this->assertEquals(1, create_feed_favicon(1, 'image/png', 'binary data'));
+        $this->assertEquals(2, create_feed_favicon(2, 'image/gif', 'some binary data'));
+
+        $feeds = Model\Feed\get_feeds(1);
+        $favicons = get_feeds_favicons($feeds);
+        $this->assertCount(2, $favicons);
+
+        $this->assertEquals(1, $favicons[1]['feed_id']);
+        $this->assertEquals('57978a20204f7af6967571041c79d907a8a8072c', $favicons[1]['hash']);
+        $this->assertEquals('image/png', $favicons[1]['type']);
+
+        $this->assertEquals(2, $favicons[2]['feed_id']);
+        $this->assertEquals('36242b50974c41478569d66616346ee5f2ad7b6e', $favicons[2]['hash']);
+        $this->assertEquals('image/gif', $favicons[2]['type']);
+    }
+
+    public function testHasFavicon()
+    {
+        $this->assertCreateFeed($this->buildFeed());
+
+        self::$functions
+            ->expects($this->once())
+            ->method('file_put_contents')
+            ->with(
+                $this->stringEndsWith('data/favicons/57978a20204f7af6967571041c79d907a8a8072c.png'),
+                $this->equalTo('binary data')
+            )
+            ->will($this->returnValue(true));
+
+        self::$functions
+            ->expects($this->once())
+            ->method('file_exists')
+            ->with(
+                $this->stringEndsWith('data/favicons/57978a20204f7af6967571041c79d907a8a8072c.png')
+            )
+            ->will($this->returnValue(true));
+
+        $this->assertEquals(1, create_feed_favicon(1, 'image/png', 'binary data'));
+        $this->assertTrue(has_favicon(1));
+        $this->assertFalse(has_favicon(2));
+    }
+
+    public function testHasFaviconWhenFileMissing()
+    {
+        $this->assertCreateFeed($this->buildFeed());
+
+        self::$functions
+            ->expects($this->any())
+            ->method('file_put_contents')
+            ->with(
+                $this->stringEndsWith('data/favicons/57978a20204f7af6967571041c79d907a8a8072c.png'),
+                $this->equalTo('binary data')
+            )
+            ->will($this->returnValue(true));
+
+        self::$functions
+            ->expects($this->once())
+            ->method('file_exists')
+            ->with(
+                $this->stringEndsWith('data/favicons/57978a20204f7af6967571041c79d907a8a8072c.png')
+            )
+            ->will($this->returnValue(false));
+
+        $this->assertEquals(1, create_feed_favicon(1, 'image/png', 'binary data'));
+        $this->assertFalse(has_favicon(1));
+    }
+
+    public function testPurgeFavicons()
+    {
+        $this->assertCreateFeed($this->buildFeed());
+        $this->assertCreateFeed($this->buildFeed('another feed url'));
+
+        $this->assertEquals(1, create_feed_favicon(1, 'image/png', 'binary data'));
+        $this->assertEquals(2, create_feed_favicon(2, 'image/gif', 'some binary data'));
+
+        self::$functions
+            ->expects($this->any())
+            ->method('file_exists')
+            ->with(
+                $this->stringEndsWith('data/favicons/57978a20204f7af6967571041c79d907a8a8072c.png')
+            )
+            ->will($this->returnValue(true));
+
+        self::$functions
+            ->expects($this->once())
+            ->method('unlink')
+            ->with(
+                $this->stringEndsWith('data/favicons/57978a20204f7af6967571041c79d907a8a8072c.png')
+            );
+
+        $this->assertTrue(Model\Feed\remove_feed(1, 1));
+
+        $favicons = get_favicons_with_data_url(1);
+        $this->assertCount(1, $favicons);
+        $this->assertEquals(2, $favicons[0]['feed_id']);
+    }
+}