Opened 15 years ago
Closed 15 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)
Change History (8)
comment:1 by , 15 years ago
Status: | new → assigned |
---|
by , 15 years ago
Attachment: | 12596_r12222.diff added |
---|
comment:2 by , 15 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 , 15 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 , 15 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 , 15 years ago
Attachment: | 12596-simple.diff added |
---|
Simpler variant of the patch that only goes half way
comment:4 by , 15 years ago
Cc: | added |
---|
comment:5 by , 15 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Whoops, forgot to use preview. Accepting, as this was already discussed with core committers in IRC.