Code

Opened 5 years ago

Closed 4 years ago

Last modified 3 years ago

#12285 closed (fixed)

Confusing error message when ModelForm does not have model set

Reported by: RaceCondition Owned by: kmtracey
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@…> 5 years ago.
12285-unit-test-ValueError.diff (1.3 KB) - added by tobias 4 years ago.
updated patch uses 'is None' and applies against trunk

Download all attachments as: .zip

Change History (13)

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

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

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 5 years ago by tobias

  • Owner changed from nobody to tobias
  • Status changed from new to assigned

reviewing patch

comment:3 Changed 5 years ago by tobias

  • Owner changed from tobias to nobody
  • Status changed from assigned to new
  • Triage Stage changed from Unreviewed to Accepted

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 5 years ago by tobias

  • Owner changed from nobody to kmtracey

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

  • Cc brian@… added

comment:6 Changed 4 years ago by peterbe

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 4 years ago by tobias

updated patch uses 'is None' and applies against trunk

comment:7 Changed 4 years ago by tobias

  • Triage Stage changed from Accepted to Ready for checkin

per jkocherhans

comment:8 Changed 4 years ago by tobias

also fixes #12051

comment:9 Changed 4 years ago by jkocherhans

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

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

comment:10 Changed 4 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 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

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.