#23130 closed Bug (fixed)
Supplying choices to BooleanField without default yields blank choice even if blank=False
Reported by: | Jonas H. | Owned by: | Jacob Walls |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | jonas-django@…, tushar747@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
BooleanField currently allows settings 'choices'. It also always overrides 'blank'. This stems from the fact that in HTML, a blank value means False, and any (?) non-blank value means True for check boxes.
Now if you override 'choices', things change in terms of HTML, since now True and False are represented by "True" and "False" in the select box. This also makes it possible to supply a null/blank value (which would have meant False in the checkbox case).
BooleanField should either handle this fact gracefully, for instance by only overriding 'blank' if 'choices' is not given and then interpreting "True" and "False" as True and False. Or it should disallow overriding 'choices' entirely.
Attachments (3)
Change History (34)
comment:1 by , 10 years ago
comment:2 by , 10 years ago
by , 10 years ago
Attachment: | initial-form.png added |
---|
by , 10 years ago
Attachment: | form-after-save.png added |
---|
by , 10 years ago
Attachment: | bug23130.tar.gz added |
---|
comment:3 by , 10 years ago
"initial-form.png" shows the admin form right before pressing "Save". "form-after-save.png" shows the data that was actually saved.
"bug23139.tar.gz" contains the code to produce this form. It's no more than a single BooleanField with some 'choices'.
comment:4 by , 10 years ago
Expected behavior when pressing "Save": The message "this field is required" should appear, since it hasn't been filled out. This doesn't work as expected because 'blank' is always set to True in the BooleanField's __init__
method. This is because a blank/absent value means False if the BooleanField widget is a checkbox. But if rendered as a choices list, things change. See initial description for a more elaborate description of the issue.
comment:5 by , 10 years ago
Cc: | added |
---|
comment:6 by , 10 years ago
Cc: | added |
---|
I've just confirmed that I get this same behaviour as described by jonash. I don't believe that disallowing the overriding of choices is an appropriate solution as it is useful in cases where the Boolean values resemble a meaning. I think the way forward is to disable the override in the init method. Here's a quick update for the init method:
def __init__(self, *args, **kwargs): if 'choices' not in kwargs: kwargs['blank'] = True super(BooleanField, self).__init__(*args, **kwargs)
This code gives the desired behaviour: if a Null value is specified when choices are overridden, then it will not be updated.
comment:7 by , 10 years ago
Summary: | BooleanField should either forbid setting 'choices' or not override 'blank' → BooleanField should not override 'blank' if choices are specified |
---|---|
Triage Stage: | Unreviewed → Accepted |
Sounds good to me.
comment:8 by , 10 years ago
Great. Wrote tests + submitted patch as described above. Here's the pull request: https://github.com/django/django/pull/3025
comment:9 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:10 by , 10 years ago
Actually, my initial suggestion feels like something that violates the "separation of concerns" principle. Interpreting browser-submitted values should happen in the checkbox widget. This is completely unrelated to the model layer. Why not have BooleanField
behave as follows:
- Don't override
blank
at all CheckboxInput
should interpret "value absent from POST" as False, any other value as True (already does this)BooleanField
should interpret None as a blank value (already does this)
Why does BooleanField
need to have blank=True
in the first place?
comment:11 by , 10 years ago
If blank=False
on a BooleanField
then False
isn't allowed so by default it would be a checkbox that must be checked.
comment:12 by , 10 years ago
Are you talking about the model or the forms layer?
In the former case, why is False considered a blank value in the first place? Shouldn't None be the only value considered blank?
In the latter case, see my last comment.
comment:13 by , 10 years ago
It's at the forms layer. I haven't looked into it, but it may be difficult or impossible to change now due to backwards compatibility. Patches/ideas welcome.
comment:14 by , 10 years ago
Having given this whole business some hours' thought, I actually believe that making a difference between BooleanField and NullBooleanField is fundamentally wrong. I know NullBooleanField has been in Django from the beginning and removing it/changing BooleanField is a nightmare in terms of backwards compatibility.
Still I'd like to propose a better implementation in form of this patch: https://gist.github.com/jonashaag/d8a9f82d88c60c103189 (This passes the complete test suite except for a handful of minor now-outdated tests!)
In this version, BooleanField behaves much more like any other field, simplifying both the implementation and the usage. blank
may be used freely as with any other field. The same applies to null
. By default, it renders as a checkbox and does not allow None
(i.e. blank=False
like with any other field). If blank
is set to True, it renders as choices with an additional blank option. It also renders as choices if choices
is given.
NullBooleanField
stays as a mere wrapper for BooleanField
with blank=null=True
.
Here is a shortened version of the new implementation:
class BooleanField(Field): empty_strings_allowed = False default_error_messages = { 'invalid': _("'%(value)s' value must be either True or False."), } description = _("Boolean (Either True or False)") def get_internal_type(self): return "BooleanField" def to_python(self, value): if value is None: return value if value in (True, False): # if value is 1 or 0 than it's equal to True or False, but we want # to return a true bool for semantic reasons. return bool(value) if value in ('t', 'True', '1'): return True if value in ('f', 'False', '0'): return False raise exceptions.ValidationError( self.error_messages['invalid'], code='invalid', params={'value': value}, ) # ... def formfield(self, **kwargs): if self.blank or self.choices: return super(BooleanField, self).formfield(**kwargs) else: # In the checkbox case, 'required' means "must be checked (=> true)", # which is different from the choices case ("must select some value"). # Since we want to allow both True and False (checked/unchecked) choices, # set 'required' to False. defaults = {'form_class': forms.BooleanField, 'required': False} defaults.update(kwargs) return super(BooleanField, self).formfield(**defaults)
comment:16 by , 10 years ago
Can't think of any documented/tested incompatibilities, except for NullBooleanField being deprecated/superseded by BooleanField. But it may have undocumented surprises, for instance for people who rely on the behavior I consider a bug in my initial ticket description. Or for people who expect the forced blank=True
on BooleanFields.
comment:17 by , 10 years ago
You mentioned some test updates were required. That suggests to me there could be some compatibility issues but difficult to say for sure without seeing them. Maybe they will be acceptable, but they will at least need to be documented in the release notes. If you can send a PR with the updates, it'll be easier to review.
comment:18 by , 10 years ago
Jonas, do you plan to submit a patch? Should we close the first PR that was proposed?
comment:19 by , 10 years ago
I plan on submitting a patch in a few weeks. I'm having trouble with my development machine at the moment. I think that PR may be closed.
comment:20 by , 8 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
I've looked at this a little but much work remains. Here's my WIP PR to allow null=True
for BooleanField
. Most everywhere NullBooleanField
is tested in Django's test suite, a parallel test for BooleanField(null=True)
should also be added. I think there may also be some trickiness with regards to migrations and changing the nullability of BooleanField
. For example, on Oracle, it requires adding or dropping a check constraint. I'm not planning to continue this in the immediate future, so someone else can continue with my initial patch.
comment:21 by , 7 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
I've got a WIP PR for this https://github.com/django/django/pull/9016
comment:22 by , 7 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
I created #29227 for the issue of allowing BooleanField
to be null=True
. The PR there is ready for review. I'll leave this ticket open as I'm not sure if there's additional work to be done to resolve it. Even if no further code changes are required, a test for this use case might be added.
comment:23 by , 4 years ago
Has patch: | set |
---|
Submitting a test to cover what I believe is the contemporary version of the original case: BooleanField(choices=choices, blank=True, null=True)
It passes, so I think this can be resolved.
comment:24 by , 4 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:25 by , 4 years ago
Easy pickings: | set |
---|
comment:26 by , 4 years ago
Easy pickings: | unset |
---|---|
Has patch: | unset |
This is not a test of described issue, please check a sample project attached to the ticket and previous versions of this patch. Issue described in the comment#4 is fixed but it's about validation of blank option for BooleanField
with choices
, e.g.
boolean = models.BooleanField(choices=[(True, "Special True"), (False, "Special False")],
so it's not a proper test. I'm also not sure if behavior for blank=True
is correct, e.g.
boolean = models.BooleanField(choices=[(True, "Special True"), (False, "Special False")], blank=True)
because choosing a blank value causes: django.core.exceptions.ValidationError: ['“” value must be either True or False.']
.
comment:27 by , 4 years ago
Summary: | BooleanField should not override 'blank' if choices are specified → Supplying choices to BooleanField without default yields blank choice even if blank=False |
---|
Thanks for pointing me in the right direction. Django already has a passing test for supplying choices
and default
to BooleanField
where '---------' is not desired, so I want to make sure I understand the desired behavior when default
is not supplied, as in the original case.
The documentation for choices
says Django will add '---------' unless both blank=False
and default
is supplied, which will cause validation issues if null=False
.
Which of these is desired:
BooleanField
supplies adefault
for the user, either True or the first element ofchoices
, so that '---------' is not displayed on the form.- Supplying
choices
and nodefault
toBooleanField
while lettingnull=False
raises aValueError
Mariusz rightly points out that explicitly setting blank=True
while letting null=False
is probably wrong in all cases, so we should consider taking the same course of action there.
Let me know if I continue to misunderstand the thrust of this.
comment:28 by , 4 years ago
Responding to David's question on another PR was clarifying.
I think it is perfectly acceptable for Django to supply a blank choice when choices
are given without default
even if blank=False
. Twice in the docs for choices
(models, forms) it says default
has to be set with blank=False
to suppress the blank choice.
So ultimately it just comes down to validating the form. For nullable fields we handle this fine: with a django.core.exceptions.ValidationError: ['This field cannot be blank.']
. I think we should do the same for not nullable fields, no matter whether blank=False
or Django supplied a blank field in the case from this ticket.
Something like:
diff --git a/django/db/models/fields/__init__.py b/django/db/models/fields/__init__.py index 28374272f4..d0afafe0a9 100644 --- a/django/db/models/fields/__init__.py +++ b/django/db/models/fields/__init__.py @@ -945,8 +945,14 @@ class BooleanField(Field): return "BooleanField" def to_python(self, value): - if self.null and value in self.empty_values: - return None + if value in self.empty_values: + if self.null: + return None + raise exceptions.ValidationError( + self.error_messages['blank'], + code='blank', + params={'value': value}, + ) if value in (True, False): # 1/0 are equal to True/False. bool() converts former to latter. return bool(value)
comment:29 by , 4 years ago
Has patch: | set |
---|
There wasn't much to do here except slightly improve the ValidationError
messaging. Included tests for the four combinations of null
and blank
arguments when choices
are given without a default
. (blank
wasn't being overridden anywhere anymore.)
comment:30 by , 4 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
I think it is perfectly acceptable for Django to supply a blank choice when choices are given without default even if blank=False. Twice in the docs for choices (models, forms) it says default has to be set with blank=False to suppress the blank choice.
This is correct. We want the blank choice to be generated. Test for that added in PR.
The validation weirdness when blank=True
is a red-herring. It causes the field to be skipped during full_clean(). At that point developers should implementing clean_* to ensure they have an acceptable value. It's quite feasible that blank=True
doesn't really make sense for a BooleanField
, but I'm not sure we can tighten it up at this latter stage (no doubt there's some use-case…)
I will close this issue as fixed.
Could you provide a code example of the problem?