Infrastructure/ghost
0
Fork 0
mirror of https://github.com/TryGhost/Ghost.git synced 2025-03-11 02:12:21 -05:00
Code Issues Activity

Restructured theme check logic

Browse source 
refs #10571

- Removes dependency on 'context' property being set in error when
checking a theme
- Refactoring was needed to be able to avoid passing checked theme as a
part of thrown error (logic was relying on error having this specific
data in context property). This created a problem where we controlled
the logic flow with data in error object.
- Introduced 2 different types of theme check handling, one behaves the
same way as before, the other gives more granulac control to the caller
to decide what to do with returned errors.
This commit is contained in:
Nazar Gargol 2019-04-22 17:31:56 +02:00
parent d989d62c10
commit cea598597b
5 changed files with 63 additions and 50 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
core/server/api/v0.1/themes.js
View file

@ -58,7 +58,7 @@ themes = {
}));
}
return themeUtils.validate.check(loadedTheme);
return themeUtils.validate.checkSafe(loadedTheme);
})
// Update setting
.then((_checkedTheme) => {
@ -100,7 +100,7 @@ themes = {
.handlePermissions('themes', 'add')(options)
// Validation
.then(() => {
return themeUtils.validate.check(zip, true);
return themeUtils.validate.checkSafe(zip, true);
})
// More validation (existence check)
.then((_checkedTheme) => {

4
core/server/api/v2/themes.js
View file

@ -49,7 +49,7 @@ module.exports = {
}));
}
return themeService.validate.check(loadedTheme)
return themeService.validate.checkSafe(loadedTheme)
.then((_checkedTheme) => {
checkedTheme = _checkedTheme;
@ -89,7 +89,7 @@ module.exports = {
});
}
return themeService.validate.check(zip, true)
return themeService.validate.checkSafe(zip, true)
.then((_checkedTheme) => {
checkedTheme = _checkedTheme;

51
core/server/services/themes/index.js
View file

@ -1,3 +1,4 @@
const _ = require('lodash');
const debug = require('ghost-ignition').debug('themes');
const common = require('../../lib/common');
const themeLoader = require('./loader');
@ -33,29 +34,37 @@ module.exports = {
return validate
.check(theme)
.then(function validationSuccess(checkedTheme) {
// CASE: inform that the theme has errors, but not fatal (theme still works)
if (checkedTheme.results.error.length) {
common.logging.warn(new common.errors.ThemeValidationError({
errorType: 'ThemeWorksButHasErrors',
message: common.i18n.t('errors.middleware.themehandler.themeHasErrors', {theme: activeThemeName}),
errorDetails: checkedTheme.results.error
}));
}
debug('Activating theme (method A on boot)', activeThemeName);
self.activate(theme, checkedTheme);
})
.catch(function validationFailure(err) {
if (err.errorDetails) {
common.logging.error(new common.errors.ThemeValidationError({
if (!validate.canActivate(checkedTheme)) {
const checkError = new common.errors.ThemeValidationError({
message: common.i18n.t('errors.middleware.themehandler.invalidTheme', {theme: activeThemeName}),
errorDetails: err.errorDetails
}));
}
errorDetails: Object.assign(
_.pick(checkedTheme, ['checkedVersion', 'name', 'path', 'version']), {
errors: checkedTheme.results.error
}
)
});
// CASE: we have to remember to show errors on blog
// `err.context` remembers the theme inside this property
self.activate(theme, err.context, err);
common.logging.error(checkError);
self.activate(theme, checkedTheme, checkError);
} else {
// CASE: inform that the theme has errors, but not fatal (theme still works)
if (checkedTheme.results.error.length) {
common.logging.warn(new common.errors.ThemeValidationError({
errorType: 'ThemeWorksButHasErrors',
message: common.i18n.t('errors.middleware.themehandler.themeHasErrors', {theme: activeThemeName}),
errorDetails: Object.assign(
_.pick(checkedTheme, ['checkedVersion', 'name', 'path', 'version']), {
errors: checkedTheme.results.error
}
)
}));
}
debug('Activating theme (method A on boot)', activeThemeName);
self.activate(theme, checkedTheme);
}
});
})
.catch(common.errors.NotFoundError, function (err) {

37
core/server/services/themes/validate.js
View file

@ -3,12 +3,16 @@ const Promise = require('bluebird');
const config = require('../../config');
const common = require('../../lib/common');
let checkTheme;
const canActivate = function canActivate(checkedTheme) {
// CASE: production and no fatal errors
// CASE: development returns fatal and none fatal errors, theme is only invalid if fatal errors
return !checkedTheme.results.error.length || (config.get('env') === 'development') && !checkedTheme.results.hasFatalErrors;
};
checkTheme = function checkTheme(theme, isZip) {
var checkPromise,
// gscan can slow down boot time if we require on boot, for now nest the require.
gscan = require('gscan');
const check = function check(theme, isZip) {
// gscan can slow down boot time if we require on boot, for now nest the require.
const gscan = require('gscan');
let checkPromise;
if (isZip) {
checkPromise = gscan.checkZip(theme, {
@ -24,10 +28,14 @@ checkTheme = function checkTheme(theme, isZip) {
onlyFatalErrors: config.get('env') === 'production'
});
// CASE: production and no fatal errors
// CASE: development returns fatal and none fatal errors, theme is only invalid if fatal errors
if (!checkedTheme.results.error.length ||
config.get('env') === 'development' && !checkedTheme.results.hasFatalErrors) {
return checkedTheme;
});
};
const checkSafe = function checkSafe(theme, isZip) {
return check(theme, isZip)
.then((checkedTheme) => {
if (canActivate(checkedTheme)) {
return checkedTheme;
}
@ -37,14 +45,11 @@ checkTheme = function checkTheme(theme, isZip) {
_.pick(checkedTheme, ['checkedVersion', 'name', 'path', 'version']), {
errors: checkedTheme.results.error
}
),
// NOTE: needs to be removed but first has to be decoupled
// from logic here: https://github.com/TryGhost/Ghost/blob/9810834/core/server/services/themes/index.js#L56-L57
context: checkedTheme
)
}));
}).catch(function (error) {
return Promise.reject(error);
});
};
module.exports.check = checkTheme;
module.exports.check = check;
module.exports.checkSafe = checkSafe;
module.exports.canActivate = canActivate;

17
core/test/unit/services/themes/validate_spec.js
View file

@ -6,7 +6,6 @@ const themes = require('../../../../server/services/themes');
const validate = themes.validate;
const gscan = require('gscan');
const common = require('../../../../server/lib/common');
describe('Themes', function () {
let checkZipStub;
@ -41,6 +40,8 @@ describe('Themes', function () {
checkStub.callCount.should.be.equal(0);
formatStub.calledOnce.should.be.true();
checkedTheme.should.be.an.Object();
should.equal(validate.canActivate(checkedTheme), true);
});
});
@ -55,6 +56,8 @@ describe('Themes', function () {
checkStub.calledWith(testTheme.path).should.be.true();
formatStub.calledOnce.should.be.true();
checkedTheme.should.be.an.Object();
should.equal(validate.canActivate(checkedTheme), true);
});
});
@ -77,14 +80,12 @@ describe('Themes', function () {
return validate.check(testTheme, true)
.then((checkedTheme) => {
checkedTheme.should.not.exist();
}).catch((error) => {
error.should.be.an.Object();
(error instanceof common.errors.ThemeValidationError).should.eql(true);
checkZipStub.calledOnce.should.be.true();
checkZipStub.calledWith(testTheme).should.be.true();
checkStub.callCount.should.be.equal(0);
formatStub.calledOnce.should.be.true();
should.equal(validate.canActivate(checkedTheme), false);
});
});
@ -107,14 +108,12 @@ describe('Themes', function () {
return validate.check(testTheme, false)
.then((checkedTheme) => {
checkedTheme.should.not.exist();
}).catch((error) => {
error.should.be.an.Object();
(error instanceof common.errors.ThemeValidationError).should.eql(true);
checkStub.calledOnce.should.be.true();
checkStub.calledWith(testTheme.path).should.be.true();
checkZipStub.callCount.should.be.equal(0);
formatStub.calledOnce.should.be.true();
should.equal(validate.canActivate(checkedTheme), false);
});
});