EmailField/URLField.clean(None) crashes

[VINAY KUMAR SHARMA]

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.

[Simon Charette]

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.

[Markus Holtermann]

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.

[Priy Werry]

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)

[Tim Graham]

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?

[Priy Werry]

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.

[Tim Graham]

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.

[Priy Werry]

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 (api change).

[Tim Graham]

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.

[Priy Werry]

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 :)

[Tim Graham <timograham@…>]

In a7b5dfd1:

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