Add check for signatures of custom error views

[Adam Johnson]

In ​https://github.com/django/django/pull/10249 I fixed the documentation for the change in #24733 to add the 'exception' argument to 3 out of the 4 custom error handlers. This came from a real life problem where I'd added handlers but without the 'exception' argument, then when the handler was triggered, an error was raised due to it not accepting the right types. The feedback loop for this failure is pretty long as these only get hit when DEBUG=False and not during tests.

We should check the signatures of these handler functions with a django check (i.e. that they accept 1 or 2 arguments, depending on which one they are) in order to prevent this happening to others.

[Carlton Gibson]

Yes. That seems more than reasonable.

[Tim Graham]

I'm not immediately convinced. These "simple checks" often turn out to be more complicated than they seem. Didn't you write some tests for your custom views? ;-)

[Carlton Gibson]

It’s also about what our ‘’Average User™’’ might do. Yes, Adam should write tests, but I can easily see people putting a handler in place and never really trying it out.

The feedback loop for this failure is pretty long as these only get hit when DEBUG=False and not during tests.

This is the bit that convinced me. You write an error handler, and then realise that it’s not straitforwardly testable, with the test client etc.

[Tim Graham]

Maybe a documentation suggestion to facilitate testing would be helpful:

if settings.DEBUG:
    urlpatterns += [
        path('500/', 'path.to.custom_500_handler'),
    )

I think it's more empowering than "check the signature and assume the rest of the view works." ;-)

[Carlton Gibson]

Yep. I can see that.

[Adam Johnson]

Maybe a documentation suggestion to facilitate testing would be helpful

I did this for my initial unit tests, it only half helps since then exception isn't passed. So a view which only takes one argument would work with such a test view.

In fact the better way to test handlers is like django's test suite for them, with other views that raise exceptions: ​https://github.com/django/django/blob/master/tests/handlers/tests_custom_error_handlers.py#L23

These "simple checks" often turn out to be more complicated than they seem. Didn't you write some tests for your custom views? ;-)

That's the worst thing here, I did, and they were wrong. So there was a double loop.

The check is actually quite simple and I have a draft implementation. Also there was no check before that the given paths are genuine so this will add defence to two different potential failures, for each of the 4 handlers.

[Tim Graham]

​PR

[Tim Graham <timograham@…>]

In 245c36d7:

Fixed #29642 -- Added check for arguments of custom error handler views.