🎨 🐛 Improve theme lib, middleware & error handling (#8145)

no issue 🎨 simplify loader - use loadOneTheme for init - use loadOneTheme for init - move updateThemeList to the one place that it is used - this just reduces the surface area of the loader 🎨 Move init up to index temporarily - need to figure out what stuff goes in here as well as loading themes - will move it again later once I've got it figured out 🎨 Reorder & cleanup theme middleware - move the order in blog/app.js so that theme middleware isn't called for shared assets - add comments & cleanup in the middleware itself, for clarity 🎨 Simplify the logic in themes middleware - Separate out config dependent on settings changing and config dependent on request - Move blogApp.set('views') - no reason why this isn't in the theme activation method as it's actually simpler if it is there, we already know the active theme exists & can remove the if-guard 🎨 Improve error handling for missing theme - ensure we display a warning - don't have complex logic for handling errors - move loading of an empty hbs object into the error-handler as this will support more cases 🐛 Fix assetHash clearing bug on theme switch - asset hash wasn't correctly being set on theme switch 🎨 Remove themes.read & test loader instead - Previously, we've simplified loader & improved error handling - We are now able to completely remove theme.read as it's nothing more than a wrapper for package.read - This also means we can change our tests from testing the theme reader to loader
TryGhost · Mar 13, 2017 · e060a4f · e060a4f
1 parent c9f551e
commit e060a4f
Show file tree

Hide file tree

Showing 8 changed files with 224 additions and 266 deletions.
diff --git a/core/server/blog/app.js b/core/server/blog/app.js
@@ -33,13 +33,6 @@ module.exports = function setupBlogApp() {
     // set the view engine
     blogApp.set('view engine', 'hbs');
 
-    // Theme middleware
-    // rightly or wrongly currently comes before theme static assets
-    // @TODO revisit where and when these are needed
-    blogApp.use(themeHandler.updateActiveTheme);
-    blogApp.use(themeHandler.configHbsForContext);
-    debug('Themes done');
-
     // you can extend Ghost with a custom redirects file
     // see https://github.com/TryGhost/Ghost/issues/7707
     customRedirects(blogApp);
@@ -58,6 +51,14 @@ module.exports = function setupBlogApp() {
     // Serve blog images using the storage adapter
     blogApp.use('/' + utils.url.STATIC_IMAGE_URL_PREFIX, storage.getStorage().serve());
 
+    // Theme middleware
+    // This should happen AFTER any shared assets are served, as it only changes things to do with templates
+    // At this point the active theme object is already updated, so we have the right path, so it can probably
+    // go after staticTheme() as well, however I would really like to simplify this and be certain
+    blogApp.use(themeHandler.updateActiveTheme);
+    blogApp.use(themeHandler.configHbsForContext);
+    debug('Themes done');
+
     // Theme static assets/files
     blogApp.use(staticTheme());
     debug('Static content done');

diff --git a/core/server/middleware/error-handler.js b/core/server/middleware/error-handler.js
@@ -110,6 +110,12 @@ _private.HTMLErrorRenderer = function HTMLErrorRender(err, req, res, /*jshint un
         templateData.stack = err.stack;
     }
 
+    // It can be that something went wrong with the theme or otherwise loading handlebars
+    // This ensures that no matter what res.render will work here
+    if (_.isEmpty(req.app.engines)) {
+        req.app.engine('hbs', hbs.express3());
+    }
+
     res.render(defaultTemplate, templateData, function renderResponse(err, html) {
         if (!err) {
             return res.send(html);

diff --git a/core/server/middleware/theme-handler.js b/core/server/middleware/theme-handler.js
@@ -4,7 +4,6 @@ var _      = require('lodash'),
     hbs    = require('express-hbs'),
     config = require('../config'),
     utils = require('../utils'),
-    logging = require('../logging'),
     errors = require('../errors'),
     i18n = require('../i18n'),
     settingsCache = require('../settings/cache'),
@@ -15,10 +14,11 @@ themeHandler = {
     // ### configHbsForContext Middleware
     // Setup handlebars for the current context (admin or theme)
     configHbsForContext: function configHbsForContext(req, res, next) {
+        // Static information, same for every request unless the settings change
+        // @TODO: bind this once and then update based on events?
         var themeData = {
                 title: settingsCache.get('title'),
                 description: settingsCache.get('description'),
-                url: utils.url.urlFor('home', {secure: req.secure}, true),
                 facebook: settingsCache.get('facebook'),
                 twitter: settingsCache.get('twitter'),
                 timezone: settingsCache.get('activeTimezone'),
@@ -29,57 +29,56 @@ themeHandler = {
                 logo: settingsCache.get('logo'),
                 amp: settingsCache.get('amp')
             },
-            labsData = _.cloneDeep(settingsCache.get('labs')),
-            blogApp = req.app;
+            labsData = _.cloneDeep(settingsCache.get('labs'));
 
+        // Request-specific information
+        // These things are super dependent on the request, so they need to be in middleware
+        themeData.url = utils.url.urlFor('home', {secure: req.secure}, true);
+
+        // Pass 'secure' flag to the view engine
+        // so that templates can choose to render https or http 'url', see url utility
+        res.locals.secure = req.secure;
+
+        // @TODO: only do this if something changed?
         hbs.updateTemplateOptions({
             data: {
                 blog: themeData,
                 labs: labsData
             }
         });
 
-        if (config.getContentPath('themes') && blogApp.get('activeTheme')) {
-            blogApp.set('views', path.join(config.getContentPath('themes'), blogApp.get('activeTheme')));
-        }
-
-        // Pass 'secure' flag to the view engine
-        // so that templates can choose to render https or http 'url', see url utility
-        res.locals.secure = req.secure;
-
         next();
     },
 
     // ### Activate Theme
     // Helper for updateActiveTheme
-    activateTheme: function activateTheme(blogApp, activeTheme) {
-        var hbsOptions,
-            themePartials = path.join(config.getContentPath('themes'), activeTheme, 'partials');
-
-        // clear the view cache
-        blogApp.cache = {};
-        // reset the asset hash
-        config.assetHash = null;
-
-        // set view engine
-        hbsOptions = {
-            partialsDir: [config.get('paths').helperTemplates],
-            onCompile: function onCompile(exhbs, source) {
-                return exhbs.handlebars.compile(source, {preventIndent: true});
-            }
-        };
+    activateTheme: function activateTheme(blogApp, activeThemeName) {
+        var themePartialsPath = path.join(config.getContentPath('themes'), activeThemeName, 'partials'),
+            hbsOptions = {
+                partialsDir: [config.get('paths').helperTemplates],
+                onCompile: function onCompile(exhbs, source) {
+                    return exhbs.handlebars.compile(source, {preventIndent: true});
+                }
+            };
 
-        fs.stat(themePartials, function stat(err, stats) {
+        fs.stat(themePartialsPath, function stat(err, stats) {
             // Check that the theme has a partials directory before trying to use it
             if (!err && stats && stats.isDirectory()) {
-                hbsOptions.partialsDir.push(themePartials);
+                hbsOptions.partialsDir.push(themePartialsPath);
             }
         });
 
+        // reset the asset hash
+        config.set('assetHash', null);
+        // clear the view cache
+        blogApp.cache = {};
+        // Set the views and engine
+        blogApp.set('views', path.join(config.getContentPath('themes'), activeThemeName));
         blogApp.engine('hbs', hbs.express3(hbsOptions));
 
         // Set active theme variable on the express server
-        blogApp.set('activeTheme', activeTheme);
+        // Note: this is effectively the "mounted" theme, which has been loaded into the express app
+        blogApp.set('activeTheme', activeThemeName);
     },
 
     // ### updateActiveTheme
@@ -88,31 +87,21 @@ themeHandler = {
     // is not yet activated.
     updateActiveTheme: function updateActiveTheme(req, res, next) {
         var blogApp = req.app,
-            activeTheme = settingsCache.get('activeTheme');
+            activeThemeName = settingsCache.get('activeTheme'),
+            mountedThemeName = blogApp.get('activeTheme');
 
-        // Check if the theme changed
-        if (activeTheme !== blogApp.get('activeTheme')) {
-            // Change theme
-            if (!themeList.get(activeTheme)) {
-                if (!res.isAdmin) {
-                    // Trying to start up without the active theme present, setup a simple hbs instance
-                    // and render an error page straight away.
-                    blogApp.engine('hbs', hbs.express3());
-                    return next(new errors.NotFoundError({
-                        message: i18n.t('errors.middleware.themehandler.missingTheme', {theme: activeTheme})
-                    }));
-                } else {
-                    // At this point the activated theme is not present and the current
-                    // request is for the admin client.  In order to allow the user access
-                    // to the admin client we set an hbs instance on the app so that middleware
-                    // processing can continue.
-                    blogApp.engine('hbs', hbs.express3());
-                    logging.warn(i18n.t('errors.middleware.themehandler.missingTheme', {theme: activeTheme}));
-                    return next();
-                }
-            } else {
-                themeHandler.activateTheme(blogApp, activeTheme);
-            }
+        // This means that the theme hasn't been loaded yet i.e. there is no active theme
+        if (!themeList.get(activeThemeName)) {
+            // This is the one place we ACTUALLY throw an error for a missing theme
+            // As it's a request we cannot serve
+            return next(new errors.InternalServerError({
+                message: i18n.t('errors.middleware.themehandler.missingTheme', {theme: activeThemeName})
+            }));
+
+            // If there is an active theme AND it has changed, call activate
+        } else if (activeThemeName !== mountedThemeName) {
+            // This is effectively "mounting" a theme into express, the theme is already "active"
+            themeHandler.activateTheme(blogApp, activeThemeName);
         }
 
         next();

diff --git a/core/server/themes/index.js b/core/server/themes/index.js
@@ -1,9 +1,33 @@
-var themeLoader = require('./loader');
+var debug = require('debug')('ghost:themes'),
+    events = require('../events'),
+    logging = require('../logging'),
+    i18n = require('../i18n'),
+    themeLoader = require('./loader'),
+    settingsCache = require('../settings/cache');
 
 // @TODO: reduce the amount of things we expose to the outside world
 // Make this a nice clean sensible API we can all understand!
 module.exports = {
-    init: themeLoader.init,
+    // Init themes module
+    // TODO: move this once we're clear what needs to happen here
+    init: function initThemes() {
+        var activeThemeName = settingsCache.get('activeTheme');
+        debug('init themes', activeThemeName);
+
+        // Register a listener for server-start to load all themes
+        events.on('server:start', function readAllThemesOnServerStart() {
+            themeLoader.loadAllThemes();
+        });
+
+        // Just read the active theme for now
+        return themeLoader
+            .loadOneTheme(activeThemeName)
+            .catch(function () {
+                // Active theme is missing, we don't want to exit because the admin panel will still work
+                logging.warn(i18n.t('errors.middleware.themehandler.missingTheme', {theme: activeThemeName}));
+            });
+    },
+    // Load themes, soon to be removed and exposed via specific function.
     loadAll: themeLoader.loadAllThemes,
     loadOne: themeLoader.loadOneTheme,
     list: require('./list'),

diff --git a/core/server/themes/loader.js b/core/server/themes/loader.js
@@ -1,50 +1,30 @@
 var debug = require('debug')('ghost:themes:loader'),
     config = require('../config'),
-    events = require('../events'),
     themeList = require('./list'),
-    read = require('./read'),
-    settingsCache = require('../settings/cache'),
-    updateThemeList,
+    read = require('../utils/packages').read,
     loadAllThemes,
-    loadOneTheme,
-    initThemes;
-
-updateThemeList = function updateThemeList(themes) {
-    debug('loading themes', Object.keys(themes));
-    themeList.init(themes);
-};
+    loadOneTheme;
 
 loadAllThemes = function loadAllThemes() {
     return read
         .all(config.getContentPath('themes'))
-        .then(updateThemeList);
+        .then(function updateThemeList(themes) {
+            debug('loading themes', Object.keys(themes));
+
+            themeList.init(themes);
+        });
 };
 
 loadOneTheme = function loadOneTheme(themeName) {
     return read
         .one(config.getContentPath('themes'), themeName)
         .then(function (readThemes) {
-            // @TODO change read one to not return a keyed object
+            debug('loaded one theme', themeName);
             return themeList.set(themeName, readThemes[themeName]);
         });
 };
 
-initThemes = function initThemes() {
-    debug('init themes', settingsCache.get('activeTheme'));
-
-    // Register a listener for server-start to load all themes
-    events.on('server:start', function readAllThemesOnServerStart() {
-        loadAllThemes();
-    });
-
-    // Just read the active theme for now
-    return read
-        .one(config.getContentPath('themes'), settingsCache.get('activeTheme'))
-        .then(updateThemeList);
-};
-
 module.exports = {
-    init: initThemes,
     loadAllThemes: loadAllThemes,
     loadOneTheme: loadOneTheme
 };
diff --git a/core/server/themes/read.js b/core/server/themes/read.js
diff --git a/core/test/unit/middleware/theme-handler_spec.js b/core/test/unit/middleware/theme-handler_spec.js
@@ -1,14 +1,13 @@
-var sinon        = require('sinon'),
-    should       = require('should'),
-    express      = require('express'),
-    fs           = require('fs'),
-    hbs          = require('express-hbs'),
-    themeList   = require('../../../server/themes').list,
+var sinon = require('sinon'),
+    should = require('should'),
+    express = require('express'),
+    fs = require('fs'),
+    hbs = require('express-hbs'),
+    themeList = require('../../../server/themes').list,
     themeHandler = require('../../../server/middleware/theme-handler'),
-    logging      = require('../../../server/logging'),
-    settingsCache  = require('../../../server/settings/cache'),
+    settingsCache = require('../../../server/settings/cache'),
 
-    sandbox      = sinon.sandbox.create();
+    sandbox = sinon.sandbox.create();
 
 describe('Theme Handler', function () {
     var req, res, next, blogApp;
@@ -130,22 +129,5 @@ describe('Theme Handler', function () {
                 done();
             });
         });
-
-        it('throws only warns if theme is missing for admin req', function (done) {
-            var activateThemeSpy = sandbox.spy(themeHandler, 'activateTheme'),
-                loggingWarnStub = sandbox.spy(logging, 'warn');
-
-            sandbox.stub(settingsCache, 'get').withArgs('activeTheme').returns('rasper');
-
-            res.isAdmin = true;
-            blogApp.set('activeTheme', 'not-casper');
-
-            themeHandler.updateActiveTheme(req, res, function () {
-                activateThemeSpy.called.should.be.false();
-                loggingWarnStub.called.should.be.true();
-                loggingWarnStub.calledWith('The currently active theme "rasper" is missing.').should.be.true();
-                done();
-            });
-        });
     });
 });