Opened 7 years ago

Closed 7 years ago

#10968 closed (fixed)

Appears like there's something missing when subclassing django.forms' ErrorList

Reported by: matehat Owned by: Luke Plant
Component: Forms Version: 1.0
Severity: Keywords:
Cc: mathieu@… Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


Django's forms feature a way to customize how errors are displayed by allowing one to subclass django.forms.util.ErrorList. However, there seems to be something missing in there to make it work completely. Let me summarize what I found :

  1. When django.forms.Form gathers cleaned data in full_clean(), it sets keyed data on its internal _errors variable like so :
        except ValidationError, e:
            self._errors[name] = e.messages
  1. That e.messages data comes from ValidationError Exception subclass :
        class ValidationError(Exception):
            def __init__(self, message):
                ValidationError can be passed any object that can be printed (usually
                a string) or a list of objects.
                if isinstance(message, list):
                    self.messages = ErrorList([smart_unicode(msg) for msg in message])
                    message = smart_unicode(message)
                    self.messages = ErrorList([message])
  1. When calling as_ul(), as_p(), etc. Form does the following :
         bf_errors = self.error_class([conditional_escape(error) for error in bf.errors]) # Escape and cache in local variable.

There, it does the right thing, i.e. wrap errors in the specified ErrorList subclass provided by the user, here available through self.error_class. As you can see though, this is not done in the ValidationError class. It just takes for granted that in should be wrapped in the original ErrorList class. Perfectly understandable since it doesn't know anything about the Form instance, nor any of its attribute. A very simple fix to this is to replace the line in (1):

    #       self._errors[name] = e.messages   (before)
             self._errors[name] = self.error_class(e.messages)

So I just attached a patch doing just that and adding some tests to make sure such customization works perfectly.

Attachments (1)

patch.diff (2.4 KB) - added by matehat 7 years ago.
Patch for and regressiontests

Download all attachments as: .zip

Change History (6)

Changed 7 years ago by matehat

Attachment: patch.diff added

Patch for and regressiontests

comment:1 Changed 7 years ago by matehat

Cc: mathieu@… added
Has patch: set
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:2 Changed 7 years ago by matehat

Owner: changed from nobody to matehat

comment:3 Changed 7 years ago by toszter@…

I second this solution after finding and reproducing the same error on my own. Nice fix.

comment:4 Changed 7 years ago by Luke Plant

Owner: changed from matehat to Luke Plant
Status: newassigned

The patch misses using error_class for the __all__ error, and the tests rely on dictionary key order. I've fixed these things, and will apply soon, thanks.

comment:5 Changed 7 years ago by Luke Plant

Resolution: fixed
Status: assignedclosed

(In [11498]) Fixed #10968 - Form.errors should use Form.error_class.

Thanks for report and initial patch, matehat.

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