Opened 7 years ago

Closed 7 years ago

Last modified 5 years ago

#12285 closed (fixed)

Confusing error message when ModelForm does not have model set

Reported by: Erik Allik Owned by: Karen Tracey
Component: Forms Version: 1.1
Severity: Keywords:
Cc: brian@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

When instantiating a ModelForm whose Meta.model has not been set (i.e. the ModelForm class was extended instead of Form), the exception message is:

'NoneType' object is not callable

This message originates from django/forms/models.py in __init__ line 218 that says self.instance = opts.model().

Adding an if there with a decent message such as "Model class not set for ModelForm" would be the fix.

Attachments (2)

12285.diff (1.1 KB) - added by Travis Cline <travis.cline@…> 7 years ago.
12285-unit-test-ValueError.diff (1.3 KB) - added by Tobias McNulty 7 years ago.
updated patch uses 'is None' and applies against trunk

Download all attachments as: .zip

Change History (13)

Changed 7 years ago by Travis Cline <travis.cline@…>

Attachment: 12285.diff added

comment:1 Changed 7 years ago by Travis Cline <travis.cline@…>

Has patch: set

comment:2 Changed 7 years ago by Tobias McNulty

Owner: changed from nobody to Tobias McNulty
Status: newassigned

reviewing patch

comment:3 Changed 7 years ago by Tobias McNulty

Owner: changed from Tobias McNulty to nobody
Status: assignednew
Triage Stage: UnreviewedAccepted

Works as intended. My personal preference is for unit tests, but that might just be me. Also the exception could be made more specific (AttributeError? ValueError?).

Otherwise, this seems like a simple, intuitive fix to an obscure error.

comment:4 Changed 7 years ago by Tobias McNulty

Owner: changed from nobody to Karen Tracey

I have a slight preference for the unittest/ValueError way but I'm not sure which patch makes the most sense in the bigger scheme of things

comment:5 Changed 7 years ago by unbracketed

Cc: brian@… added

comment:6 Changed 7 years ago by Peter Bengtsson

I think 12285.diff is good. Applied it and it works fine.
The module already raises Exception errors on a bunch of other things and the tests are already a doctest.

Raising Exception is IMHO a bad practice since it's not very explicit. A custom defined error would be much better.

Changed 7 years ago by Tobias McNulty

updated patch uses 'is None' and applies against trunk

comment:7 Changed 7 years ago by Tobias McNulty

Triage Stage: AcceptedReady for checkin

per jkocherhans

comment:8 Changed 7 years ago by Tobias McNulty

also fixes #12051

comment:9 Changed 7 years ago by jkocherhans

Resolution: fixed
Status: newclosed

(In [12526]) Fixed #12285. ModelForm raises a more informative error if it doesn't have a model class defined. Thanks, tobias.

comment:10 Changed 7 years ago by jkocherhans

(In [12530]) [1.1.X] Fixed #12285. ModelForm raises a more informative error if it doesn't have a model class defined. Backport of [12526] from trunk.

comment:11 Changed 5 years ago by Jacob

milestone: 1.2

Milestone 1.2 deleted

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