[Django] #27039: ModelFields with 'default' value set and 'required'=False in form does not use default value
Django
use default value
----------------------------+--------------------------------------------
Reporter: devbis | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.10
Severity: Normal | Keywords: default non-required modelform
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
----------------------------+--------------------------------------------
I have Model M with corresponding ModelForm
{{{
class M(models.Model):
f = models.CharField(max_length=255, default=u'default_value')
class MF(forms.ModelForm):
class Meta:
model = M
fields = ['f']
f = forms.CharField(required=False)
}}}
I save form with empty data, expecting receive default value in field
{{{
mf = MF({})
if mf.is_valid():
m = mf.save(commit=False)
m.f
>>> u''
}}}
expected behavior
{{{
m.f
>>> u'default_value'
}}}
Commits, that broke previous behavior:
https://github.com/django/django/commit/375e1cfe2b2e1c3c57f882147c34902c6e8189ac
Offered patch:
{{{
--- django/forms/models.py (revision
35225e2ade08ea32e36a994cd4ff90842c599e20)
+++ django/forms/models.py (revision )
@@ -385,7 +385,7 @@
exclude.append(name)
try:
- self.instance = construct_instance(self, self.instance,
opts.fields, opts.exclude)
+ self.instance = construct_instance(self, self.instance,
opts.fields, exclude)
except ValidationError as e:
self._update_errors(e)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27039>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
use default value
Reporter: devbis | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.10
Keywords: default non- | Triage Stage: Accepted
required modelform |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* needs_better_patch: => 0
* needs_docs: => 0
* severity: Normal => Release blocker
* needs_tests: => 1
* stage: Unreviewed => Accepted
Comment:
Thanks, but a test is still missing in the patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/27039#comment:1>
Django
use default value
Reporter: devbis | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.10
Keywords: default non- | Triage Stage: Accepted
required modelform |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Comment (by timgraham):
I'd like to hear more about the use case. It might be argued that defaults
should only apply to the population of initial forms and that if a user
removes that value and submits a blank form, Django shouldn't transform it
back to the default. Of course if we decide on that resolution, we'll have
to document the change. If we decide to keep the old behavior, I'm
attaching a regression test.
--
Ticket URL: <https://code.djangoproject.com/ticket/27039#comment:2>
Django
use default value
Reporter: devbis | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.10
Keywords: default non- | Triage Stage: Accepted
required modelform |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Changes (by timgraham):
* Attachment "27039-test.diff" added.
Django
use default value
Reporter: devbis | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.10
Keywords: default non- | Triage Stage: Accepted
required modelform |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Comment (by claudep):
If a user removes the value in a form, the form should receive `{'f':
''}`, not an empty dict. I still think we should fix the regression.
--
Ticket URL: <https://code.djangoproject.com/ticket/27039#comment:3>
Django
use default value
Reporter: devbis | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.10
Keywords: default non- | Triage Stage: Accepted
required modelform |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Comment (by timgraham):
Makes sense to me, however, on older Django versions, `{'f': ''}` as the
data also results in an instance with the model field default rather than
an empty string. Do you feel the behavior should be restored for that case
as well?
--
Ticket URL: <https://code.djangoproject.com/ticket/27039#comment:4>
Django
use default value
Reporter: devbis | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.10
Keywords: default non- | Triage Stage: Accepted
required modelform |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Comment (by claudep):
No, IMHO `{'f': ''}` should really end up with the field being assigned
the empty string, if possible. Quoting (and agreeing with) you: `if a user
removes that value and submits a blank form, Django shouldn't transform it
back to the default`.
--
Ticket URL: <https://code.djangoproject.com/ticket/27039#comment:5>
Django
use default value
Reporter: devbis | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.10
Keywords: default non- | Triage Stage: Accepted
required modelform |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* needs_tests: 1 => 0
Comment:
I drafted a [https://github.com/django/django/pull/7068 pull request],
however, it demonstrates a small problem with the idea that values not
present in POST data should fallback to their default. For HTML widgets
like checkboxes, if the element isn't checked, it won't appear in POST
data. In such a case, it's inappropriate to fallback to the model field's
default if it's True. The current patch special cases this with
`isinstance(form[f.name].field.widget, CheckboxInput)` to fix some test
failures but that hardly seems ideal.
--
Ticket URL: <https://code.djangoproject.com/ticket/27039#comment:6>
Django
use default value
Reporter: devbis | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.10
Keywords: default non- | Triage Stage: Accepted
required modelform |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
I see. So maybe another approach could be documenting that default values
are used to populate initial blank forms, but not to fill missing data
from the form input.
--
Ticket URL: <https://code.djangoproject.com/ticket/27039#comment:7>
Django
use default value
Reporter: devbis | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.10
Keywords: default non- | Triage Stage: Accepted
required modelform |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
I asked on [https://groups.google.com/d/topic/django-
developers/lpfM6QPILoQ/discussion django-developers] to try to get a
consensus on how to proceed.
--
Ticket URL: <https://code.djangoproject.com/ticket/27039#comment:8>
Django
use default value
Reporter: devbis | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.10
Keywords: default non- | Triage Stage: Accepted
required modelform |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
Comment:
The patch might be ready now.
--
Ticket URL: <https://code.djangoproject.com/ticket/27039#comment:9>
Django
use default value
Reporter: devbis | Owner: nobody
Type: Bug | Status: closed
Component: Forms | Version: 1.10
Severity: Release blocker | Resolution: fixed
Keywords: default non- | Triage Stage: Accepted
required modelform |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"4bc6b939944183533ae74791d21282e613f63a96" 4bc6b93]:
{{{
#!CommitTicketReference repository=""
revision="4bc6b939944183533ae74791d21282e613f63a96"
Fixed #27039 -- Fixed empty data fallback to model field default in model
forms.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27039#comment:10>
Django
use default value
Reporter: devbis | Owner: nobody
Type: Bug | Status: closed
Component: Forms | Version: 1.10
Severity: Release blocker | Resolution: fixed
Keywords: default non- | Triage Stage: Accepted
required modelform |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"325dd0befea3012c42eefa061f509fbdf1b6a8aa" 325dd0b]:
{{{
#!CommitTicketReference repository=""
revision="325dd0befea3012c42eefa061f509fbdf1b6a8aa"
[1.10.x] Fixed #27039 -- Fixed empty data fallback to model field default
in model forms.
Backport of 4bc6b939944183533ae74791d21282e613f63a96 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27039#comment:11>
Django
use default value
Reporter: devbis | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.10
Keywords: default non- | Triage Stage: Accepted
required modelform |
Needs tests: 0 | Patch needs improvement: 0
Changes (by timgraham):
* status: closed => new
* has_patch: 1 => 0
* resolution: fixed =>
Comment:
Reopening as
[https://github.com/django/django/commit/325dd0befea3012c42eefa061f509fbdf1b6a8aa#commitcomment-18843670
Alex Hill reported] that this isn't working with prefixed forms.
--
Ticket URL: <https://code.djangoproject.com/ticket/27039#comment:12>
Django
use default value
Reporter: devbis | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.10
Keywords: default non- | Triage Stage: Accepted
required modelform |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by AlexHill):
The issue was that when a field on a prefixed form had a default, its
(unprefixed) name wasn't found in form.data, so the instance would always
be populated with the default value. Fixed with `form.add_prefix()`.
PR at https://github.com/django/django/pull/7195
--
Ticket URL: <https://code.djangoproject.com/ticket/27039#comment:13>
Django
use default value
Reporter: devbis | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.10
Keywords: default non- | Triage Stage: Accepted
required modelform |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"d9c083cfee853272ded14c6c87623e910c9e81c4" d9c083cf]:
{{{
#!CommitTicketReference repository=""
revision="d9c083cfee853272ded14c6c87623e910c9e81c4"
Refs #27039 -- Fixed regression with field defaults in prefixed forms.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27039#comment:14>
Django
use default value
Reporter: devbis | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.10
Keywords: default non- | Triage Stage: Accepted
required modelform |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"db3eabfae5c590801a74e0b318663b6480145fb9" db3eabf]:
{{{
#!CommitTicketReference repository=""
revision="db3eabfae5c590801a74e0b318663b6480145fb9"
[1.10.x] Refs #27039 -- Fixed regression with field defaults in prefixed
forms.
Backport of d9c083cfee853272ded14c6c87623e910c9e81c4 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27039#comment:15>
Django
use default value
Reporter: devbis | Owner: nobody
Type: Bug | Status: closed
Component: Forms | Version: 1.10
Severity: Release blocker | Resolution: fixed
Keywords: default non- | Triage Stage: Accepted
required modelform |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by timgraham):
* status: new => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/27039#comment:16>
Django
use default value
Reporter: Ivan Belokobylskiy | Owner: nobody
Type: Bug | Status: closed
Component: Forms | Version: 1.10
Severity: Release blocker | Resolution: fixed
Keywords: default non- | Triage Stage: Accepted
required modelform |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Matt Davis):
I am noticing an odd behavior upgrading a Django app and I have traced it
to this ticket. I have upgraded the app from 1.8 to 1.10 and all tests
are passing, however if I upgrade from 1.10 to 1.10.1 one of my tests
fail. I have determined the reason why to be its a ModelForm and the
model has a field defined with a default value:
{{{
field_x = models.CharField(max_length=100, db_index=True, blank=True,
default='')
}}}
The model form also has a clean method that if a value isn't supplied for
field_x that it will generate one:
{{
def clean_field_x(self):
token = self.cleaned_data.get('field_x')
if not token:
token = utils.generate_token()
return token
}}
It would appear that in 1.10.1 it changed from using the generated token
in the clean method, to then preferring the model field's default value.
I think this behavior is incorrect, shouldn't it prefer the form's cleaned
data over the model defaults?
--
Ticket URL: <https://code.djangoproject.com/ticket/27039#comment:17>
Django
use default value
Reporter: Ivan Belokobylskiy | Owner: nobody
Type: Bug | Status: closed
Component: Forms | Version: 1.10
Severity: Release blocker | Resolution: fixed
Keywords: default non- | Triage Stage: Accepted
required modelform |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Tim Graham):
The behavior you describe isn't what's implemented. Hopefully it's not
documented anywhere. It should be possible to adapt your code. For
example, this might work:
{{{
if not token:
token = utils.generate_token()
self.data['field_x'] = token
}}}
by inserting a value in `self.data`, the model fallback logic won't be
triggered, I think. If you think we could improve the behavior and have a
patch to propose, feel free to open a new ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/27039#comment:18>
Django
use default value
Reporter: Ivan Belokobylskiy | Owner: nobody
Type: Bug | Status: closed
Component: Forms | Version: 1.10
Severity: Release blocker | Resolution: fixed
Keywords: default non- | Triage Stage: Accepted
required modelform |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by ljsjl):
I've run into what I would call a bug (or at least unexpected - to me -
behaviour) which is related to this, running Django 1.11.1. I'm happy to
break this out into another ticket if you'd prefer.
While the argument can be made for a user submitted blank/empty string
overriding the field default, does this make sense in the case of an
IntegerField (or any number field really)? If I have e.g.
models.IntegerField(blank=True, default=0) in a model then currently a
user can submit a blank value to a ModelForm for this model by deleting
the field initial value, and the field value will travel through
form/model cleaning as None.
If I then have couple of IntegerFields like the above and model.clean()
does a check on their sum, it will fail when attempting to add NoneType. I
would say it is reasonable to expect a number based field to contain a
number by the time it hits model.clean() via a form, either a valid user
submitted value or default if the submitted field is empty. Obviously I
can guard the arithmetic statement with a test or try: clause, but in my
mind I set a default value so I expect to see something valid. Maybe
others expect otherwise :)
--
Ticket URL: <https://code.djangoproject.com/ticket/27039#comment:19>
Django
use default value
Reporter: Ivan Belokobylskiy | Owner: nobody
Type: Bug | Status: closed
Component: Forms | Version: 1.10
Severity: Release blocker | Resolution: fixed
Keywords: default non- | Triage Stage: Accepted
required modelform |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Tim Graham):
I'm doubtful that further complexity in this area is a good idea but this
isn't a good place to discuss it. Design decisions are discussed on the
DevelopersMailingList. Providing a patch to show your idea is feasible and
wouldn't break backwards compatibility would help the discussion greatly.
--
Ticket URL: <https://code.djangoproject.com/ticket/27039#comment:20>