ModelForm.is_valid() and ModelForm.errors have backwards-incompatible side effects since model validation

[Luke Plant]

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.

[Luke Plant]

Patch to tests to demonstrate problem.

[Alex Gaynor]

I'm pretty sure this is intended behavior now (it broke one of my projects as well, but it was easily resolved).

[Russell Keith-Magee]

We need to decide on this one way or the other for 1.2 - if this has changed, we need to make sure the change is intentional.

[jkocherhans]

I'm going to think about this more, but I don't see a way around it without extensively altering things or copying the instance. Form.errors and Form.is_valid() have had side effects for as long as I can remember, they trigger validation. The side effects are more extensive now, but side effects of those calls in general aren't anything new. I think the solution is just to document it in the backwards compatibility notes, but I'll try to track Honza down at PyCon to chat about it.

[Jacob]

I'll leave the final call to Joseph, but I think he's right and this should just be noted in the docs.

[jkocherhans]

Model.full_clean() modifies the model in-place. I hope no one will argue that's the wrong thing to do. So, to fix this I see a couple of solutions, both of which I think are unacceptable.

1. Copy the instance. Copying could be an expensive operation, and if the developer really wants to do that, they can just explicitly do it themselves and pass the copy to the ModeForm constructor.

2. Change the internals of Model.full_clean() to optionally not modify the model in place, and change ModelForm to use that option. I think this option is just needlessly confusing.

I'll add a warning to the release notes and to the ModelForm docs.

[jkocherhans]

(In [12693]) Fixed #12896. Documented the new side-effects of ModelForm validation.

[Jacob]

Milestone 1.2 deleted