Opened 9 years ago

Closed 6 years ago

#6092 closed (wontfix)

URL and email fields need to allow custom validators

Reported by: Jacob Owned by: floguy
Component: Core (Other) Version: master
Severity: Keywords:
Cc: joel@…, listuser@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

See #3989 and #6019 -- in both cases, people want to define "email" or "URL" a bit differently than Django does by default. In both cases, changing the default isn't the answer since that'll just make new people unhappy with the new defaults.

So, I should be able to do something like this:

    rfc2822pattern = re.compile(...)

    # ...

    email = models.EmailField(pattern=rfc2822pattern)

That's just a strawman proposal, of course -- I'm not sure I like that syntax.

Attachments (4)

6092.diff (6.2 KB) - added by floguy 9 years ago.
Initial patch to add pattern keyword argument to several model and form fields.
6092-docs.diff (3.0 KB) - added by floguy 9 years ago.
Adds documentation to my previous patch.
6092.2.diff (4.8 KB) - added by floguy 9 years ago.
Updated patch to be more idiomatic Python and to require a compiled regular expression object.
6092-docs.2.diff (2.9 KB) - added by floguy 9 years ago.
Updated documentation to match the new patch.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 9 years ago by Pierre THIERRY <nowhere.man@…>

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

Note that parsing an email address with a regex may be considered an abuse of the regex system, and people previous complained that being able to parse all valid email addresses would need an unbearbly complex and hard to debug regex. Email addresses are a full language, and there's a reason they are defined in ABNF. They should be parsed with a real parser, IMHO.

comment:2 Changed 9 years ago by floguy

Has patch: set
Needs documentation: set
Owner: changed from nobody to floguy
Status: newassigned

I have created an initial patch for this, which makes models.EmailField, models.URLField, (new)forms.EmailField, (new)forms.URLField all take a pattern keyword argument.

This pattern kwarg can be either a string or a compiled regular expression.

There are also regression tests for the newforms modifications.

Changed 9 years ago by floguy

Attachment: 6092.diff added

Initial patch to add pattern keyword argument to several model and form fields.

Changed 9 years ago by floguy

Attachment: 6092-docs.diff added

Adds documentation to my previous patch.

comment:3 Changed 9 years ago by floguy

Needs documentation: unset

comment:4 Changed 9 years ago by Simon Greenhill <dev@…>

Triage Stage: UnreviewedReady for checkin

comment:5 Changed 9 years ago by Malcolm Tredinnick

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

There are a few problems with the code patch that need work here.

  1. The get() method on a dictionary returns None by default; no need to specify it.
  2. Don't write == None or != None. Instead, use is and is not to compare with None, since there's only one None object in Python. However, this is moot here anyway, since you might as well just check for "truthiness" -- if pattern: ... -- rather than all the None checks. It's slightly more idiomatic which means people won't be caught wondering why we aren't using the obvious construct.
  3. Let's simplify the internals a bit by saying that people should pass in a reg-exp object here (so the parameter should probably be called something other than "pattern"). Then you can just use duck-typing assumptions and you don't need to check the type or anything. Simply call .match() on the passed in object when we need it. That will remove a bunch of if-blocks. Bear in mind that this is for a bit of a corner case, so it's not going to be a huge burden to ask them to call re.compile once first.

I don't see a huge advantage in allowing both reg-exp objects *and* strings and by choosing the reg-exp approach, we allow the user to pass in anything that has a match() method, since we only use that, which could be something more complex than a reg-exp if they really wanted it.

Changed 9 years ago by floguy

Attachment: 6092.2.diff added

Updated patch to be more idiomatic Python and to require a compiled regular expression object.

Changed 9 years ago by floguy

Attachment: 6092-docs.2.diff added

Updated documentation to match the new patch.

comment:6 Changed 8 years ago by Joel Watts

Cc: joel@… added

comment:7 Changed 7 years ago by nix

Cc: listuser@… added

Any update on the status of this patch?

comment:8 Changed 7 years ago by Adam Nelson

#11551 has a simple workaround that addresses this. The patch was declined but it can be used if necessary.

comment:9 Changed 6 years ago by Mark Lavin

This ticket hasn't seen much attention lately and I question how relevant this ticket is given the changes in the 1.2 release allowing pluggable validators. http://docs.djangoproject.com/en/1.2/ref/validators/

Both the default email and url validation logic can be overridden with a sub-class of the appropriate field which I would argue as more DRY than a new kwarg. An example of the email field overrides (without the actual regex) is given below:

import re
from django import forms
from django.core.validators EmailValidator
from django.db import models
from django.utils.translation import ugettext_lazy as _

rfc2822pattern = re.compile(...)
validate_email = EmailValidator(rfc2822pattern, _(u'Enter a valid e-mail address.'), 'invalid')

class RFC2822EmailFormField(forms.EmailField)
    default_validators = [validate_email]


class RFC2822EmailField(models.EmailField)
    default_validators = [validate_email]

    def formfield(self, **kwargs):
        defaults = {
            'form_class': RFC2822EmailFormField,
        }
        defaults.update(kwargs)
        return super(RFC2822EmailField, self).formfield(**defaults)

comment:10 Changed 6 years ago by Adam Nelson

Resolution: wontfix
Status: assignedclosed

I agree with mlavin. I think validators make this feature unnecessary.

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