🎨 gscan 1.1.0 & optimisations

refs #8222

- differentiate between errors and fatal errors
- use gscan errors in theme middleware
- Adds a new `error()` method to `currentActiveTheme` constructor which will return the errors we receive from gscan
- In middleware, if a theme couldn't be activated because it's invalid, we'll fetch the erros and send them to our error handler. We also use a new property `hideStack` to control, if the stack (in dev mode and if available) should be shown or the gscan errors (in prod mode, or in dev if no stack error)
- In our error handler we use this conditional to send a new property `gscan` to our error theme
- In `error.hbs` we'll iterate through possible `gscan` error objects and render them.
- remove stack printing
- stack for theme developers in development mode doesn't make sense
- stack in production doesn't make sense
- the stack is usually hard to read
- if you are developer you can read the error stack on the server log
- utils.packages: transform native error into Ghost error
- use `onlyFatalErrors` for gscan format and differeniate fatal errors vo.2
- optimise bootstrap error handling
- transform theme is missing into an error
- add new translation key
- show html tags for error.hbs template: rule

core/server/middleware/error-handler.js

@@ -1,6 +1,5 @@
 var _ = require('lodash'),
     hbs = require('express-hbs'),
-    config = require('../config'),
     errors = require('../errors'),
     i18n = require('../i18n'),
     templates = require('../controllers/frontend/templates'),
@@ -19,47 +18,6 @@ _private.createHbsEngine = function createHbsEngine() {
     return engine.express4();
 };
 
-/**
- * This function splits the stack into pieces, that are then rendered using the following handlebars code:
- * ```
- * {{#each stack}}
- *   <li>
- *   at
- *   {{#if function}}<em class="error-stack-function">{{function}}</em>{{/if}}
- *   <span class="error-stack-file">({{at}})</span>
- *   </li>
- * {{/each}}
- * ```
- * @TODO revisit whether this is useful as part of #7491
- */
-_private.parseStack = function parseStack(stack) {
-    if (!_.isString(stack)) {
-        return stack;
-    }
-
-    var stackRegex = /\s*at\s*(\w+)?\s*\(([^\)]+)\)\s*/i;
-
-    return (
-        stack
-            .split(/[\r\n]+/)
-            .slice(1)
-            .map(function (line) {
-                var parts = line.match(stackRegex);
-                if (!parts) {
-                    return null;
-                }
-
-                return {
-                    function: parts[1],
-                    at: parts[2]
-                };
-            })
-            .filter(function (line) {
-                return !!line;
-            })
-    );
-};
-
 /**
  * Get an error ready to be shown the the user
  *
@@ -115,13 +73,10 @@ _private.JSONErrorRenderer = function JSONErrorRenderer(err, req, res, /*jshint
 _private.HTMLErrorRenderer = function HTMLErrorRender(err, req, res, /*jshint unused:false */ next) {
     var templateData = {
         message: err.message,
-            code: err.statusCode
+        code: err.statusCode,
+        errorDetails: err.errorDetails || []
     };
 
-    if (err.statusCode === 500 && config.get('printErrorStack')) {
-        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)) {

core/server/themes/active.js

@@ -21,6 +21,7 @@ var _ = require('lodash'),
     themeConfig = require('./config'),
     config = require('../config'),
     engine = require('./engine'),
+    _ = require('lodash'),
     // Current instance of ActiveTheme
     currentActiveTheme;
 
@@ -29,16 +30,19 @@ 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
+     * @param {object} error - bootstrap validates the active theme, we would like to remember this error
      */
-    constructor(loadedTheme, checkedTheme) {
+    constructor(loadedTheme, checkedTheme, error) {
         // Assign some data, mark it all as pseudo-private
         this._name = loadedTheme.name;
         this._path = loadedTheme.path;
         this._mounted = false;
+        this._error = error;
 
         // @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$/);
@@ -68,6 +72,10 @@ class ActiveTheme {
         return this._mounted;
     }
 
+    get error() {
+        return this._error;
+    }
+
     hasTemplate(templateName) {
         return this._templates.indexOf(templateName) > -1;
     }
@@ -105,8 +113,8 @@ module.exports = {
      * @param {object} checkedTheme - the result of gscan.format for the theme we're activating
      * @return {ActiveTheme}
      */
-    set(loadedTheme, checkedTheme) {
+    set(loadedTheme, checkedTheme, error) {
-        currentActiveTheme = new ActiveTheme(loadedTheme, checkedTheme);
+        currentActiveTheme = new ActiveTheme(loadedTheme, checkedTheme, error);
         return currentActiveTheme;
     }
 };

core/server/themes/index.js

@@ -1,5 +1,4 @@
 var debug = require('debug')('ghost:themes'),
-    _ = require('lodash'),
     events = require('../events'),
     errors = require('../errors'),
     logging = require('../logging'),
@@ -35,21 +34,35 @@ module.exports = {
                 return validate
                     .check(theme)
                     .then(function resultHandler(checkedTheme) {
-                        // Activate! (sort of)
+                        // CASE: inform that the theme has errors, but not fatal (theme still works)
+                        if (checkedTheme.results.error.length) {
+                            logging.warn(new errors.ThemeValidationError({
+                                errorType: 'ThemeWorksButHasErrors',
+                                message: i18n.t('errors.middleware.themehandler.themeHasErrors', {theme: activeThemeName}),
+                                errorDetails: JSON.stringify(checkedTheme.results.error, null, '\t')
+                            }));
+                        }
+
                         debug('Activating theme (method A on boot)', activeThemeName);
                         self.activate(theme, checkedTheme);
                     })
                     .catch(function (err) {
-                        // Active theme is not valid, we don't want to exit because the admin panel will still work
+                        if (err.errorDetails) {
-                        logging.error(new errors.InternalServerError({
+                            logging.error(new errors.ThemeValidationError({
                                 message: i18n.t('errors.middleware.themehandler.invalidTheme', {theme: activeThemeName}),
-                            err: err
+                                errorDetails: JSON.stringify(err.errorDetails, null, '\t')
                             }));
+                        }
+
+                        // CASE: we have to remember to show errors on blog
+                        // `err.context` remembers the theme inside this property
+                        active.set(theme, err.context, err);
                     });
             })
-            .catch(function () {
+            .catch(function (err) {
-                // Active theme is missing, we don't want to exit because the admin panel will still work
+                // CASE: 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}));
+                err.message = i18n.t('errors.middleware.themehandler.missingTheme', {theme: activeThemeName});
+                logging.error(err);
             });
     },
     // Load themes, soon to be removed and exposed via specific function.
@@ -65,12 +78,7 @@ module.exports = {
     toJSON: require('./to-json'),
     getActive: active.get,
     activate: function activate(loadedTheme, checkedTheme) {
-        if (!_.has(checkedTheme, 'results.score.level') || checkedTheme.results.score.level !== 'passing') {
+        // no need to check the score, activation should be used in combination with validate.check
-            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);
     },

core/server/themes/middleware.js

@@ -2,6 +2,7 @@ var _  = require('lodash'),
     hbs = require('./engine'),
     utils = require('../utils'),
     errors = require('../errors'),
+    config = require('../config'),
     i18n = require('../i18n'),
     settingsCache = require('../settings/cache'),
     activeTheme = require('./active'),
@@ -12,7 +13,7 @@ var _  = require('lodash'),
 // If there is no active theme, throw an error
 // Else, ensure the active theme is mounted
 themeMiddleware.ensureActiveTheme = function ensureActiveTheme(req, res, next) {
-    // This means that the theme hasn't been loaded yet i.e. there is no active theme
+    // CASE: this means that the theme hasn't been loaded yet i.e. there is no active theme
     if (!activeTheme.get()) {
         // 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({
@@ -22,6 +23,16 @@ themeMiddleware.ensureActiveTheme = function ensureActiveTheme(req, res, next) {
         }));
     }
 
+    // CASE: bootstrap theme validation failed, we would like to show the errors on the blog [only production]
+    if (activeTheme.get().error && config.get('env') === 'production') {
+        return next(new errors.InternalServerError({
+            // 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.
+            message: i18n.t('errors.middleware.themehandler.invalidTheme', {theme: settingsCache.get('active_theme')}),
+            errorDetails: activeTheme.get().error.errorDetails
+        }));
+    }
+
     // If the active theme has not yet been mounted, mount it into express
     if (!activeTheme.get().mounted) {
         activeTheme.get().mount(req.app);

core/server/themes/to-json.js

@@ -27,6 +27,10 @@ module.exports = function toJSON(name, checkedTheme) {
         if (checkedTheme && checkedTheme.results.warning.length > 0) {
             themeResult[0].warnings = _.cloneDeep(checkedTheme.results.warning);
         }
+
+        if (checkedTheme && checkedTheme.results.error.length > 0) {
+            themeResult[0].errors = _.cloneDeep(checkedTheme.results.error);
+        }
     }
 
     return {themes: themeResult};

core/server/themes/validate.js

@@ -1,4 +1,5 @@
 var Promise = require('bluebird'),
+    config = require('../config'),
     errors = require('../errors'),
     i18n = require('../i18n'),
     checkTheme;
@@ -9,22 +10,29 @@ checkTheme = function checkTheme(theme, isZip) {
         gscan = require('gscan');
 
     if (isZip) {
-        checkPromise = gscan.checkZip(theme, {keepExtractedDir: true});
+        checkPromise = gscan.checkZip(theme, {
+            keepExtractedDir: true
+        });
     } else {
         checkPromise = gscan.check(theme.path);
     }
 
     return checkPromise.then(function resultHandler(checkedTheme) {
-        checkedTheme = gscan.format(checkedTheme);
+        checkedTheme = gscan.format(checkedTheme, {
+            onlyFatalErrors: config.get('env') === 'production'
+        });
 
-        // @TODO improve gscan results
+        // CASE: production and no fatal errors
-        if (!checkedTheme.results.error.length) {
+        // 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;
         }
 
         return Promise.reject(new errors.ThemeValidationError({
             message: i18n.t('errors.api.themes.invalidTheme'),
-            errorDetails: checkedTheme.results.error
+            errorDetails: checkedTheme.results.error,
+            context: checkedTheme
         }));
     });
 };

core/server/translations/en.json

@@ -106,7 +106,8 @@
             },
             "themehandler": {
                 "missingTheme": "The currently active theme \"{theme}\" is missing.",
-                "invalidTheme": "The currently active theme \"{theme}\" is invalid."
+                "invalidTheme": "The currently active theme \"{theme}\" is invalid.",
+                "themeHasErrors": "The currently active theme has errors, but theme still works."
             }
         },
         "utils": {

core/server/utils/packages/read-packages.js

@@ -3,6 +3,7 @@
  */
 
 var parsePackageJson = require('./parse-package-json'),
+    errors = require('../../errors'),
     Promise = require('bluebird'),
     _ = require('lodash'),
     join = require('path').join,
@@ -56,8 +57,13 @@ readPackage = function readPackage(packagePath, packageName) {
                     return res;
                 });
         })
-        .catch(function () {
+        .catch(function (err) {
-            return Promise.reject(new Error('Package not found'));
+            return Promise.reject(new errors.NotFoundError({
+                message: 'Package not found',
+                err: err,
+                help: 'path: ' + packagePath,
+                context: 'name: ' + packageName
+            }));
         });
 };
 

core/server/views/error.hbs

@@ -36,16 +36,20 @@
                     </section>
                   </section>
                 </section>
-                {{#if stack}}
+
+                {{#if errorDetails}}
                     <section class="error-stack">
-                    <h3>Stack Trace</h3>
+                        <h3>Theme errors</h3>
-                    <p><strong>{{message}}</strong></p>
+
                         <ul class="error-stack-list">
-                      {{#each stack}}
+                            {{#each errorDetails}}
                                 <li>
-                          at
+                                    <em class="error-stack-function">{{{rule}}}</em>
-                          {{#if function}}<em class="error-stack-function">{{function}}</em>{{/if}}
+
-                          <span class="error-stack-file">({{at}})</span>
+                                    {{#each failures}}
+                                        <p><span class="error-stack-file">Ref: {{ref}}</span></p>
+                                        <p><span class="error-stack-file">Message: {{message}}</span></p>
+                                    {{/each}}
                                 </li>
                             {{/each}}
                         </ul>

core/test/functional/routes/api/themes_spec.js

@@ -46,7 +46,7 @@ describe('Themes API (Forked)', function () {
         fs.mkdirSync(join(tmpContentPath.name, 'themes', 'test-theme'));
         fs.writeFileSync(
             join(tmpContentPath.name, 'themes', 'test-theme', 'package.json'),
-            JSON.stringify({name: 'test-theme', version: '0.5'})
+            JSON.stringify({name: 'test-theme', version: '0.5.9', author: {email: 'test@example.org'}})
         );
         fs.writeFileSync(join(tmpContentPath.name, 'themes', 'test-theme', 'index.hbs'));
         fs.writeFileSync(join(tmpContentPath.name, 'themes', 'test-theme', 'post.hbs'));

package.json

@@ -52,7 +52,7 @@
     "ghost-ignition": "2.8.11",
     "ghost-storage-base": "0.0.1",
     "glob": "5.0.15",
-    "gscan": "1.0.0",
+    "gscan": "https://github.com/TryGhost/gscan/tarball/b45192207cb11c43998f489b72ec6d3ee03cc6b0",
     "html-to-text": "3.2.0",
     "icojs": "0.7.2",
     "image-size": "0.5.2",

yarn.lock

@@ -2131,9 +2131,9 @@ grunt@~0.4.0:
     underscore.string "~2.2.1"
     which "~1.0.5"
 
-gscan@1.0.0:
+"gscan@https://github.com/TryGhost/gscan/tarball/b45192207cb11c43998f489b72ec6d3ee03cc6b0":
   version "1.0.0"
-  resolved "https://registry.yarnpkg.com/gscan/-/gscan-1.0.0.tgz#7b4e9856e8d987c4d0d9bd39f6aa6e7a2d29c415"
+  resolved "https://github.com/TryGhost/gscan/tarball/b45192207cb11c43998f489b72ec6d3ee03cc6b0#4ebf8ab83fbeb619cfb8fa76e2f5fafaeade3ba5"
   dependencies:
     bluebird "3.4.6"
     chalk "1.1.1"
@@ -2146,9 +2146,10 @@ gscan@1.0.0:
     glob "7.0.5"
     lodash "3.10.1"
     multer "1.1.0"
-    package-json-validator "0.6.0"
     require-dir "0.1.0"
+    semver "^5.3.0"
     uuid "^3.0.0"
+    validator "^6.3.0"
 
 gulp-util@*:
   version "3.0.8"
@@ -3480,13 +3481,13 @@ mkdirp@0.3.2:
   version "0.3.2"
   resolved "https://registry.yarnpkg.com/mkdirp/-/mkdirp-0.3.2.tgz#4bfb891e9c48b93d6b567f2c3cf2dd3f56bcdef8"
 
-mkdirp@0.5.0:
+mkdirp@0.5.0, mkdirp@^0.5.0:
   version "0.5.0"
   resolved "https://registry.yarnpkg.com/mkdirp/-/mkdirp-0.5.0.tgz#1d73076a6df986cd9344e15e71fcc05a4c9abf12"
   dependencies:
     minimist "0.0.8"
 
-mkdirp@0.5.1, mkdirp@0.5.x, mkdirp@0.x.x, "mkdirp@>=0.5 0", mkdirp@^0.5.0, mkdirp@^0.5.1, mkdirp@~0.5.0, mkdirp@~0.5.1:
+mkdirp@0.5.1, mkdirp@0.5.x, mkdirp@0.x.x, "mkdirp@>=0.5 0", mkdirp@^0.5.1, mkdirp@~0.5.0, mkdirp@~0.5.1:
   version "0.5.1"
   resolved "https://registry.yarnpkg.com/mkdirp/-/mkdirp-0.5.1.tgz#30057438eac6cf7f8c4767f38648d6697d75c903"
   dependencies:
@@ -3836,7 +3837,7 @@ once@1.x, once@^1.3.0, once@^1.3.3, once@^1.4.0:
   dependencies:
     wrappy "1"
 
-optimist@^0.6.1, optimist@~0.6.0:
+optimist@^0.6.1:
   version "0.6.1"
   resolved "https://registry.yarnpkg.com/optimist/-/optimist-0.6.1.tgz#da3ea74686fa21a19a111c326e90eb15a0196686"
   dependencies:
@@ -3875,12 +3876,6 @@ osenv@^0.1.4:
     os-homedir "^1.0.0"
     os-tmpdir "^1.0.0"
 
-package-json-validator@0.6.0:
-  version "0.6.0"
-  resolved "https://registry.yarnpkg.com/package-json-validator/-/package-json-validator-0.6.0.tgz#5629c7328b1307d09e7f291027bd15952bbe80c2"
-  dependencies:
-    optimist "~0.6.0"
-
 pako@~0.2.0:
   version "0.2.9"
   resolved "https://registry.yarnpkg.com/pako/-/pako-0.2.9.tgz#f3f7522f4ef782348da8161bad9ecfd51bf83a75"
@@ -5473,7 +5468,7 @@ validate-npm-package-license@^3.0.1:
     spdx-correct "~1.0.0"
     spdx-expression-parse "~1.0.0"
 
-validator@6.3.0:
+validator@6.3.0, validator@^6.3.0:
   version "6.3.0"
   resolved "https://registry.yarnpkg.com/validator/-/validator-6.3.0.tgz#47ce23ed8d4eaddfa9d4b8ef0071b6cf1078d7c8"