🎨 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
TryGhost · Jun 6, 2017 · 8680099 · 8680099
1 parent a61e6e7
commit 8680099
Show file tree

Hide file tree

Showing 14 changed files with 107 additions and 107 deletions.
diff --git a/core/server/middleware/error-handler.js b/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
  *
@@ -114,13 +72,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
-        };
-
-    if (err.statusCode === 500 && config.get('printErrorStack')) {
-        templateData.stack = err.stack;
-    }
+        message: err.message,
+        code: err.statusCode,
+        errorDetails: err.errorDetails || []
+    };
 
     // 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

diff --git a/core/server/themes/active.js b/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;
+        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) {
-        currentActiveTheme = new ActiveTheme(loadedTheme, checkedTheme);
+    set(loadedTheme, checkedTheme, error) {
+        currentActiveTheme = new ActiveTheme(loadedTheme, checkedTheme, error);
         return currentActiveTheme;
     }
 };
diff --git a/core/server/themes/index.js b/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
-                        logging.error(new errors.InternalServerError({
-                            message: i18n.t('errors.middleware.themehandler.invalidTheme', {theme: activeThemeName}),
-                            err: err
-                        }));
+                        if (err.errorDetails) {
+                            logging.error(new errors.ThemeValidationError({
+                                message: i18n.t('errors.middleware.themehandler.invalidTheme', {theme: activeThemeName}),
+                                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 () {
-                // 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}));
+            .catch(function (err) {
+                // CASE: active theme is missing, we don't want to exit because the admin panel will still work
+                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') {
-            throw new errors.InternalServerError({
-                message: i18n.t('errors.middleware.themehandler.invalidTheme', {theme: loadedTheme.name})
-            });
-        }
-
+        // no need to check the score, activation should be used in combination with validate.check
         // Use the two theme objects to set the current active theme
         active.set(loadedTheme, checkedTheme);
     },

diff --git a/core/server/themes/middleware.js b/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);

diff --git a/core/server/themes/to-json.js b/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};

diff --git a/core/server/themes/validate.js b/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
-        if (!checkedTheme.results.error.length) {
+        // 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;
         }
 
         return Promise.reject(new errors.ThemeValidationError({
             message: i18n.t('errors.api.themes.invalidTheme'),
-            errorDetails: checkedTheme.results.error
+            errorDetails: checkedTheme.results.error,
+            context: checkedTheme
         }));
     });
 };

diff --git a/core/server/translations/en.json b/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": {

diff --git a/core/server/utils/packages/read-packages.js b/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 () {
-            return Promise.reject(new Error('Package not found'));
+        .catch(function (err) {
+            return Promise.reject(new errors.NotFoundError({
+                message: 'Package not found',
+                err: err,
+                help: 'path: ' + packagePath,
+                context: 'name: ' + packageName
+            }));
         });
 };
 

diff --git a/core/server/views/error.hbs b/core/server/views/error.hbs
@@ -36,20 +36,24 @@
                     </section>
                   </section>
                 </section>
-                {{#if stack}}
-                  <section class="error-stack">
-                    <h3>Stack Trace</h3>
-                    <p><strong>{{message}}</strong></p>
-                    <ul class="error-stack-list">
-                      {{#each stack}}
-                        <li>
-                          at
-                          {{#if function}}<em class="error-stack-function">{{function}}</em>{{/if}}
-                          <span class="error-stack-file">({{at}})</span>
-                        </li>
-                      {{/each}}
-                    </ul>
-                  </section>
+
+                {{#if errorDetails}}
+                    <section class="error-stack">
+                        <h3>Theme errors</h3>
+
+                        <ul class="error-stack-list">
+                            {{#each errorDetails}}
+                                <li>
+                                    <em class="error-stack-function">{{{rule}}}</em>
+
+                                    {{#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>
+                    </section>
                 {{/if}}
               </div>
           </div>

diff --git a/core/test/functional/routes/api/themes_spec.js b/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'));

diff --git a/core/test/utils/fixtures/themes/valid.zip b/core/test/utils/fixtures/themes/valid.zip
diff --git a/core/test/utils/fixtures/themes/warnings.zip b/core/test/utils/fixtures/themes/warnings.zip
diff --git a/package.json b/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",