Inconsistent empty value for EmailField(blank=True, null=True) due to strip after check for empty value

[Olav Morken]

Hi,

I just noticed that if I enter a single space in an EmailField in a ModelForm where the backing model field is EmailField(blank=True, null=True), the value is stored as an empty string. On the other hand, if I don't enter anything in the field, it is stored as a None value.

Looking at the code (​https://github.com/django/django/blob/1.11.4/django/forms/fields.py#L234-L241), I think the cause is that it first checks for empty values, and then strips the string.

To reproduce this behavior, try something like:

# models.py
from django.db import models

class SomeUser(models.Model):
    address = models.EmailField(null=True, blank=True)


# views.py
import pprint
from django import forms
from django.http import HttpResponse
from django.views.decorators.csrf import csrf_exempt
from .models import SomeUser

class SomeUserForm(forms.ModelForm):
    class Meta:
        fields = [ 'address' ]
        model = SomeUser

@csrf_exempt
def test_view(request):
    form = SomeUserForm(request.POST)
    some_user = form.save()
    return HttpResponse(pprint.pformat(some_user.address) + "\n")

Testing this with curl gives something like this:

$ curl -XPOST -d address= http://127.0.0.1:1253/test
None
curl -XPOST -d address=%20 http://127.0.0.1:1253/test
u''

[Josh Schneier]

​PR

[Tim Martin]

The change looks good to me, but it should be squashed into a single commit.

[Tim Graham]

Thanks for reviewing. Squashing commits can be done easily enough when merging so you don't need to check "Patch needs improvement" for that.

[Josh Schneier]

Replying to Tim Martin:

The change looks good to me, but it should be squashed into a single commit.

In this case I intentionally did not rebase down because of the additional fix/patch I added. I'm not sure if it's necessary and wanted to leave the history for the reviewer to help with that determination.

Thanks for the review!

[Tim Martin]

I think the non_string unit test makes it pretty clear why you reworked it the way you did. At least, I was able to follow without looking through all the patches.

[Tim Graham <timograham@…>]

In 48c394a6:

Fixed #28555 -- Made CharField convert whitespace-only values to the empty_value when strip is enabled.

[Jon Dufresne]

I was wondering if this fix could be eligible for a backport to 1.11.x. It fixes a bug in the new features of:

​https://docs.djangoproject.com/en/dev/releases/1.11/#forms

The new CharField.empty_value attribute allows specifying the Python value to use to represent “empty”.

​https://docs.djangoproject.com/en/dev/releases/1.11/#miscellaneous

In model forms, CharField with null=True now saves NULL for blank values instead of empty strings.

The ​documentation says the following about fixing bugs in newly introduced features:

Major functionality bugs in new features of the latest stable release.

[Tim Graham <timograham@…>]

In 1d1a56c:

[1.11.x] Fixed #28555 -- Made CharField convert whitespace-only values to the empty_value when strip is enabled.

Backport of 48c394a6fc2594891f766293afec8f86d63e1015 from master

[Tim Graham <timograham@…>]

In 3d2c3905:

Refs #28555 -- Forwardported 1.11.6 release note.