Opened 7 years ago

Closed 7 months ago

#12915 closed Bug (duplicate)

formfield_callback is lost in an inherited ModelForm

Reported by: Ilya Semenov Owned by: nobody
Component: Forms Version: 1.1
Severity: Normal Keywords:
Cc: tom@…, vlastimil.zima@… 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 Ilya Semenov 7 years ago.
formfield_callback_inheritance_v2.diff (1.1 KB) - added by Ilya Semenov 7 years ago.
a simplier version which leaves the inheritance/lookup logic to Python

Download all attachments as: .zip

Change History (20)

comment:1 Changed 7 years ago by Ramiro Morales

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 7 years ago by Ilya 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 7 years ago by Ilya Semenov

comment:3 Changed 7 years ago by Ilya Semenov

Has patch: set
Needs tests: set

comment:4 Changed 7 years ago by Ilya 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 7 years ago by Ilya Semenov

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

comment:5 Changed 7 years ago by Russell Keith-Magee

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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

comment:6 Changed 6 years ago by Luke Plant

Type: Bug

comment:7 Changed 6 years ago by Luke Plant

Severity: Normal

comment:8 Changed 6 years ago by Tom Christie

Cc: tom@… added
Easy pickings: unset

comment:9 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:10 Changed 4 years ago by Jannis Leidel

Needs documentation: set

comment:11 Changed 3 years ago by melinath

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

comment:12 Changed 2 years ago by Ilya Semenov

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

comment:13 Changed 2 years ago by Claude Paroz

Writing a test would probably be the next step.

comment:14 Changed 2 years ago by Ilya Semenov

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 2 years ago by Ilya Semenov

Needs tests: unset

comment:16 in reply to:  14 Changed 2 years ago by Claude Paroz

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.

comment:17 Changed 8 months ago by Vlastimil Zíma

Cc: vlastimil.zima@… added

comment:18 Changed 7 months ago by Tim Graham

Resolution: duplicate
Status: newclosed

Duplicate of #24974 which is fixed in Django 1.10.

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