Opened 10 years ago

Closed 10 years ago

Last modified 4 years ago

#23547 closed Bug (worksforme)

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

Reported by: Lagovas Owned by: nobody
Component: Forms Version: 1.6
Severity: Normal Keywords: Form BooleanField
Cc: Lagovas 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 Tim Graham)

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.

Attachments (2)

0001-fix-BooleanField-in-case-when-field-is-required-and-.patch (918 bytes ) - added by Lagovas 10 years ago.
Patch for ticket
0001-fix-BooleanField-ticket-23547-in-case-when-field-is-.patch (2.5 KB ) - added by Lagovas 10 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 by Tim Graham, 10 years ago

Description: modified (diff)
Resolution: duplicate
Status: newclosed

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

comment:2 by Lagovas, 10 years ago

Cc: Lagovas added
Has patch: set
Resolution: duplicate
Status: closednew

comment:3 by Tim Graham, 10 years ago

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

comment:4 by Lagovas, 10 years ago

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.

comment:5 by Tim Graham, 10 years ago

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."

comment:6 by Lagovas, 10 years ago

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.

comment:7 by Lagovas, 10 years ago

Resolution: worksforme
Status: newclosed

in reply to:  6 comment:8 by Krish Ravindranath, 4 years ago

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.

Version 0, edited 4 years ago by Krish Ravindranath (next)
Note: See TracTickets for help on using tickets.
Back to Top