Opened 6 years ago

Closed 5 years ago

#15912 closed Cleanup/optimization (fixed)

CharField.widget_attrs should respect its super

Reported by: Ruy Asan Owned by: mmcnickle
Component: Forms Version: 1.3
Severity: Normal Keywords:
Cc: mathieu.agopian@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

The contract of the Field.widget_attrs method is that it returns a dictionary (even says so in the docstring), however CharField.widget_attrs may return None (when the if clause fails). It should at least return an empty dict, or better yet, simply modify whatever the super-method returns.

Pull Request Here
Patch Here

Attachments (3)

15912.diff (2.0 KB) - added by mmcnickle 6 years ago.
Make sure CharField? always returns a dictionary from widget_attrs()
15912.2.diff (1.6 KB) - added by Preston Timmons 5 years ago.
Slightly modified patch
15912.super.diff (1.8 KB) - added by Preston Timmons 5 years ago.
Same patch using super method

Download all attachments as: .zip

Change History (8)

comment:1 Changed 6 years ago by Mathieu Agopian

Cc: mathieu.agopian@… added
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

Hello rubyruy,

Thanks for the feedback, and indeed, i believe django.forms.fields.CharField.widget_attrs should return a dictionary in all cases, as written in django.forms.fields.Field.widget_attrs, but even more, it should return an updated dict from it.

I'm thus marking this ticket as accepted, but i also added the "needs tests" and "patch needs improvement" flags.
Regarding the improvements, you use attrs.setdefault, while it should rather be using attrs.update: child's attributes should have precedence.

As a side note, the github django repository is only a mirror, the official repository is on SVN (on this very track). Thus the pull request won't be accepted on github. Also, please attach the patch (with tests if possible, that would be brilliant) to this ticket, as it'll ease the review and everybody's work!

comment:2 Changed 6 years ago by Mathieu Agopian

Component: File uploads/storageForms

comment:3 Changed 6 years ago by mmcnickle

Needs tests: unset
Owner: changed from nobody to mmcnickle
Patch needs improvement: unset
Status: newassigned

The attached patch makes sure widget_attrs() always returns a dictionary, even if it's empty.

I didn't think that the super call was necessary as Field only returns an empty dictionary by default. If this ever changes, then we can add the super call then.

Changed 6 years ago by mmcnickle

Attachment: 15912.diff added

Make sure CharField? always returns a dictionary from widget_attrs()

Changed 5 years ago by Preston Timmons

Attachment: 15912.2.diff added

Slightly modified patch

Changed 5 years ago by Preston Timmons

Attachment: 15912.super.diff added

Same patch using super method

comment:4 Changed 5 years ago by Preston Timmons

UI/UX: unset

The patch and test look good. I added a slightly modified patch to fix a typo, added a reference to this ticket, and remove a redundant test in15912.2.diff.

Also attached a version using a super call for the committers disgression.

comment:5 Changed 5 years ago by Julien Phalip

Resolution: fixed
Status: assignedclosed

In [17096]:

Fixed #15912 -- Ensured that forms.CharField.widget_attrs() always returns a dictionary. Thanks to tsabi and rubyruy for the report and to mmcnickle and prestontimmons for the patch.

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