[Django] #24974: Cannot create subclass of form created by `modelform_factoy()`, e.g. in `ModelAdmin.get_form()`
Django
`ModelAdmin.get_form()`
-------------------------+-------------------------------------------------
Reporter: | Owner: nobody
mrmachine |
Type: Bug | Status: new
Component: Forms | Version: 1.8
Severity: Normal | Keywords: form modelform modelform_factory
| modeladmin get_site metaclass
Triage Stage: | modelformmetaclass
Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------+-------------------------------------------------
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>)])
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24974>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
`ModelAdmin.get_form()`
-------------------------------------+-------------------------------------
Reporter: mrmachine | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.8
Keywords: form modelform | Triage Stage:
modelform_factory modeladmin | Unreviewed
get_site metaclass |
modelformmetaclass |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by mrmachine):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/24974#comment:1>
Django
`ModelAdmin.get_form()`
-------------------------------------+-------------------------------------
Reporter: mrmachine | Owner: nobody
Component: Forms | Version: 1.8
Keywords: form modelform | Triage Stage: Accepted
modelform_factory modeladmin |
get_site metaclass |
modelformmetaclass |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
Comment:
Discussion in #django-dev seemed to indicate that this is a problem we
should solve.
--
Ticket URL: <https://code.djangoproject.com/ticket/24974#comment:2>
Django
`ModelAdmin.get_form()`
-------------------------------------+-------------------------------------
Reporter: mrmachine | Owner: nobody
Component: Forms | Version: 1.8
Keywords: form modelform | Triage Stage: Accepted
modelform_factory modeladmin |
get_site metaclass |
modelformmetaclass |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by tolomea):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24974#comment:3>
Django
`ModelAdmin.get_form()`
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Forms | Version: 1.8
Severity: Normal | Resolution:
Keywords: form modelform | Triage Stage: Accepted
modelform_factory modeladmin |
get_site metaclass |
modelformmetaclass |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by yoongkang):
* owner: nobody => yoongkang
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/24974#comment:4>
Django
`ModelAdmin.get_form()`
-------------------------------------+-------------------------------------
Reporter: mrmachine | Owner: yoongkang
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: form modelform | Triage Stage: Accepted
modelform_factory modeladmin |
get_site metaclass |
modelformmetaclass |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by yoongkang):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/6048 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/24974#comment:5>
Django
`ModelAdmin.get_form()`
-------------------------------------+-------------------------------------
Reporter: mrmachine | Owner: yoongkang
Component: Forms | Version: 1.8
Severity: Normal | Resolution: fixed
Keywords: form modelform | Triage Stage: Accepted
modelform_factory modeladmin |
get_site metaclass |
modelformmetaclass |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"d5f89ff6e873dbb2890ed05ce2aeae628792c8f7" d5f89ff]:
{{{
#!CommitTicketReference repository=""
revision="d5f89ff6e873dbb2890ed05ce2aeae628792c8f7"
Fixed #24974 -- Fixed inheritance of formfield_callback for
modelform_factory forms.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24974#comment:6>