Decision for ticket #6362 - Remove blank spaces with strip when validating the data

[thebitguru]

Hi,

What do we need to make a decision for ticket 6362 (http://
code.djangoproject.com/ticket/6362)?

Thanks,
Farhan

[Russell Keith-Magee]

On Wed, May 13, 2009 at 7:48 AM, thebitguru <fah...@gmail.com> wrote:
>
> Hi,
>
> What do we need to make a decision for ticket 6362 (http://
> code.djangoproject.com/ticket/6362)?

We need to wait until we're not trying to get v1.1 out the door.

We are well past the feature deadline for v1.1; the focus of the
community is on fixing bugs and finalizing the v1.1 release. Once that
happens, we will be in a position to concentrate on design decisions
again.

Yours,
Russ Magee %-)

[julianb]

On May 13, 2:56 am, Russell Keith-Magee <freakboy3...@gmail.com>
wrote:

> We need to wait until we're not trying to get v1.1 out the door.

Since the ticket is one year old, that time had come and passed.

[Collin Grady]

On Wed, May 13, 2009 at 2:36 AM, julianb <julia...@gmail.com> wrote:
> Since the ticket is one year old, that time had come and passed.

The ticket's age is irrelevant. *right this minute*, everyone is
focused on 1.1, so making a design decision for this ticket is not
important right now.

Once 1.1 is out, it can be revisited. Even if you got a decision right
now, it would never have a chance of making it in to 1.1, so rushing
the decision would be pointless anyway.

--
Collin Grady

[Ryan]

Umm... How about now?  I've been bitten by trailing/leading blank spaces in form inputs several times now, and I'm tired of monkey patching django to fix this.  Thanks.

Ryan

[Cal Leeming [Simplicity Media Ltd]]

+1

> --
> You received this message because you are subscribed to the Google Groups
> "Django developers" group.
> To view this discussion on the web visit
> https://groups.google.com/d/msg/django-developers/-/qF5BK8fUsKMJ.
> To post to this group, send email to django-d...@googlegroups.com.
> To unsubscribe from this group, send email to
> django-develop...@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/django-developers?hl=en.
>

[Jacob Kaplan-Moss]

On Mon, Jul 4, 2011 at 12:13 AM, Ryan <rfu...@gmail.com> wrote:
> Umm... How about now?

Sorry, but this isn't going to happen. I left more information on the
ticket: https://code.djangoproject.com/ticket/6362#comment:43.

Jacob

[Ole Laursen]

On Jul 4, 4:52 pm, Jacob Kaplan-Moss <ja...@jacobian.org> wrote:

> On Mon, Jul 4, 2011 at 12:13 AM, Ryan <rfug...@gmail.com> wrote:
> > Umm... How about now?
>
> Sorry, but this isn't going to happen. I left more information on the
> ticket:https://code.djangoproject.com/ticket/6362#comment:43.

It sounds like you've never been bitten by strip bugs. Here's the ones
I've been through:

1. User inputs "foo " in some kind of identifier that is matched
verbatim when doing a DB query. User can now not login/find his stuff/
whatever. Or the error is catched by a check regexp and instead the
poor sod cannot create a user and don't understand why as the space is
invisible. EmailField and date/time fields used to have this bug:

https://code.djangoproject.com/log/django/trunk/django/forms/fields.py

2. User clears a field by selecting the text and hitting space bar
(why you would do that is beyond me, but some people do it). Now the
field is submitted as " " instead of "" which to Django is not blank.
Code that kicks in if the field is not blank now kicks in even though
there's no value. Something breaks.

3. User inputs "Foo " as first name, everything looks fine in HTML
because HTML strips extraneous spaces, but once you send the person a
text confirmation, you send it to "Dear Foo ," instead of "Dear Foo,".

The truth is that almost all char form fields intended for humans
should be stripped (do you think this is incorrect?), and it's easy to
forget to do it.


Ole

[Jacob Kaplan-Moss]

On Mon, Jul 4, 2011 at 10:59 AM, Ole Laursen <ol...@iola.dk> wrote:
> It sounds like you've never been bitten by strip bugs.

Then I must haven't been clear enough -- sorry -- because I certainly
have, many times. I've also found it incredibly easy to solve in my
user code: ``form.cleaned_data['field'].strip()``. Explicit is better
than implicit, remember? :)

Doesn't do anything to change my point, though: a framework can't go
about stripping user input. That's a user-code decision. If Django
strips out data I wanted, there's nothing I can do to get it back.

Jacob

[Sam Lai]

On 5 July 2011 02:20, Jacob Kaplan-Moss <ja...@jacobian.org> wrote:
> Doesn't do anything to change my point, though: a framework can't go
> about stripping user input. That's a user-code decision. If Django
> strips out data I wanted, there's nothing I can do to get it back.

I concur. The consensus seems to be shifting towards a 'strip' flag
though (defaulting to false), and I'm +1 on that. That would make it
explicit, minimise repetitive boilerplate code and also make it less
likely to accidentally forget to strip a field somewhere in the
process.

Maybe a revisit to this related ticket is in order?
https://code.djangoproject.com/ticket/4960

> Jacob

>
> --
> You received this message because you are subscribed to the Google Groups "Django developers" group.

[Andrew Ingram]

On 9 Jul 2011, at 10:44, Sam Lai wrote:

> I concur. The consensus seems to be shifting towards a 'strip' flag
> though (defaulting to false), and I'm +1 on that. That would make it
> explicit, minimise repetitive boilerplate code and also make it less
> likely to accidentally forget to strip a field somewhere in the
> process.
>
> Maybe a revisit to this related ticket is in order?
> https://code.djangoproject.com/ticket/4960

I'm on the fence here, I like the idea of a strip flag, but I do agree that it seems too specific. Perhaps something more generic, eg:

myfield = models.CharField(max_length=255, text_filters=[StripFilter, UpperFilter])

The filters would chain into each other. I'm not sure I like the syntax though, seems a bit ugly. I do like the elegance of just having a strip flag.

- Andy

[Chris Beaven]

On Sunday, July 10, 2011 12:06:34 AM UTC+12, Andrew Ingram wrote:

I'm on the fence here, I like the idea of a strip flag, but I do agree that it seems too specific. Perhaps something more generic, eg:

myfield = models.CharField(max_length=255, text_filters=[StripFilter, UpperFilter])

-1 for 'text_filters' attribute (we've got clean_[fieldname] methods for cases like this, or subclassing the field if you're in to that)

+0 for a 'strip' attribute (it's a common case)

[Chris Beaven]

To clarify, didn't even notice we were talking about models.Field, I'm +0 for a 'strip' attribute on the form's field, nothing on the model.

[Dmitry Gladkov]

I don't see what's wrong with 'strip' attribute for models.Field,
we've already non database-related 'blank' attribute, but I agree that
it does not solve the general problem.

What I'm thinking about is something like django.core.validators [1],
but called 'processors' with the only and main difference that
processor returns value which gets passed to the next processor in
chain.

I'm not sure that it plays nice with existing clean_[fieldname]
methods and that 'processor' is a good name either, but I'd like to
hear what do you think about this idea.

Thanks.

[1] https://docs.djangoproject.com/en/dev/ref/forms/validation/#using-validators

--
Best wishes,
Dmitry Gladkov, mailto:dmitry....@gmail.com

+380 91 303-37-46

On Mon, Jul 11, 2011 at 12:26 AM, Chris Beaven <smile...@gmail.com> wrote:
> To clarify, didn't even notice we were talking about models.Field, I'm +0
> for a 'strip' attribute on the form's field, nothing on the model.
>

> --
> You received this message because you are subscribed to the Google Groups
> "Django developers" group.

> To view this discussion on the web visit

> https://groups.google.com/d/msg/django-developers/-/r9DLUCsK6rUJ.

[bur...@gmail.com]

Hi Dmitry,

I think we could have combination of "validators" + "processors":

they will return either exception or cleaned value.

For example,

SomeField(cleaners=[clean_and_validate_email()])

Did you mean exactly this or rather separated SomeField(validators=[...], processors=[...]) ?

--
Best regards, Yuri V. Baburov, Skype: yuri.baburov, MSN: bu...@live.com

[Dmitry Gladkov]

Hi Yuri,

At first I thought that we should extend validators logic, but then I
realized that validation and cleaning (agree that 'cleaners' is a
better term than 'processors') are rather different tasks and mixing
them could be ambiguous, not mentioning backwards incompatibile as a
cleaner should return a value whether a validator should not.

--
Best wishes,
Dmitry Gladkov, mailto:dmitry....@gmail.com

+380 91 303-37-46

[Alex Kamedov]

Hi all!

I like patch 2 [1] for ticket #6362. It has docs and tests, but Yuri propose more interesting way.

+1 for somthing like SomeField(validators=[...], processors=[...]) where `processors` is used for normalize value to common format, and `validators` is used for validate this value.

[1] https://code.djangoproject.com/attachment/ticket/6362/6362.diff

Cheers!

Alex Kamedov

skype: kamedov    www: kamedov.ru

[Simon Charette]

How is that supposed to interact with the `cleaning` mechanism of the field?

[Luke Plant]

On 10/07/11 22:26, Chris Beaven wrote:
> To clarify, didn't even notice we were talking about models.Field, I'm
> +0 for a 'strip' attribute on the form's field, nothing on the model.

Like Chris, I don't think we can put this feature anywhere on model
definition. It is clearly an issue of how a form should be processed,
not an issue of what data can exist in the model. strip=True means "when
receiving input from user, strip leading/trailing whitespace" - and
something similar for any 'text_filter' attribute. That really cannot
belong on a field definition.

With that said, the next option because an option on form fields. This
isn't particularly attractive to me, because for ModelForm it isn't
going to be automatically applied (for the reasons Jacob has given), and
now you need some fairly hacky or non-DRY way to specify it.

My solution to date has been this form mixin that need it:

class StripStringsMixin(object):
def clean(self):
for field,value in self.cleaned_data.items():
if isinstance(value, basestring):
self.cleaned_data[field] = value.strip()
return self.cleaned_data

This mixin is easy to use - just add it to a form's base class. This is
even still easy to use in the context of the admin. Although this may
not be perfect, it's probably the least of all the various evils.

Luke

--
A former CEO: "Some of you think that only half of the Board of
Directors do all the work and the rest do nothing, actually the
reverse is true." (True Quotes From Induhviduals, Scott Adams)

Luke Plant || http://lukeplant.me.uk/

[Tom Evans]

That would strip whitespace from every field. I suppose it could be
specialized from the meta class..

Specializing automatically generated fields in model forms is pretty
common when you want to specialize their behaviour, for example if you
wish to use a different widget for a field, or if you want to provide
a custom label.

I don't see why this is considered hacky or anti-DRY.

Eg:

class FooForm(forms.ModelForm, StripWhiteSpaceMixin):
class Meta:
strip_whitespace_fields = ( 'username', )
model = Foo

or

class FooForm(forms.ModelForm):
username = forms.CharField(strip_whitespace=True)
class Meta:
model = Foo

So much discussion over one tiny form field normalization.

Cheers

Tom

[Luke Plant]

On 18/07/11 14:33, Tom Evans wrote:

> I don't see why this is considered hacky or anti-DRY.
>
> Eg:
>
> class FooForm(forms.ModelForm, StripWhiteSpaceMixin):
> class Meta:
> strip_whitespace_fields = ( 'username', )
> model = Foo

This one would be OK, I hadn't thought about that.

I guess I would be +0 for the 'strip_whitespace_fields' attribute and
functionality added to the Form class, so that it could be used with any
Form or ModelForm. Unfortunately we don't yet have 'Meta' on Form, only
on ModelForm. I think this could be fixed - we would probably need a
'FormOptions' class, and the existing 'ModelFormOptions' should inherit
from that. It might require some thought to get it to work correctly for
the case of multiple inheritance.

> class FooForm(forms.ModelForm):
> username = forms.CharField(strip_whitespace=True)
> class Meta:
> model = Foo

This is not very DRY because the 'CharField' is duplicating something
that can be deduced from the model (and there might be other attributes
too, like max_length). Doing this correctly for a model with lots of
fields would be tedious.

Luke

--
"Alcohol and calculus don't mix: never drink and derive."

Luke Plant || http://lukeplant.me.uk/