Opened 4 years ago

Closed 3 years ago

#24974 closed Bug (fixed)

Cannot create subclass of form created by `modelform_factory()`, e.g. in `ModelAdmin.get_form()`

Reported by: Tai Lee Owned by: Yoong Kang Lim
Component: Forms Version: 1.8
Severity: Normal Keywords: form modelform modelform_factory modeladmin get_site metaclass modelformmetaclass
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The default implementation of ModelAdmin.get_form() uses modelform_factory() to dynamically create a new ModelForm class, and apply form field overrides, etc.

The docs mention that I can specify a new base form that is passed to this implementation or just return a new ModelForm class. But when designing a generic ModelAdmin class that will be used with a number of yet unspecified models, I want to use the form returned by the super class as the base, in case one has already been set on a ModelAdmin class my class will be mixed into.

I would normally expect that this just works:

def get_form(self, *args, **kwargs):
    form_class = super(MyModelAdmin, self).get_form(*args, **kwargs)
    class Form(form_class):
        # do stuff.
    return Form

But while form_class does all the things like form field overrides, Form does not. All form fields are rendered in the admin with regular form widgets, not the admin variants. The reason appears to be because when Form is created, ModelFormMetaclass is executed again but without the formfield_overrides argument.

To make it work again, I need to do:

def get_form(self, *args, **kwargs):
    form_class = super(MyModelAdmin, self).get_form(*args, **kwargs)
    class Form(form_class):
        # do stuff.
    kwargs['form'] = Form
    return super(MyModelAdmin, self).get_form(*args, **kwargs)

This runs through the whole default implementation of get_form() twice, but it does work.

Alternatively, if I wanted to override the __init__() method on the form, I could replace it instead of creating a subclass, with something like:

def get_form(self, *args, **kwargs):
    form_class = super(FluentLayoutsMixin, self).get_form(*args, **kwargs)
    old_init = form_class.__init__
    def new_init(self, *args, **kwargs):
        old_init.__get__(self, type(self))(*args, **kwargs)
        # do stuff.
    form_class.__init__ = new_init.__get__(None, form_class)
    return form_class

This won't execute the default get_form() implementation twice, but it's getting pretty hairy.

This took three people a few hours to work out, stepping through many pdb sessions in django admin and model form code. A note in the docs might save someone else the trouble.

Something like:

Note: You cannot just create and return a subclass of form from the super class in this method. Due to the way ModelFormMetaclass works, the subclass will ignore formfield_overrides unless you pass it back through the default implementation.

# NO: `formfield_overrides` not applied.
def get_form(self, *args, **kwargs):
    form_class = super(MyModelAdmin, self).get_form(*args, **kwargs)
    class Form(form_class):
        # do stuff.
    return Form

# YES: `formfield_overrides` applied.
def get_form(self, *args, **kwargs):
    form_class = super(MyModelAdmin, self).get_form(*args, **kwargs)
    class Form(form_class):
        # do stuff.
    kwargs['form'] = Form
    return super(MyModelAdmin, self).get_form(*args, **kwargs)

This is actually more of a general problem with modelform_factory() than a problem with ModelAdmin.get_form(), so perhaps a note for the modelform_factory() docs instead, with a link to it from the ModelAdmin.get_form() docs.

Here is an interactive shell session demonstrating the problem with modelform_factory().

>>> from django import forms
>>> from django.forms.models import modelform_factory
>>> from django.contrib.sites.models import Site

# Create a form class where all fields are converted to `DateTimeField` via callback.
>>> Form = modelform_factory(Site, formfield_callback=lambda f: forms.DateTimeField)
>>> Form.base_fields
OrderedDict([(u'id', <class 'django.forms.fields.DateTimeField'>), ('domain', <class 'django.forms.fields.DateTimeField'>), ('name', <class 'django.forms.fields.DateTimeField'>)])

# Note that for any subclass, all base fields have reverted to their original values, instead of being inherited.
>>> class FooForm(Form): pass
>>> FooForm.base_fields
OrderedDict([('domain', <django.forms.fields.CharField object at 0x112db5f90>), ('name', <django.forms.fields.CharField object at 0x112d74a90>)])

Change History (6)

comment:1 Changed 4 years ago by Tai Lee

Summary: Cannot create subclass of form created by `modelform_factoy()`, e.g. in `ModelAdmin.get_form()`Cannot create subclass of form created by `modelform_factory()`, e.g. in `ModelAdmin.get_form()`

comment:2 Changed 4 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

Discussion in #django-dev seemed to indicate that this is a problem we should solve.

comment:3 Changed 3 years ago by Gordon Wrigley

As another example we're doing this in our common ModelAdmin extension:

    def get_form(self, request, obj=None, **kwargs):
        field_overrides = self.get_field_overrides()
        readonly_fields = set(self.get_readonly_fields(request, obj))

        fields = {name: field for name, field in field_overrides.iteritems() if name not in readonly_fields}

        form = type(self.form)(self.form.__name__ + b"_", (self.form,), fields)

        return super(YPModelAdmin, self).get_form(request, obj, form=form, **kwargs)

I ended up here because the more natural approach would be:

    def get_form(self, request, obj=None, **kwargs):
        form = super(YPModelAdmin, self).get_form(request, obj, **kwargs)

        field_overrides = self.get_field_overrides()
        readonly_fields = set(self.get_readonly_fields(request, obj))

        fields = {name: field for name, field in field_overrides.iteritems() if name not in readonly_fields}

        return type(form)(form.__name__ + b"_", (form,), fields)

But that doesn't work right due to this issue.

comment:4 Changed 3 years ago by Yoong Kang Lim

Owner: changed from nobody to Yoong Kang Lim
Status: newassigned

comment:5 Changed 3 years ago by Yoong Kang Lim

Has patch: set

comment:6 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In d5f89ff:

Fixed #24974 -- Fixed inheritance of formfield_callback for modelform_factory forms.

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