unchecked BooleanField does not raise ValidationError even if it is required

[che@…]

I'm afraid that required=True doesn't quite work on newforms' ​BooleanField. According to the docs, it should trigger an error in case the field is unchecked, but it doesn't. I suspect the reason is that ​changeset 6563 altered return value of CheckboxInput from None to False in unchecked cases, but "required" check of the BooleanField still relies on default "None_to_ValidationError" method of its parent class.

trunk rev 6678

>>> import django.newforms as forms
>>> class MyForm(forms.Form):
...   bfield = forms.BooleanField(required=True)
...
>>> f = MyForm({})
>>> f.is_valid()
True

trunk rev 6562

>>> import django.newforms as forms
>>> class MyForm(forms.Form):
...   bfield = forms.BooleanField(required=True)
...
>>> f = MyForm({})
>>> f.is_valid()
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
  File "/usr/lib/python2.4/site-packages/django/newforms/forms.py", line 106, in is_valid
    return self.is_bound and not bool(self.errors)
  File "/usr/lib/python2.4/site-packages/django/newforms/forms.py", line 97, in _get_errors
    self.full_clean()
  File "/usr/lib/python2.4/site-packages/django/newforms/forms.py", line 192, in full_clean
    value = field.clean(value)
  File "/usr/lib/python2.4/site-packages/django/newforms/fields.py", line 454, in clean
    super(BooleanField, self).clean(value)
  File "/usr/lib/python2.4/site-packages/django/newforms/fields.py", line 93, in clean
    raise ValidationError(ugettext(u'This field is required.'))
  File "/usr/lib/python2.4/site-packages/django/utils/translation/__init__.py", line 62, in ugettext
    return real_ugettext(message)
  File "/usr/lib/python2.4/site-packages/django/utils/translation/__init__.py", line 32, in delayed_loader
    if settings.USE_I18N:
  File "/usr/lib/python2.4/site-packages/django/conf/__init__.py", line 28, in __getattr__
    self._import_settings()
  File "/usr/lib/python2.4/site-packages/django/conf/__init__.py", line 55, in _import_settings
    raise EnvironmentError, "Environment variable %s is undefined." % ENVIRONMENT_VARIABLE
EnvironmentError: Environment variable DJANGO_SETTINGS_MODULE is undefined.

(Maybe I should have run this in django shell, but we can still see that ValidationError got raised.)

trunk rev 6563

>>> import django.newforms as forms
>>> class MyForm(forms.Form):
...   bfield = forms.BooleanField(required=True)
...
>>> f = MyForm({})
>>> f.is_valid()
True

(After changeset 6563, unchecked field is valid even if required.)

trunk rev 6678, with patch

>>> import django.newforms as forms
>>> class MyForm(forms.Form):
...   bfield = forms.BooleanField(required=True)
...
>>> f = MyForm({})
>>> f.is_valid()
False

(Supplied patch seems to fix the issue.)

[Brian Rosner]

Marked #6156 as a duplicate.

[anonymous]

Updated patch and tests

[Tai Lee <real.human@…>]

Updated to r7000.

[Chris Beaven]

This comes down to the interpretation of an "empty value". The docs say:

By default, each Field class assumes the value is required, so if you pass an empty value — either None or the empty string ("") — then clean() will raise a ValidationError exception

Technically, the CheckboxInput widget must assume False if nothing was in the data dict.

Since False *is* a "value", it passes the "value is required" test in my opinion. I know that my opinion differs from others though, so I'll leave as a design decision.

(In fact, it is clear from Malcolm's follow-up changeset of [6564] this isn't how core wants it and the documentation changes there clearly contradict the current behavior - but I still argue that it makes more sense the way it works at the moment even if it is a [currently undocumented] backwards incompatible change)

Sure, you can argue that "" is a value too but if you are going to go down the "required means not true" route then what about numeric fields and 0? At the end of the day, if you are using the CheckboxInput widget then you are always going to get back a valid value, so required shouldn't change anything (IMO). If you want to do your own checking for the case of an "I agree to the terms and conditions" checkbox then you should use a custom clean_* method on your form.

[Tai Lee <real.human@…>]

It's impossible for a checkbox to ever not provide a value in the "required" context. It is always either True, or False (since the recent "fix" that normalized None to False). The current implementation makes required=True redundant, except for a useless edge case where you have a Form object which is trying to process a checkbox field that wasn't coded into the HTML form. In a boolean context where either True or False will always be provided, required=True should require that a checkbox is selected.

[Chris Beaven]

While you are correct that it is impossible for the CheckboxInput widget so it makes the required setting redundant if you are using this (the default) widget, you shouldn't assume that just this widget is being used and therefore make a blanket statement about it's redundancy. And you're mixing your field/widget logic to talk about "requiring a checkbox to be selected" when talking about a field (which shouldn't care about its widget -- even its default widget).

At the end of the day, the design decision to be made by core is to which of the two different ways to look at it (as summarized by you and me) is most logical.

[Daniel Pope <dan@…>]

It would not be inconsistent for CheckboxInput to return only True or None because this is what the HTML actually submits. Guessing False is wrong in some cases, such as when the checkbox is removed by Javascript.

However, the only use case I run into for the old required=True validation is for checkboxes of the form "I have read and agree to the terms and conditions". If that's the only use case, it could properly be considered a subclass of BooleanField. AgreementField perhaps.

This ticket has been lingering and meanwhile the documentation is just misleading. The documentation at least should be fixed.

[Chris Beaven]

Replying to Daniel Pope <dan@mauveinternet.co.uk>:

It would not be inconsistent for CheckboxInput to return only True or None because this is what the HTML actually submits. Guessing False is wrong in some cases, such as when the checkbox is removed by Javascript.

But guessing None is going to be wrong more times than the minority of cases where the checkbox was removed by Javascript.

However, the only use case I run into for the old required=True validation is for checkboxes of the form "I have read and agree to the terms and conditions". If that's the only use case, it could properly be considered a subclass of BooleanField. AgreementField perhaps.

Yep, that'd be fine. Or alternatively the advice can be to just use a clean_agreement() method for those cases.

This ticket has been lingering and meanwhile the documentation is just misleading. The documentation at least should be fixed.

I concur, but the line drawn by core is that you shouldn't fix documentation if it's a bug. Personally, I don't think this is a valid bug so the documentation should be changed - but that needs discussion in django-dev. Perhaps you could start a new topic there.

[Daniel Pope <dan@…>]

Replying to SmileyChris:

But guessing None is going to be wrong more times than the minority of cases where the checkbox was removed by Javascript.

It's not really wrong, though. True/False is cooked data whereas the raw data is more like True/None. Perhaps, rather than touching the Field part, it would be better to amend the CheckboxInput so that it derives from a superclass RawCheckboxInput, whose value_from_datadict returns True/None. So to implement a required checkbox, you'd just need widget=RawCheckboxInput.

It might be fairly obscure why forms.BooleanField(widget=RawCheckboxInput) gives a checkbox that must be checked (you'd need to understand the rationale and it's foundation in HTML forms), but at least the conceptual purity of what a BooleanField is and what a widget is would be maintained. And just a trivial thing, but some users may prefer to derive custom widgets or MultiWidgets from RawCheckboxInput for some reason.

[Tai Lee <real.human@…>]

It is wrong. If you want to talk about the raw data from a checkbox input field, it's not True/None, it's whatever string is specified in the value attribute (Django uses "on") or None. We're talking about boolean fields and widgets here, so it does make sense to normalise the data to True/False, just like we normalise string dates into datetime objects.

[hanne.moa@…]

This and variants have become an FPT (Frequently Posted Ticket): see #7190, #7051, #7038, #6937, #6914, #6229, #6209, #6156, #5957, #5563 (that's as far back as I bothered to check). The problems with formtools.formwizard and formtools.preview is no doubt due to preoptimization by reading request.POST directly and not special-casing checkbox-input instead of using cleaned data (see #7051). There are no good workarounds at present though (the one presented in #7038 is dangerous), meaning that formwizard and preview at present cannot be used if there is a checkbox in a form. This is the visible tip of an iceberg, people.

[Tai Lee <real.human@…>]

putting this in 1.0 milestone as a bug (functionality differs to documentation).

[Alex Gaynor]

Works fine, needed to special case it, but I think that can be changed in Py3k :D

[Alex Gaynor]

Removed an import that hung around.

[Chris Beaven]

I (still) strongly disagree with this bug. r6564 (an 8 month old changeset) introduced this change and I still think that the documentation can change rather than the code.

And once again, False is an VALID data type to a BooleanField. If I have a Select widget with three options: "", "False", "True" then it should raise a ValidationError on "", but the other two values are perfectly acceptable.

[Alex Gaynor]

I disagree, if you have required=True, it's pretty clear you want it to be True, else that settings has no meaning, seeing as how the settings exists it should have a meaning, how else, for example, can I guarantee that a user agree to the license, if required=True, doesn't actually require them to check the box.

[Chris Beaven]

It's only for the default widget that it has no effect. But it still has a purpose - did you read my example of the Select widget?

If you want to guarantee that a user agrees to a license, then you can use a clean_* method.

Anyway, this is a decision to be left to the big cheeses, hence why I pushed back to design decision. Especially since it's been like this for 8 months now. With an increasing number of people using trunk, reverting it would probably be more of an incompatible change than the change in the first place ;)

[Tai Lee <real.human@…>]

The documentation clearly states the desired behaviour, which makes this a bug. The bug should be fixed immediately. It's a simple patch. There's nothing lost by fixing this bug now and continuing to petition for a change in the dev list or another ticket, if you feel strongly about it. By leaving this ticket as design decision (in obscurity) we've allowed an easily fixed bug to remain in trunk for 6 months already (a bug in documentation or a bug in behaviour is still a bug, whichever way you look at it).

If a decision by a core developer is desired, marking this as ready for checkin is the easiest way to get their attention. If they then feel more discussion is required, they can bump it down and bring it to the list. Otherwise they can make the decision and resolve this bug (either by changing the documentation or committing the patch).

Personally I'd be more inclined to suggest using a custom clean_* method for the edge case rather than to make the default widget work logically.

Just because this has been sitting around for 6 months, doesn't make FIXING the behaviour to match the documentation a backwards incompatible change ;)

[Chris Beaven]

How would you suggest that a custom clean_* method would work in the reverse case?

​http://www.djangoproject.com/documentation/contributing/#ticket-triage clearly states that tickets needing "discussion or review from a core developer" should be placed in Design Decision. But for the sake of peacefulness, I'll leave it as is. I'm sure a core committer will see the noise and review the patch before committing :)

Malcolm reviewed my patch which caused this whole kerfuffle (sorry, I misstated the changeset before, r6563), was ​happy with it and checked it in. It was a pretty obvious change IMO and he's a smart man, but potentially he didn't understand the ramifications (which r6564 does suggest).

[Chris Beaven]

Oops, I meant to leave that in Checkin, honest :D

[Chris Beaven]

Oh, and speaking of edge cases, I've made a lot of Django apps and used a lot of BooleanFields. There would only be maybe 2 cases where I *needed& the value to just be True.

I guess my dream is to see the combination of NullBooleanField and BooleanField... but yes, now I'm getting sidetracked from this ticket.

Back on topic, this is a backwards incompatible change. Just because documentation says something, doesn't make it True. In the last 8 months, people will either have forgotten to add required=False in their code, or they may be using custom ValidationError messages in their own clean_* methods which won't get used if this patch goes in.

[Alex Gaynor]

IMO the fact that the current docs were checked in at the same time the change that led to this ticket was leads me to believe this is indeed a bug, in any event, I've made a post on django-dev.

[Ludvig Ericson]

Slight note on the patch, call it nitpick, but this:

if not bool(x):
    ...

is the equivalent of the shorter and more readable (IMO) form:

if not x:
    ...

[Alex Gaynor]

tiny style fix

[Malcolm Tredinnick]

(In [7799]) Fixed #5957 -- Enforce the "required" attribute on BooleanField in newforms.

This has been the documented behaviour for ages, but it wasn't correctly
implemented. A required BooleanField must be True/checked, since False values
aren't submitted. Ideal for things like "terms of service" agreements.

Backwards incompatible (since required=True is the default for all fields).

Unclear who the original patch was from, but Tai Lee and Alex have kept it up
to date recently.

[Jacob]

Milestone 1.0 deleted