Opened 7 months ago

Last modified 6 months ago

#23130 assigned Bug

BooleanField should not override 'blank' if choices are specified

Reported by: jonash Owned by: tushar
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: jonas-django@…, tushar747@… Triage Stage: Accepted
Has patch: no 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)

initial-form.png (27.2 KB) - added by jonash 7 months ago.
form-after-save.png (31.0 KB) - added by jonash 7 months ago.
bug23130.tar.gz (7.4 KB) - added by jonash 7 months ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 7 months ago by tushar

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

comment:2 Changed 7 months ago by timo

Could you provide a code example of the problem?

Changed 7 months ago by jonash

Changed 7 months ago by jonash

Changed 7 months ago by jonash

comment:3 Changed 7 months ago 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'.

comment:4 Changed 7 months ago 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.

comment:5 Changed 7 months ago by jonash

  • Cc jonas-django@… added

comment:6 Changed 7 months ago by tushar

  • Cc tushar747@… 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.

Last edited 7 months ago by tushar (previous) (diff)

comment:7 Changed 7 months ago by timo

  • Summary changed from BooleanField should either forbid setting 'choices' or not override 'blank' to BooleanField should not override 'blank' if choices are specified
  • Triage Stage changed from Unreviewed to Accepted

Sounds good to me.

comment:8 Changed 7 months ago by tushar

Great. Wrote tests + submitted patch as described above. Here's the pull request: https://github.com/django/django/pull/3025

comment:9 Changed 7 months ago by tushar

  • Owner changed from nobody to tushar
  • Status changed from new to assigned

comment:10 Changed 7 months ago 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?

Last edited 7 months ago by jonash (previous) (diff)

comment:11 Changed 7 months ago 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.

comment:12 Changed 7 months ago 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.

comment:13 Changed 7 months ago by timgraham

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 Changed 7 months ago 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:

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:15 Changed 7 months ago by timgraham

What are the backwards compatibility ramifications of the proposal?

comment:16 Changed 7 months ago 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.

comment:17 Changed 7 months ago 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.

comment:18 Changed 6 months ago by timgraham

Jonas, do you plan to submit a patch? Should we close the first PR that was proposed?

comment:19 Changed 6 months ago 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.

Note: See TracTickets for help on using tickets.
Back to Top