Always call custom error handler.

[alex-oleshkevich]

Fixes #1802

[euri10]

I think the logic is good here, we definitely want the custom handler to be processed first or there's no point having one,
I'll let soneone else approve it since I know Starlette a little but less but as far as I know there's no issue changing the logic here,
any thoughts @adriangb or @Kludex ?

[Kludex]

This behavior is explicitly mentioned on the documentation:

starlette/docs/exceptions.md

Lines 31 to 32 in f6ea760

	If `debug` is enabled and an error occurs, then instead of using the installed
	500 handler, Starlette will respond with a traceback response.

• For reference, implemented on Rejig ExceptionMiddleware and ServerErrorMiddleware #193.

[alex-oleshkevich]

Well, this is counter-intuitive. Any option to reconsider it?

[Kludex]

I don't really see how it's counter-intuitive, because on a normal flow you usually have the handlers, and then you set the debug parameter. Losing priority would make people set the flag, and also comment the handlers.

As a note, in the whole code source, we are effectively using the debug flag only in this middleware.

Any option to reconsider it?

We can wait for other people to share their thoughts here.

Also, we should invite @alex-oleshkevich to encode. cc @tomchristie

[alex-oleshkevich]

I don't really see how it's counter-intuitive

I mean that Starlette supports error handlers, and you can define custom 500/Exception handler via error_handlers. But personally, I expect that the exception handler to be called always, and it is up to the user to determine what he wants to see when debug=True.

Losing priority would make people set the flag, and also comment the handlers.

In the current implementation, debug=True only affects exception page. This PR does the same but calls 500-handler (when available) instead of built-it and delegates debug argument to the handler.

I agree that this will break any existing user 500-handler as they don't handle debug and instead of exception page they will have their handler always called.

[euri10]

yes I agree with @alex-oleshkevich and find that counter intuitive too

[Kludex]

I've converted the issue into a discussion (#1867).

I think there's something we need to do regarding this, but we need to achieve consensus. Until then, I'll close this PR. 🙏

[alex-oleshkevich]

As we head to 1.0 we may get back to this issue. My selling points are:

1. an exception handler decides how to handle request.app.debug
2. makes possible to integrate 3rd-party debug pages natively without middleware hack

[Kludex]

Reopening for discussion. The previous arguments against are still too strong imo.

[euri10]

Providing a better DX at a minor cost ie a breaking change for users who already have a 500 handlers is a no brainer to me, it should be done

[alex-oleshkevich]

Closing it as it seems, this issue is not going to be addressed.
I monkey patch the ServerErrorMiddleware.debug_response overriding it with custom logic.
Hackish, but works.