On Fri, Jan 8, 2010 at 11:59 AM, koenb <[email protected]> wrote:
>
>
> On 8 jan, 10:03, James Bennett <[email protected]> wrote:
>
>> Suppose I have a ModelForm and call save(commit=False) to get the
>> instance so I can do some more work on it. I'm basically saying to
>> Django "I don't think this object is ready to be saved yet, but I need
>> the object so I can do stuff to it". If Django goes ahead and does
>> implicit model validation there and raises an exception, it's just
>> telling me what I already knew: the object's not ready to be saved.
>> But now I get to do extra work to catch the exception, and that's bad
>> and wrong.
>>
>> Calling ModelForm.save(commit=False) should simply disable model
>> validation, period. If I want to validate the instance before saving
>> it, I'll validate the instance before saving it, but commit=False is
>> as clear a way as we have of saying "I'm not going to save this yet"
>> and model validation should respect that.
I just hate the name save(commit=False) when it has nothing to do with
saving or committing, it just DOESN'T call save, it's imho misleading.
I understand why that is and how it came to be, I just don't like it.
I wasn't, however, proposing we get rid of this feature, just that we
extract it to a separate method or just tell people to use
form.instance (which is all what it does + creating save_m2m which can
be put elsewhere).
>
> The problem is that in the idiom the is_valid() call on the modelform
> tries to full_validate the instance, it is not in the save
> (commit=False) call.
Yes
> I'm with Simon here: on an incomplete modelform, let's just ignore the
> errors on excluded fields and only validate the user input at that
> point. If the developer wants to be sure the model validates after he
> adds the necessary extra information, it is his job to call validation
> before saving.
The only problem I see here is that you cannot run model.validate (so
check for unique etc.) because the user might define some custom
validation there that you have no control over and that can check
fields you don't want it to touch. We could move validate_unique
elsewhere, but that could create problem for people not wanting to
have their fields validated for uniqueness (expensive hits to the DB)
But overall I feel that "we only call model.full_validate when no
field is excluded or we are editing an existing instance" is the
desired behavior by most people.
In code that would mean that self.validate in full_clean would only be
called if not exclude and excluded form fields would be passed to
model.full_clean when adding an instance (not when editing), that's a
very simple change. We just need to document this behavior thoroughly
because it can cause confusion.
The question remains how to validate inlines in Admin. I think there
is no question that we want to call full_validate on the inlined model
eventually, the question is how to do it in a most backwards
compatible and sane way.
We need the FK to do the validation and we cannot get the FK without
the save() of the parent model. So imho it's just a question of how
much risk we are taking and at which point do we call model.save() on
the parent (after the form validates, after the inline form fields
validate).
If we take the new proposal for ModelForm behavior it could work:
MainForm.clean()
for f in inline: f.validate()
if all.valid:
MainForm.save()
for inline in inlines:
for form in inline;
f.instance.full_validate()
inline.save()
the problem is when f.instance.full_validate() fails on the inline.
That's a point where the FK must exist so the parent object has to be
saved already. There is no way around this if we want to have
model-validation in admin uncrippled.
--
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=en.
