core: Self-handling exceptions should not have priority in Exception handler

Exception handler should be a central place to define custom handlers for exceptions thrown in application.

Yet, turns out that exceptions with handle() method have higher priority than custom handlers:

https://github.com/adonisjs/adonis-framework/blob/d6a9d893e5ddc1e89c5df105175a39df80db463d/src/Exception/index.js#L70-L86

To handle such exception it’s required to add a custom hook.

IMO handle() method of exception should have lower priority then custom handlers bound in main handler.

About this issue

  • Original URL
  • State: closed
  • Created 7 years ago
  • Comments: 46 (35 by maintainers)

Commits related to this issue

Most upvoted comments

I believe we should have an ExceptionHandler that will reside on the core of the framework.

This will check if the exception can handle itself as @radmen propose and fallback to the Youch screen if it’s not possible.

Then, the ExceptionHandler created by the developer/cli (let’s say CustomExceptionHandler) will call super.handle(error, cli) to hook over every single exception.

The flow will be basically.

  1. System throw an exception
  2. Is CustomExceptionHandler available?
  3. Call CustomExceptionHandler.handle(), if the exception is not hooked here --> super.handle()
  4. ExceptionHandler.handle(), self-handled?, Fallback to Youch error page

This provides a good way to hook overall exceptions you can have on the system and doesn’t feel like magic.

Okay, So I am sync with you guys now. I will working on it in a way that it should not introduce any breaking changes, since every app will be using custom exception handlers.

@radmen Thanks for staying patient 😄

@swkimA Yes this is what going to happen in the future. For now I recommend using the workaround mentioned here https://github.com/adonisjs/adonis-framework/issues/718#issuecomment-350831415

Which literally means it will not call handle method on the exceptions, even when those exceptions are created by you.

Actually what I wanted to say is that the Exception handler should make use of exceptions handle() method.

Something like:

class ExceptionHandler {
    async handle (error, ctx) {
        if ('handle' in error) {
            error.handle(ctx)
        } else {
            // something different
        }
}

This way all error handling goes through the handler and is redirected to exceptions handle() methods.

Having this, developer will only have to overwrite handle() method of ExceptionHandler if they want different results.

Overall I’d say that this will change the priority of error handling yet, the behaviour (for defaults) will remain the same.

@AyozeVera the whole point of this discussion is that I suggest that exception handler should unify handling of all exceptions (like in Laravel) 😃

I need to handle all the exceptions, to return them as JSON, where is the best place to do it?

If you’re defining own exceptions generic exception handler will be enough. Just create new one (using adonis make:ehandler) and define response format in handle() method.

For Adonis bultin exceptions (specially the ones which are self-handling) you need to define custom handlers.

Here’s the excerpt of start/hooks.js file from one of mine projects. Basically, you can copy&paste it to your project 😃

const { hooks } = require('@adonisjs/ignitor')
const requestMacro = require('../app/Validators/Macro/request')

hooks.after.providersBooted(() => {
  const Exception = use('Exception')

  const genericExceptionsHandler = use('App/Exceptions/Handler/genericExceptionsHandler')

  Exception.handle('PasswordMisMatchException', genericExceptionsHandler)
  Exception.handle('UserNotFoundException', genericExceptionsHandler)
  Exception.handle('ValidationException', genericExceptionsHandler)
})

To be complete, here’re contents of App/Exceptions/Handler/genericExceptionsHandler

module.exports = async (error, { response }) =>
  response.status(error.status)
    .send({
      error: error.name,
      details: error.messages
    })