TryGhost / Ghost

Independent technology for modern publishing, memberships, subscriptions and newsletters.
https://ghost.org
MIT License
46.27k stars 10.11k forks source link

Better error handling when starting Ghost #3864

Closed ErisDS closed 9 years ago

ErisDS commented 9 years ago

There are a number of things that can go wrong when starting Ghost, which are currently not handled very well. All of these things need catching, and handling with nice error messages styled using our error logging tool in the errors module: errors.logError(error, context, help), which outputs something like:

ghost errors

If any of these are hard to explain how to fix, we create and link to an article in support.ghost.org.

TLDR;

  1. [x] Bad Database configuration
  2. [x] Invalid content path / other config
  3. [x] npm install failures
  4. [x] Bad node version
  5. [x] SQLITE_READONLY and other permissions errors

    Long list

    1. Bad Database configuration

Originally from #3849:

a misconfigured config file produces very opaque errors. For example, if database.connection.filename is incorrectly set, SQLite complains with

cannot read property _cid of undefined.

This error also appears if the mysql database does not exist, or essentially if the database configuration is valid, but broken. This possibly needs looking at in bookshelf / knex, but we should catch these errors and provide a more sane one.

Additionally, providing the wrong name for pg results in weird errors, one reported via twitter:

And another on GitHub: https://github.com/TryGhost/Ghost/issues/3509#issuecomment-50886603

We should validate that the client is one of sqlite3, mysql or pg and provide a sensible error if it isn't. Perhaps even a warning if it's pg saying support is unofficial?

2. Invalid content path / other config

Originally from #3849:

If paths.contentPath is incorrect, a cryptic Casper not installed error is thrown:

If paths.contentPath is set, we ought to test that that path exists, and that it contains the 4 folders that we expect. If it doesn't, again we should give a clear error message.

3. npm install failures

Originall from #3509:

On a fresh copy of Ghost if you run npm start --production before running npm install --production there is no useful messaging indicating why things have failed

Any missing module produces the following error:

module.js:340
    throw err;
          ^
Error: Cannot find module 'when'

This is obvious to Ghost devs because we know what when or any other module is. To anyone new, this is baffling. It needs catching and replacing with a clear message stating that npm install has failed.

4. Bad node version

Having the wrong node version has always failed during npm install with unclear messaging. I believe that the main module we have which causes this, sqlite3 now handles this better, but sqlite3 supports node >0.11.13 which Ghost does not yet. We have an error message which should appear if you get as far as starting Ghost, this needs testing.

5. SQLITE_READONLY and other permissions errors

from #3687

When Ghost starts, we should detect if we have permissions to write to /content/data and /content/images. if the database client is sqlite3 we should also specifically check if we have permissions to write to the db file. If the permissions are wrong, we should output a sensible error.

jaswilli commented 9 years ago

I can confirm that # 4 is working (Unsupported version of node).

I've also been running a branch on node 0.11.13 for testing and everything is looking good. Since 0.11.13 is (claimed to be) the last unstable release before 0.12 (stable) it would be really nice to be out in front of that.

ErisDS commented 9 years ago

@jtw totally agree

jaswilli commented 9 years ago
  1. Database configuration

    1a. That error gets thrown async out of bookshelf, so it's not easy for us to handle. I'm going to see if it's a straightforward fix and then possibly submit a PR against bookshelf or knex.

    1b. knex will try to match up the database (postgres, postgresql, pg => pg) and if it can't, throw a sensible error about checking the database type and making sure it's correct. I think we're okay on this one.

jaswilli commented 9 years ago

Just talked to @halfdan in IRC and he's going to do 1 (database) and I'm going to put something together for 2 (contentPath).

halfdan commented 9 years ago
  1. Database configuration
    • The __cid error is async as @jaswilli has already noted and appears on the first query executed on the knex instance. Knex doesn't provide a way to check whether a connection to the database is active. I proposed a small fix to tgriesser/knex#456 that would at least yield a helpful error message
    • The No support for database: postgres error was caused by our client implementation. Knex supports multiple client names that all map to postgres (https://github.com/tgriesser/knex/blob/f3874ac6512ea51169e3d387142b0b01e5aaf1f3/knex.js#L37-L50). PR #3914 does the same for our client implementation.
halfdan commented 9 years ago

I think this can be closed and all items checked.

novaugust commented 9 years ago

I'll leave it for @jaswilli to do :D

ErisDS commented 9 years ago

The PR isn't merged yet, I started looking at the 2 of them earlier, will get to it today.

jaswilli commented 9 years ago

Closed via #3913 and #3914.