BooleanField that is required and have value False always will raise ValidationError

[Lagovas]

class BooleanField(Field):
   def to_python(self, value):
        if isinstance(value, six.string_types) and value.lower() in ('false', '0'):
            value = False
        else:
            value = bool(value)
        return super(BooleanField, self).to_python(value)

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

In method "validate" value is "True" or "False" as python object, because called after to_python.
So if value is valid ('0' or 'false') and field is required, will be raised ValidationError.
Anyway value in "validate" will be False if value is not '0' or 'false', so method validate should be empty because always is valid.

[Tim Graham]

I think your complaint is the same as #23130. If not, could you please reopen with a proposed patch?

[Lagovas]

Patch for ticket

[Tim Graham]

The tests do not pass with your proposed change:

======================================================================
FAIL: test_booleanfield (forms_tests.tests.test_error_messages.FormsErrorMessagesTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tim/code/django/tests/forms_tests/tests/test_error_messages.py", line 169, in test_booleanfield
    self.assertFormErrors(['REQUIRED'], f.clean, '')
  File "/home/tim/code/django/tests/forms_tests/tests/test_error_messages.py", line 23, in assertFormErrors
    self.fail("Testing the 'clean' method on %s failed to raise a ValidationError.")
AssertionError: Testing the 'clean' method on %s failed to raise a ValidationError.

======================================================================
FAIL: test_booleanfield_1 (forms_tests.tests.test_fields.FieldsTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tim/code/django/tests/forms_tests/tests/test_fields.py", line 891, in test_booleanfield_1
    self.assertRaisesMessage(ValidationError, "'This field is required.'", f.clean, '')
  File "/home/tim/code/django/django/test/testcases.py", line 579, in assertRaisesMessage
    re.escape(expected_message), callable_obj, *args, **kwargs)
  File "/home/tim/code/django/django/utils/six.py", line 690, in assertRaisesRegex
    return getattr(self, _assertRaisesRegex)(*args, **kwargs)
AssertionError: ValidationError not raised

[Lagovas]

Added new patch. For correct way, in "to_python" will saving raw_value and then in "validate" check that it value not in empty_values if field is required. And removed 2 tests, that expect ValidationError with correct false value.

[Tim Graham]

Your proposed change is backwards incompatible. You can't just call the existing behavior a bug and remove tests that don't conform to how you expect things to work.

​As documented: "Since all Field subclasses have required=True by default, the validation condition here is important. If you want to include a boolean in your form that can be either True or False (e.g. a checked or unchecked checkbox), you must remember to pass in required=False when creating the BooleanField."

[Lagovas]

Sorry. Revised documentation and saw note about this (after error first of all went to code to see from what and why raises and code behavior was nonobvious. But in my opinion, it's not good that field can't be required and return all valid values. Ticket can be closed i guess.

[Krish Ravindranath]

Replying to Lagovas:

Sorry. Revised documentation and saw note about this (after error first of all went to code to see from what and why raises and code behavior was nonobvious. But in my opinion, it's not good that field can't be required and return all valid values. Ticket can be closed i guess.

agreed with this- what purpose does required=True serve? If you want to have a boolean field for some reason that is always set to True, wouldn't it be better to add a validator for that? Enforcing this via required=True is unintuitive- just because it's documented doesn't mean it's the best design IMO. perhaps I'm missing something, but if so would like to understand what it is.