Fixed #6337. Refs #3632 -- Fixed ModelForms subclassing, to the extent that it can be made to work.

This ended up being an almost complete rewrite of ModelForms.__new__, but
should be backwards compatible (although the text of one error message has
changed, which is only user visible and only if you pass in invalid code).

Documentation updated, also.

This started out as a patch from semenov (many thanks!), but by the time all
the problems were hammered out, little of the original was left. Still, it was
a good starting point.


git-svn-id: http://code.djangoproject.com/svn/django/trunk@7112 bcc190cf-cafb-0310-a4f2-bffc1f526a37

2 files changed, 57 insertions, 52 deletions

diff --git a/django/newforms/forms.py b/django/newforms/forms.py
index 04bf62e445..7bc5e2ac02 100644
--- a/django/newforms/forms.py
+++ b/django/newforms/forms.py
@@ -22,23 +22,26 @@ def pretty_name(name):
     name = name[0].upper() + name[1:]
     return name.replace('_', ' ')
 
+def get_declared_fields(bases, attrs):
+    fields = [(field_name, attrs.pop(field_name)) for field_name, obj in attrs.items() if isinstance(obj, Field)]
+    fields.sort(lambda x, y: cmp(x[1].creation_counter, y[1].creation_counter))
+
+    # If this class is subclassing another Form, add that Form's fields.
+    # Note that we loop over the bases in *reverse*. This is necessary in
+    # order to preserve the correct order of fields.
+    for base in bases[::-1]:
+        if hasattr(base, 'base_fields'):
+            fields = base.base_fields.items() + fields
+
+    return SortedDict(fields)
+
 class DeclarativeFieldsMetaclass(type):
     """
     Metaclass that converts Field attributes to a dictionary called
     'base_fields', taking into account parent class 'base_fields' as well.
     """
     def __new__(cls, name, bases, attrs):
-        fields = [(field_name, attrs.pop(field_name)) for field_name, obj in attrs.items() if isinstance(obj, Field)]
-        fields.sort(lambda x, y: cmp(x[1].creation_counter, y[1].creation_counter))
-
-        # If this class is subclassing another Form, add that Form's fields.
-        # Note that we loop over the bases in *reverse*. This is necessary in
-        # order to preserve the correct order of fields.
-        for base in bases[::-1]:
-            if hasattr(base, 'base_fields'):
-                fields = base.base_fields.items() + fields
-
-        attrs['base_fields'] = SortedDict(fields)
+        attrs['base_fields'] = get_declared_fields(bases, attrs)
         return type.__new__(cls, name, bases, attrs)
 
 class BaseForm(StrAndUnicode):
diff --git a/django/newforms/models.py b/django/newforms/models.py
index 0ee911a82f..324b7b2bf8 100644
--- a/django/newforms/models.py
+++ b/django/newforms/models.py
@@ -11,7 +11,7 @@ from django.utils.datastructures import SortedDict
 from django.core.exceptions import ImproperlyConfigured
 
 from util import ValidationError, ErrorList
-from forms import BaseForm
+from forms import BaseForm, get_declared_fields
 from fields import Field, ChoiceField, EMPTY_VALUES
 from widgets import Select, SelectMultiple, MultipleHiddenInput
 
@@ -211,57 +211,58 @@ class ModelFormOptions(object):
         self.fields = getattr(options, 'fields', None)
         self.exclude = getattr(options, 'exclude', None)
 
+
 class ModelFormMetaclass(type):
     def __new__(cls, name, bases, attrs,
                 formfield_callback=lambda f: f.formfield()):
-        fields = [(field_name, attrs.pop(field_name)) for field_name, obj in attrs.items() if isinstance(obj, Field)]
-        fields.sort(lambda x, y: cmp(x[1].creation_counter, y[1].creation_counter))
+        try:
+            parents = [b for b in bases if issubclass(b, ModelForm)]
+        except NameError:
+            # We are defining ModelForm itself.
+            parents = None
+        if not parents:
+            return super(ModelFormMetaclass, cls).__new__(cls, name, bases,
+                    attrs)
 
-        # If this class is subclassing another Form, add that Form's fields.
-        # Note that we loop over the bases in *reverse*. This is necessary in
-        # order to preserve the correct order of fields.
-        for base in bases[::-1]:
-            if hasattr(base, 'base_fields'):
-                fields = base.base_fields.items() + fields
-        declared_fields = SortedDict(fields)
+        new_class = type.__new__(cls, name, bases, attrs)
+        declared_fields = get_declared_fields(bases, attrs)
+        opts = new_class._meta = ModelFormOptions(getattr(new_class, 'Meta', None))
+        if opts.model:
+            # If a model is defined, extract form fields from it.
+            fields = fields_for_model(opts.model, opts.fields,
+                                      opts.exclude, formfield_callback)
+            # Fields defined on the base classes override local fields and are
+            # always included.
+            fields.update(declared_fields)
+        else:
+            fields = declared_fields
+        new_class.base_fields = fields
 
-        opts = ModelFormOptions(attrs.get('Meta', None))
-        attrs['_meta'] = opts
+        # XXX: The following is a sanity check for the user to avoid
+        # inadvertent attribute hiding.
 
-        # Don't allow more than one Meta model definition in bases. The fields
-        # would be generated correctly, but the save method won't deal with
-        # more than one object.
-        base_models = []
-        for base in bases:
+        # Search base classes, but don't allow more than one Meta model
+        # definition. The fields would be generated correctly, but the save
+        # method won't deal with more than one object. Also, it wouldn't be
+        # clear what to do with multiple fields and exclude lists.
+        first = None
+        current = opts.model
+        for base in parents:
             base_opts = getattr(base, '_meta', None)
             base_model = getattr(base_opts, 'model', None)
-            if base_model is not None:
-                base_models.append(base_model)
-        if len(base_models) > 1:
-            raise ImproperlyConfigured("%s's base classes define more than one model." % name)
+            if base_model:
+                if current:
+                    if base_model is not current:
+                        raise ImproperlyConfigured("%s's base classes define more than one model." % name)
+                else:
+                    current = base_model
 
-        # If a model is defined, extract form fields from it and add them to base_fields
-        if attrs['_meta'].model is not None:
-            # Don't allow a subclass to define a different Meta model than a
-            # parent class has. Technically the right fields would be generated,
-            # but the save method will not deal with more than one model.
-            for base in bases:
-                base_opts = getattr(base, '_meta', None)
-                base_model = getattr(base_opts, 'model', None)
-                if base_model and base_model is not opts.model:
-                    raise ImproperlyConfigured('%s defines a different model than its parent.' % name)
-            model_fields = fields_for_model(opts.model, opts.fields,
-                    opts.exclude, formfield_callback)
-            # fields declared in base classes override fields from the model
-            model_fields.update(declared_fields)
-            attrs['base_fields'] = model_fields
-        else:
-            attrs['base_fields'] = declared_fields
-        return type.__new__(cls, name, bases, attrs)
+        return new_class
 
 class BaseModelForm(BaseForm):
     def __init__(self, data=None, files=None, auto_id='id_%s', prefix=None,
-                 initial=None, error_class=ErrorList, label_suffix=':', instance=None):
+                 initial=None, error_class=ErrorList, label_suffix=':',
+                 instance=None):
         opts = self._meta
         if instance is None:
             # if we didn't get an instance, instantiate a new one
@@ -277,7 +278,8 @@ class BaseModelForm(BaseForm):
 
     def save(self, commit=True):
         """
-        Saves this ``form``'s cleaned_data into model instance ``self.instance``.
+        Saves this ``form``'s cleaned_data into model instance
+        ``self.instance``.
 
         If commit=True, then the changes to ``instance`` will be saved to the
         database. Returns ``instance``.