[Django] #20199: Allow ModelForm to override error messages defined in model fields.
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
-----------------------------+--------------------
Currently error messages generated by model fields bypass error messages
defined in `ModelForm` fields and leak to the end user.
This issue was brough up on the ML: https://groups.google.com/d/msg
/django-developers/KlmdlR59vuc/e1HQpAwMYhoJ
After discussion with carljm on IRC, we came to the conclusion that
allowing such override would be an improvement.
--
Ticket URL: <https://code.djangoproject.com/ticket/20199>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: loic84 | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/20199#comment:1>
Django
Reporter: loic84 | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by loic84):
* has_patch: 0 => 1
Comment:
To address this issue we need to bubble up error codes and error params
which are currently lost before they reach the `ModelForm`.
The fact that we don't keep track of `ValidationError` instances is
actually a broader problem than this `ModelForm` issue.
I've attempted a refactor of `ValidationError` that aims to address that.
It's a tricky issue because of backward compatibility, but the proposed
solution should remain compatible. It keeps track of the individual
instances of `ValidationError` while keeping the previous API of
`ValidationError.message_dict` and `ValidationError.messages`.
The Django test suite currently pass without errors.
https://github.com/loic/django/compare/ticket20199
--
Ticket URL: <https://code.djangoproject.com/ticket/20199#comment:2>
Django
Reporter: loic84 | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bmispelon):
* cc: bmispelon@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20199#comment:3>
Django
Reporter: loic84 | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by loic84):
The refactor of `ValidationError` is more or less complete.
I've also made the necessary changes to `ModelForm` to enable overriding
messages when provided with sufficient information.
Currently only `ValidationError` from model validators can take advantage
of the new system because these are the only exceptions where an error
code is provided and where the message params are not coerced into the
final message.
Most other validation errors are raised using the following pattern:
{{{
raise ValidationError(self.error_messages['null'])
}}}
In order to override such messages we need to know their code, so these
would have to be rewritten as follows:
{{{
raise ValidationError(self.error_messages['null'], code='null')
}}}
I think we should take advantage of this refactor to also make params
available to people who rewrite error messages.
Given the following error message:
{{{
'invalid_choice': _('Value %r is not a valid choice.')
}}}
Currently raised as follows:
{{{
msg = self.error_messages['invalid_choice'] % value
raise ValidationError(msg)
}}}
We should write:
{{{
raise ValidationError(self.error_messages['invalid_choice'],
code='invalid_choice', params={'choice': value})
}}}
There is a catch, to do that, we need to rewrite the messages to use
mapping keys instead of positional formatting. I believe it's a good move,
as not everyone want to display values in their messages and that wouldn't
be possible with positional formatting.
It's a big task to update every occurrence of `ValidationError`, so I
would like confirmation by a core dev that this would be accepted before I
start.
--
Ticket URL: <https://code.djangoproject.com/ticket/20199#comment:4>
Django
Reporter: loic84 | Owner: loic84
Type: New feature | Status: assigned
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by loic84):
* status: new => assigned
* owner: nobody => loic84
--
Ticket URL: <https://code.djangoproject.com/ticket/20199#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 claudep):
Replying to [comment:4 loic84]:
> There is a catch, to do that, we need to rewrite the messages to use
mapping keys instead of positional formatting. I believe it's a good move,
as not everyone want to display values in their messages and that wouldn't
be possible with positional formatting.
I'm all in favour of this change. Could you check the patch on #17840 and
tell me if it goes in the direction you suggest?
--
Ticket URL: <https://code.djangoproject.com/ticket/20199#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 loic84):
Replying to [comment:6 claudep]:
> I'm all in favour of this change. Could you check the patch on #17840
and tell me if it goes in the direction you suggest?
It is in the right direction, but these are just a subset of the error
messages that need editing. Most of the others live in
`django.db.models.fields.__init__.py`.
The changes to `default_error_messages` is exactly what needs to happen
throughout the system, but the part that raises the exception would have
to be rewritten to provide the error code and error params to the
`ValidationError` constructor.
So maybe this ticket should take over?
--
Ticket URL: <https://code.djangoproject.com/ticket/20199#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 loic84):
PR https://github.com/django/django/pull/1019
--
Ticket URL: <https://code.djangoproject.com/ticket/20199#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 timo):
* cc: timograham@… (added)
Comment:
Updated PR to merge cleanly: https://github.com/django/django/pull/1247
--
Ticket URL: <https://code.djangoproject.com/ticket/20199#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 jezdez):
-0
--
Ticket URL: <https://code.djangoproject.com/ticket/20199#comment:10>
Django
Reporter: loic84 | Owner: loic84
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by jezdez):
* needs_better_patch: 0 => 1
* needs_docs: 0 => 1
Comment:
Thinking about it a bit more, I'm still not thrilled this change in
behavior would be introduced but can't come up with a better approach than
storing stuff in the `ValidationError`. This definitely needs a hands-on
explanation in the docs how to use this in the case where you want to
override an unwanted error message from a 3rd party app form.
--
Ticket URL: <https://code.djangoproject.com/ticket/20199#comment:11>
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 timo):
* needs_better_patch: 1 => 0
* needs_docs: 1 => 0
Comment:
Updated pull request with documentation. Looks good to me, but wouldn't
mind one more set of eyes before it gets committed.
--
Ticket URL: <https://code.djangoproject.com/ticket/20199#comment:12>
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 Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"ee77d4b25360a9fc050c32769a334fd69a011a63"]:
{{{
#!CommitTicketReference repository=""
revision="ee77d4b25360a9fc050c32769a334fd69a011a63"
Fixed #20199 -- Allow ModelForm fields to override error_messages from
model fields
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20199#comment:13>
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:"71093d22b62548fc2667468f1ae9e28f4fef30db"]:
{{{
#!CommitTicketReference repository=""
revision="71093d22b62548fc2667468f1ae9e28f4fef30db"
Fixed #16986 -- Model.clean() can report errors on individual fields.
This commit provides the tests for this issue but the actual problem was
solved
by the ValidationError refactor in f34cfec and ee77d4b.
Refs #20199.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20199#comment:14>
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:"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/20199#comment:15>
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 ispivey@…):
The changes in the initial refactor commit appear to have caused a
regression for us when upgrading to 1.6.
If you look at the unit test that was changed in tests/validators/tests.py
in
https://github.com/django/django/commit/f34cfec0fa1243b4a3b9865b961a2360f211f0d8
, it used to be possible to create a ValidatonError by passing a dict of
string values like
{{{ ValidationError({ 'field_name': 'error' }) }}}
instead of a dict of list values like
{{{ ValidationError({'field_name': ['error',]}) }}}.
Now, however, if you pass a string instead of a list, ValidationError
treats the string as a list and thus returns an error dictionary like:
{{{
{
"first": [
"F",
"i",
"r",
"s",
"t",
" ",
"P",
"r",
"o",
"b",
"l",
"e",
"m"
]
}
}}}
As a result, some of our error messages now have a lot of commas in them.
In reading the docs in more detail, I realize we weren't following the
best practice, which is to directly set {{{self._errors}}} instead of
passing a dictionary to the {{{ValidationError}}} constructor. So we'll go
fix our code, but I wanted to let Loic know in case others were possibly
depending on this old behavior.
--
Ticket URL: <https://code.djangoproject.com/ticket/20199#comment:16>
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 loic84):
I remember discussing this with a core dev on IRC at the time, if I
remember well, we concluded that this actually was a mistake, as it only
worked as the consequence of `reduce(operator.add, message.values())` in
conjunction with strings being iterables, and that it doesn't work well
with more than one error message; for instance `{'first': 'First Problem',
'second': 'Second Problem'}` would lead to a `ValidationError.message`
attribute of `'Second ProblemFirst Problem'`. Also the
`ValidationError(dict)` construct is a private API, it was never
documented.
FWIW [f563c339] in Django 1.7 further tightened the `ValidationError`
constructor, and `{'first': 'First Problem', 'second': 'Second Problem'}`
would work properly there. That commit also introduces the
`Form.add_error()` API which may solve your original problem of adding
error to a specific field.
--
Ticket URL: <https://code.djangoproject.com/ticket/20199#comment:17>
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 ispivey@…):
Makes sense. After looking over the docs, we guessed we'd fallen into
using a private API. Thanks for the response and the heads-up re: 1.7
changes!
--
Ticket URL: <https://code.djangoproject.com/ticket/20199#comment:18>
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 Tim Graham <timograham@…>):
In [changeset:"8847a0c601e4261823b1726b2db73eec2ac17940"]:
{{{
#!CommitTicketReference repository=""
revision="8847a0c601e4261823b1726b2db73eec2ac17940"
Fixed #16192 -- Made unique error messages in ModelForm customizable.
Overriding the error messages now works for both unique fields,
unique_together
and unique_for_date.
This patch changed the overriding logic to allow customizing
NON_FIELD_ERRORS
since previously only fields' errors were customizable.
Refs #20199.
Thanks leahculver for the suggestion.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20199#comment:19>