Fix BooleanField's allow_null behavior #8614
Conversation
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
There was a problem hiding this comment.
As I understand, right now if you want to migrate and preserve the initial value being None, you need NullBooleanField(allow_null=True, initial=None) ? I don't think we need to change BooleanField for this.
I don't quite follow why the default_empty_html change is needed - it doesn't look like NullBooleanField ever had that:
django-rest-framework/rest_framework/fields.py
Lines 715 to 729 in 51f1aff
rest_framework/fields.py
Outdated
| @@ -720,6 +720,12 @@ class BooleanField(Field): | |||
| } | |||
| NULL_VALUES = {'null', 'Null', 'NULL', '', None} | |||
|
|
|||
| def __init__(self, **kwargs): | |||
| if 'allow_null' in kwargs: | |||
There was a problem hiding this comment.
what if allow_null=False?
There was a problem hiding this comment.
Indeed, it should be if it's there and it's true
There was a problem hiding this comment.
As I understand, right now if you want to migrate and preserve the initial value being None, you need NullBooleanField(allow_null=True, initial=None) ? I don't think we need to change BooleanField for this.
That's the problem, NullBooleanField (NBF) is deprecated but the current implementation of BooleanField does not fully support null values. NBF will be removed from Django eventually and DRF won't be aligned.
I don't quite follow why the
default_empty_htmlchange is needed - it doesn't look likeNullBooleanFieldever had that:
Because otherwise it will be treated as False instead of None
There was a problem hiding this comment.
Sorry I meant to write:
As I understand, right now if you want to migrate and preserve the initial value being
None, you needBooleanField(allow_null=True, initial=None)? I don't think we need to changeBooleanFieldfor this.
That, is, to migrate from NullBooleanField precisely, it seems like you can use just BooleanField(allow_null=True, initial=None).
NBF will be removed from Django eventually and DRF won't be aligned.
NullBooleanField was already removed, in version 3.14: b658915#diff-7768b980611e24de782acc70ad64cd57b9d169d42f80b200cb7c6ef277fd74dbL715
Because otherwise it will be treated as False instead of None
I understnad this, but default_empty_html was never set from the default in NullBooleanField. See the above linked removal. To set it in BooleanField would actually be backwards incompatible.
There was a problem hiding this comment.
That would be the fix, the template for BooleanField did not allow to set it to None and treated it as False. This was spotted here: https://github.com/math-a3k/covid-ht/blob/master/data/serializers.py#L14
The current implementation needs those in order to behave as expected - otherwise the current implementation will treat Nones as Falses. Without this patch, this test will fail: https://github.com/encode/django-rest-framework/pull/8614/files#diff-a828467ae280cb3ae472f4c3fa7fafe22f9e672829114f1ecccc14435fa205f8R394 (initial=None is not necessary, just allow_null=True)
There was a problem hiding this comment.
we have some pointer here in this comment #7582 (comment)
- Use .get with default value for 'allow_null' kwarg in BooleanField's init
| @@ -690,6 +690,12 @@ class BooleanField(Field): | |||
| } | |||
| NULL_VALUES = {'null', 'Null', 'NULL', '', None} | |||
|
|
|||
| def __init__(self, **kwargs): | |||
| if kwargs.get('allow_null', False): | |||
There was a problem hiding this comment.
allow null false or true? can you elaborate please
There was a problem hiding this comment.
HTML checkboxes do not send any value and should be treated
as None by BooleanField if allow_null is True. from a comment of the tests in this PR
There was a problem hiding this comment.
If allow_null is true (false if not set) then set those default values, that was the problem,. If not set, It was consideres as false on booleanfield, It needs another widget to have the there values (none, true, false). As nullboolean field addesed that but It has been deprecated, the current booleanfield treats unexistant as false, the fix correcta that as show in the tests.
There was a problem hiding this comment.
I think for compatibility reason we can consider this PR. just have to carefully handle any breaking or compatibility breaking changes. may be we can just mention that on release notes later.
|
Great! There is no compatibility issues that i detected as all testsuite
pass and its a fix for an intended behaviour, im surprised anyone hasnt
stumble upon It so far
El dom., 4 dic. 2022 13:52, Asif Saif Uddin ***@***.***>
escribió:
… ***@***.**** commented on this pull request.
------------------------------
In rest_framework/fields.py
<#8614 (comment)>
:
> @@ -690,6 +690,12 @@ class BooleanField(Field):
}
NULL_VALUES = {'null', 'Null', 'NULL', '', None}
+ def __init__(self, **kwargs):
+ if kwargs.get('allow_null', False):
I think for compatibility reason we can consider this PR. just have to
carefully handle any breaking or compatibility breaking changes. may be we
can just mention that on release notes later.
—
Reply to this email directly, view it on GitHub
<#8614 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHQPBRQ32XXPUHX5YYXKPTDWLTD3FANCNFSM565ARHZQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Hi Rodrigo, if you think we should document the change here let me know or feel free to come with a PR. |
Discussed in #8269