ModelForm.is_valid() and ModelForm.errors have backwards-incompatible side effects since model validation
|Reported by:||lukeplant||Owned by:||jkocherhans|
|Cc:||Triage Stage:||Design decision needed|
|Has patch:||no||Needs documentation:||no|
|Needs tests:||no||Patch needs improvement:||no|
Since r12098 (model validation merge), ModelForm.is_valid() and ModelForm.errors have the side effect of modifying the bound instance.
Attached is a (rather lazily written) patch to the tests which demonstrates the problem - it ought to pass, and does pass before r12098 (noting that the patch doesn't apply cleanly before r12098), but not after.
This is a significant backwards incompatibility, but rather subtle too - in my case it was caught only due to fairly extensive tests in one of my projects. It is also quite counter-intuitive that either form.errors or form.is_valid should have any side effects. At the very least this needs to be clearly documented. I'm guessing it might be tricky to fix, given earlier problems surrounding the order of events in model validation.
For your information, my use case is detecting that a change has occurred to a certain field:
if form.is_valid(): orig_val = obj.val obj = form.save() if obj.val != orig_val: do_something()
The side effects of form.is_valid() mean that do_something() is no longer called.
Change History (8)
Changed 6 years ago by lukeplant
comment:1 Changed 6 years ago by Alex
- Needs documentation unset
- Needs tests unset
- Patch needs improvement unset
comment:2 Changed 6 years ago by russellm
- Component changed from Uncategorized to Forms
- milestone set to 1.2
- Triage Stage changed from Unreviewed to Design decision needed
comment:5 Changed 6 years ago by jkocherhans
- Owner changed from nobody to jkocherhans
- Status changed from new to assigned
comment:6 Changed 6 years ago by jkocherhans
- Resolution set to fixed
- Status changed from assigned to closed