Opened 14 years ago

Closed 14 years ago

Last modified 12 years ago

#12901 closed (fixed)

Forms _get_validations_exclusions is still not backwards compatible

Reported by: Chris Beaven Owned by: nobody
Component: Forms Version: dev
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

In my user registration form, I have a field named password, but the modelform's Meta has fields = ('first_name', 'last_name', 'email').

Submitting the form causes a "This field cannot be blank." validation error for the password field.

The current logic doesn't actually refer to those fields, but to the fields dictionary (which will also contain any user-generated fields).

Attachments (4)

12901.diff (664 bytes ) - added by Chris Beaven 14 years ago.
12901-against-12454.diff (2.0 KB ) - added by Honza Král 14 years ago.
test case and fix
12901-against-12454-2.diff (3.8 KB ) - added by Honza Král 14 years ago.
12901-against-12557.diff (615 bytes ) - added by ammarr 14 years ago.

Download all attachments as: .zip

Change History (21)

by Chris Beaven, 14 years ago

Attachment: 12901.diff added

comment:1 by Chris Beaven, 14 years ago

Needs tests: set

comment:2 by Chris Beaven, 14 years ago

The original code was introduced in [12206]

comment:3 by Russell Keith-Magee, 14 years ago

Triage Stage: UnreviewedAccepted

comment:4 by Honza Král, 14 years ago

Resolution: worksforme
Status: newclosed
Version: 1.1SVN

Closing this as worksforme - I cannot replicate it and when I look at UserCreationForm in contrib.auth (which isusing the same approach) at

http://code.djangoproject.com/browser/django/trunk/django/contrib/auth/forms.py#L23

It works and is actually tested at

http://code.djangoproject.com/browser/django/trunk/django/contrib/auth/tests/forms.py#L72

Could you please provide a test case or more detailed example of what you are doing?

comment:5 by Chris Beaven, 14 years ago

Resolution: worksforme
Status: closedreopened

Hi Honza,

UserCreationForm doesn't use password, it uses password1 which skirts around this backwards incompatibility.
I'll see if I can get around to a test case, but it's pretty obvious (for those that know the code and previous behaviour) that this is an error if you look at the patch.

by Honza Král, 14 years ago

Attachment: 12901-against-12454.diff added

test case and fix

comment:6 by Honza Král, 14 years ago

Needs tests: unset

I see what you had a problem with, managed to produce a test case and a fix. Sorry for the initial confusion.

comment:7 by Chris Beaven, 14 years ago

Triage Stage: AcceptedReady for checkin

comment:8 by jkocherhans, 14 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

This fixes the original problem, but it also breaks unique validation. Here's the test that breaks http://code.djangoproject.com/browser/django/trunk/tests/modeltests/model_formsets/models.py?rev=12369#L935

I'm don't have a strong opinion here. By replacing a default form field should you be opting out of unique validation on that field as well? Seems like either way would be a surprise.

comment:9 by Chris Beaven, 14 years ago

I'm not really "replacing a default form field" because of the meta option, but I understand what you're saying.

IMO, if the fields meta option is used, I think that it's pretty explicit that those are the *only* fields should be want tied to the model.

by Honza Král, 14 years ago

Attachment: 12901-against-12454-2.diff added

comment:10 by jkocherhans, 14 years ago

Resolution: fixed
Status: reopenedclosed

(In [12496]) Fixed #12901. Exclude overridden form fields from model field validation. Thanks, Honza Král.

comment:11 by ammarr, 14 years ago

From the test case, it works with the following code:

class IncompleteCategoryForm(ModelForm): 
    url = forms.CharField(required=False) 	 
    class Meta: 
        fields = ('name', 'slug') 
        model = Category 

but not this:

class IncompleteCategoryForm(ModelForm): 
    url = forms.CharField(required=False) 	 
    class Meta: 
        # removed field tuple
        model = Category 

Is this intentional? IMO, if you declare a field manually, then it should be excluded from validation in any case (regardless of whether the field tuple is present or not).

in reply to:  11 ; comment:12 by Honza Král, 14 years ago

Replying to ammarr:

From the test case, it works with the following code:

class IncompleteCategoryForm(ModelForm): 
    url = forms.CharField(required=False) 	 
    class Meta: 
        fields = ('name', 'slug') 
        model = Category 

but not this:

class IncompleteCategoryForm(ModelForm): 
    url = forms.CharField(required=False) 	 
    class Meta: 
        # removed field tuple
        model = Category 

Is this intentional? IMO, if you declare a field manually, then it should be excluded from validation in any case (regardless of whether the field tuple is present or not).

No. The key difference here is that if you omit the field in Meta.fields, it doesn't get populated on the model so validation doesn't make sense. If you, however, just override the field definition, the model still get populated with this field and validation should then be run.

comment:13 by ammarr, 14 years ago

I see (and agree with) what you're saying, but the resultant behavior is not backwards compatible. The code snippet without the fields tuple validates in 1.1.1.

in reply to:  12 comment:14 by ammarr, 14 years ago

Replying to Honza_Kral:

No. The key difference here is that if you omit the field in Meta.fields, it doesn't get populated on the model so validation doesn't make sense. If you, however, just override the field definition, the model still get populated with this field and validation should then be run.

In addition to the previous message, the only way to exclude a field from model validation is to provide the entire fields tuple, exclude that one field, and then manually declare it. This behavior isn't symmetrical though (declare a field manually, and then just exclude it using the exclude tuple doesn't work). Perhaps this check should be added:

elif self._meta.exclude and field in self._meta.exclude:
exclude.append(f.name) 

by ammarr, 14 years ago

Attachment: 12901-against-12557.diff added

comment:15 by ammarr, 14 years ago

Resolution: fixed
Status: closedreopened

comment:16 by jkocherhans, 14 years ago

Resolution: fixed
Status: reopenedclosed

(In [12590]) Fixed #12901. Again. Model validation will not be performed on excluded fields that were overridden in the form. Thanks, ammarr.

comment:17 by Jacob, 12 years ago

milestone: 1.2

Milestone 1.2 deleted

Note: See TracTickets for help on using tickets.
Back to Top