Code

Opened 2 years ago

Closed 22 months ago

#17888 closed Cleanup/optimization (fixed)

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

Reported by: brutasse Owned by: nobody
Component: Forms Version: master
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 brutasse 2 years ago.
Patch that breaks the current regression tests

Download all attachments as: .zip

Change History (5)

Changed 2 years ago by brutasse

Patch that breaks the current regression tests

comment:1 Changed 2 years ago by jezdez

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

Yes, and yes!

comment:2 Changed 2 years ago by brutasse

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

comment:3 Changed 22 months ago by mjtamlyn

  • Cc marc.tamlyn@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready 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 Changed 22 months ago by Alex Gaynor <alex.gaynor@…>

  • Resolution set to fixed
  • Status changed from new to closed

In [6a5a12ea3e3193b3658b9237b86d98bc37d668d7]:

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.