Opened 7 years ago
Closed 6 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 , 7 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|---|
| Version: | 2.1 → master |
comment:2 by , 7 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
comment:3 by , 6 years ago
| Has patch: | set |
|---|---|
| Owner: | changed from to |
comment:4 by , 6 years ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
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?