Code

Opened 6 years ago

Closed 5 weeks ago

#7837 closed Bug (fixed)

Hierarchy in forms metaclasses can miss declarated fields in forms

Reported by: msaelices Owned by: msaelices
Component: Forms Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

This was occurs after [7847]. Ok, [7847] is a good commit for make able to metaclass subclassing.

This is the scenary:

#### model
class FooModel(models.Model):
    name = models.CharField(max_length=100)

#### forms
class GenericForm(forms.Form):
    extra_field = forms.CharField()

class FooForm(GenericForm, forms.ModelForm):
    # to avoid python metaclass conflict error:
    __metaclass__ = type('FooFormMetaclass', (GenericForm.__metaclass__, forms.ModelForm.__metaclass__), {})
    non_existing_field = forms.CharField()

    class Meta:
        model = FooModel

In previous scenario, FooForm should contain both name, extra_field and nonexisting_field. But only appears name field in base_fields.

See this test:

>>> from testapp.forms import FooForm
>>> FooForm.base_fields.keys()
['name']

I will attach a patch thats fixes problem.

Attachments (6)

forms_metaclass_hierarchy_problem_fix.diff (3.6 KB) - added by msaelices 6 years ago.
This patch fix problems
forms_metaclass_hierarchy_problem_fix.2.diff (3.6 KB) - added by msaelices 6 years ago.
It's only a Form naming change in tests
forms_metaclass_hierarchy_problem_fix.3.diff (3.6 KB) - added by msaelices 6 years ago.
Sorry... this is the good patch
forms_metaclass_hierarchy_problem_fix.4.diff (3.6 KB) - added by msaelices 6 years ago.
We put an incorrect ticket link in test... now is "Regression test for #7837" (before was "#6926")
forms_metaclass_hierarchy_problem_fix_r8049.diff (3.9 KB) - added by msaelices 6 years ago.
Updated patch for [8049]
forms_metaclass_hierarchy_problem_fix_r8335.diff (2.4 KB) - added by msaelices 6 years ago.
Patch updated for [8335]

Download all attachments as: .zip

Change History (19)

Changed 6 years ago by msaelices

This patch fix problems

Changed 6 years ago by msaelices

It's only a Form naming change in tests

Changed 6 years ago by msaelices

Sorry... this is the good patch

Changed 6 years ago by msaelices

We put an incorrect ticket link in test... now is "Regression test for #7837" (before was "#6926")

comment:1 Changed 6 years ago by msaelices

  • milestone set to 1.0 beta
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Changed 6 years ago by msaelices

Updated patch for [8049]

comment:2 Changed 6 years ago by ericholscher

  • milestone changed from 1.0 beta to 1.0
  • Triage Stage changed from Unreviewed to Accepted

Changed 6 years ago by msaelices

Patch updated for [8335]

comment:3 Changed 6 years ago by msaelices

  • Status changed from new to assigned

comment:4 Changed 6 years ago by jacob

I'm frankly surprised that the business with __metaclass__ works at all -- I don't see why you want to subclass both Form and ModelForm, and I'm not entirely sure it ought to be possible at all.

In fact, you can just make GenericForm a ModelForm and then you don't have this weird metaclass hack.

comment:5 Changed 6 years ago by msaelices

A custom ModelForm class that inherits another custom Form was very useful in many ways. In my company there are several projects where we are using this patch, because is needed that functionality.

For example, you could use a ModelForm with WTForm (one of the django snippets most rated), because you want having rapid and flexible layout design in your model forms.

comment:6 Changed 6 years ago by msaelices

I think multiple inneritance of ModelForm and a normal Form shouldn't be problematic. I don't like also use of __metaclass__ = ... but this is only way to avoid python metaclass conflicts.

Could you find other way to achieve this?

comment:7 Changed 6 years ago by jacob

  • milestone changed from 1.0 to post-1.0

I think this needs to wait for a proper fix -- if it's gonna work, it should work without the __metaclass__ hack. We can make that happen, but it'll be post-1.0.

comment:8 Changed 6 years ago by msaelices

There is a recipe of avoiding metaclass conflicts here: http://code.activestate.com/recipes/204197/

comment:9 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:10 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:11 Changed 3 years ago by julien

  • Easy pickings unset
  • Patch needs improvement set

Patch needs improvement as per Jacob's comments.

comment:12 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:13 Changed 5 weeks ago by loic84

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

Fixed in #19617.

Note that as documented the ModelForm subclass should appear first in the MRO:

class FooForm(forms.ModelForm, GenericForm):
   pass

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.