Opened 4 years ago

Closed 4 years ago

#15912 closed Cleanup/optimization (fixed)

CharField.widget_attrs should respect its super

Reported by: rubyruy 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 4 years ago.
Make sure CharField? always returns a dictionary from widget_attrs()
15912.2.diff (1.6 KB) - added by prestontimmons 4 years ago.
Slightly modified patch
15912.super.diff (1.8 KB) - added by prestontimmons 4 years ago.
Same patch using super method

Download all attachments as: .zip

Change History (8)

comment:1 Changed 4 years ago by magopian

  • Cc mathieu.agopian@… added
  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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 4 years ago by magopian

  • Component changed from File uploads/storage to Forms

comment:3 Changed 4 years ago by mmcnickle

  • Needs tests unset
  • Owner changed from nobody to mmcnickle
  • Patch needs improvement unset
  • Status changed from new to assigned

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 4 years ago by mmcnickle

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

Changed 4 years ago by prestontimmons

Slightly modified patch

Changed 4 years ago by prestontimmons

Same patch using super method

comment:4 Changed 4 years ago by prestontimmons

  • 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 4 years ago by julien

  • Resolution set to fixed
  • Status changed from assigned to closed

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