Code

Opened 5 years ago

Closed 5 years ago

#10968 closed (fixed)

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

Reported by: matehat Owned by: lukeplant
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:

Description

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])
                else:
                    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 5 years ago.
Patch for django.forms.forms.py and regressiontests

Download all attachments as: .zip

Change History (6)

Changed 5 years ago by matehat

Patch for django.forms.forms.py and regressiontests

comment:1 Changed 5 years ago by matehat

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

comment:2 Changed 5 years ago by matehat

  • Owner changed from nobody to matehat

comment:3 Changed 5 years ago by toszter@…

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

comment:4 Changed 5 years ago by lukeplant

  • Owner changed from matehat to lukeplant
  • Status changed from new to assigned

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 5 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from assigned to closed

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

Thanks for report and initial patch, matehat.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.