[Django] #24756: Unexpected behavior of ModelForm.save for fields with blank=False and overridden required attribute
Django
overridden required attribute
----------------------------+----------------------------------------------
Reporter: bboogaard | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.8
Severity: Release | Keywords: forms models save blank required
blocker |
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+----------------------------------------------
Since upgrading to Django 1.8.1, I'm getting unexpected behavior when
setting the required attribute for ModelForm fields to False for fields
with blank=False in their field declaration on the model. Contrary to
previous Django versions, the field is now ignored when calling the save()
method of the ModelForm.
The model:
{{{
from django.db import models
class CodeName(models.Model):
code = models.CharField('Code', max_length=20)
name = models.CharField('Name', max_length=100)
}}}
The form:
{{{
from django import forms
from .models import CodeName
class CodeNameForm(forms.ModelForm):
class Meta:
model = CodeName
fields = ['code', 'name']
def __init__(self, *args, **kwargs):
super(CodeNameForm, self).__init__(*args, **kwargs)
self.fields['code'].required = False
}}}
Failing test:
{{{
from django.test import TestCase
from .forms import CodeNameForm
from .models import CodeName
class TestCodeNameForm(TestCase):
def test_save(self):
instance = CodeName(code='foo', name='Foobar')
data = {
'code': '',
'name': 'Foobar'
}
form = CodeNameForm(data, instance=instance)
form.full_clean()
instance = form.save()
self.assertEqual(instance.code, '')
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24756>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
overridden required attribute
Reporter: bboogaard | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.8
Keywords: forms models save blank required | Triage Stage: Unreviewed
Has patch: 0 | Easy pickings: 0
UI/UX: 0 |
Changes (by bboogaard):
* Attachment "models.patch" added.
Proposed patch (django/forms/models.py)
Django
overridden required attribute
Reporter: bboogaard | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.8
Keywords: forms models save | Triage Stage:
blank required | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
I am not sure if we should support this use case because with `code=''`,
the submitted model is invalid from a model validation standpoint. Could
you elaborate on the use case to omit `blank=True` from the model field
definition but accept blank values on the form?
The relevant code in
[https://github.com/django/django/blob/4dcc6493418c78db07761180bf6265f5b2bbccbf/django/forms/models.py#L373-L383
forms/models.py].
Your proposed patch doesn't pass the test suite:
{{{
======================================================================
ERROR: test_blank_with_null_foreign_key_field
(model_forms.tests.ModelFormBaseTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/tim/code/django/tests/model_forms/tests.py", line 231, in
test_blank_with_null_foreign_key_field
self.assertTrue(f1.is_valid())
File "/home/tim/code/django/django/forms/forms.py", line 168, in
is_valid
return self.is_bound and not self.errors
File "/home/tim/code/django/django/forms/forms.py", line 160, in errors
self.full_clean()
File "/home/tim/code/django/django/forms/forms.py", line 378, in
full_clean
self._post_clean()
File "/home/tim/code/django/django/forms/models.py", line 433, in
_post_clean
self.instance = construct_instance(self, self.instance, opts.fields,
construct_instance_exclude)
File "/home/tim/code/django/django/forms/models.py", line 60, in
construct_instance
f.save_form_data(instance, cleaned_data[f.name])
File "/home/tim/code/django/django/db/models/fields/__init__.py", line
856, in save_form_data
setattr(instance, self.name, data)
File "/home/tim/code/django/django/db/models/fields/related.py", line
634, in __set__
(instance._meta.object_name, self.field.name)
ValueError: Cannot assign None: "Student.character" does not allow null
values.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24756#comment:1>
Django
overridden required attribute
Reporter: bboogaard | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.8
Keywords: forms models save | Triage Stage:
blank required | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by bboogaard):
>> I am not sure if we should support this use case because with code='',
the submitted model is invalid from a model validation standpoint.
Do you mean that the pre-1.8 behavior was unintended?
--
Ticket URL: <https://code.djangoproject.com/ticket/24756#comment:2>
Django
overridden required attribute
Reporter: bboogaard | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.8
Keywords: forms models save | Triage Stage:
blank required | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
In my mind, yes. There weren't any tests in Django's test suite to claim
it was officially supported, at least.
--
Ticket URL: <https://code.djangoproject.com/ticket/24756#comment:3>
Django
overridden required attribute
Reporter: bboogaard | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.8
Keywords: forms models save | Triage Stage:
blank required | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by bboogaard):
All right, if it was an unintended effect of previous versions, I'll
simply change my model field declarations and code logic. I thought the
override was supposed to work the way I described (i.e. in principle
required, but for some form classes not required).
--
Ticket URL: <https://code.djangoproject.com/ticket/24756#comment:4>
Django
overridden required attribute
Reporter: bboogaard | Owner: nobody
Type: Bug | Status: closed
Component: Forms | Version: 1.8
Severity: Release blocker | Resolution: invalid
Keywords: forms models save | Triage Stage:
blank required | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: new => closed
* resolution: => invalid
--
Ticket URL: <https://code.djangoproject.com/ticket/24756#comment:5>