[Django] #12596: Calling ModelForm.clean() is no longer optional

0 views
Skip to first unread message

Django

unread,
Jan 12, 2010, 5:19:27 PM1/12/10
to djang...@holovaty.com, django-...@googlegroups.com
#12596: Calling ModelForm.clean() is no longer optional
--------------------+-------------------------------------------------------
Reporter: carljm | Owner: carljm
Status: new | Milestone:
Component: Forms | Version: 1.1
Keywords: | Stage: Unreviewed
Has_patch: 0 |
--------------------+-------------------------------------------------------
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.

--
Ticket URL: <http://code.djangoproject.com/ticket/12596>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,
Jan 12, 2010, 5:21:00 PM1/12/10
to djang...@holovaty.com, django-...@googlegroups.com
#12596: Calling ModelForm.clean() is no longer optional
---------------------------------+------------------------------------------
Reporter: carljm | Owner: carljm
Status: assigned | Milestone:
Component: Forms | Version: 1.1
Resolution: | Keywords:
Stage: Unreviewed | Has_patch: 0
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
---------------------------------+------------------------------------------
Changes (by carljm):

* status: new => assigned
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0

Comment:

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

--
Ticket URL: <http://code.djangoproject.com/ticket/12596#comment:1>

Django

unread,
Jan 12, 2010, 7:26:06 PM1/12/10
to djang...@holovaty.com, django-...@googlegroups.com
#12596: Calling ModelForm.clean() is no longer optional
---------------------------------+------------------------------------------
Reporter: carljm | Owner: carljm
Status: assigned | Milestone:
Component: Forms | Version: 1.1
Resolution: | Keywords:
Stage: Unreviewed | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
---------------------------------+------------------------------------------
Changes (by carljm):

* has_patch: 0 => 1

Comment:

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.

--
Ticket URL: <http://code.djangoproject.com/ticket/12596#comment:2>

Django

unread,
Jan 12, 2010, 7:28:11 PM1/12/10
to djang...@holovaty.com, django-...@googlegroups.com
#12596: Calling ModelForm.clean() is no longer optional
---------------------------------+------------------------------------------
Reporter: carljm | Owner: carljm
Status: assigned | Milestone:
Component: Forms | Version: 1.1
Resolution: | Keywords:
Stage: Unreviewed | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
---------------------------------+------------------------------------------
Comment (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.

--
Ticket URL: <http://code.djangoproject.com/ticket/12596#comment:3>

Django

unread,
Jan 14, 2010, 1:00:29 PM1/14/10
to djang...@holovaty.com, django-...@googlegroups.com
#12596: Calling ModelForm.clean() is no longer optional
---------------------------------+------------------------------------------
Reporter: carljm | Owner: carljm
Status: assigned | Milestone:
Component: Forms | Version: 1.1
Resolution: | Keywords:
Stage: Unreviewed | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
---------------------------------+------------------------------------------
Changes (by ramusus):

* cc: ram...@gmail.com (added)

--
Ticket URL: <http://code.djangoproject.com/ticket/12596#comment:4>
Reply all
Reply to author
Forward
0 new messages