[Django] #25548: Can't Form.add_error() in FormView.form_valid()
Django
---------------------------+-----------------------------------------------
Reporter: acatton | Owner: nobody
Type: | Status: new
Uncategorized |
Component: Generic | Version: master
views |
Severity: Normal | Keywords: performance FormView form_invalid
Triage Stage: | Has patch: 1
Unreviewed |
Easy pickings: 1 | UI/UX: 0
---------------------------+-----------------------------------------------
Let's consider this example:
{{{#!python
class MyView(FormView):
def form_valid(self, form):
value = form.cleaned_data['value']
try:
do_action(value)
except ValueError as e:
form.add_error(None, e.message)
return self.form_invalid(form)
return super().form_valid(form)
}}}
The error message is ignored. The main reason being that the form in
discarded in `FormView.form_invalid()`. Here's the current implementation
on master:
{{{#!python
def form_invalid(self, form):
"""
If the form is invalid, re-render the context data with the
data-filled form and errors.
"""
return self.render_to_response(self.get_context_data())
}}}
I proposed to modify this implementation to:
{{{#!python
def form_invalid(self, form):
return self.render_to_response(self.get_context_data(form=form))
}}}
since `Form.get_context_data()` is doing `kwargs.setdefault('form',
self.get_form())`. I did that on my view, but it does feel this is
something that belongs in Django.
Also, from my little understanding of the code, this also mean that if I
have a form like this:
{{{#!python
class MyForm(Form):
def clean_value(self):
value = self.cleaned_data['value']
try:
heavy_computation(value)
except ValueError as e:
raise ValidationError(e.message)
return value
}}}
The `heavy_computation()` is going run twice.
--
Ticket URL: <https://code.djangoproject.com/ticket/25548>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: acatton | Owner: nobody
Type: Uncategorized | Status: new
Component: Generic views | Version: master
Severity: Normal | Resolution:
Keywords: performance | Triage Stage:
FormView form_invalid | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by acatton):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Old description:
New description:
Let's consider this example:
{{{#!python
class MyView(FormView):
def form_valid(self, form):
value = form.cleaned_data['value']
try:
do_action(value)
except ValueError as e:
form.add_error(None, e.message)
return self.form_invalid(form)
return super().form_valid(form)
}}}
The error message is ignored. The main reason being that the form in
discarded in `FormView.form_invalid()`. Here's the current implementation
on master:
{{{#!python
def form_invalid(self, form):
"""
If the form is invalid, re-render the context data with the
data-filled form and errors.
"""
return self.render_to_response(self.get_context_data())
}}}
I propose to modify this implementation to:
{{{#!python
def form_invalid(self, form):
return self.render_to_response(self.get_context_data(form=form))
}}}
since `Form.get_context_data()` is doing `kwargs.setdefault('form',
self.get_form())`. I did that on my view, but it does feel this is
something that belongs in Django.
Also, from my little understanding of the code, this also mean that if I
have a form like this:
{{{#!python
class MyForm(Form):
def clean_value(self):
value = self.cleaned_data['value']
try:
heavy_computation(value)
except ValueError as e:
raise ValidationError(e.message)
return value
}}}
The `heavy_computation()` is going run twice.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/25548#comment:1>
Django
validation
-------------------------------------+-------------------------------------
Reporter: acatton | Owner: nobody
Type: | Status: new
Component: Generic views | Version: master
Severity: Normal | Resolution:
FormView form_invalid |
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by charettes):
* stage: Unreviewed => Accepted
* type: Uncategorized => Cleanup/optimization
* needs_tests: 0 => 1
Comment:
This makes a lot of sense.
--
Ticket URL: <https://code.djangoproject.com/ticket/25548#comment:2>
Django
validation
-------------------------------------+-------------------------------------
Reporter: acatton | Owner:
Cleanup/optimization | Status: assigned
Component: Generic views | Version: master
Severity: Normal | Resolution:
Keywords: performance | Triage Stage: Accepted
FormView form_invalid |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Changes (by Aristarhys):
* status: new => assigned
* owner: nobody => Aristarhys
--
Ticket URL: <https://code.djangoproject.com/ticket/25548#comment:3>
Django
validation
-------------------------------------+-------------------------------------
Reporter: acatton | Owner:
Cleanup/optimization |
Component: Generic views | Version: master
Severity: Normal | Resolution:
Keywords: performance | Triage Stage: Accepted
FormView form_invalid |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Changes (by Aristarhys):
* owner: Aristarhys =>
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/25548#comment:4>
Django
validation
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization |
Component: Generic views | Version: master
Severity: Normal | Resolution:
Keywords: performance | Triage Stage: Accepted
FormView form_invalid |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Changes (by dudepare):
* status: new => assigned
* owner: => dudepare
--
Ticket URL: <https://code.djangoproject.com/ticket/25548#comment:5>
Django
validation
-------------------------------------+-------------------------------------
Reporter: acatton | Owner: dudepare
Type: | Status: assigned
Cleanup/optimization |
Component: Generic views | Version: master
Severity: Normal | Resolution:
Keywords: performance | Triage Stage: Accepted
FormView form_invalid |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Comment (by alexmorozov):
@dudepare, if you don't mind, I can make a PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/25548#comment:6>
Django
validation
-------------------------------------+-------------------------------------
Reporter: acatton | Owner: dudepare
Type: | Status: assigned
Cleanup/optimization |
Component: Generic views | Version: master
Severity: Normal | Resolution:
Keywords: performance | Triage Stage: Accepted
FormView form_invalid |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Comment (by dudepare):
Sure go ahead, I'm curious to see the unittest for this.
--
Ticket URL: <https://code.djangoproject.com/ticket/25548#comment:7>
Django
validation
-------------------------------------+-------------------------------------
Reporter: acatton | Owner: dudepare
Type: | Status: assigned
Cleanup/optimization |
Component: Generic views | Version: master
Severity: Normal | Resolution:
Keywords: performance | Triage Stage: Accepted
FormView form_invalid |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Comment (by alexmorozov):
https://github.com/django/django/pull/5620
@dudepare, you mean there should be something tricky with a unittest? I
feel like I've missed something big :).
--
Ticket URL: <https://code.djangoproject.com/ticket/25548#comment:8>
Django
validation
-------------------------------------+-------------------------------------
Reporter: acatton | Owner: dudepare
Type: | Status: assigned
Cleanup/optimization |
Component: Generic views | Version: master
Severity: Normal | Resolution:
FormView form_invalid | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by charettes):
* needs_better_patch: 0 => 1
* needs_tests: 1 => 0
* stage: Accepted => Ready for checkin
Comment:
Provided patch LGTM pending a small docstring rephrasing.
--
Ticket URL: <https://code.djangoproject.com/ticket/25548#comment:9>
Django
validation
-------------------------------------+-------------------------------------
Reporter: acatton | Owner: dudepare
Type: | Status: assigned
Cleanup/optimization |
Component: Generic views | Version: master
Severity: Normal | Resolution:
Keywords: performance | Triage Stage: Ready for
FormView form_invalid | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by alexmorozov):
@charettes, fixed.
--
Ticket URL: <https://code.djangoproject.com/ticket/25548#comment:10>
Django
validation
-------------------------------------+-------------------------------------
Reporter: acatton | Owner: dudepare
Cleanup/optimization |
Component: Generic views | Version: master
Keywords: performance | Triage Stage: Ready for
FormView form_invalid | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Simon Charette <charette.s@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"e171a83b1516a3a2a393a5300527af1aab21c213" e171a83]:
{{{
#!CommitTicketReference repository=""
revision="e171a83b1516a3a2a393a5300527af1aab21c213"
Fixed #25548 -- Prevented FormView.form_invalid() from discarding its form
argument.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25548#comment:11>
Django
validation
-------------------------------------+-------------------------------------
Reporter: acatton | Owner: dudepare
Type: | Status: closed
Cleanup/optimization |
Component: Generic views | Version: master
Severity: Normal | Resolution: fixed
Keywords: performance | Triage Stage: Ready for
FormView form_invalid | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by coagulant):
Could this be backported to upcoming 1.9.1 please? This is a fix for
regression in 1.9, so I hope this makes sense )
--
Ticket URL: <https://code.djangoproject.com/ticket/25548#comment:12>
Django
validation
-------------------------------------+-------------------------------------
Reporter: acatton | Owner: dudepare
Type: | Status: closed
Cleanup/optimization |
Component: Generic views | Version: master
Severity: Normal | Resolution: fixed
Keywords: performance | Triage Stage: Ready for
FormView form_invalid | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by timgraham):
Yes, I'll backport now. I think the only reason it wasn't is that there's
no obvious indication that this is a regression (introduced in #24643).
--
Ticket URL: <https://code.djangoproject.com/ticket/25548#comment:13>
Django
validation
-------------------------------------+-------------------------------------
Reporter: acatton | Owner: dudepare
Type: | Status: closed
Cleanup/optimization |
Component: Generic views | Version: master
Severity: Normal | Resolution: fixed
Keywords: performance | Triage Stage: Ready for
FormView form_invalid | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Tim Graham <timograham@…>):
In [changeset:"0154702a987bd06d20e53fe23284b2c4a93b9063" 0154702a]:
{{{
#!CommitTicketReference repository=""
revision="0154702a987bd06d20e53fe23284b2c4a93b9063"
[1.9.x] Fixed #25548 -- Prevented FormView.form_invalid() from discarding
its form argument.
Backport of e171a83b1516a3a2a393a5300527af1aab21c213 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25548#comment:14>
Django
validation
-------------------------------------+-------------------------------------
Reporter: acatton | Owner: dudepare
Type: | Status: closed
Cleanup/optimization |
Component: Generic views | Version: master
Severity: Normal | Resolution: fixed
Keywords: performance | Triage Stage: Ready for
FormView form_invalid | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Tim Graham <timograham@…>):
In [changeset:"285b08abc121fce878dbe536b0a68b421ec4300d" 285b08ab]:
{{{
#!CommitTicketReference repository=""
revision="285b08abc121fce878dbe536b0a68b421ec4300d"
Refs #25548 -- Forwardported 1.9.1 release note.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25548#comment:15>