[Django] #20867: Allow Form.clean() to target specific fields when raising ValidationError.
Django
-----------------------------+--------------------
Reporter: loic84 | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+--------------------
The goal would be to unlock the following API:
{{{
def clean(self):
errors = {}
if condition1:
errors['field1'] = ValidationError('message1', code='code')
if condition2:
errors['field2'] = ValidationError('message2', code='code')
raise ValidationError(errors)
}}}
Most of the groundwork has been done for #20199.
We would need to document passing a `dict` to `ValidationError`, just like
we already document passing a `list` in
[https://docs.djangoproject.com/en/dev/ref/forms/validation/#raising-
validationerror ref/forms/validation/#raising-validationerror].
This would probably deprecate or reduce the need for the pattern discussed
in [https://docs.djangoproject.com/en/dev/ref/forms/validation/#form-
subclasses-and-modifying-field-errors ref/forms/validation/#form-
subclasses-and-modifying-field-errors].
Refs #20199 #16986.
--
Ticket URL: <https://code.djangoproject.com/ticket/20867>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: loic84 | Owner: loic84
Type: New feature | Status: assigned
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by loic84):
* status: new => assigned
* needs_better_patch: => 1
* needs_tests: => 0
* owner: nobody => loic84
* needs_docs: => 1
* has_patch: 0 => 1
Comment:
PR https://github.com/django/django/pull/1443
--
Ticket URL: <https://code.djangoproject.com/ticket/20867#comment:1>
Django
Reporter: loic84 | Owner: loic84
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Changes (by timo):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/20867#comment:2>
Django
Reporter: loic84 | Owner: loic84
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by loic84):
* needs_better_patch: 1 => 0
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/20867#comment:3>
Django
Reporter: loic84 | Owner: loic84
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by loic84):
Quite simultaneously yet in different contexts @mjtamlyn and @akaariai had
the idea of having a `Form` method to manipulate the `_error` dict.
@akaariai on IRC:
{{{
(07:56:59 AM) akaariai: I am reading
https://docs.djangoproject.com/en/dev/ref/forms/validation/#form-
subclasses-and-modifying-field-errors and wondering why this must be so
complicated? Why not Form.add_error(field, message)?
(07:59:26 AM) jtiai: akaariai: Maybe because no one bothered to figure out
API for that?
(07:59:53 AM) timograham: akaariai: WIP by loic84 is
https://github.com/django/django/pull/1443
(08:04:04 AM) akaariai: timograham: hmmh, seems to me that having
self.add_error instead of documenting self._errors would be an improvement
even if ValidationError(errors_dict) went in.
(08:05:19 AM) akaariai: there are some cases where you want to add errors
post-clean, and just getting rid of that self._errors docs seems nice.
(08:06:08 AM) akaariai: compare that to add_error(field, error_message):
Adds an error to specified field. If field is None, then adds a
non_field_error.
}}}
@mjtamlyn talking about a private utility method that I used in the PR
above:
{{{
[...] It could even be made public - so this is a shortcut for the old way
of doing things, with the clever ValidationError exceptions being a
further improvement.
}}}
As a result I've cleaned up my utility method and replaced it with
`Form.add_errors(self, field, errors)`.
Now the question is what do we do in term of public API, the current patch
documents raising `ValidationError` with a dictionary argument. We could
also document `add_errors()`, or we could only document `add_errors()`.
One good thing in having a public `add_errors()` is that it would be
accessible from outside the form. Many time I wished I could add some
errors to a form instance from a view without having to fiddle with
`_errors`.
--
Ticket URL: <https://code.djangoproject.com/ticket/20867#comment:4>
Django
Reporter: loic84 | Owner: loic84
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by mjtamlyn):
This got me wondering a bit, is there any need for `ValidationError` to
support dictionaries if we have `add_errors`? The original aim was to make
`clean()` methods nicer when affecting multiple fields.
--
Ticket URL: <https://code.djangoproject.com/ticket/20867#comment:5>
Django
Reporter: loic84 | Owner: loic84
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by loic84):
We need good support for dict because that's how errors travel from the
Model layer to the Form layer. Also ValidationError error is the vehicle
to carry metadata like error code and error params which are lost as soon
as they reach ErrorDict. That said, we don't need to advertise it.
--
Ticket URL: <https://code.djangoproject.com/ticket/20867#comment:6>
Django
Reporter: loic84 | Owner: loic84
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by mjtamlyn):
Yup, of course. I think it's worth documenting both personally.
--
Ticket URL: <https://code.djangoproject.com/ticket/20867#comment:7>
Django
Reporter: loic84 | Owner: loic84
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by mjtamlyn):
Any resolution to this will not also include the `add_errors()`
functionality, which fixes #5335. I've closed that ticket as its topic has
been subsumed into this one.
--
Ticket URL: <https://code.djangoproject.com/ticket/20867#comment:8>
Django
Reporter: loic84 | Owner: loic84
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by guettli):
* cc: hv@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20867#comment:9>
Django
Reporter: loic84 | Owner: loic84
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by loic84):
I've updated [https://github.com/django/django/pull/1443 the PR].
I went with the `add_error(field, error)` API (note the singular) which
seems to be the most popular from the feedback I've received on IRC and on
the ML thread https://groups.google.com/d/topic/django-
developers/rTbfg3JtLkA/discussion.
--
Ticket URL: <https://code.djangoproject.com/ticket/20867#comment:10>
Django
Reporter: loic84 | Owner: loic84
Component: Forms | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Loic Bistuer <loic.bistuer@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"f563c339ca2eed81706ab17726c79a6f00d7c553"]:
{{{
#!CommitTicketReference repository=""
revision="f563c339ca2eed81706ab17726c79a6f00d7c553"
Fixed #20867 -- Added the Form.add_error() method.
Refs #20199 #16986.
Thanks @akaariai, @bmispelon, @mjtamlyn, @timgraham for the reviews.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20867#comment:11>
Django
Reporter: loic84 | Owner: loic84
Type: New feature | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Loic Bistuer <loic.bistuer@…>):
In [changeset:"2e3c7d882015375c130c21884d83cb9fb7759d94"]:
{{{
#!CommitTicketReference repository=""
revision="2e3c7d882015375c130c21884d83cb9fb7759d94"
Trigger AttributeError in ValidationError.message_dict when error_dict is
missing.
The goal of this change is twofold; firstly, matching the behavior of
Django 1.6
and secondly, an AttributeError is more informative than an obscure
ValueError
about mismatching sequence lengths.
Refs #20867.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20867#comment:12>