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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23130>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:1>
Comment (by timo):
Could you provide a code example of the problem?
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:2>
Comment (by jonash):
"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'.
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:3>
Comment (by jonash):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:4>
* cc: jonas-django@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:5>
* cc: tushar747@… (added)
Comment:
I've just confirmed that I get this same behaviour as described by jonash.
I am willing to work on a patch for this if this bug is accepted.
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:6>
* stage: Unreviewed => Accepted
Comment:
Sounds good to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:7>
Comment (by tushar):
Great. Wrote tests + submitted patch as described above. Here's the pull
request: https://github.com/django/django/pull/3025
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:8>
* status: new => assigned
* owner: nobody => tushar
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:9>
Comment (by jonash):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:10>
Comment (by timo):
If `blank=False` on a `BooleanField` then `False` isn't allowed so by
default it would be a checkbox that must be checked.
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:11>
Comment (by anonymous):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:12>
Comment (by timgraham):
It's at the
[https://github.com/django/django/blob/f676305ecede116332884ea682fc953c29813232/django/forms/fields.py#L744
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:13>
Comment (by jonashaag):
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:
{{{
#!python
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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:14>
Comment (by timgraham):
What are the backwards compatibility ramifications of the proposal?
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:15>
Comment (by jonashaag):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:16>
Comment (by timgraham):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:17>
Comment (by timgraham):
Jonas, do you plan to submit a patch? Should we close the first PR that
was proposed?
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:18>
Comment (by jonashaag):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:19>
* owner: Tushar Bhatia => (none)
* status: assigned => new
Comment:
I've looked at this a little but much work remains. Here's my WIP
[https://github.com/django/django/pull/8467 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
[https://github.com/django/django/blob/e86f4786a7f39b0ed833c0699addf0c27811d864/django/db/backends/oracle/base.py#L112
a check constraint]. I'm not planning to continue this in the immediate
future, so someone else can continue with my initial patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:20>
* status: new => assigned
* owner: (none) => Lynn Cyrin
Comment:
I've got a WIP PR for this https://github.com/django/django/pull/9016
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:21>
* status: assigned => new
* owner: Lynn Cyrin => (none)
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:22>
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/13271 PR]
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:23>
* owner: (none) => Jacob Walls
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:24>
* easy: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:25>
* has_patch: 1 => 0
* easy: 1 => 0
Comment:
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 [https://code.djangoproject.com/ticket/23130#comment:4
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.']`.
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:26>
Comment (by Jacob Walls):
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 a `default` for the user, either True or the
first element of `choices`, so that '---------' is not displayed on the
form.
- Supplying `choices` and no `default` to `BooleanField` while letting
`null=False` raises a `ValueError`
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:27>
Comment (by Jacob Walls):
Responding to David's question on another
[https://github.com/django/django/pull/12946 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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:28>
* has_patch: 0 => 1
Comment:
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.)
[https://github.com/django/django/pull/13286 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:29>
* status: assigned => closed
* resolution: => fixed
Comment:
> 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.
[https://github.com/django/django/pull/13399 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:30>
Comment (by Carlton Gibson <carlton@…>):
In [changeset:"1db8d8e3a9264eb38e8d0e0d515f598db7520ce6" 1db8d8e3]:
{{{
#!CommitTicketReference repository=""
revision="1db8d8e3a9264eb38e8d0e0d515f598db7520ce6"
Refs #23130 -- Added test for BooleanField choices generation.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23130#comment:31>