Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error & debug handling for developers #8222

Closed
11 tasks done
ErisDS opened this issue Mar 23, 2017 · 1 comment · Fixed by #9129
Closed
11 tasks done

Error & debug handling for developers #8222

ErisDS opened this issue Mar 23, 2017 · 1 comment · Fixed by #9129
Assignees
Labels
server / core Issues relating to the server or core of Ghost

Comments

@ErisDS
Copy link
Member

ErisDS commented Mar 23, 2017

#Refs #7491

I've heavily reworked how themes are loaded, validated & how they work in general over the course of the last couple of weeks.

This has resulted in what is sort of a bug and sort of a concept that hasn't been worked through correctly.

Bug report:

Starting Ghost with an invalid theme results in a small log warning that says "The active theme is invalid". This is just a warning, because the admin panel will still load. This is the correct behaviour, although the warning should probably be bigger and clearer.

However, when you finally come to try to access a page of your blog, the error you get says:

"The currently active theme is missing".

It's missing because due to the theme being invalid, we didn't load it in. So now Ghost (somewhat correctly) thinks your active theme isn't present.

For those users who miss the invalid warning (which will be pretty much everyone) this will be the first message they see, and it's effectively wrong and very confusing.

Where it gets interesting

There are 2 cases for which an active theme can be invalid:

  • CASE 1: you are a developer updating the theme files locally on your machine / dev server.
  • CASE 2: we do an upgrade in which gscan rules change

CASE 1: only affects developers / development mode. CASE 2: will impact all users and is effectively us doing something wrong (we should have known that this would happen and ensured the theme was updated first), but the impact is huge - the whole blog will refuse to render.

Note that the reason why the error message is wrong is because we don't activate the invalid theme. This means we don't keep reference to the gscan output or state of the theme, and so in the current iteration we cannot tell the user a better error. It's just missing.

Even worse, if you're a developer working on a theme, the most common error that will happen is malforming some handlebars, in which case you want handlebars to give you the compile errors when you try to render the page, not gscan to fail to boot. However, gscan will detect the error, the theme won't activate, and you'll be left with gscan errors, which we currently don't even output 😖

Potential solution

There are 3 mechanisms for activating a theme:

  • on boot
  • on activate
  • on uploading with override

Both of these cases ONLY impact on the boot. This got me thinking we need to further evolve the concept of "mounting" and "activating".

If a theme is being actively activated by a user (via activate or upload endpoints), we should return the output from gscan and go no further.

On boot, we need a more evolved approach. I think the rules need to be something like this:

DEV MODE

  • Gscan warning -> log Gscan warning, mount
  • Gscan compile error -> log Gscan error, mount -> allow handlebars to attempt compile
  • Gscan other error -> log Gscan error, mount with error -> allow gscan error to be rendered as 500

PROD MODE

  • Gscan warning -> output, mount
  • Gscan error -> log Gscan error, mount with error -> allow gscan error to be rendered as 500

I'm not 100% sure, I first need to implement a way to pass through gscan errors and see what they can look like rendered.

  • first task: go over gscan errors and mark fatal and none fatal errors (K)
  • if bootstrap and theme is invalid, navigate to blog frontend and you will see "theme is missing" -> change to theme is invalid and show errors if possible? (A)
  • usage of valid hbs helper with invalid usage (K)
  • differentiate other errors from compile errors - show other errors (which are probably gscan errors) as 500 errors in frontend (A)
  • differentiate fatal errors from none fatal errors in Ghost (K)
  • consider/check usability to only show fatal errors in production mode when activating/uploading theme (A)
  • when hbs compile errors, mount in dev and show hbs error, don't mount and show gscan error in prod (K)
  • produce gscan warning and check bootstrap behaviour (A)
  • log errors on bootstrap if theme is invalid
  • optimise admin report - structure a little (e.g. little boxes per error, show (and order) fatal error attribute) (J+A+K)
  • look at gscan warnings again
@ErisDS ErisDS added the themes label Mar 23, 2017
@ErisDS ErisDS added this to the 1.0.0 Beta Ready milestone Mar 23, 2017
@ErisDS ErisDS mentioned this issue Mar 23, 2017
14 tasks
@ErisDS ErisDS modified the milestones: 1.0.0 Beta Ready, 1.0.0 Launch Apr 3, 2017
@kirrg001 kirrg001 removed this from the 1.0.0 Backlog milestone May 12, 2017
@kirrg001 kirrg001 added the server / core Issues relating to the server or core of Ghost label May 12, 2017
@kirrg001 kirrg001 self-assigned this May 31, 2017
kirrg001 added a commit to TryGhost/gscan that referenced this issue Jun 1, 2017
refs TryGhost/Ghost#8222

- we differentiate between errors and fatal errors
- based on this property, Ghost can decide to activate a theme
- no tests yet (coming soon)
kirrg001 added a commit to kirrg001/gscan that referenced this issue Jun 1, 2017
kirrg001 added a commit to kirrg001/gscan that referenced this issue Jun 1, 2017
kirrg001 added a commit to kirrg001/gscan that referenced this issue Jun 1, 2017
kirrg001 added a commit to TryGhost/gscan that referenced this issue Jun 1, 2017
kirrg001 added a commit to kirrg001/Ghost that referenced this issue Jun 4, 2017
refs TryGhost#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
aileen pushed a commit that referenced this issue Jun 6, 2017
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
@ErisDS
Copy link
Member Author

ErisDS commented Oct 11, 2017

I'm still seeing some problems with theme error handling, whereby Ghost shows "the currently active theme X is missing" when the theme is fine and something else is wrong.

Reproduction case A - gscan is broken:

  1. uninstall the gscan module
  2. npm link gscan to a slightly older version that doesn't have custom templates

Reproduction case B - invalid node version:

  • Users trying to use node v8 get this incorrect error

@ErisDS ErisDS reopened this Oct 11, 2017
@ErisDS ErisDS assigned ErisDS and unassigned kirrg001 and aileen Oct 11, 2017
ErisDS added a commit to ErisDS/Ghost that referenced this issue Oct 11, 2017
closes TryGhost#8222

- There are still some cases where Ghost shows "the currently active theme X is missing" when it isn't
- This is due to the error handling masking several cases
- This PR resolves that, ensuring errors from gscan and the underlying environment don't get masked
kirrg001 pushed a commit that referenced this issue Oct 11, 2017
closes #8222

- There are still some cases where Ghost shows "the currently active theme X is missing" when it isn't
- This is due to the error handling masking several cases
- This PR resolves that, ensuring errors from gscan and the underlying environment don't get masked
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
server / core Issues relating to the server or core of Ghost
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants