Opened 14 years ago

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

Download all attachments as: .zip

Change History (20)

comment:1 by Ramiro Morales, 14 years ago

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

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.

by Ilya Semenov, 14 years ago

comment:3 by Ilya Semenov, 14 years ago

Has patch: set
Needs tests: set

comment:4 by Ilya Semenov, 14 years ago

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.

by Ilya Semenov, 14 years ago

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

comment:5 by Russell Keith-Magee, 14 years ago

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 by Luke Plant, 13 years ago

Type: Bug

comment:7 by Luke Plant, 13 years ago

Severity: Normal

comment:8 by Tom Christie, 13 years ago

Cc: tom@… added
Easy pickings: unset

comment:9 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:10 by Jannis Leidel, 12 years ago

Needs documentation: set

comment:11 by Stephen Burrows, 10 years ago

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

comment:12 by Ilya Semenov, 10 years ago

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

comment:13 by Claude Paroz, 10 years ago

Writing a test would probably be the next step.

comment:14 by Ilya Semenov, 10 years ago

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

Needs tests: unset

in reply to:  14 comment:16 by Claude Paroz, 10 years ago

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 by Vlastimil Zíma, 8 years ago

Cc: vlastimil.zima@… added

comment:18 by Tim Graham, 8 years ago

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