Opened 16 years ago

Closed 15 years ago

#6138 closed (fixed)

newforms: when accessing directly form.errors, error_class is not used

Reported by: Michal Moroz Owned by: Peter Mott
Component: Forms Version: dev
Severity: Keywords:
Cc: michalmoroz@…, Jesse Young, gz, Peter Mott Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Tested with SVN Django, rev 6897.

Short guide for reproducing the problem:

  • write custom form class and ErrorList-based class
  • make instance of the form which defined error_class as our ErrorList-based class and some non-valid data
  • watch customform.errors and customform['customfield'].errors, they are instances of ErrorList, not our error_class

The long guide (small app for reproducing the problem):
urls.py (within project dtest)

from django.conf.urls.defaults import *

urlpatterns = patterns('',
    url(r'^form/$', 'dtest.newforms.views.microform', {'form':'form.html'}),
    url(r'^form2/$', 'dtest.newforms.views.microform', {'form':'form2.html'}),
)

newforms/forms.py

from django.utils.encoding import force_unicode
from django import newforms as forms

attrs_dict = { 'class': 'required' }

class ErrList(forms.util.ErrorList):
    def __unicode__(self):
        return 'errors: %s' % ''.join([unicode(e) for e in self])

class MicroForm(forms.Form):
    username = forms.CharField(max_length=30,
                               widget=forms.TextInput(attrs=attrs_dict),
                               label=u'username')
    def clean(self):
        if self.cleaned_data.get('username', '') is not 'blah':
            raise forms.ValidationError('we have a problem here')

newforms/views.py

from django.http import HttpResponse
from django.shortcuts import render_to_response
from django.template import RequestContext

from dtest.newforms.forms import ErrList, MicroForm

def microform(request, form='form.html'):
    if request.method == 'POST':
        m = MicroForm(request.POST, error_class=ErrList)
        if m.is_valid():
            return HttpResponse('OK')
    else:
        m = MicroForm()

    return render_to_response(form,
        { 'form': m },
        context_instance=RequestContext(request))

templates/form.html

<form action="" method="POST">
<dl>
{% for field in form %}
    <dt>{{ field.label_tag }}</dt>
    <dd {% if field.errors %}class="errorfield" {% endif %}>{{ field }}</dd>
    {% if field.help_text %}<dd>{{ field.help_text }}</dd>{% endif %}
    {% if field.errors %}<dd class="errors">{{ field.errors }}</dd>{% endif %}
{% endfor %}
</dl>
<div class="submit"><input id="submit" type="submit" value="Sign up" />
</form>

templates/form2.html

<form action="" method="POST">
<ul>
{{ form.as_ul }}
</ul>
<div class="submit"><input id="submit" type="submit" value="Sign up" />
  • When accessing form/, the ErrList does not work.
  • When accessing form2/, the to_ul() (and _html_output() too) method works with ErrList, but non_field_errors() do not.

A simple patch is included below. It fixes the problem, but I do not guarantee working with the rest of the code in all cases.

Attachments (5)

svn.diff (878 bytes ) - added by Michal Moroz 16 years ago.
A simple patch
error_class.diff (1.9 KB ) - added by Matt Magin 15 years ago.
Updated version - applies error_class to non_field_errors as well as the boundfield errors.
error_class2_test.py (1.2 KB ) - added by Peter Mott 15 years ago.
error_class2.diff (1.5 KB ) - added by Peter Mott 15 years ago.
error_class_and_tests.diff (2.6 KB ) - added by Peter Mott 15 years ago.
Patch and tests as a single diff

Download all attachments as: .zip

Change History (23)

by Michal Moroz, 16 years ago

Attachment: svn.diff added

A simple patch

comment:1 by Italomaia, 16 years ago

The redered errorlist is comming escaped! Shouldn't it come unnescaped? This patch outputs escaped text. Django docs per example, uses <div> in the
returned error message. With unnescaped, that example wouldn't work.

comment:2 by Matt Magin, 16 years ago

Resolution: fixed
Status: newclosed

I've added a patch for this that makes non_field_errors honour error_class to match the behaviour of bound fields.

comment:3 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: closedreopened

This hasn't been fixed yet. Please read contributing.txt to see what the various stages of a ticket are, but "fixed" happens only when something is committed to the repository.

comment:4 by Matt Magin, 16 years ago

Patch needs improvement: set

Sorry!

I've realized that this patch only fixes the helper functions, not accessing the errors directly. Obviously to be complete it needs to do this.

comment:5 by James Bennett, 16 years ago

What Malcolm meant is that "fixed" doesn't happen until a Django developer actually checks the code into the repository; just posting a patch doesn't do that, and any patch posted still needs to be reviewed by a Django developer.

comment:6 by Jacob, 16 years ago

Triage Stage: UnreviewedAccepted

comment:7 by Leo Shklovskii, 16 years ago

the bug is still here in 1,0,alfa version

comment:8 by issya, 16 years ago

I can confirm that this is still a problem. If I try to access field.errors, the custom error list does not work. If I just print my form out with {{ form }}, the custom field.errors work.

comment:9 by Jesse Young, 16 years ago

Cc: Jesse Young added
milestone: post-1.0

+1; it's still a bug in 1.0 and the current svn revision (9232)... is this patch waiting on anything?

by Matt Magin, 15 years ago

Attachment: error_class.diff added

Updated version - applies error_class to non_field_errors as well as the boundfield errors.

comment:10 by (none), 15 years ago

milestone: post-1.0

Milestone post-1.0 deleted

comment:11 by gz, 15 years ago

Cc: gz added

Is there anything we can do to help getting this fixed?

comment:12 by Peter Mott, 15 years ago

It's still a bug in 1.1 rc1 11,332 so I guess no-one cares.

comment:13 by Peter Mott, 15 years ago

Cc: Peter Mott added

comment:14 by Karen Tracey, 15 years ago

Needs tests: set

If the latest patch fixes the reason that "patch needs improvement" was checked then it should be unchecked. Also, I don't see any tests nor any reason why tests aren't possible/appropriate.

comment:15 by Alex Gaynor, 15 years ago

Please put the tests in the same diff as the patch itself, it is possible to use svn add locally.

comment:16 by Peter Mott, 15 years ago

Needs tests: unset
Owner: changed from nobody to Peter Mott
Patch needs improvement: unset
Status: reopenednew

Forms have an optional parameter 'error_class' which defaults to ErrorList.
ErrorList subclasses 'list'. It holds a list of Form errors and has methods to
present its errors in various formats, typically an HTML <ul>. A custom
error_class might change the HTML display of Form errors. Forms add errors
by trapping a ValidationError

ValidationError.messages is an ErrorList instance. Present code just adds
this ErrorList to the Form's errors. Instead we initialise an error_class
instance with ErrorList.messages and add this instance to Form's errors.

error_class is an optional parameter for BaseForm, BaseFormset and BaseModelForm.
Changes are only needed for the first two.

ValidationError is trapped at:

1. forms/fields.py:805 
2. forms/forms.py:245 
3. forms/forms.py:252 
4. forms/formsets.py:254    

Changes are made for 2-4. Any issues there might be with 1. are not addressed.

I have included a group of tests. To run these using the constant
'NON_FIELD_ERRORS' instead of its present value I needed to change the _ _all_ _
list of Forms module.

The changes proposed here are those of AzMoo 12/14/08 except for the change to
_html_output which does not appear to be necessary.

Finally if documentation is added for 'error_class' it would probably be worth
warning error_messages for fields are not strings. I included some tests
to illustrate this

The changes are in error_class2.diff, test for the patch in error_class2_test.py

by Peter Mott, 15 years ago

Attachment: error_class2_test.py added

by Peter Mott, 15 years ago

Attachment: error_class2.diff added

in reply to:  15 comment:17 by Peter Mott, 15 years ago

Replying to Alex:

Please put the tests in the same diff as the patch itself, it is possible to use svn add locally.

I'm afraid I don't understand this. Can you explain or point me to an example patch.

by Peter Mott, 15 years ago

Attachment: error_class_and_tests.diff added

Patch and tests as a single diff

comment:18 by Peter Mott, 15 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top