Opened 5 years ago

Last modified 11 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: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Consider the following code:

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

class AddTicketForm(BaseTicketForm):
    pass

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 5 years ago.
formfield_callback_inheritance_v2.diff (1.1 KB) - added by semenov 5 years ago.
a simplier version which leaves the inheritance/lookup logic to Python

Download all attachments as: .zip

Change History (18)

comment:1 Changed 5 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?: http://code.djangoproject.com/browser/django/trunk/django/forms/models.py?rev=12402#L204

comment:2 Changed 5 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 5 years ago by semenov

comment:3 Changed 5 years ago by semenov

  • Has patch set
  • Needs tests set

comment:4 Changed 5 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 5 years ago by semenov

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

comment:5 Changed 5 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 4 years ago by lukeplant

  • Type set to Bug

comment:7 Changed 4 years ago by lukeplant

  • Severity set to Normal

comment:8 Changed 4 years ago by tomchristie

  • Cc tom@… added
  • Easy pickings unset

comment:9 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:10 Changed 3 years ago by jezdez

  • Needs documentation set

comment:11 Changed 17 months ago by melinath

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

comment:12 Changed 11 months ago by IlyaSemenov

How'd you like to improve the latest patch? It's been 5 years, and I'm suffering from this again.

comment:13 Changed 11 months ago by claudep

Writing a test would probably be the next step.

comment:14 follow-up: Changed 11 months ago by IlyaSemenov

Added tests: https://github.com/IlyaSemenov/django/tree/ticket_12915

I am not happy with the patch either, but the main problem here is the original design decision to provide formfield_callback as a class-level lambda rather than @staticmethod or a Meta class attribute.

Will it make it better if I come up with a patch which moves formfield_callback into the Meta class (where it would make perfect sense along with widgets and labels), or even allows to use either way (for backward compatibility)?

comment:15 Changed 11 months ago by IlyaSemenov

  • Needs tests unset

comment:16 in reply to: ↑ 14 Changed 11 months ago by claudep

Replying to IlyaSemenov:

Will it make it better if I come up with a patch which moves formfield_callback into the Meta class (where it would make perfect sense along with widgets and labels), or even allows to use either way (for backward compatibility)?

Indeed, I think it would be an interesting alternative to consider.

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