Opened 8 years ago

Closed 8 years ago

#26821 closed Bug (fixed)

EmailField/URLField.clean(None) crashes

Reported by: VINAY KUMAR SHARMA Owned by: Priy Werry
Component: Forms Version: dev
Severity: Normal Keywords: error, form fields, strip
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (13)

comment:1 by Claude Paroz, 8 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Simon Charette, 8 years ago

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.

comment:3 by Priy Werry, 8 years ago

Owner: changed from nobody to Priy Werry
Status: newassigned

comment:4 by Markus Holtermann, 8 years ago

Severity: Release blockerNormal

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.

comment:5 by Priy Werry, 8 years ago

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.

Edit: It seems a real space (not %20) is invalid for URLs, therefore the same fix is applied to the URLField (deprecation of Strip parameter)

Last edited 8 years ago by Priy Werry (previous) (diff)

comment:6 by Tim Graham, 8 years ago

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?

in reply to:  6 comment:7 by Priy Werry, 8 years ago

Replying to 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.

comment:8 by Tim Graham, 8 years ago

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.

in reply to:  8 comment:9 by Priy Werry, 8 years ago

Replying to 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.

Version 0, edited 8 years ago by Priy Werry (next)

comment:10 by Tim Graham, 8 years ago

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.

Last edited 8 years ago by Tim Graham (previous) (diff)

in reply to:  10 comment:11 by Priy Werry, 8 years ago

Replying to 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::.

The overall diff is quite clean, although the commits are a bit polluted. If a rebase with some clearer messages are necessary please tell me :)

Last edited 8 years ago by Priy Werry (previous) (diff)

comment:12 by Tim Graham, 8 years ago

Has patch: set
Summary: 'NoneType' object has no attribute 'strip'EmailField/URLField.clean(None) crashes
Triage Stage: AcceptedReady for checkin

comment:13 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: assignedclosed

In a7b5dfd1:

Fixed #26821 -- Fixed forms.Email/URLField crash on None value.

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