Opened 4 years ago

Last modified 4 months ago

#12915 new Bug

formfield_callback is lost in an inherited ModelForm

Reported by: semenov Owned by: nobody
Component: Forms Version: 1.1
Severity: Normal Keywords:
Cc: tom@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no


Consider the following code:

class BaseTicketForm(forms.ModelForm):
    class Meta:
        fields = (...)
    formfield_callback = lambda f: ....

class AddTicketForm(BaseTicketForm):

class EditTicketForm(BaseTicketForm):
    # some logic related to editing tickets here

When an instance of AddTicketForm or EditTicketForm is created, a user would expect that formfield_callback is applied. However, the current implementation doesn't do that; instead, a user is forced to copy-paste formfield_callback to each and every derived class.

Attachments (2)

formfield_callback_inheritance.diff (1.7 KB) - added by semenov 4 years ago.
formfield_callback_inheritance_v2.diff (1.1 KB) - added by semenov 4 years ago.
a simplier version which leaves the inheritance/lookup logic to Python

Download all attachments as: .zip

Change History (13)

comment:1 Changed 4 years ago by ramiro

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I'd say the fact that the value of formfield_callback is stored as a ModelForm class attribute is an internal implementation detail and that's why it isn't documented as such.

Anyway, maybe changing the pop() to get() here does it?:

comment:2 Changed 4 years ago by semenov

No, changing pop() to get() doesn't help. That does store a manually specified formfield_callback for a class; but that doesn't lookup a missing formfield_callback from a parent. Moreover, it will break on a double inheritance chain, e.g. when class A(ModelForm) stores a formfield_classback, class B(A) doesn't store but still can lookup parent, then class C(B) can't even lookup since B didn't store formfield_callback.

I am attaching a patch which adds the desired behavior accurately. Please let me know if it's of any interest to commiters and I will come up with a regression test.

Changed 4 years ago by semenov

comment:3 Changed 4 years ago by semenov

  • Has patch set
  • Needs tests set

comment:4 Changed 4 years ago by semenov

A little note for those who don't understand the importance of the problem. I truly believe that an empty inherited class should always behave exactly as its parent. That is a fundamental principle of polymorphism, which is not followed by ModelForms.

Changed 4 years ago by semenov

a simplier version which leaves the inheritance/lookup logic to Python

comment:5 Changed 4 years ago by russellm

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

I'm not completely happy with either of the patches you present, but I accept the problem.

comment:6 Changed 3 years ago by lukeplant

  • Type set to Bug

comment:7 Changed 3 years ago by lukeplant

  • Severity set to Normal

comment:8 Changed 3 years ago by tomchristie

  • Cc tom@… added
  • Easy pickings unset

comment:9 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:10 Changed 2 years ago by jezdez

  • Needs documentation set

comment:11 Changed 4 months ago by melinath

Also, just FTR - formfield_callback use *is* documented with modelform_factory.

Add Comment

Modify Ticket

Change Properties
<Author field>
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'

E-mail address and user name can be saved in the Preferences.

Note: See TracTickets for help on using tickets.