[Django] #26821: 'NoneType' object has no attribute 'strip'
Django
---------------------------------+--------------------------------------
Reporter: vinaykrsharma | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Release blocker | Keywords: error,form fields, strip
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+--------------------------------------
Getting this error on admin form save.
Reason, have not any None check at:
[https://github.com/django/django/blob/master/django/forms/fields.py#L700]
Must should be check:
{{{
value = self.to_python(value)
if value:
value = value.strip()
}}}
Error Report:
{{{
...
Django Version: 1.11.dev20160629191130
Exception Location: /opt/django-trunk/django/forms/fields.py in clean,
line 700
Python Version: 3.5.1
...
}}}
The below commit caused the error:
[https://github.com/django/django/commit/267dc4adddd2882182f71a7f285a06b1d4b15af0
#diff-d7e069df501fd643847061cbdb376670]
Before that at line 230 it was returning empty string, but after it is
returning None.
--
Ticket URL: <https://code.djangoproject.com/ticket/26821>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: vinaykrsharma | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Keywords: error,form fields, | Triage Stage: Accepted
strip |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* needs_better_patch: => 0
* stage: Unreviewed => Accepted
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/26821#comment:1>
Django
Reporter: vinaykrsharma | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Keywords: error,form fields, | Triage Stage: Accepted
strip |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by charettes):
It looks like both `EmailField` and `URLField` are affected. I suggest we
make them set their `CharField.strip` option to `True` instead of
adjusting both their `clean()` method in order to prevent code
duplication.
--
Ticket URL: <https://code.djangoproject.com/ticket/26821#comment:2>
Django
Reporter: vinaykrsharma | Owner: PriyWerry
Type: Bug | Status: assigned
Component: Forms | Version: master
Severity: Release blocker | Resolution:
Keywords: error,form fields, | Triage Stage: Accepted
strip |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by PriyWerry):
* status: new => assigned
* owner: nobody => PriyWerry
--
Ticket URL: <https://code.djangoproject.com/ticket/26821#comment:3>
Django
Reporter: vinaykrsharma | Owner: PriyWerry
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: error,form fields, | Triage Stage: Accepted
strip |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by MarkusH):
* severity: Release blocker => Normal
Comment:
The handling worked fine for 1.6 to 1.10 for the reason that CharFields
won't be saved with NULL values when supplied through model forms. Given
the change in 1.11 on how to handle empty values this is a regression in
1.11 but is not a release blocker IMO.
--
Ticket URL: <https://code.djangoproject.com/ticket/26821#comment:4>
Django
Reporter: vinaykrsharma | Owner: PriyWerry
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: error,form fields, | Triage Stage: Accepted
strip |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by PriyWerry):
PR opened at https://github.com/django/django/pull/6876. After a
discussion with Markus and Erik, it seems that the URLField should be able
to process trailing spaces. Therefore, the deprecation warning and forcing
of strip=True is only applied to the EmailField.
--
Ticket URL: <https://code.djangoproject.com/ticket/26821#comment:5>
Django
Reporter: vinaykrsharma | Owner: PriyWerry
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: error,form fields, | Triage Stage: Accepted
strip |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by MarkusH):
Since `http://example.com/ ` (trailing space) is not consider a valid URL
the URLValidator would need to be updated.
--
Ticket URL: <https://code.djangoproject.com/ticket/26821#comment:6>
Django
Reporter: vinaykrsharma | Owner: PriyWerry
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: error,form fields, | Triage Stage: Accepted
strip |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
I don't understand the reason for a deprecation here. Passing
`strip=False` doesn't have an effect on these fields anyway, does it? The
value is always stripped regardless, isn't it?
--
Ticket URL: <https://code.djangoproject.com/ticket/26821#comment:6>
Django
Reporter: vinaykrsharma | Owner: PriyWerry
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: error,form fields, | Triage Stage: Accepted
strip |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by PriyWerry):
Replying to [comment:6 timgraham]:
> I don't understand the reason for a deprecation here. Passing
`strip=False` doesn't have an effect on these fields anyway, does it? The
value is always stripped regardless, isn't it?
The idea was to use a simple mechanism without having to pop the keyword
arg strip from kwargs.
This is for example also done in django.db.models.aggregates for the Count
class. If someone would execute the following command, an error will be
thrown:
{{{
>>> Count(some_field, output_field=FloatField())
TypeError: __init__() got multiple values for keyword argument
'output_field'
}}}
I think (and I discussed this with Erk as well) that we also want this for
EmailFields and URLFields. If we keep silently popping the strip argument,
developers can be confused when passing {{{strip=False}}}, because it
still behaves as if {{{strip=True}}}.
--
Ticket URL: <https://code.djangoproject.com/ticket/26821#comment:7>
Django
Reporter: vinaykrsharma | Owner: PriyWerry
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: error,form fields, | Triage Stage: Accepted
strip |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
I don't see a problem with raising an error right away (without a
deprecation period). Since the argument doesn't have any effect in
released versions of Django, it's not a big deal for projects to simply
remove it.
--
Ticket URL: <https://code.djangoproject.com/ticket/26821#comment:8>
Django
Reporter: vinaykrsharma | Owner: PriyWerry
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: error,form fields, | Triage Stage: Accepted
strip |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by PriyWerry):
Replying to [comment:8 timgraham]:
> I don't see a problem with raising an error right away (without a
deprecation period). Since the argument doesn't have any effect in
released versions of Django, it's not a big deal for projects to simply
remove it.
Ok, we can remove the {{{pop()}}} and raising the deprecation warning, but
projects can still break when updating as the {{{strip}}} parameter has
been present for a long time.
--
Ticket URL: <https://code.djangoproject.com/ticket/26821#comment:9>
Django
Reporter: vinaykrsharma | Owner: PriyWerry
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: error,form fields, | Triage Stage: Accepted
strip |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Yes, this will be a backwards-incompatible change, but I don't think it
should affect many users -- `strip` was only added in 1.9 and specifying
`strip` doesn't have any effect, so there's no reason for a developer to
do it except if they don't realize that. I think this course of action is
less confusing than a deprecation path which changes the behavior in the
meantime and makes `strip=False` have an effect.
--
Ticket URL: <https://code.djangoproject.com/ticket/26821#comment:10>
Django
Reporter: vinaykrsharma | Owner: PriyWerry
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: error,form fields, | Triage Stage: Accepted
strip |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by PriyWerry):
Replying to [comment:10 timgraham]:
> Yes, this will be a backwards-incompatible change in the sense that
developers may have to remove useless `strip` arguments, but I don't think
it should affect many users -- `strip` was only added in 1.9 and
specifying `strip` doesn't have any effect, so there's no reason for a
developer to do it except if they don't realize that. I think this course
of action is less confusing than a deprecation path which changes the
behavior in the meantime and makes `strip=False` have an effect.
Sounds good to me! I've updated the pull-request to reflect your
suggestion. The deprecation warning and mentioning in the docs are removed
and replaced by a ..versionadd::.
--
Ticket URL: <https://code.djangoproject.com/ticket/26821#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: vinaykrsharma | Owner: PriyWerry
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: error,form fields, | Triage Stage: Ready for
strip | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* has_patch: 0 => 1
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/26821#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: vinaykrsharma | Owner: PriyWerry
Component: Forms | Version: master
Severity: Normal | Resolution: fixed
Keywords: error,form fields, | Triage Stage: Ready for
strip | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"a7b5dfd1703a8fbed70b7aca5e6dda092af51154" a7b5dfd1]:
{{{
#!CommitTicketReference repository=""
revision="a7b5dfd1703a8fbed70b7aca5e6dda092af51154"
Fixed #26821 -- Fixed forms.Email/URLField crash on None value.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26821#comment:13>