Opened 13 years ago
Last modified 12 years ago
#16306 new Bug
Form field documentation documents optional keyword arguments as field attributes.
Reported by: | ShawnMilo | Owned by: | nobody |
---|---|---|---|
Component: | Forms | Version: | dev |
Severity: | Normal | Keywords: | form field documentation |
Cc: | bmispelon@… | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | yes |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
I've tested this for forms.IntegerField and forms.DecimalField -- setting a max_value when the field is declared works normally, but if set in init (because it's unknown till runtime) no validators are assigned although the max_value value is properly set.
from django import forms class TestForm(forms.Form): amount = forms.IntegerField(max_value = 10) class TestForm2(forms.Form): amount = forms.IntegerField() def __init__(self, *args, **kwargs): super(TestForm2, self).__init__(*args, **kwargs) self.fields['amount'].max_value = 10 x = TestForm({'amount': 12}) print x.is_valid() #returns False print x.fields['amount'].max_value # returns 10 print x.fields['amount'].validators # [<django.core.validators.MaxValueValidator object at 0x7febd5240610>] print x.errors #complains about size x = TestForm2({'amount': 12}) print x.is_valid() #returns True print x.fields['amount'].max_value #returns 10 print x.fields['amount'].validators # [] print x.errors #nothing
Evidently this bug has existed for some time, because almost a year ago it was discussed on StackOverflow and the suggestion was to just completely replace the field at runtime:
http://stackoverflow.com/questions/3470741/django-forms-integerfield-set-max-value-on-runtime
Attachments (1)
Change History (8)
comment:1 by , 13 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
comment:2 by , 13 years ago
Component: | Forms → Documentation |
---|---|
Easy pickings: | set |
Keywords: | form field documentation added |
Needs documentation: | set |
Needs tests: | set |
Resolution: | invalid |
Severity: | Release blocker → Normal |
Status: | closed → reopened |
Summary: | Validators not set for form field when max_value set in __init__ → Form field documentation documents optional keyword arguments as field attributes. |
Triage Stage: | Unreviewed → Accepted |
Version: | 1.3 → SVN |
Meh. Reread the documentation and although the words say "optional arguments" the examples are indeed attributes. I'm marking this as a documentation bug. I think that a feature request would be valid, but that's a separate issue.
comment:3 by , 13 years ago
Component: | Documentation → Forms |
---|---|
Triage Stage: | Accepted → Design decision needed |
Also, a comment about using one of the following workarounds:
- Replace the field completely in the form's init.
- Add the field in the form's init (don't have it in the original definition).
Both of these would cause confusion for the maintenance developer: Either a field is appearing 'by magic' or it's not as defined in the form originally. Neither case is desirable.
by , 13 years ago
Attachment: | 16306.diff added |
---|
comment:5 by , 12 years ago
Status: | reopened → new |
---|
comment:6 by , 12 years ago
Triage Stage: | Design decision needed → Accepted |
---|
comment:7 by , 12 years ago
Cc: | added |
---|
I took a closer look at this ticket and investigated several approaches (including the proposed patch)
I came up with 3 approaches on this ticket (the 3rd one being SmileyChris's one), but all of them have some issue:
1) move the creation of validators from __init__()
to run_validators()
This approach is the less intrusive one.
It works quite well but breaks some tests in the test suite, which shows there are some potential backwards-compatibility issues.
The root of these issues is code which expects the field instance'svalidators
attribute to be populated before the field has been actually validated [1].
There's also two possibilities with this approach, the difference being whether we append the validators insiderun_validators
(in which case aMinValueValidator
would be placed after any custom one for example), or whether we prepend them (this is more compatible with the existing behavior).
2) Use lazy values
This sort of works, but there are two problems:
- The validators need to be changed because they can now be passed a None value to validate against.
- The error messages are broken because
"%d" % lazy_int
does not work (and as far as I know, that's not something that can be made to work).
3) descriptors (proposed patch)
This approach is quite elegant on paper, and while the patch looks good, I think the proposed solution can't work because of some new features that were added since the patch was proposed.
It's a bit tricky to explain why, but let me try.
Let's consider what happens when creating an
IntegerField
with amax_value
argument:
IntegerField(max_value=42)
callsIntegerField.__init__
- The
__init__
needs to do two things: setself.min_value
to 42, and callField.__init__
. - Setting
self.min_value = 42
callsValidatorAttribute.__set__
ValidatorAttribute.__set__
requires the field'svalidators
attributes (a list of validators) to exist.- This attribute is set in
Field.__init__
- This means that
IntegerField.__init__
needs to callField.__init__
before settingself.min_value
- However ...
Field.__init__
callsself.widget_attrs()
, which allows the field to pass custom attributes to the widget.IntegerField.widget_attrs
checks the value ofself.min_value
...- ... which hasn't been set for the reasons stated above.
It might be possible to add some more descriptors to make the whole thing work.
For example, I tried makingField.widget
into a property that would only callwidget_attrs
when being accessed.
This however failed with a weird error message and it started getting hard to trace the problem with all the different layers of descriptors.
All things considered, I'm not sure that adding all these additional layers is worth it.
I think approach 1 is the best one, even at the cost of minor backwards-compatibility.
Field.validators
is not really documented and the documentation has no examples
of directly manipulating it.
Additionally, other fields have attributes which could use a similar fix:
- CharField.max_length
- charField.min_length
- DecimalField.max_digits
- DecimalField.decimal_places
- RegexField.regex (current solution is based on @property)
[1] https://github.com/django/django/blob/master/tests/forms_tests/tests/forms.py#L822
max_value and min_value are documented as arguments to the __init__ method of the DecimalField and IntegerField. They are not documented as attributes on the field instances. The don't even seem to be used internally, so I'm not really sure why they even get set. In any case, this is not a bug, and there are ways to change the field at runtime already in place.
If you want to request this as a new feature, reopen the ticket.