Merge pull request #1382 from loic/ticket19617

Fixed #19617 -- Refactored form metaclasses to support more inheritance scenarios.

django/forms/forms.py

@@ -11,7 +11,7 @@
 from django.core.exceptions import ValidationError
 from django.forms.fields import Field, FileField
 from django.forms.utils import flatatt, ErrorDict, ErrorList
-from django.forms.widgets import Media, media_property, TextInput, Textarea
+from django.forms.widgets import Media, MediaDefiningClass, TextInput, Textarea
 from django.utils.html import conditional_escape, format_html
 from django.utils.encoding import smart_text, force_text, python_2_unicode_compatible
 from django.utils.safestring import mark_safe
@@ -29,6 +29,7 @@ def pretty_name(name):
         return ''
     return name.replace('_', ' ').capitalize()
 
+
 def get_declared_fields(bases, attrs, with_base_fields=True):
     """
     Create a list of form field instances from the passed in 'attrs', plus any
@@ -40,6 +41,13 @@ def get_declared_fields(bases, attrs, with_base_fields=True):
     used. The distinction is useful in ModelForm subclassing.
     Also integrates any additional media definitions.
     """
+
+    warnings.warn(
+        "get_declared_fields is deprecated and will be removed in Django 1.9.",
+        PendingDeprecationWarning,
+        stacklevel=2,
+    )
+
     fields = [(field_name, attrs.pop(field_name)) for field_name, obj in list(six.iteritems(attrs)) if isinstance(obj, Field)]
     fields.sort(key=lambda x: x[1].creation_counter)
 
@@ -57,19 +65,42 @@ def get_declared_fields(bases, attrs, with_base_fields=True):
 
     return OrderedDict(fields)
 
-class DeclarativeFieldsMetaclass(type):
+
+class DeclarativeFieldsMetaclass(MediaDefiningClass):
     """
-    Metaclass that converts Field attributes to a dictionary called
-    'base_fields', taking into account parent class 'base_fields' as well.
+    Metaclass that collects Fields declared on the base classes.
     """
-    def __new__(cls, name, bases, attrs):
-        attrs['base_fields'] = get_declared_fields(bases, attrs)
-        new_class = super(DeclarativeFieldsMetaclass,
-                     cls).__new__(cls, name, bases, attrs)
-        if 'media' not in attrs:
-            new_class.media = media_property(new_class)
+    def __new__(mcs, name, bases, attrs):
+        # Collect fields from current class.
+        current_fields = []
+        for key, value in list(attrs.items()):
+            if isinstance(value, Field):
+                current_fields.append((key, value))
+                attrs.pop(key)
+        current_fields.sort(key=lambda x: x[1].creation_counter)
+        attrs['declared_fields'] = OrderedDict(current_fields)
+
+        new_class = (super(DeclarativeFieldsMetaclass, mcs)
+            .__new__(mcs, name, bases, attrs))
+
+        # Walk through the MRO.
+        declared_fields = OrderedDict()
+        for base in reversed(new_class.__mro__):
+            # Collect fields from base class.
+            if hasattr(base, 'declared_fields'):
+                declared_fields.update(base.declared_fields)
+
+            # Field shadowing.
+            for attr in base.__dict__.keys():
+                if attr in declared_fields:
+                    declared_fields.pop(attr)
+
+        new_class.base_fields = declared_fields
+        new_class.declared_fields = declared_fields
+
         return new_class
 
+
 @python_2_unicode_compatible
 class BaseForm(object):
     # This is the main implementation of all the Form logic. Note that this
@@ -398,6 +429,7 @@ def visible_fields(self):
         """
         return [field for field in self if not field.is_hidden]
 
+
 class Form(six.with_metaclass(DeclarativeFieldsMetaclass, BaseForm)):
     "A collection of Fields, plus their associated data."
     # This is a separate class from BaseForm in order to abstract the way
@@ -406,6 +438,7 @@ class Form(six.with_metaclass(DeclarativeFieldsMetaclass, BaseForm)):
     # to define a form using declarative syntax.
     # BaseForm itself has no way of designating self.fields.
 
+
 @python_2_unicode_compatible
 class BoundField(object):
     "A Field plus data"

django/forms/models.py

@@ -10,11 +10,11 @@
 
 from django.core.exceptions import ValidationError, NON_FIELD_ERRORS, FieldError
 from django.forms.fields import Field, ChoiceField
-from django.forms.forms import BaseForm, get_declared_fields
+from django.forms.forms import DeclarativeFieldsMetaclass, BaseForm
 from django.forms.formsets import BaseFormSet, formset_factory
 from django.forms.utils import ErrorList
 from django.forms.widgets import (SelectMultiple, HiddenInput,
-    MultipleHiddenInput, media_property, CheckboxSelectMultiple)
+    MultipleHiddenInput, CheckboxSelectMultiple)
 from django.utils.encoding import smart_text, force_text
 from django.utils import six
 from django.utils.text import get_text_list, capfirst
@@ -61,6 +61,7 @@ def construct_instance(form, instance, fields=None, exclude=None):
 
     return instance
 
+
 def save_instance(form, instance, fields=None, fail_message='saved',
                   commit=True, exclude=None, construct=True):
     """
@@ -138,6 +139,7 @@ def model_to_dict(instance, fields=None, exclude=None):
             data[f.name] = f.value_from_object(instance)
     return data
 
+
 def fields_for_model(model, fields=None, exclude=None, widgets=None,
                      formfield_callback=None, localized_fields=None,
                      labels=None, help_texts=None, error_messages=None):
@@ -207,6 +209,7 @@ def fields_for_model(model, fields=None, exclude=None, widgets=None,
         )
     return field_dict
 
+
 class ModelFormOptions(object):
     def __init__(self, options=None):
         self.model = getattr(options, 'model', None)
@@ -219,22 +222,16 @@ def __init__(self, options=None):
         self.error_messages = getattr(options, 'error_messages', None)
 
 
-class ModelFormMetaclass(type):
-    def __new__(cls, name, bases, attrs):
+class ModelFormMetaclass(DeclarativeFieldsMetaclass):
+    def __new__(mcs, name, bases, attrs):
         formfield_callback = attrs.pop('formfield_callback', None)
-        try:
-            parents = [b for b in bases if issubclass(b, ModelForm)]
-        except NameError:
-            # We are defining ModelForm itself.
-            parents = None
-        declared_fields = get_declared_fields(bases, attrs, False)
-        new_class = super(ModelFormMetaclass, cls).__new__(cls, name, bases,
-                attrs)
-        if not parents:
+
+        new_class = (super(ModelFormMetaclass, mcs)
+                        .__new__(mcs, name, bases, attrs))
+
+        if bases == (BaseModelForm,):
             return new_class
 
-        if 'media' not in attrs:
-            new_class.media = media_property(new_class)
         opts = new_class._meta = ModelFormOptions(getattr(new_class, 'Meta', None))
 
         # We check if a string was passed to `fields` or `exclude`,
@@ -253,7 +250,6 @@ def __new__(cls, name, bases, attrs):
 
         if opts.model:
             # If a model is defined, extract form fields from it.
-
             if opts.fields is None and opts.exclude is None:
                 # This should be some kind of assertion error once deprecation
                 # cycle is complete.
@@ -263,7 +259,7 @@ def __new__(cls, name, bases, attrs):
                               DeprecationWarning, stacklevel=2)
 
             if opts.fields == ALL_FIELDS:
-                # sentinel for fields_for_model to indicate "get the list of
+                # Sentinel for fields_for_model to indicate "get the list of
                 # fields from the model"
                 opts.fields = None
 
@@ -274,22 +270,24 @@ def __new__(cls, name, bases, attrs):
 
             # make sure opts.fields doesn't specify an invalid field
             none_model_fields = [k for k, v in six.iteritems(fields) if not v]
-            missing_fields = set(none_model_fields) - \
-                             set(declared_fields.keys())
+            missing_fields = (set(none_model_fields) -
+                              set(new_class.declared_fields.keys()))
             if missing_fields:
                 message = 'Unknown field(s) (%s) specified for %s'
                 message = message % (', '.join(missing_fields),
                                      opts.model.__name__)
                 raise FieldError(message)
             # Override default model fields with any custom declared ones
             # (plus, include all the other declared fields).
-            fields.update(declared_fields)
+            fields.update(new_class.declared_fields)
         else:
-            fields = declared_fields
-        new_class.declared_fields = declared_fields
+            fields = new_class.declared_fields
+
         new_class.base_fields = fields
+
         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=None,
@@ -438,9 +436,11 @@ def save(self, commit=True):
 
     save.alters_data = True
 
+
 class ModelForm(six.with_metaclass(ModelFormMetaclass, BaseModelForm)):
     pass
 
+
 def modelform_factory(model, form=ModelForm, fields=None, exclude=None,
                       formfield_callback=None, widgets=None, localized_fields=None,
                       labels=None, help_texts=None, error_messages=None):
@@ -780,6 +780,7 @@ def pk_is_not_editable(pk):
             form.fields[self._pk_field.name] = ModelChoiceField(qs, initial=pk_value, required=False, widget=widget)
         super(BaseModelFormSet, self).add_fields(form, index)
 
+
 def modelformset_factory(model, form=ModelForm, formfield_callback=None,
                          formset=BaseModelFormSet, extra=1, can_delete=False,
                          can_order=False, max_num=None, fields=None, exclude=None,
@@ -1021,6 +1022,7 @@ def clean(self, value):
     def _has_changed(self, initial, data):
         return False
 
+
 class ModelChoiceIterator(object):
     def __init__(self, field):
         self.field = field
@@ -1047,6 +1049,7 @@ def __len__(self):
     def choice(self, obj):
         return (self.field.prepare_value(obj), self.field.label_from_instance(obj))
 
+
 class ModelChoiceField(ChoiceField):
     """A ChoiceField whose choices are a model QuerySet."""
     # This class is a subclass of ChoiceField for purity, but it doesn't
@@ -1141,6 +1144,7 @@ def _has_changed(self, initial, data):
         data_value = data if data is not None else ''
         return force_text(self.prepare_value(initial_value)) != force_text(data_value)
 
+
 class ModelMultipleChoiceField(ModelChoiceField):
     """A MultipleChoiceField whose choices are a model QuerySet."""
     widget = SelectMultiple

django/forms/widgets.py

@@ -131,12 +131,16 @@ def _media(self):
     return property(_media)
 
 class MediaDefiningClass(type):
-    "Metaclass for classes that can have media definitions"
-    def __new__(cls, name, bases, attrs):
-        new_class = super(MediaDefiningClass, cls).__new__(cls, name, bases,
-                                                           attrs)
+    """
+    Metaclass for classes that can have media definitions.
+    """
+    def __new__(mcs, name, bases, attrs):
+        new_class = (super(MediaDefiningClass, mcs)
+            .__new__(mcs, name, bases, attrs))
+
         if 'media' not in attrs:
             new_class.media = media_property(new_class)
+
         return new_class
 
 @python_2_unicode_compatible

docs/internals/deprecation.txt

@@ -467,6 +467,8 @@ these changes.
 * The ``use_natural_keys`` argument for ``serializers.serialize()`` will be
   removed. Use ``use_natural_foreign_keys`` instead.
 
+* ``django.forms.get_declared_fields`` will be removed.
+
 2.0
 ---
 

docs/ref/forms/api.txt

@@ -854,6 +854,13 @@ classes::
     <li>Instrument: <input type="text" name="instrument" /></li>
     <li>Haircut type: <input type="text" name="haircut_type" /></li>
 
+.. versionadded:: 1.7
+
+* It's possible to opt-out from a ``Field`` inherited from a parent class by
+  shadowing it. While any non-``Field`` value works for this purpose, it's
+  recommended to use ``None`` to make it explicit that a field is being
+  nullified.
+
 .. _form-prefix:
 
 Prefixes for forms

docs/releases/1.7.txt

@@ -288,6 +288,14 @@ Forms
   :func:`~django.forms.formsets.formset_factory` to allow validating
   a minimum number of submitted forms.
 
+* The metaclasses used by ``Form`` and ``ModelForm`` have been reworked to
+  support more inheritance scenarios. The previous limitation that prevented
+  inheriting from both ``Form`` and ``ModelForm`` simultaneously have been
+  removed as long as ``ModelForm`` appears first in the MRO.
+
+* It's now possible to opt-out from a ``Form`` field declared in a parent class
+  by shadowing it with a non-``Field`` value.
+
 Internationalization
 ^^^^^^^^^^^^^^^^^^^^
 
@@ -513,6 +521,12 @@ Miscellaneous
   still worked, even though it was not documented or officially supported. If
   you're still using it, please update to the current :setting:`CACHES` syntax.
 
+* The default ordering of ``Form`` fields in case of inheritance has changed to
+  follow normal Python MRO. Fields are now discovered by iterating through the
+  MRO in reverse with the topmost class coming last. This only affects you if
+  you relied on the default field ordering while having fields defined on both
+  the current class *and* on a parent ``Form``.
+
 Features deprecated in 1.7
 ==========================
 

docs/topics/forms/modelforms.txt

@@ -644,11 +644,24 @@ There are a couple of things to note, however.
   used. This means the child's ``Meta``, if it exists, otherwise the
   ``Meta`` of the first parent, etc.
 
-* For technical reasons, a subclass cannot inherit from both a ``ModelForm``
-  and a ``Form`` simultaneously.
+.. versionchanged:: 1.7
 
-Chances are these notes won't affect you unless you're trying to do something
-tricky with subclassing.
+* It's possible to inherit from both ``Form`` and ``ModelForm`` simultaneuosly,
+  however, you must ensure that ``ModelForm`` appears first in the MRO. This is
+  because these classes rely on different metaclasses and a class can only have
+  one metaclass.
+
+.. versionadded:: 1.7
+
+* It's possible to opt-out from a ``Field`` inherited from a parent class by
+  shadowing it. While any non-``Field`` value works for this purpose, it's
+  recommended to use ``None`` to make it explicit that a field is being
+  nullified.
+
+  You can only use this technique to opt out from a field defined declaratively
+  by a parent class; it won't prevent the ``ModelForm`` metaclass from generating
+  a default field. To opt-out from default fields, see
+  :ref:`controlling-fields-with-fields-and-exclude`.
 
 .. _modelforms-factory:
 
@@ -748,6 +761,8 @@ instances of the model, you can specify an empty QuerySet::
    >>> AuthorFormSet(queryset=Author.objects.none())
 
 
+.. _controlling-fields-with-fields-and-exclude:
+
 Controlling which fields are used with ``fields`` and ``exclude``
 -----------------------------------------------------------------
 

tests/forms_tests/tests/test_forms.py

@@ -1302,10 +1302,10 @@ class Beatle(Person, Instrument):
             haircut_type = CharField()
 
         b = Beatle(auto_id=False)
-        self.assertHTMLEqual(b.as_ul(), """<li>First name: <input type="text" name="first_name" /></li>
+        self.assertHTMLEqual(b.as_ul(), """<li>Instrument: <input type="text" name="instrument" /></li>
+<li>First name: <input type="text" name="first_name" /></li>
 <li>Last name: <input type="text" name="last_name" /></li>
 <li>Birthday: <input type="text" name="birthday" /></li>
-<li>Instrument: <input type="text" name="instrument" /></li>
 <li>Haircut type: <input type="text" name="haircut_type" /></li>""")
 
     def test_forms_with_prefixes(self):