#35542 closed Cleanup/optimization (worksforme)
BoundField's label and help_text and renderer should be properties not class members
Reported by: | Christophe Henry | Owned by: | Christophe Henry |
---|---|---|---|
Component: | Forms | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Christophe Henry, David Smith | Triage Stage: | Unreviewed |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
BoundField
exposes label help_text and renderer, all of them are copies of underlying members of Field
and Form
.
This is a bit misleanding since you can modify Field
s member in Form
's __init__
like documented here but any modification of help_text
, label
or renderer
after super().__init__
won't be reflected on the bound which can leand to incomprehensible behavior in templates
Change History (10)
follow-up: 2 comment:1 by , 6 months ago
Resolution: | → needsinfo |
---|---|
Status: | assigned → closed |
Type: | Uncategorized → Cleanup/optimization |
Version: | 5.0 → dev |
follow-up: 4 comment:2 by , 6 months ago
Replying to Natalia Bidart:
Hello Natalia!
Well the documentation I linked features this as the first example:
class FooMultipleChoiceForm(forms.Form): foo_select = forms.ModelMultipleChoiceField(queryset=None) def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.fields["foo_select"].queryset = ...
The same way, you can do this, for instance:
class FooMultipleChoiceForm(forms.Form): foo_select = forms.CharField() def __init__(self, user: User, *args, **kwargs): super().__init__(*args, **kwargs) self.fields["foo_select"].help_text = f"Indicate {user.first_name}'s pet name"
But if you do this, you won't get the result you expect when rendering form with {{ form }}
since what will be rendered is self["foo_select"].help_text.
(the associated BoundField
) and not self.fields["foo_select"].help_text
. In order for the help text to correctly be rendered, you need to write:
class FooMultipleChoiceForm(forms.Form): foo_select = forms.CharField() def __init__(self, user: User, *args, **kwargs): super().__init__(*args, **kwargs) # Not self.field self["foo_select"].help_text = f"Indicate {user.first_name}'s pet name"
This is confusing and is inconsistant with the queryset
example of the documentation for anyone who's not very familiar with Django's Form
API and looks very much like a bug.
My proposed solution does not introduce any breaking changes and can even be easily reverted to its original behavior with the patch I also proposed for #35192.
comment:3 by , 6 months ago
Resolution: | needsinfo |
---|---|
Status: | closed → new |
follow-up: 5 comment:4 by , 6 months ago
Resolution: | → worksforme |
---|---|
Status: | new → closed |
Replying to Christophe Henry:
Hello Christophe, thank you for the extra details. In this sentence below:
But if you do this, you won't get the result you expect when rendering form with
{{ form }}
I would need you to describe in detail what is the expected result from your view, because what I would usually expect is what I get when doing a local test.
For reference, this is my form and view, inspired by your code:
from django import forms from django.contrib import messages from django.core.exceptions import ValidationError from django.http import HttpResponseRedirect from django.shortcuts import render class FooForm(forms.Form): foo_select = forms.CharField() def __init__(self, user, *args, **kwargs): super().__init__(*args, **kwargs) self.fields["foo_select"].help_text = f"Indicate {user.first_name}'s pet name" def clean_foo_select(self): value = self.cleaned_data.get("foo_select") if value != "fido": raise ValidationError("foo_select should be fido") def foo_view(request): form = FooForm(user=request.user) if request.method == "POST": form = FooForm(user=request.user, data=request.POST) if form.is_valid(): messages.success(request, f"Valid {form.cleaned_data=}! :-)") return HttpResponseRedirect(".") messages.error(request, f"INVALID {form.data=}! :-(") return render(request, "foo.html", {"form": form})
And in my template, the correct help text is shown at all times (on GET, on form submission with errors and on form submission without errors).
Because of the above, I'll close as worksforme
, but please fell free to comment with further details. Ideally you would provide a minimal sample Django project showcasing the issue and describing what you expect to get.
Thanks again!
Natalia.
comment:5 by , 5 months ago
Replying to Natalia Bidart:
And in my template, the correct help text is shown at all times
That's because when you do self.fields["foo_select"].help_text = f"Indicate {user.first_name}'s pet name"
, at this stage, self._bound_fields_cache may not be populated. But if you do the following:
class FooForm(forms.Form): foo_select = forms.CharField() ... # Other fields def __init__(self, user, *args, **kwargs): super().__init__(*args, **kwargs) # candid dev not knowing the difference between self[] and self.fields[] # setting avariable for every field for field in self: field.should_do_something = False self.fields["foo_select"].help_text = f"Indicate {user.first_name}'s pet name" # demonstration assert self["foo_select"].help_text == self.fields["foo_select"].help_text
Please don't tell me this is not a valid situation. This happened to me on a project recently and I got a hard time tracking down why this happened.
comment:6 by , 5 months ago
Resolution: | worksforme |
---|---|
Status: | closed → new |
comment:7 by , 5 months ago
BTW, I think BoundField.html_name
, BoundField.html_initial_name
and BoundField.html_initial_id
should also be properties since they are purely computed variables from Form
. That can't hurt too.
comment:8 by , 5 months ago
Cc: | added |
---|---|
Resolution: | → worksforme |
Status: | new → closed |
Hey Christophe, thank you for the extra details. For what it's worth, I'm not saying that your issue is invalid; I fully acknowledge that it caused an error in your project and that you've invested time in finding the root cause.
However, there's another aspect to triaging a bug in Django, which involves determining whether a report pertains to a niche use case or something that would affect the majority of users. Django is a framework designed to provide robust and reliable solutions for common scenarios. It's built on a mature code base where code changes should be carefully evaluated.
To accept a ticket like this, we need two things:
- A Django minimal project showcasing the issue. I added the
assert
suggested above and it's not failing for me. My code is exactly the one posted above with the adding ofassert self["foo_select"].help_text == self.fields["foo_select"].help_text
.
- To fully describe and understand the use case for your code. Even if the
assert
would fail (which as I mentioned is not failing for me), it's unclear if this is actually a valid Django logic pattern. Could you please describe in words what the goal is in accessingself[<field name>]
as you propose? It seems unusual to access a BoundField inside the Form's__init__
(since the form hasn't been fully created yet). See the docs for Accessing the fields from the form.
I know we've mentioned this in other tickets you've created, but it's important to reiterate: Django is a community-driven project. While the Fellows oversee maintenance and ensure timely releases, most design decisions and feature changes/additions are made through community consensus. The forum is the primary place to seek that consensus, as it notifies the most interested parties (unlike here). Since I can't reproduce the issue with the provided details and this requires broader discussion, the best course of action to post in the Django Forum presenting the use case for wider feedback.
Thanks again!
comment:9 by , 5 months ago
I'm sorry but I'm tired of this back and forth, both here #35532. Merging such basic code should not be that much of a burden, espacially when I took extra care to write the tests and update the doc so that the PR is directly mergeable. In both thos tickets, I was advised to take it to the forum to achieve community consensus but what am I supposed to do when the threads I open don't get attention at all and that even debating to try to achive consensus is impossible, while in the meantime, I need bugfixes or features merged? I had this problem with this thread in which noone responded to my reply, this thread to which noone responded, and this thread in which only one user responded to oppose my proposition and then just stopped replying… How does that build a consensus?
There's definitely something wrong in the development process of Django and that won't help attracting new contributors.
comment:10 by , 5 months ago
Hello Christophe,
I understand your frustration, and I acknowledge the effort you've put into writing tests and updating documentation for your PRs. It's clear you've encountered challenges in achieving consensus on certain issues. As mentioned, Django is a community-driven project, so contributions and engagement vary based on availability and interest. To generate more engagement in proposals and discussions:
- Clearly present the use case or need for change with a practical example.
- Evaluate multiple solutions rather than focusing on a single implementation.
- Seek input on potential framework-provided solutions that may not be widely known.
Without these foundational steps, gaining attention and consensus can be challenging. Given Django's maturity and widespread adoption, each proposed change requires thorough evaluation and justification.
Specifically about this ticket, your proposed change in PR #18289 introduces a breaking change, necessitating a clear deprecation plan and strong rationale to justify it (think of those child classes that use label
or help_text
as instance variables, and how assignments to these would break when converted to properties). Furthermore, it's important to note that you have not yet provided a reproducible case or detailed use case explanation. These are essential for initiating meaningful discussions and gaining broader community support.
Hello again Christophe Henry! Thank you for your report.
We would need more information to understand your use case. The documentation you linked about "Fields which handle relationships" refers to redefining a field's queryset, not the label/help_text/renderer/etc.
With the information provided so far, I cannot determine whether this is a very specific issue arising from a niche use case or something that applies to the broader ecosystem. Django is a framework designed to provide robust and reliable solutions for common scenarios. Therefore, before accepting any changes to well-established and mature code, it is crucial to demonstrate clear benefits from the proposed modification and ensure there are no risks of breaking existing functionality.
Specifically, we would need a small Django test project or a failing test case that shows the code path or use case that needs fixing. I'll close as
needsinfo
in the meantime.