Opened 6 years ago

Closed 5 years ago

#13511 closed Cleanup/optimization (fixed)

RegexValidator pattern is optional and should be required

Reported by: David Fischer Owned by: David Fischer
Component: Core (Other) Version: master
Severity: Normal Keywords: validators, regexvalidator
Cc: djfische@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX:


While working on ticket #13341, I noticed that the code for RegexValidator could use a slight cleanup. Specifically, the first parameter of RegexValidator -- the regex pattern -- is optional. However, if it is not provided, a TypeError is thrown because the default value of None is passed to re.compile. The regex pattern should be required.

Attachments (2)

13511-regexvalidator.diff (1.6 KB) - added by David Fischer 6 years ago.
switched regex parameter to be required
13511-regexvalidator2.diff (2.0 KB) - added by David Fischer 5 years ago.
Patch leaves "regex" as optional, but documents and tests functionality

Download all attachments as: .zip

Change History (11)

comment:1 Changed 6 years ago by Russell Keith-Magee

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

comment:2 Changed 6 years ago by David Fischer

Cc: djfische@… added
Owner: changed from nobody to David Fischer

Changed 6 years ago by David Fischer

Attachment: 13511-regexvalidator.diff added

switched regex parameter to be required

comment:3 Changed 6 years ago by David Fischer

Has patch: set
Status: newassigned

I switched the regex parameter to required to match the documentation. While running the unit tests (forms & validation), I noticed that URLValidator relied on the fact that the regex parameter was optional by having a regex instance variable. I corrected that. However, this brings up a potential backwards compatibility issue.

The issue with this patch is that anybody who subclassed RegexValidator and relied on regex to be optional will have to update their code. However, it should be pretty obvious what went wrong and they probably should not have done that in the first place. I'm not sure how strict to be on maintaining backwards compatibility, but I think that breaking it in this small way will result in the cleanest code.

comment:4 Changed 6 years ago by Rolando Espinoza La fuente

I just face this bug when subclassing RegexValidator. I think using self.regex instead regex as re.compile's parameter works and keeps backward compatilibity. Though, using RegexValidator without regex parameter will work in the same way that re.compile("") works.

  • django/core/

    diff --git a/django/core/ b/django/core/
    a b  
    2929            self.code = code
    3131        if isinstance(self.regex, basestring):
    32             self.regex = re.compile(regex)
     32            self.regex = re.compile(self.regex)
    3434    def __call__(self, value):
    3535        """

comment:5 Changed 6 years ago by David Fischer


That looks like it will work and it is better than what I had attached. It passes the unit test suite (validators) and it allows you to subclass RegexValidator. As you mentioned, creating a RegexValidator without specifying the "regex" will function the same as if the empty string was passed (a RegexValidator that always matches). I think that is fine, but maybe it needs to be communicated in the docs. The docs currently specify that "regex" is not optional. It can either be left like it is because a RegexValidator that matches everything is pretty useless, but perhaps the docs should just say that by default it matches everything.

comment:6 Changed 5 years ago by Julien Phalip

Severity: Normal
Type: Cleanup/optimization

comment:7 Changed 5 years ago by Julien Phalip

Needs tests: set

Changed 5 years ago by David Fischer

Attachment: 13511-regexvalidator2.diff added

Patch leaves "regex" as optional, but documents and tests functionality

comment:8 Changed 5 years ago by David Fischer

Easy pickings: unset

I added a patch based on darkrho's suggestion. It will not break backward compatibility with subclasses of RegexValidator. I added a note in the docs that if the user does not specify any regex, then RegexValidator will match any passed string.

comment:9 Changed 5 years ago by Chris Beaven

Resolution: fixed
Status: assignedclosed

In [16351]:

Fixes #13511 -- make regex parameter to RegexValidator to be optional. Also tidies up related docs (parameters aren't attributes). Thanks for the patch work, davidfischer.

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