Opened 14 years ago

Closed 14 years ago

#12596 closed (fixed)

Calling ModelForm.clean() is no longer optional

Reported by: Carl Meyer Owned by: Carl Meyer
Component: Forms Version: 1.1
Severity: Keywords:
Cc: ramusus@… Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

It's documented http://docs.djangoproject.com/en/1.1/topics/forms/modelforms/#overriding-the-clean-method that if you override clean() on a ModelForm and fail to call ModelForm's clean() method, all you lose is validate_unique. With the recent model validation changes, this is no longer true; ModelForm.clean does quite a bit more (including constructing self.instance), and failing to call it will almost certainly break things.

Per discussion with jkocherhans in IRC, it should be possible to rearrange things to maintain the previously documented behavior.

Attachments (3)

12596_r12222.diff (4.9 KB ) - added by Carl Meyer 14 years ago.
12596.diff (5.9 KB ) - added by jkocherhans 14 years ago.
This takes carljm's test, but makes ModelForm?.clean() call validate_unique() like it used to. Haven't decided if this is the right approach.
12596-simple.diff (3.4 KB ) - added by Honza Král 14 years ago.
Simpler variant of the patch that only goes half way

Download all attachments as: .zip

Change History (8)

comment:1 by Carl Meyer, 14 years ago

Status: newassigned

Whoops, forgot to use preview. Accepting, as this was already discussed with core committers in IRC.

by Carl Meyer, 14 years ago

Attachment: 12596_r12222.diff added

comment:2 by Carl Meyer, 14 years ago

Has patch: set

Comments on the patch:

  • I removed the documentation about validate_unique happening in ModelForm.clean, as that is no longer the case.
  • Is it necessary to document the existence of ModelForm.clean_instance?
  • There's some extra rigmarole necessary at the beginning and end of ModelForm.clean_instance to re-add an empty self.cleaned_data if Form.full_clean removed it, and re-remove it if there are errors when done. These extra lines could be removed if we just move the last two lines of Form.full_clean (the removal of cleaned_data if there are errors) into Form._get_errors; but jkocherhans notes that might surprise people who overrode full_clean in order to modify or get rid of the cleaned_data removal.

comment:3 by Carl Meyer, 14 years ago

Oh, and I made Form.full_clean return a boolean signifying whether this form instance requires validation, in order to avoid duplicating the checks for is_bound, empty_permitted etc that cause it to bail out early.

by jkocherhans, 14 years ago

Attachment: 12596.diff added

This takes carljm's test, but makes ModelForm?.clean() call validate_unique() like it used to. Haven't decided if this is the right approach.

by Honza Král, 14 years ago

Attachment: 12596-simple.diff added

Simpler variant of the patch that only goes half way

comment:4 by ramusus, 14 years ago

Cc: ramusus@… added

comment:5 by jkocherhans, 14 years ago

Resolution: fixed
Status: assignedclosed

(In [12269]) Fixed #12596. Calling super from a ModelForm's clean method is once again optional. Failing to call super only skips unique validation as documented. Thanks for the initial patch and tests, carljm.

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