#10427 closed (fixed)
Bound field needs an easy way to get form value
Reported by: | Ludvig Ericson | Owned by: | Chris Beaven |
---|---|---|---|
Component: | Forms | Version: | dev |
Severity: | Keywords: | form field value | |
Cc: | michal@…, gabor@…, viksit@…, paulschreiber@…, kmike84@…, proppy@…, matt@…, martin.paquette@…, wsartori@…, postal2600@…, dirleyrls@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
When you want to render your own forms in templates, without any sort of widgetery, you bump into an issue: it is impossible to get the actual value of a field in a succinct and clean way.
Looking at Django's code for figuring this out, it clearly is a non-trivial task, and one you can easily get wrong.
So, to remedy this, I suggest adding a value
attribute to BoundField
, which simply returns that exact value. This could be used in templates like so:
<input type="text" name="{{ form.username.name }}" value="{{ form.username.value }}">
I for one find this both intuitive and readable.
Attachments (9)
Change History (47)
by , 16 years ago
Attachment: | django-forms-value.2.diff added |
---|
by , 16 years ago
Attachment: | django-forms-value.4.diff added |
---|
Use empty string even if value is from initial data
comment:1 by , 16 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
This already exists, it's just spelled {{ form.field.data }}
`.
comment:2 by , 16 years ago
Resolution: | invalid |
---|---|
Status: | closed → reopened |
I'm going to take the liberty of reopening this ticket, because they're not the same.
f.value
takes initial data into account (both on form level and field level), as well as data. It only uses data when the form is a bound form.
follow-up: 5 comment:3 by , 16 years ago
Yeah, this doesn't already exist. All the initial computations can't be simulated in a form. I thought this would have been a dupe of something else, though, since I've asked at least three people on the mailing list to open a ticket when they've asked about it. Apparently "will to fix" doesn't match "will to complain". Thanks for going the next step, Mike (but, geez, dude... how many attempts does it take to get the patch correct?)
comment:4 by , 16 years ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
The patch itself looks ok, although I'd get rid of the property. A method will do fine, since it indicates in Python code that there's some computation going on and still looks the same in a template.
Needs documentation, too.
comment:5 by , 16 years ago
Replying to mtredinnick:
Yeah, this doesn't already exist. All the initial computations can't be simulated in a form. I thought this would have been a dupe of something else, though, since I've asked at least three people on the mailing list to open a ticket when they've asked about it. Apparently "will to fix" doesn't match "will to complain". Thanks for going the next step, Mike (but, geez, dude... how many attempts does it take to get the patch correct?)
It might be useful to note that I'm Ludvig Ericson, or lericson
. (And would love for my Trac & Subversion username to change.)
It took some attempts because I was a bit quick on the draw, and I started noticing issues after starting to use it.
Replying to mtredinnick:
The patch itself looks ok, although I'd get rid of the property. A method will do fine, since it indicates in Python code that there's some computation going on and still looks the same in a template.
Needs documentation, too.
Regarding it being a property, I chose that to be consistent with the data property. I imagine it won't only be used from templates either, and my_form['field'].value()
looks weird.
follow-up: 7 comment:6 by , 16 years ago
Sorry about the name mixup, that was just me not concentrating. You choose your username; live with the decision.
Calling a function using parentheses doesn't look at all weird. Normal code doesn't need to access the data attribute (yes, it should also have been a method) on a BoundField
, so that's not particular relevant in the public API.
Anyway, add documentation and I, or somebody, will bikeshed the rest at commit time.
comment:7 by , 16 years ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | unset |
I'm very sorry about attaching this massive amount of patches, but I figured using "replace existing" when there are already renamed patches would get confusing.
Replying to mtredinnick:
Calling a function using parentheses doesn't look at all weird. Normal code doesn't need to access the data attribute (yes, it should also have been a method) on a
BoundField
, so that's not particular relevant in the public API.
Yeah, sure, but then it should really be a verb/verb+noun, and not a noun. Like "get_value". I don't see the big win in adding asymmetry and yet not gaining anything; it could just as well be an actual instance attribute (and probably should be?)
Bikeshed away, however.
comment:8 by , 16 years ago
Cc: | added |
---|
comment:9 by , 16 years ago
Two issues here:
- Why doesn't it take "initial" attribute into account?
- With ChoiceField the db value is returned, not the display one, which is not useful within templates.
by , 16 years ago
Attachment: | django-display_value-r10755.patch added |
---|
with "display_value' method added
comment:10 by , 16 years ago
Replying to emes:
- Why doesn't it take "initial" attribute into account?
Ok, we're in bound form. Sorry for this question ;)
- With ChoiceField the db value is returned, not the display one, which is not useful within templates.
And here's the patch which does it, by adding "display_value" method which looks up choices for the matching description.
comment:11 by , 15 years ago
Cc: | added |
---|
comment:13 by , 15 years ago
I've started using this patch - it's very useful (Thanks!). One thing I'm finding is that it display_value does not return a useful string in the case where the form field is a TypedChoiceField that is in a form that is a ModelForm. For example, if I have a model like this:
class Task(models.Model): PRIORITY_CHOICES = ( (1, _('1 (Critical)')), (2, _('2 (High)')), (3, _('3 (Normal)')), ) priority = models.IntegerField(choices=PRIORITY_CHOICES, blank=True, null=True, help_text=_('1 = Highest Priority, 5 = Low Priority'))
and the form is a ModelForm, like this:
class TaskForm(forms.ModelForm): class Meta: model = Task
When I get the display_value for the form field, I get
<django.utils.functional.__proxy__ object at 0x2a9b486f90>
Here's some pdb output that shows the problem:
(Pdb) type(bf) <class 'django.forms.forms.BoundField'> (Pdb) print bf <select name="priority" id="id_priority"> <option value="">---------</option> <option value="1">1 (Critical)</option> <option value="2">2 (High)</option> <option value="3">3 (Normal)</option> <option value="4" selected="selected">4 (Low)</option> <option value="5">5 (Very Low)</option> </select> (Pdb) bf.display_value <django.utils.functional.__proxy__ object at 0x2a9b486f90> (Pdb)
I tried replicating this by creating a ChoiceField in my form, and in that case I don't get the same behavior, so this is somehow related to it being a ModelForm. For example, if in my TaskForm I define priority like this:
priority = forms.ChoiceField(choices=PRIORITY_CHOICES)
In that case display_value returns the expected string:
(Pdb) bf.display_value '4 (low)'
I'm familiar with a lot of the django source, but it's not clear to me what this proxy is or why it is there, so I just wanted to report the issue, since I'm sure the intended behvior is not to have display_value return the proxy string.
Anyway, aside from this small problem, I think this is very nice refactoring of the code.
Margie
comment:14 by , 15 years ago
display_value returns a transalatable string because you have it marked for translation(the _ which are ugettext_lazy calls). To convert it to a string (which also translates it), use force_unicode from django.utils.encoding.
comment:15 by , 15 years ago
Ah ... I see! Sorry, my mistake. You know, I have been ignorant of all those little '_' floating around my code which I don't even need. Thanks for clarifying Alex. I have read up on the whole internationalization scheme and your response makes sense, of course. So my report above and assessment of the problem was obviously wrong - so developers, just ignore it. Sorry for the extra noise.
Margie
comment:16 by , 15 years ago
This would be very handy for me having spent hours trying and failing to do related things.
comment:17 by , 15 years ago
I'm using this patch and it's working well for me (in fact, I was in the process of re-inventing this fix for myself when I found this ticket). Just one comment: with the display_value
method available, one obvious place to use it would be in the preview template used by contrib.formtools.preview.FormPreview. It would be a simple one-line change: instead of:
field.data
it would say:
field.display_value
I'm attaching a patch against the current trunk (r11298) that's identical to the r10755 patch except that it adds the above one-line change. I realize that most users probably won't directly use the formtools templates as-is, but I expect many will look at them for guidance (I know I did), so it seems to me that they should reflect the "best practice".
by , 15 years ago
Attachment: | django-display_value-r11298.patch added |
---|
adds one-line change to formtools/preview.html template
comment:18 by , 15 years ago
Alternatively, you can use the following template filter until this patch is added to Django core.
@register.filter(name='field_value') def field_value(field): value = field.form.initial[field.name] if not value: return '' if isinstance(field.field, ChoiceField): value = str(value) for (val, desc) in field.field.choices: if val == value: return desc return value
Usage:
{{ form.field|field_value }} {{{ }}}
comment:19 by , 15 years ago
As taken from the last patch, I think if your going to do this with a template tag it should be as follows;
@register.filter(name='field_value') def field_value(field): """ Returns the value for this BoundField, as rendered in widgets. """ if not field.form.is_bound: val = field.form.initial.get(field.name, field.field.initial) if callable(val): val = val() else: if isinstance(field.field, FileField) and field.data is None: val = field.form.initial.get(field.name, field.field.initial) else: val = field.data if val is None: val = '' return val @register.filter(name='display_value') def display_value(field): """ Returns the displayed value for this BoundField, as rendered in widgets. """ value = field.value if isinstance(field.field, ChoiceField): for (val, desc) in field.field.choices: if val == value: return desc return field.value display_value = property(_display_value)
The one above produced errors for me.
comment:20 by , 15 years ago
Sorry... correction!
@register.filter(name='field_value') def field_value(field): """ Returns the value for this BoundField, as rendered in widgets. """ if not field.form.is_bound: val = field.form.initial.get(field.name, field.field.initial) if callable(val): val = val() else: if isinstance(field.field, FileField) and field.data is None: val = field.form.initial.get(field.name, field.field.initial) else: val = field.data if val is None: val = '' return val @register.filter(name='display_value') def display_value(field): """ Returns the displayed value for this BoundField, as rendered in widgets. """ value = field.value if isinstance(field.field, ChoiceField): for (val, desc) in field.field.choices: if val == value: return desc return field.value
follow-up: 22 comment:21 by , 15 years ago
Cc: | added |
---|
comment:22 by , 15 years ago
Replying to paulschreiber:
In display_value, I think
value = field.value
should be
value = field_value(field)
field.value would make this whole ticket pointless :)
comment:23 by , 15 years ago
To update the filters by zanuxzan:
from django.forms import ChoiceField from django import template register = template.Library() @register.filter(name='field_value') def field_value(field): """ Returns the value for this BoundField, as rendered in widgets. """ if field.form.is_bound: if isinstance(field.field, FileField) and field.data is None: val = field.form.initial.get(field.name, field.field.initial) else: val = field.data else: val = field.form.initial.get(field.name, field.field.initial) if callable(val): val = val() if val is None: val = '' return val @register.filter(name='display_value') def display_value(field): """ Returns the displayed value for this BoundField, as rendered in widgets. """ value = field_value(field) if isinstance(field.field, ChoiceField): for (val, desc) in field.field.choices: if val == value: return desc return value
comment:24 by , 15 years ago
I had an issue with this, causing this error:
TemplateSyntaxError at /first/admin
Caught an exception while rendering: global name 'FileField' is not defined
importing 'FileField' at the top fixed it
from django.forms import ChoiceField, FileField from django import template register = template.Library() @register.filter(name='field_value') def field_value(field): """ Returns the value for this BoundField, as rendered in widgets. """ if field.form.is_bound: if isinstance(field.field, FileField) and field.data is None: val = field.form.initial.get(field.name, field.field.initial) else: val = field.data else: val = field.form.initial.get(field.name, field.field.initial) if callable(val): val = val() if val is None: val = '' return val @register.filter(name='display_value') def display_value(field): """ Returns the displayed value for this BoundField, as rendered in widgets. """ value = field_value(field) if isinstance(field.field, ChoiceField): for (val, desc) in field.field.choices: if val == value: return desc return value
comment:25 by , 15 years ago
Cc: | added |
---|
comment:26 by , 14 years ago
Cc: | added |
---|
comment:27 by , 14 years ago
Cc: | added |
---|
comment:28 by , 14 years ago
Cc: | added |
---|
follow-up: 30 comment:29 by , 14 years ago
Why isn't this merged into trunk yet? It's been accepted for over a year; Django 1.2 was just released. People are clearly interested in this. What's the holdup? Sure core committers limited time etc. but for the love of god, a year?
comment:30 by , 14 years ago
Cc: | added |
---|
Replying to toxik:
Why isn't this merged into trunk yet? It's been accepted for over a year; Django 1.2 was just released. People are clearly interested in this. What's the holdup? Sure core committers limited time etc. but for the love of god, a year?
I second this!
comment:31 by , 14 years ago
Cc: | added |
---|
comment:32 by , 14 years ago
Cc: | added |
---|---|
milestone: | → 1.3 |
comment:33 by , 14 years ago
Owner: | changed from | to
---|---|
Status: | reopened → new |
I'm looking to commit at least an abstraction of the values generation in the BoundField.as_widget
method.
Here's my branch comparison: https://github.com/SmileyChris/django/compare/master...10427-BoundField.value
comment:34 by , 14 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Use empty string instead of None