Code

Opened 4 years ago

Closed 3 years ago

#14082 closed Bug (fixed)

modelform_factory should use the form's metaclass

Reported by: jspiros Owned by: Honza_Kral
Component: Forms Version: master
Severity: Normal Keywords: ModelForm, ModelFormMetaclass, modelform_factory
Cc: mmitar@…, melinath Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX:

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 jspiros 4 years ago.
patch_r16322.diff (2.0 KB) - added by melinath 3 years ago.

Download all attachments as: .zip

Change History (11)

Changed 4 years ago by jspiros

comment:1 follow-up: Changed 4 years ago by mitar

  • Cc mmitar@… added

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

comment:2 in reply to: ↑ 1 Changed 4 years ago by jspiros

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 Changed 4 years ago by Honza_Kral

  • Owner changed from nobody to Honza_Kral
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

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

comment:4 Changed 3 years ago by jakub_vysoky

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 Changed 3 years ago by jakub_vysoky

  • Resolution set to worksforme
  • Status changed from assigned to closed

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 Changed 3 years ago by melinath

  • Cc melinath added
  • Resolution worksforme deleted
  • Status changed from closed to reopened

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 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to New feature

comment:8 Changed 3 years ago by melinath

  • Easy pickings unset
  • Needs tests unset
  • Type changed from New feature to Bug

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.

Changed 3 years ago by melinath

comment:9 Changed 3 years ago by carljm

  • Resolution set to fixed
  • Status changed from reopened to closed

In [16334]:

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.