refs https://github.com/TryGhost/Toolbox/issues/151
refs cbec6aa49e
- Without the await the try/catch block does not catch a pottential validation error straight away, which leads to a 500 error instead of a validation error being returned. The regression was introduced during the refactor (part of referenced commit).
This reverts commit 303ea87897.
- Although gscan catches these now, we have a number of sites that have slipped through the net
- Reverting until we get them all cleaned up
- one big file full of stuff is never good for clarity
- separating it out helps us see what requires what
- it also highlights the awful naming and opaque behaviour we have in themes - much to do, but this helps us start
refs https://github.com/TryGhost/Toolbox/issues/151
refs cbec6aa49e
- The error was happening due to incorrect "this" context. Because the filename and extension are only used once in this class and only for the purposes of the error message have moved the whole thing into the error message itself. No need to keep additional variables around when there's no clear usecase.
refs https://github.com/TryGhost/Team/issues/1067
As part of the work of automatically logging members in after payment,
we want to revisit the emails. Currently after payment we send an email
asking a member to _confirm_ their subscription, and that they can
ignore the email to cancel the subscription. This is not the case
however, as the member has already been subscribed.
refs: TryGhost/Toolbox#147
* Replaces all references to isIgnitionError with isGhostError
* Switches use of GhostError to InternalServerError - as GhostError is no longer public
There are places where InternalServerError is not the valid error, and new errors should be added to the @tryghost/errors package to ensure that we can use semantically correct errors in those cases.
refs https://github.com/TryGhost/Team/issues/1234
Sharp can occasionally fail resizing, this is usually due the the
underlying libvips library failing. We do not want this to cause an
error however, instead we should just show the original image - as
resizing is an optimisation, rather than a requirement.
refs https://github.com/TryGhost/Team/issues/1001
We fall back to existing behaviour if no API key is present, or if there
is an error communicating with the Twitter API. We're also currently
requesting all the data, which will be thinned down once we understand
what we need.
This also includes a custom renderer for embeds of type "twitter" which
will be used to output the custom HTML for emails
- our themeErrorRenderer is only used in the frontend.. move it there
- this required exposing prepareError as shared middleware
- TODO: move these shared compontents to @tryghost/error
refs: 0799f02e80
refs: 5e931e2e37
- with the referenced two commits I replaced our old HTML renderer with some code borrowed heavily from finalHandler
- I had intended to modify this further to out put our message, context and help error messages
- However, I ended up doing this in prepareError so it's done for all error renderers
- There's now very little point keeping duplicated code from finalHandler just to output the status code
- If we remove this code, express will fall back to finalHandler anyway, so the output is near identical
- got rid of old _private & variable pattern in favour of const and module.exports
- changed weird capitalisation naming conventions to be camelCase
- removed some very old TODOs that we're never gonna get TODONE
- these are mostly old ideas that never made it, and it's been so long they're clearly not important
refs: https://github.com/TryGhost/Toolbox/issues/105
Lint rules prevent:
* Invalid naming conventions for new migrations
* Loop constructs in migrations - these should be used with caution
and are therefore a warning rule, use `// eslint-disable-next-line
no-restricted-syntax` to prevent this rule from firing where a loop is
required
* Returing within a loop - this is usually meant to be a
continue/break
* Multiple joins - these can be badly performing migrations, so should
be treated with caution, disable the rule for the line if the risk is
understood / the migration cannot be written without it
refs: 4474ca1a1d
refs: 0799f02e80
The BasicErrorRenderer was created as a fallback for when we needed to not render templates, which is
chiefly when we're trying to render a 404 for an image. Using a template puts us at risk of an infinite 404 loop
if the missing image is referenced in the 404 template.
As of 0799f02e, the HTMLErrorRenderer no longer uses templates - instead we serve a very simple HTML page.
This can be used instead of the BasicErrorRenderer, as it results in a properly formatted error.
Even when sending responses in plain text, the content type is returned as HTML and therefore having an
unformatted error makes no sense - if we really need a non-html format I guess there should be no body at all.
refs https://github.com/TryGhost/Toolbox/issues/120
- Having an "options" parameter in the controller definition was missleading as if the `url` or `ref` parameters were expected as a part of the qurey parameter. These variables should be provided as a part of the request body, thus having them in "data" attribute is more accurate
refs https://github.com/TryGhost/Toolbox/issues/139
- Having tight coupling with backup file path calculation for redirects makes it extremely hard to test. In addition, having it injected will make it easier to swap this dependency to the mechanism similar to one used for routes files
refs https://github.com/TryGhost/Toolbox/issues/139
- The custom redirects services belong in the initServicesForFrontend because frontend depends on these to function properly. When placed in general init section the middleware would not get initialized properly before it's used by the "frontend express app"
refs https://github.com/TryGhost/Toolbox/issues/139
- The pattern we use accross the codebase is a single "options" object passed into a constructor instead of passing multiple parametes. Fixed the broken pattern in CustomRedirectsAPI constructor
refs 91efa4605c
- Referenced commit introduced a double json-stringification to uploaded redirects.json files.
- The endpoint has no stability index of any sort and is meant to be dropped in Ghost v5. It's best to rework the redirects to the yaml format as descirbe here - https://ghost.org/docs/tutorials/implementing-redirects/#file-structure
- moving this middleware because we're about to add a second piece of middleware
- it's easier to see what we have when each middleware is in its own file rather than in one big middleware.js file
refs https://github.com/TryGhost/Team/issues/1236
We use Offer names for the Stripe Coupon name - which has a limit of 40
characters. We are now introducing a limit of 40 characters to Offer
names too. This migration ensures that all our data in the DB is valid.
no issue
When switching the oembed service to async/await the error handling was not correctly refactored. `this.errorHandler(url)` was returning a curried function so it could be used as `.catch(this.errorHandler(url))` but that's not how it's being used after the async/await change meaning we were returning a function rather than the result of that function.
- `this.errorHandler(url)` is now only used in one place where `url` is available so removed the method and moved the body of the curried function inline into the `catch` handler
- added a message to the logged error so it's more clear what the log refers to
refs https://github.com/TryGhost/Team/issues/1236
We use Offer names for the Stripe Coupon name - which has a limit of 40
characters. We are now introducing a limit of 40 characters to Offer
names too. This migration ensures that all our data in the DB is valid.
- When we handle errors in Ghost, we are supposed to use a pattern of supplying 3 messages:
- message: what went wrong
- context: details about why how or where the error happened
- help: where the user can go to get help with this error
- We do this in many places and our JSON error handler and CLI error logging tools are designed to output this extra information
- However, stack traces, which start with message as the first line and then output the stack are totally missing this
- By injecting the additional messages into the stack once an error has been "ghostified" we should get clearer messages everywhere
Notes:
- I've additionally injected a "Stack Trace:" line that makes it easier to read the error vs the stack
- This code looks a little weird because the lines are inserted backwards, but that allows us to always to the insert at position 1 as per the comment,
so we don't have to keep track of whether we already injected something or not
refs: 2af9e2e12
- This new HTMLErrorRenderer is borrowed heavily from finalHandler
- This is the module that express uses to render errors if there is no custom errorhandler
- It just renders a really simple html page wrapping err.stack in a <pre>
- This results in a nicely formatted, but unstyled error page
- I also updated BasicErrorRenderer to use the same res.statusCode + err.stack pattern rather than err.message
Note: This error renderer is _only_ used for renderering errors on the `/ghost/` route
- In almost all cases, errors here are rendered by Ember
- The only error that can be rendered here is a missing template error see: 2af9e2e12
- If the admin templates default.html or default-prod.html are missing, don't throw a 500
- Instead throw a well considered 400 error with extra help for what to do to fix it
- Reduced our maintenance middleware code down to the bare minimum!
- We have an old maintenance middleware in place to handle when a site is forcibly put into maintenance mode, or the urlService hasn't finished booting
- This maintenance middleware was mounted on every sub app, instead of globally for reasons I no longer remember
- Recently, we introduced a new, static version of maintenence middleware to show during the boot process so we can get the server started earlier & not drop requests
- This version has its own HTML template and doesn't depend on any of Ghost's error rendering code
- To simplify and help with decoupling, this commit merges the two middleware, so that the new independent & static middleware renders its template for any one of the 3 possible maintenance modes
- It only needs to exist in the top level app 🙌
TODO: move the maintenance middleware to its own file/package so it's not part of the app.js as that is weird
- throughout the theme activation flow there are several missing awaits and necessary async keywords
- we should be waiting on these processes, not letting them complete indeterministically
