Opened 14 years ago

Closed 14 years ago

Last modified 12 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@…> 14 years ago.
12285-unit-test-ValueError.diff (1.3 KB ) - added by Tobias McNulty 14 years ago.
updated patch uses 'is None' and applies against trunk

Download all attachments as: .zip

Change History (13)

by Travis Cline <travis.cline@…>, 14 years ago

Attachment: 12285.diff added

comment:1 by Travis Cline <travis.cline@…>, 14 years ago

Has patch: set

comment:2 by Tobias McNulty, 14 years ago

Owner: changed from nobody to Tobias McNulty
Status: newassigned

reviewing patch

comment:3 by Tobias McNulty, 14 years ago

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 by Tobias McNulty, 14 years ago

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 by unbracketed, 14 years ago

Cc: brian@… added

comment:6 by Peter Bengtsson, 14 years ago

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.

by Tobias McNulty, 14 years ago

updated patch uses 'is None' and applies against trunk

comment:7 by Tobias McNulty, 14 years ago

Triage Stage: AcceptedReady for checkin

per jkocherhans

comment:8 by Tobias McNulty, 14 years ago

also fixes #12051

comment:9 by jkocherhans, 14 years ago

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 by jkocherhans, 14 years ago

(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 by Jacob, 12 years ago

milestone: 1.2

Milestone 1.2 deleted

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