Opened 14 years ago

Closed 13 years ago

#14082 closed Bug (fixed)

modelform_factory should use the form's metaclass

Reported by: Joseph Spiros Owned by: Honza Král
Component: Forms Version: dev
Severity: Normal Keywords: ModelForm, ModelFormMetaclass, modelform_factory
Cc: mmitar@…, Stephen Burrows Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently, django.forms.models.modelform_factory always uses ModelFormMetaclass to create the returned form class, even in cases when the provided form class has specified its own metaclass. I've attached a patch which fixes this. Even though metaclass will always be set for ModelForm subclasses, I retained the default functionality should there have been a case where a form class without a metaclass was provided to modelform_factory.

Attachments (2)

modelform_factory_custom_metaclasses.diff (690 bytes ) - added by Joseph Spiros 14 years ago.
patch_r16322.diff (2.0 KB ) - added by Stephen Burrows 13 years ago.

Download all attachments as: .zip

Change History (11)

by Joseph Spiros, 14 years ago

comment:1 by Mitar, 14 years ago

Cc: mmitar@… added

I am not sure that this is the case. I play a lot with metaclasses here and it seems to work?

in reply to:  1 comment:2 by Joseph Spiros, 14 years ago

Replying to mitar:

I am not sure that this is the case. I play a lot with metaclasses here and it seems to work?

Metaclasses work, but only if you directly instantiate the ModelForm subclass yourself. modelform_factory dynamically creates a new form class with the passed-in class as the parent, and uses ModelFormMetaclass as the constructor. So, when using modelform_factory with ModelForm subclasses that have a custom metaclass set, that custom metaclass is ignored.

comment:3 by Honza Král, 13 years ago

Owner: changed from nobody to Honza Král
Status: newassigned
Triage Stage: UnreviewedAccepted

We still need some tests to verify the fix, but the issue is valid

comment:4 by jakub_vysoky, 13 years ago

I've made some tests for this, but it seems it really works.. Can you look at it?

https://github.com/kvbik/django/commit/c342c3089ac169fbcf285b34f65a73fd834886a7

comment:5 by jakub_vysoky, 13 years ago

Resolution: worksforme
Status: assignedclosed

I hope I made myself clear, that it does work for me even without the patch. But maybe you need something more specific and you can put some more tests for that.

comment:6 by Stephen Burrows, 13 years ago

Cc: Stephen Burrows added
Resolution: worksforme
Status: closedreopened

I've looked into this issue and there actually is a problem here. Essentially, the ModelFormMetaclass will be run twice.

Suppose I define the following modelform subclass:

class MyModelFormMetaclass(ModelFormMetaclass):
    run = 0
    def __new__(*args, **kwargs):
        return ModelFormMetaclass.__new__(*args, **kwargs)

class MyModelForm(ModelForm):
    __metaclass__ = MyModelFormMetaclass

If I use modelform_factory in its current state, the following calls occur:

  1. a call to ModelFormMetaclass.__new__
  2. a super() call by ModelFormMetaclass [1]
  3. a call to MyModelFormMetaclass.__new__ in the course of the super call. This is how type works, apparently.
  4. a call to ModelFormMetaclass.__new__ during MyModelFormMetaclass.__new__

This should be corrected not only because it's inelegant to be calling ModelFormMetaclass.__new__ twice and not only because it's odd that the code currently doesn't call the form's actual metaclass on principle, but also because this can cause real issues.

Just to pick an example, if I try to modify the base_fields of the form in my custom metaclass *after* calling ModelFormMetaclass myself, my changes will be overridden by ModelFormMetaclass once the stack gets back up to it.

[1] http://code.djangoproject.com/browser/django/trunk/django/forms/models.py#L194

comment:7 by Julien Phalip, 13 years ago

Severity: Normal
Type: New feature

comment:8 by Stephen Burrows, 13 years ago

Easy pickings: unset
Needs tests: unset
Type: New featureBug

Updated the patch to r16322 - tests run with no unexpected failures. Added tests that fail iff the patch is not applied. Changed type to Bug since an erroneous behavior is being corrected.

by Stephen Burrows, 13 years ago

Attachment: patch_r16322.diff added

comment:9 by Carl Meyer, 13 years ago

Resolution: fixed
Status: reopenedclosed

In [16334]:

Fixed #14082 -- Use metaclass of provided ModelForm subclass in modelform_factory. Thanks jspiros and Stephen Burrows for the patch.

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