Opened 6 months ago

Closed 6 months ago

Last modified 6 months ago

#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 Fields 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)

comment:1 by Natalia Bidart, 6 months ago

Resolution: needsinfo
Status: assignedclosed
Type: UncategorizedCleanup/optimization
Version: 5.0dev

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.

in reply to:  1 ; comment:2 by Christophe Henry, 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.

Last edited 6 months ago by Christophe Henry (previous) (diff)

comment:3 by Christophe Henry, 6 months ago

Resolution: needsinfo
Status: closednew

in reply to:  2 ; comment:4 by Natalia Bidart, 6 months ago

Resolution: worksforme
Status: newclosed

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.

in reply to:  4 comment:5 by Christophe Henry, 6 months ago

Replying to Natalia Bidart:

Hello, Natalia,

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.

Last edited 6 months ago by Christophe Henry (previous) (diff)

comment:6 by Christophe Henry, 6 months ago

Resolution: worksforme
Status: closednew

comment:7 by Christophe Henry, 6 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.

Last edited 6 months ago by Christophe Henry (previous) (diff)

comment:8 by Natalia Bidart, 6 months ago

Cc: David Smith added
Resolution: worksforme
Status: newclosed

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:

  1. 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 of assert self["foo_select"].help_text == self.fields["foo_select"].help_text.
  1. 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 accessing self[<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 Christophe Henry, 6 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.

Last edited 6 months ago by Christophe Henry (previous) (diff)

comment:10 by Natalia Bidart, 6 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.

Note: See TracTickets for help on using tickets.
Back to Top