Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#20982 closed Bug (needsinfo)

forms.BooleanField field issue

Reported by: george.li Owned by: nobody
Component: Forms Version: 1.7
Severity: Normal Keywords:
Cc: aryeh.hillman@… Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Aryeh Hillman)

in django 1.4 1.5.1

from django import forms
forms.BooleanField().clean(True)

this case is success

But

forms.BooleanField().clean(False)
forms.BooleanField().clean('false')

in this two case are
* ValidationError: [u'This field is required.']

source code

 621 class BooleanField(Field):
 622     widget = CheckboxInput
 623
 624     def to_python(self, value):
 625     ¦   """Returns a Python boolean object."""
 626     ¦   # Explicitly check for the string 'False', which is what a hidden field
 627     ¦   # will submit for False. Also check for '0', since this is what
 628     ¦   # RadioSelect will provide. Because bool("True") == bool('1') == True,
 629     ¦   # we don't need to handle that explicitly.
 630     ¦   if isinstance(value, six.string_types) and value.lower() in ('false', '0'):
 631     ¦   ¦   value = False
 632     ¦   else:
 633     ¦   ¦   value = bool(value)
 634     ¦   value = super(BooleanField, self).to_python(value)
 635     ¦   if not value and self.required:
 636     ¦   ¦   raise ValidationError(self.error_messages['required'])
 637     ¦   return value

in 630, 'false' and '0' Transform to boolean False
but 635 then decide False is no input value

Change History (8)

comment:1 by Aymeric Augustin, 11 years ago

Resolution: worksforme
Status: newclosed

If you want the boolean field not to be required, you must create it with the required=False option.

comment:2 by Aryeh Hillman, 10 years ago

Cc: aryeh.hillman@… added
Description: modified (diff)
Has patch: set
Resolution: worksforme
Status: closednew
Type: UncategorizedBug
Version: 1.51.7

comment:3 by Aryeh Hillman, 10 years ago

In django 1.7, this is an issue. There may have been some regression. For example:

>>> from django import forms
>>> forms.BooleanField().clean(True)
True
>>> forms.BooleanField().clean(False)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/Library/Python/2.7/site-packages/django/forms/fields.py", line 151, in clean
    self.validate(value)
  File "/Library/Python/2.7/site-packages/django/forms/fields.py", line 735, in validate
    raise ValidationError(self.error_messages['required'], code='required')
ValidationError: [u'This field is required.']

The bug is in the validate method on forms.BooleanField.

It's code is:

    def validate(self, value):
        if not value and self.required:
            raise ValidationError(self.error_messages['required'], code='required')

On forms.Field, however, there is a class variable, empty_values that should be used. That is, the correct implementation would be:

    def validate(self, value):
        if value in self.empty_values and self.required:
            raise ValidationError(self.error_messages['required'], code='required')

Thanks!

Version 0, edited 10 years ago by Aryeh Hillman (next)

comment:4 by Tim Graham, 10 years ago

Looking at your example, the field is working as expected. As noted in comment 1, if you want to accept False, you need to use required=False. The docs for BooleanField describe the behavior.

comment:5 by Tim Graham, 10 years ago

Resolution: worksforme
Status: newclosed

comment:6 by Aryeh Hillman, 10 years ago

@timgraham. This is not the correct implementation. If you set required=False, then supplying an empty value will validate properly, which is definitely undesirable. I understand that there could be a backwards compatibility issue here, which is fine. But it seems to me that this is almost definitely the wrong implementation. Your thoughts?

Last edited 10 years ago by Aryeh Hillman (previous) (diff)

comment:7 by Aryeh Hillman, 10 years ago

It seems to me that the original thinking on this was the following: making a boolean field required is good for cases where a user must, say, check a box to accept terms of service. This seems to me, however, that it should be handled at the form-level clean or with a custom validator on the field — not by setting required=False. Another argument is that BooleanField is one of the only fields that overrides forms.Field's validate method. Whereas the other fields rely on self.empty_values, forms.BooleanField is the only field that relies on the provided value's implicit boolean value. Definitely inconsistent.

But again, I understand completely if this is a backwards compatibility case. It just is terribly confusing in many cases, especially when wanting to, say, use forms for validating some JSON data. Say you always want a boolean field to show up in the data, then, really, the best way to deal with this situation is to sub-class BooleanField and override its validate method. Or write a custom validator — but its a bit hacky to circularly reference the calling class' empty_values from an outside method.

Again, I understand your reasoning to some degree. It IS in fact documented, if otherwise confusing.

comment:8 by Aryeh Hillman, 10 years ago

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