🐛 Blog icon improvements (#8298)

refs #7688 - renders the correct `/favicon.ico` or `/favcicon.png` in `{{ghost_head}}` - removes an regex issue in `serve-favicon`
TryGhost · Apr 10, 2017 · 4ba5cc8 · 4ba5cc8
1 parent a413d70
commit 4ba5cc8
Show file tree

Hide file tree

Showing 8 changed files with 57 additions and 36 deletions.
diff --git a/core/server/data/meta/asset_url.js b/core/server/data/meta/asset_url.js
@@ -8,7 +8,7 @@ var config = require('../../config'),
  */
 function getFaviconUrl() {
     if (settingsCache.get('icon')) {
-        return utils.url.urlJoin(utils.url.getSubdir(), utils.url.urlFor('image', {image: settingsCache.get('icon')}));
+        return settingsCache.get('icon').match(/\.ico$/i) ? utils.url.urlJoin(utils.url.getSubdir(), '/favicon.ico') : utils.url.urlJoin(utils.url.getSubdir(), '/favicon.png');
     }
 
     return utils.url.urlJoin(utils.url.getSubdir(), '/favicon.ico');

diff --git a/core/server/helpers/ghost_head.js b/core/server/helpers/ghost_head.js
@@ -98,8 +98,9 @@ module.exports = function ghost_head(options) {
         },
         blogIcon = settingsCache.get('icon'),
         // CASE: blog icon is not set in config, we serve the default
-        iconType = !blogIcon ? 'x-icon' : blogIcon.match(/\/favicon\.ico$/i) ? 'x-icon' : 'png',
-        favicon = !blogIcon ? '/favicon.ico' : url.urlFor('image', {image: blogIcon});
+        iconType = !blogIcon ? 'x-icon' : blogIcon.match(/\.ico$/i) ? 'x-icon' : 'png',
+        favicon = !blogIcon ? url.urlFor({relativeUrl: '/favicon.ico'}) :
+                  blogIcon.match(/\.ico$/i) ? url.urlFor({relativeUrl: '/favicon.ico'}) : url.urlFor({relativeUrl: '/favicon.png'});
 
     return Promise.props(fetch).then(function (response) {
         client = response.client;
@@ -111,7 +112,7 @@ module.exports = function ghost_head(options) {
                 head.push('<meta name="description" content="' + escapeExpression(metaData.metaDescription) + '" />');
             }
 
-            head.push('<link rel="shortcut icon" href="' + favicon + '" type="' + iconType + '" />');
+            head.push('<link rel="shortcut icon" href="' + favicon + '" type="image/' + iconType + '" />');
             head.push('<link rel="canonical" href="' +
                 escapeExpression(metaData.canonicalUrl) + '" />');
             head.push('<meta name="referrer" content="' + referrerPolicy + '" />');

diff --git a/core/server/middleware/serve-favicon.js b/core/server/middleware/serve-favicon.js
@@ -51,7 +51,7 @@ function serveFavicon() {
                 storage.getStorage()
                     .read({path: filePath})
                     .then(function readFile(buf) {
-                        iconType = settingsCache.get('icon').match(/\/favicon\.ico$/i) ? 'x-icon' : 'png';
+                        iconType = settingsCache.get('icon').match(/\.ico$/i) ? 'x-icon' : 'png';
                         content = buildContentResponse(iconType, buf);
 
                         res.writeHead(200, content.headers);

diff --git a/core/test/unit/metadata/asset_url_spec.js b/core/test/unit/metadata/asset_url_spec.js
@@ -44,7 +44,7 @@ describe('getAssetUrl', function () {
             testUrl.should.equal('/favicon.ico');
         });
 
-        it.skip('should correct favicon path for custom png', function () {
+        it('should correct favicon path for custom png', function () {
             sandbox.stub(settingsCache, 'get').withArgs('icon').returns('/content/images/2017/04/my-icon.png');
             var testUrl = getAssetUrl('favicon.ico');
             testUrl.should.equal('/favicon.png');
@@ -113,7 +113,7 @@ describe('getAssetUrl', function () {
                 testUrl.should.equal('/blog/favicon.ico');
             });
 
-            it.skip('should return correct favicon path for custom png', function () {
+            it('should return correct favicon path for custom png', function () {
                 sandbox.stub(settingsCache, 'get').withArgs('icon').returns('/content/images/2017/04/my-icon.png');
                 var testUrl = getAssetUrl('favicon.ico');
                 testUrl.should.equal('/blog/favicon.png');

diff --git a/core/test/unit/middleware/serve-favicon_spec.js b/core/test/unit/middleware/serve-favicon_spec.js
@@ -88,6 +88,26 @@ describe('Serve Favicon', function () {
                 middleware(req, res, next);
             });
 
+            it('custom uploaded myicon.ico', function (done) {
+                var middleware = serveFavicon();
+                req.path = '/favicon.ico';
+
+                storage.getStorage().storagePath = path.join(__dirname, '../../../test/utils/fixtures/images/');
+                localSettingsCache.icon = 'myicon.ico';
+
+                res = {
+                    writeHead: function (statusCode) {
+                        statusCode.should.eql(200);
+                    },
+                    end: function (body) {
+                        body.length.should.eql(15086);
+                        done();
+                    }
+                };
+
+                middleware(req, res, next);
+            });
+
             it('default favicon.ico', function (done) {
                 var middleware = serveFavicon();
                 req.path = '/favicon.ico';

diff --git a/core/test/unit/server_helpers/asset_spec.js b/core/test/unit/server_helpers/asset_spec.js
@@ -42,14 +42,14 @@ describe('{{asset}} helper', function () {
             // with png
             rendered = helpers.asset('favicon.png');
             should.exist(rendered);
-            String(rendered).should.equal('/content/images/favicon.png');
+            String(rendered).should.equal('/favicon.png');
 
             localSettingsCache.icon = '/content/images/favicon.ico';
 
             // with ico
             rendered = helpers.asset('favicon.ico');
             should.exist(rendered);
-            String(rendered).should.equal('/content/images/favicon.ico');
+            String(rendered).should.equal('/favicon.ico');
         });
 
         it('handles public assets correctly', function () {
@@ -96,14 +96,14 @@ describe('{{asset}} helper', function () {
             // with  png
             rendered = helpers.asset('favicon.png');
             should.exist(rendered);
-            String(rendered).should.equal('/blog/content/images/favicon.png');
+            String(rendered).should.equal('/blog/favicon.png');
 
             localSettingsCache.icon = '/content/images/favicon.ico';
 
             // with ico
             rendered = helpers.asset('favicon.ico');
             should.exist(rendered);
-            String(rendered).should.equal('/blog/content/images/favicon.ico');
+            String(rendered).should.equal('/blog/favicon.ico');
         });
 
         it('handles public assets correctly', function () {