Skip to content

Commit

Permalink
✨ New fully-loaded & validated activeTheme concept (#8146)
Browse files Browse the repository at this point in the history
📡 Add debug for the 3 theme activation methods
There are 3 different ways that a theme can be activated in Ghost:

A. On boot: we load the active theme from the file system, according to the `activeTheme` setting
B. On API "activate": when an /activate/ request is triggered for a theme, we validate & change the `activeTheme` setting
C. On API "override": if uploading a theme with the same name, we override. Using a dirty hack to make this work.

A: setting is done, should load & validate + next request does mounting
B: load is done, should validate & change setting + next request does mounting
C: load, validate & setting are all done + a hack is needed to ensure the next request does mounting

✨ Validate w/ gscan when theme activating on boot
- use the new gscan validation validate.check() method when activating on boot

✨ New concept of active theme
- add ActiveTheme class
- make it possible to set a theme to be active, and to get the active theme
- call the new themes.activate() method in all 3 cases where we activate a theme

🎨 Use new activeTheme to simplify theme code
- make use of the new concept where we can, to reduce & simplify code
- use new hasPartials() method so we don't have to do file lookups
- use path & name getters to reduce use of getContentPath etc
- remove requirement on req.app.get('activeTheme') from static-theme middleware (more on this soon)

🚨 Improve theme unit tests (TODO: fix inter-dep)
- The theme unit tests are borked! They all pass because they don't test the right things.
- This improves them, but they are still dependent on each-other
- configHbsForContext tests don't pass if the activateTheme tests aren't run first
- I will fix this in a later PR
  • Loading branch information
ErisDS authored and kirrg001 committed Mar 13, 2017
1 parent 7556e68 commit b06f03b
Show file tree
Hide file tree
Showing 7 changed files with 338 additions and 151 deletions.
18 changes: 15 additions & 3 deletions core/server/api/themes.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// # Themes API
// RESTful API for Themes
var Promise = require('bluebird'),
var debug = require('debug')('ghost:api:themes'),
Promise = require('bluebird'),
fs = require('fs-extra'),
config = require('../config'),
errors = require('../errors'),
Expand Down Expand Up @@ -55,7 +56,10 @@ themes = {
return settingsModel.edit(newSettings, options);
})
.then(function hasEditedSetting() {
// @TODO actually do things to activate the theme, other than just the setting?
// Activate! (sort of)
debug('Activating theme (method B on API "activate")', themeName);
themeUtils.activate(loadedTheme, checkedTheme);

return themeUtils.toJSON(themeName, checkedTheme);
});
},
Expand Down Expand Up @@ -107,7 +111,15 @@ themes = {
// Sets the theme on the themeList
return themeUtils.loadOne(zip.shortName);
})
.then(function () {
.then(function (loadedTheme) {
// If this is the active theme, we are overriding
// This is a special case of activation
if (zip.shortName === settingsCache.get('activeTheme')) {
// Activate! (sort of)
debug('Activating theme (method C, on API "override")', zip.shortName);
themeUtils.activate(loadedTheme, checkedTheme);
}

// @TODO: unify the name across gscan and Ghost!
return themeUtils.toJSON(zip.shortName, checkedTheme);
})
Expand Down
5 changes: 3 additions & 2 deletions core/server/middleware/static-theme.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ var _ = require('lodash'),
express = require('express'),
path = require('path'),
config = require('../config'),
themeUtils = require('../themes'),
utils = require('../utils');

function isBlackListedFileType(file) {
Expand All @@ -17,12 +18,12 @@ function isWhiteListedFile(file) {
}

function forwardToExpressStatic(req, res, next) {
if (!req.app.get('activeTheme')) {
if (!themeUtils.getActive()) {
next();
} else {
var configMaxAge = config.get('caching:theme:maxAge');

express.static(path.join(config.getContentPath('themes'), req.app.get('activeTheme')),
express.static(themeUtils.getActive().path,
{maxAge: (configMaxAge || configMaxAge === 0) ? configMaxAge : utils.ONE_YEAR_MS}
)(req, res, next);
}
Expand Down
28 changes: 12 additions & 16 deletions core/server/middleware/theme-handler.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
var _ = require('lodash'),
fs = require('fs'),
path = require('path'),
hbs = require('express-hbs'),
config = require('../config'),
utils = require('../utils'),
errors = require('../errors'),
i18n = require('../i18n'),
settingsCache = require('../settings/cache'),
themeList = require('../themes').list,
themeUtils = require('../themes'),
themeHandler;

themeHandler = {
Expand Down Expand Up @@ -52,33 +50,29 @@ themeHandler = {

// ### Activate Theme
// Helper for updateActiveTheme
activateTheme: function activateTheme(blogApp, activeThemeName) {
var themePartialsPath = path.join(config.getContentPath('themes'), activeThemeName, 'partials'),
hbsOptions = {
activateTheme: function activateTheme(blogApp) {
var hbsOptions = {
partialsDir: [config.get('paths').helperTemplates],
onCompile: function onCompile(exhbs, source) {
return exhbs.handlebars.compile(source, {preventIndent: true});
}
};

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(themePartialsPath);
}
});
if (themeUtils.getActive().hasPartials()) {
hbsOptions.partialsDir.push(themeUtils.getActive().partialsPath);
}

// 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.set('views', themeUtils.getActive().path);
blogApp.engine('hbs', hbs.express3(hbsOptions));

// Set active theme variable on the express server
// Note: this is effectively the "mounted" theme, which has been loaded into the express app
blogApp.set('activeTheme', activeThemeName);
blogApp.set('activeTheme', themeUtils.getActive().name);
},

// ### updateActiveTheme
Expand All @@ -87,11 +81,13 @@ themeHandler = {
// is not yet activated.
updateActiveTheme: function updateActiveTheme(req, res, next) {
var blogApp = req.app,
// We use the settingsCache here, because the setting will be set, even if the theme itself is
// not usable because it is invalid or missing.
activeThemeName = settingsCache.get('activeTheme'),
mountedThemeName = blogApp.get('activeTheme');

// This means that the theme hasn't been loaded yet i.e. there is no active theme
if (!themeList.get(activeThemeName)) {
if (!themeUtils.getActive()) {
// 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({
Expand All @@ -101,7 +97,7 @@ themeHandler = {
// 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);
themeHandler.activateTheme(blogApp);
}

next();
Expand Down
88 changes: 88 additions & 0 deletions core/server/themes/active.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
'use strict';

/**
* # Active Theme
*
* This file defines a class of active theme, and also controls the getting and setting a single instance, as there
* can only ever be one active theme. Unlike a singleton, the active theme can change, however only in a controlled way.
*
* I've made use of the new class & constructor syntax here, as we are now only supporting Node v4 and above, which has
* full support for these features.
*
* There are several different patterns available for keeping data private. Elsewhere in Ghost we use the
* naming convention of the _ prefix. Even though this has the downside of not being truly private, it is still one
* of the preferred options for keeping data private with the new class syntax, therefore I have kept it.
*
* No properties marked with an _ should be used directly.
*
*/
var _ = require('lodash'),
join = require('path').join,
// Current instance of ActiveTheme
currentActiveTheme;

class ActiveTheme {
/**
* @TODO this API needs to be simpler, but for now should work!
* @param {object} loadedTheme - the loaded theme object from the theme list
* @param {object} checkedTheme - the result of gscan.format for the theme we're activating
*/
constructor(loadedTheme, checkedTheme) {
// Assign some data, mark it all as pseudo-private
this._name = loadedTheme.name;
this._path = loadedTheme.path;

// @TODO: get gscan to return validated, useful package.json fields for us!
this._packageInfo = loadedTheme['package.json'];
this._partials = checkedTheme.partials;
// @TODO: get gscan to return a template collection for us
this._templates = _.reduce(checkedTheme.files, function (templates, entry) {
var tplMatch = entry.file.match(/(^[^\/]+).hbs$/);
if (tplMatch) {
templates.push(tplMatch[1]);
}
return templates;
}, []);
}

get name() {
return this._name;
}

get path() {
return this._path;
}

get partialsPath() {
return join(this.path, 'partials');
}

hasPartials() {
return this._partials.length > 0;
}

hasTemplate(templateName) {
return this._templates.indexOf(templateName) > -1;
}
}

module.exports = {
get() {
return currentActiveTheme;
},
/**
* Set theme
*
* At this point we trust that the theme has been validated.
* Any handling for invalid themes should happen before we get here
*
* @TODO this API needs to be simpler, but for now should work!
* @param {object} loadedTheme - the loaded theme object from the theme list
* @param {object} checkedTheme - the result of gscan.format for the theme we're activating
* @return {ActiveTheme}
*/
set(loadedTheme, checkedTheme) {
currentActiveTheme = new ActiveTheme(loadedTheme, checkedTheme);
return currentActiveTheme;
}
};
37 changes: 34 additions & 3 deletions core/server/themes/index.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
var debug = require('debug')('ghost:themes'),
_ = require('lodash'),
events = require('../events'),
errors = require('../errors'),
logging = require('../logging'),
i18n = require('../i18n'),
themeLoader = require('./loader'),
active = require('./active'),
validate = require('./validate'),
settingsCache = require('../settings/cache');

// @TODO: reduce the amount of things we expose to the outside world
Expand All @@ -11,7 +15,9 @@ module.exports = {
// Init themes module
// TODO: move this once we're clear what needs to happen here
init: function initThemes() {
var activeThemeName = settingsCache.get('activeTheme');
var activeThemeName = settingsCache.get('activeTheme'),
self = this;

debug('init themes', activeThemeName);

// Register a listener for server-start to load all themes
Expand All @@ -22,6 +28,20 @@ module.exports = {
// Just read the active theme for now
return themeLoader
.loadOneTheme(activeThemeName)
.then(function activeThemeHasLoaded(theme) {
// Validate
return validate
.check(theme)
.then(function resultHandler(checkedTheme) {
// Activate! (sort of)
debug('Activating theme (method A on boot)', activeThemeName);
self.activate(theme, checkedTheme);
})
.catch(function () {
// Active theme is not valid, we don't want to exit because the admin panel will still work
logging.warn(i18n.t('errors.middleware.themehandler.invalidTheme', {theme: 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}));
Expand All @@ -31,6 +51,17 @@ module.exports = {
loadAll: themeLoader.loadAllThemes,
loadOne: themeLoader.loadOneTheme,
list: require('./list'),
validate: require('./validate'),
toJSON: require('./to-json')
validate: validate,
toJSON: require('./to-json'),
getActive: active.get,
activate: function activate(loadedTheme, checkedTheme) {
if (!_.has(checkedTheme, 'results.score.level') || checkedTheme.results.score.level !== 'passing') {
throw new errors.InternalServerError({
message: i18n.t('errors.middleware.themehandler.invalidTheme', {theme: loadedTheme.name})
});
}

// Use the two theme objects to set the current active theme
active.set(loadedTheme, checkedTheme);
}
};
Loading

0 comments on commit b06f03b

Please sign in to comment.