Code

Opened 7 years ago

Closed 5 years ago

#6138 closed (fixed)

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

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

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 7 years ago.
A simple patch
error_class.diff (1.9 KB) - added by AzMoo 6 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 peter2108 5 years ago.
error_class2.diff (1.5 KB) - added by peter2108 5 years ago.
error_class_and_tests.diff (2.6 KB) - added by peter2108 5 years ago.
Patch and tests as a single diff

Download all attachments as: .zip

Change History (23)

Changed 7 years ago by Michal Moroz

A simple patch

comment:1 Changed 7 years ago by Italomaia

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 Changed 6 years ago by AzMoo

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

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

comment:3 Changed 6 years ago by mtredinnick

  • Resolution fixed deleted
  • Status changed from closed to reopened

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 Changed 6 years ago by AzMoo

  • 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 Changed 6 years ago by ubernostrum

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 Changed 6 years ago by jacob

  • Triage Stage changed from Unreviewed to Accepted

comment:7 Changed 6 years ago by Leo

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

comment:8 Changed 6 years ago by issya

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 Changed 6 years ago by adunar

  • Cc adunar added
  • milestone set to post-1.0

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

Changed 6 years ago by AzMoo

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

comment:10 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:11 Changed 5 years ago by gz

  • Cc gz added

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

comment:12 Changed 5 years ago by peter2108

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

comment:13 Changed 5 years ago by peter2108

  • Cc peter2108 added

comment:14 Changed 5 years ago by kmtracey

  • 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 follow-up: Changed 5 years ago by Alex

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

comment:16 Changed 5 years ago by peter2108

  • Needs tests unset
  • Owner changed from nobody to peter2108
  • Patch needs improvement unset
  • Status changed from reopened to new

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

Changed 5 years ago by peter2108

Changed 5 years ago by peter2108

comment:17 in reply to: ↑ 15 Changed 5 years ago by peter2108

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.

Changed 5 years ago by peter2108

Patch and tests as a single diff

comment:18 Changed 5 years ago by peter2108

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

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.