Opened 6 years ago

Closed 5 years ago

#30261 closed Bug (fixed)

Calling a form method _html_output modifies the self._errors dict for NON_FIELD_ERRORS if there are hidden field with errors

Reported by: Benjamin Mampaey Owned by: Hasan Ramezani
Component: Forms Version: dev
Severity: Normal Keywords: form errors
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Each time the _html_output method of a form is called, it appends the errors of the hidden field errors to the NON_FIELD_ERRORS (all) entry.

This happen for example when the form methods as_p() as_table() as_ul() are called multiple time, or any other method that themselves call one of them.

For example, a test form with an hidden input field that add errors during the clean call.

Python 3.6.5 (default, Apr 25 2018, 14:26:36)
Type 'copyright', 'credits' or 'license' for more information
IPython 6.4.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import django

In [2]: django.__version__
Out[2]: '2.1.7'

In [3]: from django import forms
   ...:

In [4]: class TestForm(forms.Form):
   ...:     hidden_input = forms.CharField(widget=forms.HiddenInput)
   ...:
   ...:     def clean(self):
   ...:          self.add_error(None, 'Form error')
   ...:          self.add_error('hidden_input', 'Hidden input error')
   ...:

In [5]: test_form = TestForm({})

In [6]: test_form.errors
Out[6]:
{'hidden_input': ['This field is required.', 'Hidden input error'],
 '__all__': ['Form error']}

In [7]: print(test_form.as_table())
<tr><td colspan="2"><ul class="errorlist nonfield"><li>Form error</li><li>(Hidden field hidden_input) This field is required.</li><li>(Hidden field hidden_input) Hidden input error</li></ul><input type="hidden" name="hidden_input" id="id_hidden_input"></td></tr>

In [8]: test_form.errors
Out[8]:
{'hidden_input': ['This field is required.', 'Hidden input error'],
 '__all__': ['Form error', '(Hidden field hidden_input) This field is required.', '(Hidden field hidden_input) Hidden input error']}

In [9]: print(test_form.as_table())
<tr><td colspan="2"><ul class="errorlist nonfield"><li>Form error</li><li>(Hidden field hidden_input) This field is required.</li><li>(Hidden field hidden_input) Hidden input error</li><li>(Hidden field hidden_input) This field is required.</li><li>(Hidden field hidden_input) Hidden input error</li></ul><input type="hidden" name="hidden_input" id="id_hidden_input"></td></tr>

In [10]: test_form.errors
Out[10]:
{'hidden_input': ['This field is required.', 'Hidden input error'],
 '__all__': ['Form error', '(Hidden field hidden_input) This field is required.', '(Hidden field hidden_input) Hidden input error', '(Hidden field hidden_input) This field is required.', '(Hidden field hidden_input) Hidden input error']}

In [11]: test_form.non_field_errors()
Out[11]: ['Form error', '(Hidden field hidden_input) This field is required.', '(Hidden field hidden_input) Hidden input error', '(Hidden field hidden_input) This field is required.', '(Hidden field hidden_input) Hidden input error']

This bug affects probably also version 2.2.

A simple fix would be to use a copy of the error list before adding the hidden field errors in the file django/forms/forms.py:

--- forms.py	2019-03-17 18:59:04.000000000 +0100
+++ forms_fixed.py	2019-03-17 19:00:08.000000000 +0100
@@ -194,7 +194,7 @@

     def _html_output(self, normal_row, error_row, row_ender, help_text_html, errors_on_separate_row):
         "Output HTML. Used by as_table(), as_ul(), as_p()."
-        top_errors = self.non_field_errors()  # Errors that should be displayed above all fields.
+        top_errors = self.non_field_errors().copy()  # Errors that should be displayed above all fields.
         output, hidden_fields = [], []

         for name, field in self.fields.items():

Change History (5)

comment:1 by Simon Charette, 6 years ago

Triage Stage: UnreviewedAccepted
Version: 2.1master

I didn't reproduce but the report and the suggested patch make sense, accepting on that basis.

Are you interested in submitting a Github pull request incorporating your patch and a regression test?

comment:2 by Abhishek Bera, 6 years ago

Owner: changed from nobody to Abhishek Bera
Status: newassigned

comment:3 by Hasan Ramezani, 5 years ago

Has patch: set
Owner: changed from Abhishek Bera to Hasan Ramezani

comment:4 by Mariusz Felisiak, 5 years ago

Triage Stage: AcceptedReady for checkin

comment:5 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 49275c54:

Fixed #30261 -- Prevented Form._html_output() from mutating errors if hidden fields have errors.

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