Opened 5 years ago

Closed 5 years ago

Last modified 19 months ago

#31369 closed Cleanup/optimization (fixed)

Deprecate the model NullBooleanField.

Reported by: David Smith Owned by: Hristiyan Ivanov
Component: Database layer (models, ORM) Version: 3.0
Severity: Normal Keywords:
Cc: Timothy Schilling Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

#29227 allowed BooleanField to be null=true, a step towards deprecating NullBooleanField. #27917 also mentions an aim to deprecate this field.

Comments on the PR discussed deprecating NullBooleanField with the conclusion being that we should wait a while. Now that we are almost 2 years on, and with Django 4.0 being sometime away I suggest now is a good time to revisit this decision.

This ticket therefore proposes we deprecate NullBooleanField. It would be good to also tidy up the associated fields / widgets e.g. #23681 discussed deprecation of NullBooleanSelect

Note: I was slightly unsure if this is best raised as a ticket or as a topic on the mailing list.

Change History (15)

comment:1 by Carlton Gibson, 5 years ago

Resolution: needsinfo
Status: newclosed

I've posted to the mailing list to get feedback. https://groups.google.com/d/topic/django-developers/pSvFXcUUkRs/discussion

I'm inclined to Accept, but let's see what folks say.
(I'll close as needsinfo for the moment.)

comment:2 by Carlton Gibson, 5 years ago

Resolution: needsinfo
Status: closednew
Triage Stage: UnreviewedAccepted

Support on the mailing list, so let's progress. Thanks David.

comment:3 by Hasan Ramezani, 5 years ago

Has patch: set

comment:4 by Hasan Ramezani, 5 years ago

Has patch: unset

comment:5 by Hristiyan Ivanov, 5 years ago

Owner: changed from nobody to Hristiyan Ivanov
Status: newassigned

comment:6 by Hristiyan Ivanov, 5 years ago

"Deprecate NullBooleanField" is a bit of a broad naming. If I understand correctly, for now just adding a "NullBooleanField has been deprecated." message should be enough, and later on removing the field in favor of the BooleanField.

system_check_deprecated_details = {
        'msg': (
            'NullBooleanField has been deprecated. Support '
            'for it (except in historical migrations) will be removed '
            'in Django 4.0.'
        ),
        'hint': (
            'Use BooleanField(null=true) instead.'
        ),
        'id': 'fields.W903',
    }

comment:7 by David Smith, 5 years ago

Hi hristiy4n,

Thanks for picking this up.

I had a brief look at the code before raising the issue but thought the principle was important to agree before lots of detailed work. I agree a deprecation message is key to this. I also think a bit more thought at this stage would be beneficial in the long run. I'd suggest that 'just' issuing a deprecation warning at this stage we may uncover something later which is tricky to resolve.

Here are my more detailed notes having spent some time looking at it in more detail.

There are three elements which we should consider deprecating. Is it just the first one at this stage, the top two (I think this is the intent), or a bit more of a wider tidy up and go for all three?

My main concern about just adding a deprecation notice is that the two BooleanFields work slightly differently.

The models.fields.BooleanField acts more like the forms.fields.NullBooleanField than forms.fields.BooleanField. Whilst I think the preference should be to deprecate both NullBooleanFields do we need to change the behaviour of forms.fields.BooleanField to mirror models.fields.BooleanField when null=True.

I hope my code sample below, explains it more clearly that what I've written out above!

>>> from django.forms.fields import NullBooleanField
>>> f = NullBooleanField()
>>> f.clean("True")
True
>>> f.clean("") # returns null
>>> f.clean(False)
False

>>> from django.forms.fields import BooleanField
>>> f = BooleanField()
>>> f.required = False
>>> f.clean(True)
True
>>> f.clean("")
False
>>> f.clean(False)
False


>>> from django.db.models import fields
>>> f = fields.BooleanField(null=True)
>>> f.blank = True
>>> f.clean(True, "test")
True
>>> f.clean("", "test") # returns null
>>> f.clean(False, "test")
False

Version 0, edited 5 years ago by David Smith (next)

comment:8 by Hristiyan Ivanov, 5 years ago

I had a quick look at the code myself and I think I understand the problem you are raising. The behavior of the BooleanFields is indeed different, which is due to forms.BooleanField's to_python() method as it uses bool(value) if the value is not False.

I do agree that if models.NullBooleanField is getting deprecated, forms.NullBooleanField should also be deprecated and that just adding the deprecation message will introduce work later on, as the forms.BooleanField will need to be changed.

I will have a look at the widgets that the two form fields are using to see if there is any substantial difference and if not, I guess we can alter the widgets.CheckboxInput and also deprecate widgets.NullBooleanSelect, as well as improve the to_python() method of forms.BooleanField and therefor deprecate all three NullBooleanField related items.

comment:9 by Timothy Schilling, 5 years ago

Cc: Timothy Schilling added
Has patch: set

I made an attempt at the deprecation of both NullBooleanField's. I left NullBooleanSelect alone for the time being as it seems like a much larger change. If that's a show stopper, I can take a swing at that as well.

comment:11 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In a92cc84b:

Refs #31369 -- Deprecated models.NullBooleanField in favor of BooleanField(null=True).

comment:12 by Mariusz Felisiak, 5 years ago

I'm not sure about removing forms.NullBooleanField (see comment).

comment:13 by Mariusz Felisiak, 5 years ago

Resolution: fixed
Status: assignedclosed
Summary: Deprecate NullBooleanFieldDeprecate the model NullBooleanField.

We agree on the mailing list that we should deprecate only the model NullBooleanField.

comment:14 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In d992f4e:

Refs #31369 -- Removed models.NullBooleanField per deprecation timeline.

comment:15 by GitHub <noreply@…>, 19 months ago

In 3b62d8c:

Refs #31369 -- Improved hint message in NullBooleanField's deprecation warning.

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