> > It does? Can you give me a link to that please? > Pard me, it does not explicitly say to set values programmatically, but that this is the place to go when fields depend on each other, and since clean is a multi-step process which does not include only field validation, but settings values too, it kind of makes sense.
1) Form and field validation <https://docs.djangoproject.com/en/1.11/ref/forms/validation/#cleaning-and-validating-fields-that-depend-on-each-other> The form subclass’s clean() method can perform validation that requires > access to multiple form fields. This is where you might put in checks such > as “if field A is supplied, field B must contain a valid email address”. > *This > method can return a completely different dictionary if it wishes, which > will be used as the cleaned_data*. > 2) Cleaning and validating fields that depend on each other <https://docs.djangoproject.com/en/1.11/ref/forms/validation/#cleaning-and-validating-fields-that-depend-on-each-other> Suppose we add another requirement to our contact form: if the cc_myself > field is True, the subject must contain the word "help". *We are > performing validation on more than one field at a time, so the form’s > clean() method is a good spot to do this.* > 3) Model.clean() <https://docs.djangoproject.com/en/1.11/ref/models/instances/#django.db.models.Model.clean> This method should be used to provide custom model validation, *and to > modify attributes on your model if desired*. For instance, you could use > it to automatically provide a value for a field, or to do validation that > requires access to more than a single field. > Please, don't get me wrong, I'm far away from the idea of deprecating `commit=False`. I just personally try to void it and trying to explain my arguments. The generic error message is something that I supply in some forms of mine > where race conditions can happen due to high concurrency. This is why I > guard form.save, catch database errors and then use form.add_error to add a > generic error message asking for a retry. > Yes, that's definitely the place when something outside the validation process can happen, but you don't need `commit=False` here ;) One alternative approach to validate the instance w/o `commit=False` right now would be to change the form like so: class BookForm(forms.ModelForm): class Meta: model = Book fields = ('name', 'user') widgets = {'user': forms.HiddenInput()} def __init__(self, *args, **kwargs): super(BookForm, self).__init__(*args, **kwargs) self.fields['user'].disabled = True #in the view form = BookForm(data={'name': 'Harry Potter'}, initial={'user': user}) But if we compare this with the form in the first post, we are just Fixing/Patching it, i.e. fighting with it in order to make it work for us. However, there is one more little difference here, which I want to point out. Image we didn't had the unique constrain, and we just wanted to hide the user field from the form, and we do it this way (instead of the one with the `instance` kwarg as in the first post). Doing this and adding the unique_constrain later on would yield to no more core changes (except changing the error msg if too ambiguous). While using the `commit=False` approach would require further code changes and it would be multiplied by the number of views the form is being used, and by the number of forms where the field has been omitted (i.e. commit=False has been used). Its a slight difference, but can lead to a big wins. About the error message, to be honest I don't know, gonna keep thinking about it, one thing that came to mind is to strip the missing fields, i.e. instead of "Book with this User and Name already exists." to become: "Book with this Name already exists." but it really depends on the context. The one thing that I know for sure is that personally I would definitely prefer an ambiguous message, rather than a 500 server error. On Monday, October 9, 2017 at 10:52:50 AM UTC+3, Florian Apolloner wrote: > > Hi, > > On Monday, October 9, 2017 at 8:52:53 AM UTC+2, Todor Velichkov wrote: >> >> Settings values programmatically is a cumulative operation most of the >> time, however when its not and things depend on each other (like your >> example), then even the docs suggests than one can use the form.clean >> method. >> > > It does? Can you give me a link to that please? > > If there is some other dependency outside form.cleaned_data I would prefer >> to use dependency injection in order to get this data and do the job done. >> I'm sorry I just see commit=False as an anti-pattern, because the >> validation needs to be repeated after that (as your example in the first >> post). >> > > Repeated is a bit overreaching, it also checks new fields… > > Showing an error message which the user cannot correct is a horrible UX >> indeed, but still its a UX, and some people may find it as a better >> alternative to a `500 server error page`, where there is no UX at all. Even >> a generic message like 'sorry we messed up' would be useful, because the >> user data that will be preserved into the form. However, in the example >> shown here, this is not even the case, there is something that the user can >> do to prevent the error. >> > > The generic error message is something that I supply in some forms of mine > where race conditions can happen due to high concurrency. This is why I > guard form.save, catch database errors and then use form.add_error to add a > generic error message asking for a retry. In the example shown, the user > can do something about the error, this is correct, but the default error > message would be rather confusing since it would cover a field which is not > part of the form. > > That said, form.save(commit=False) is there and will stay, even if it is > just for backwards compatibility. Same goes for the partial validation of > the instance, this is just something to many people rely on and changing is > not really an option. > > One could add a new flag to the Meta object ala validate_full_model, but > for that to work you will have to tell us a sensible UX approach first. I > am opposed to adding something which we agree is horrible just because the > alternative (like I've shown) requires a few additional lines of code. > > Cheers, > Florian > -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@googlegroups.com. Visit this group at https://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/220eda0a-5f34-4859-b0e1-db65575359df%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
