Opened 16 years ago

Closed 16 years ago

Last modified 16 years ago

#8306 closed (fixed)

BaseModelAdmin.formfield_for_dbfield() in desperate need of refactor

Reported by: James Bennett Owned by: nobody
Component: contrib.admin Version: dev
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

See attached patch, which moves all of the model-field-specific overrides out into a dictionary. This has the side effect of increasing customizability :)

Attachments (2)

admin_field_overrides.diff (3.7 KB ) - added by James Bennett 16 years ago.
admin-field-overrides.patch (17.9 KB ) - added by Jacob 16 years ago.

Download all attachments as: .zip

Change History (10)

by James Bennett, 16 years ago

Attachment: admin_field_overrides.diff added

comment:1 by James Bennett, 16 years ago

Has patch: set
Triage Stage: UnreviewedDesign decision needed

comment:2 by Malcolm Tredinnick, 16 years ago

Triage Stage: Design decision neededAccepted

I'd like to hear if brosner has any particular qualms about this, but it looks like a win to me. That function is a little scary and small children might accidentally look at it.

(Brian: wontfix if you have some really good reason why this is a bad idea.)

comment:3 by Jacob, 16 years ago

milestone: 1.0 beta1.0

comment:4 by Daniel Pope <dan@…>, 16 years ago

This fixes #8326 but breaks admin widgets for subclasses of model fields.

I suggest the dictionary keys are not model.Fields but forms.Widgets. It is the widgets that we are replacing, so as long as the admin widgets are drop-in replacements, we can just call .formfield() and check for replacement widgets for the widget class in the forms.Field returned.

comment:5 by Jacob, 16 years ago

milestone: 1.0post-1.0

This is an obvious win, but I see no reason to let the code churn around at this point in the 1.0 process. Let's do this later.

comment:6 by Jacob, 16 years ago

I've attached a more complete patch with tests/docs; it's also on my github. Since James' version, I've done a couple things:

  • Broke what was left of formfield_for_dbfield into a handful of smaller methods to make those easier to screw around with.
  • Separated the defaults and the overrides so that it's easier for subclasses to override the defaults without accidentally emptying out the dictionary (as I did while testing).
  • Made sure that the ordering is correct (**kwargs overrides formfield_overrides which overrides the defaults) to fix #9148 while we at it.

by Jacob, 16 years ago

Attachment: admin-field-overrides.patch added

comment:7 by Jacob, 16 years ago

Resolution: fixed
Status: newclosed

(In [9760]) Cleaned up and refactored ModelAdmin.formfield_for_dbfield:

  • The new method uses an admin configuration option (formfield_overrides); this makes custom admin widgets especially easy.
  • Refactored what was left of formfield_for_dbfield into a handful of smaller methods so that it's easier to hook in and return custom fields where needed.
  • These formfield_for_* methods now pass around request so that you can easily modify fields based on request (as in #3987).

Fixes #8306, #3987, #9148.

Thanks to James Bennet for the original patch; Alex Gaynor and Brian Rosner also contributed.

comment:8 by (none), 16 years ago

milestone: post-1.0

Milestone post-1.0 deleted

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