Code

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#12901 closed (fixed)

Forms _get_validations_exclusions is still not backwards compatible

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

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 SmileyChris 4 years ago.
12901-against-12454.diff (2.0 KB) - added by Honza_Kral 4 years ago.
test case and fix
12901-against-12454-2.diff (3.8 KB) - added by Honza_Kral 4 years ago.
12901-against-12557.diff (615 bytes) - added by ammarr 4 years ago.

Download all attachments as: .zip

Change History (21)

Changed 4 years ago by SmileyChris

comment:1 Changed 4 years ago by SmileyChris

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset

comment:2 Changed 4 years ago by SmileyChris

The original code was introduced in [12206]

comment:3 Changed 4 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 4 years ago by Honza_Kral

  • Resolution set to worksforme
  • Status changed from new to closed
  • Version changed from 1.1 to SVN

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 Changed 4 years ago by SmileyChris

  • Resolution worksforme deleted
  • Status changed from closed to reopened

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.

Changed 4 years ago by Honza_Kral

test case and fix

comment:6 Changed 4 years ago by Honza_Kral

  • 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 Changed 4 years ago by SmileyChris

  • Triage Stage changed from Accepted to Ready for checkin

comment:8 Changed 4 years ago by jkocherhans

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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 Changed 4 years ago by SmileyChris

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.

Changed 4 years ago by Honza_Kral

comment:10 Changed 4 years ago by jkocherhans

  • Resolution set to fixed
  • Status changed from reopened to closed

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

comment:11 follow-up: Changed 4 years ago by 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).

comment:12 in reply to: ↑ 11 ; follow-up: Changed 4 years ago by Honza_Kral

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 Changed 4 years ago by ammarr

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.

comment:14 in reply to: ↑ 12 Changed 4 years ago by ammarr

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) 

Changed 4 years ago by ammarr

comment:15 Changed 4 years ago by ammarr

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:16 Changed 4 years ago by jkocherhans

  • Resolution set to fixed
  • Status changed from reopened to closed

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

comment:17 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.