Code

Opened 4 years ago

Closed 4 years ago

#12596 closed (fixed)

Calling ModelForm.clean() is no longer optional

Reported by: carljm Owned by: carljm
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: UI/UX:

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 carljm 4 years ago.
12596.diff (5.9 KB) - added by jkocherhans 4 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_Kral 4 years ago.
Simpler variant of the patch that only goes half way

Download all attachments as: .zip

Change History (8)

comment:1 Changed 4 years ago by carljm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Status changed from new to assigned

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

Changed 4 years ago by carljm

comment:2 Changed 4 years ago by carljm

  • 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 Changed 4 years ago by carljm

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.

Changed 4 years ago by jkocherhans

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.

Changed 4 years ago by Honza_Kral

Simpler variant of the patch that only goes half way

comment:4 Changed 4 years ago by ramusus

  • Cc ramusus@… added

comment:5 Changed 4 years ago by jkocherhans

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

(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.

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.