Opened 12 years ago

Closed 12 years ago

#17888 closed Cleanup/optimization (fixed)

CheckboxInput.render() shouldn't catch exceptions from check_test

Reported by: Bruno Renié Owned by: nobody
Component: Forms Version: dev
Severity: Normal Keywords:
Cc: marc.tamlyn@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

https://github.com/django/django/blob/master/django/forms/widgets.py#L502-505

As the docs state, check_test is a callable that returns True if the checkbox should be checked for a specific value. The default implementation doesn't do anything complicated and shouldn't raise exceptions. If user-provided callables break, it django shouldn't catch the error silently.

My proposal is to simply remove that bare except and let check_test raise exceptions.

The current behaviour is actually expected and tested:

https://github.com/django/django/blob/master/tests/regressiontests/forms/tests/widgets.py#L218-224

I still think it's quite a bad API and should be changed. Since that exception-swallowing behaviour isn't documented (although tested), backwards compatibility isn't that much of a concern.

Attachments (1)

17888.patch (652 bytes ) - added by Bruno Renié 12 years ago.
Patch that breaks the current regression tests

Download all attachments as: .zip

Change History (5)

by Bruno Renié, 12 years ago

Attachment: 17888.patch added

Patch that breaks the current regression tests

comment:1 by Jannis Leidel, 12 years ago

Needs documentation: set
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

Yes, and yes!

comment:2 by Bruno Renié, 12 years ago

Pull request with tests and docs is at https://github.com/django/django/pull/125

comment:3 by Marc Tamlyn, 12 years ago

Cc: marc.tamlyn@… added
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

There is an updated pull request at https://github.com/django/django/pull/130/. I think this is all good to go so I'm going to mark it as RFC.

comment:4 by Alex Gaynor <alex.gaynor@…>, 12 years ago

Resolution: fixed
Status: newclosed

In [6a5a12ea3e3193b3658b9237b86d98bc37d668d7]:

Fixed #17888 -- no longer silence exceptions inside of check_test. Thanks to brutasse for the patch.

Note: See TracTickets for help on using tickets.
Back to Top