Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#32855 closed Bug (fixed)

BoundWidget.id_for_label ignores id set by ChoiceWidget.options

Reported by: Jacob Rief Owned by: Jacob Rief
Component: Forms Version: 3.2
Severity: Normal Keywords: auto_id, id_for_label
Cc: Dmytro Litvinov Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

If you look at the implementation of BoundField.subwidgets

class BoundField:
    ...
    def subwidgets(self):
        id_ = self.field.widget.attrs.get('id') or self.auto_id
        attrs = {'id': id_} if id_ else {}
        attrs = self.build_widget_attrs(attrs)
        return [
            BoundWidget(self.field.widget, widget, self.form.renderer)
            for widget in self.field.widget.subwidgets(self.html_name, self.value(), attrs=attrs)
        ]

one sees that self.field.widget.subwidgets(self.html_name, self.value(), attrs=attrs) returns a dict and assigns it to widget. Now widget['attrs']['id'] contains the "id" we would like to use when rendering the label of our CheckboxSelectMultiple.

However BoundWidget.id_for_label() is implemented as

class BoundWidget:
    ...
    def id_for_label(self):
        return 'id_%s_%s' % (self.data['name'], self.data['index'])

ignoring the id available through self.data['attrs']['id']. This re-implementation for rendering the "id" is confusing and presumably not intended. Nobody has probably realized that so far, because rarely the auto_id-argument is overridden when initializing a form. If however we do, one would assume that the method BoundWidget.id_for_label renders that string as specified through the auto_id format-string.

By changing the code from above to

class BoundWidget:
    ...
    def id_for_label(self):
        return self.data['attrs']['id']

that function behaves as expected.

Please note that this error only occurs when rendering the subwidgets of a widget of type CheckboxSelectMultiple. This has nothing to do with the method BoundField.id_for_label().

Change History (10)

comment:1 by Carlton Gibson, 3 years ago

Needs tests: set
Triage Stage: UnreviewedAccepted

Hey Jacob — Sounds right: I didn't look in-depth but, if you can put your example in a test case it will be clear enough in the PR. Thanks.

comment:2 by Jacob Rief, 3 years ago

Owner: changed from nobody to Jacob Rief
Status: newassigned

Thanks Carlton,
I will create a pull request asap.

comment:3 by Jacob Rief, 3 years ago

Here is a pull request fixing this bug: https://github.com/django/django/pull/14533 (closed without merging)

Last edited 3 years ago by Jacob Rief (previous) (diff)

comment:4 by Jacob Rief, 3 years ago

Here is the new pull request https://github.com/django/django/pull/14534 against main

comment:5 by Simon Charette, 3 years ago

Needs tests: unset

comment:6 by ᴙɘɘᴙgYmɘᴙɘj, 3 years ago

Triage Stage: AcceptedReady for checkin

The regression test looks good; fails before fix, passes afterward.

I don't think this one qualifies for a backport, so I'm changing it to "Ready for checkin."

Do the commits need to be squashed?

Last edited 3 years ago by ᴙɘɘᴙgYmɘᴙɘj (previous) (diff)

comment:7 by Carlton Gibson <carlton.gibson@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In db1fc5c:

Fixed #32855 -- Corrected BoundWidget.id_for_label() with custom auto_id.

comment:8 by Dmytro Litvinov, 3 years ago

Cc: Dmytro Litvinov added

Stacked also with that issue and found that ticket. Thanks for providing fix and merging it 🙏
Any deadline to see it released for 3.2 version? I understand there are a lot of pull requests and it can take a while for your patch to get reviewed

comment:9 by Jacob Rief, 3 years ago

Any deadline to see it released for 3.2 version?

I don't believe it ever will be backported to version 3.2.

in reply to:  8 comment:10 by Mariusz Felisiak, 3 years ago

Replying to Dmytro Litvinov:

Stacked also with that issue and found that ticket. Thanks for providing fix and merging it 🙏
Any deadline to see it released for 3.2 version? I understand there are a lot of pull requests and it can take a while for your patch to get reviewed

The issue has been present since the feature was introduced. Per our backporting policy this means it doesn't qualify for a backport to 3.2.x anymore, see Django’s release process for more details. Moreover, Django 3.2 is in extended support so it doesn't receive bugfixes (except security issues) anymore.

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