From ee4edb1eda2ac8f09eb298929282b44776930c89 Mon Sep 17 00:00:00 2001 From: Tim Graham Date: Fri, 21 Mar 2014 20:44:34 -0400 Subject: [PATCH] Made ModelForms raise ImproperlyConfigured if the list of fields is not specified. Also applies to modelform(set)_factory and generic model views. refs #19733. --- django/forms/models.py | 38 +++++++------------ django/views/generic/edit.py | 10 ++--- docs/ref/class-based-views/mixins-editing.txt | 11 ++++-- docs/ref/forms/models.txt | 12 +++--- .../class-based-views/generic-editing.txt | 15 ++++---- docs/topics/forms/modelforms.txt | 10 ++--- tests/generic_views/test_edit.py | 38 +++++++------------ tests/model_forms/tests.py | 33 ++++++---------- tests/model_formsets/tests.py | 10 +++++ 9 files changed, 77 insertions(+), 100 deletions(-) diff --git a/django/forms/models.py b/django/forms/models.py index 4f5cb261462..0e544a06c69 100644 --- a/django/forms/models.py +++ b/django/forms/models.py @@ -6,10 +6,9 @@ and database field objects. from __future__ import unicode_literals from collections import OrderedDict -import warnings from django.core.exceptions import ( - ValidationError, NON_FIELD_ERRORS, FieldError) + ImproperlyConfigured, ValidationError, NON_FIELD_ERRORS, FieldError) from django.forms.fields import Field, ChoiceField from django.forms.forms import DeclarativeFieldsMetaclass, BaseForm from django.forms.formsets import BaseFormSet, formset_factory @@ -17,7 +16,6 @@ from django.forms.utils import ErrorList from django.forms.widgets import (SelectMultiple, HiddenInput, MultipleHiddenInput) from django.utils import six -from django.utils.deprecation import RemovedInDjango18Warning from django.utils.encoding import smart_text, force_text from django.utils.text import get_text_list, capfirst from django.utils.translation import ugettext_lazy as _, ugettext @@ -266,12 +264,11 @@ class ModelFormMetaclass(DeclarativeFieldsMetaclass): 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. - warnings.warn("Creating a ModelForm without either the 'fields' attribute " - "or the 'exclude' attribute is deprecated - form %s " - "needs updating" % name, - RemovedInDjango18Warning, stacklevel=2) + raise ImproperlyConfigured( + "Creating a ModelForm without either the 'fields' attribute " + "or the 'exclude' attribute is prohibited; form %s " + "needs updating." % name + ) if opts.fields == ALL_FIELDS: # Sentinel for fields_for_model to indicate "get the list of @@ -528,14 +525,12 @@ def modelform_factory(model, form=ModelForm, fields=None, exclude=None, 'formfield_callback': formfield_callback } - # The ModelFormMetaclass will trigger a similar warning/error, but this will - # be difficult to debug for code that needs updating, so we produce the - # warning here too. if (getattr(Meta, 'fields', None) is None and getattr(Meta, 'exclude', None) is None): - warnings.warn("Calling modelform_factory without defining 'fields' or " - "'exclude' explicitly is deprecated", - RemovedInDjango18Warning, stacklevel=2) + raise ImproperlyConfigured( + "Calling modelform_factory without defining 'fields' or " + "'exclude' explicitly is prohibited." + ) # Instatiate type(form) in order to use the same metaclass as form. return type(form)(class_name, (form,), form_class_attrs) @@ -814,20 +809,15 @@ def modelformset_factory(model, form=ModelForm, formfield_callback=None, """ Returns a FormSet class for the given Django model class. """ - # modelform_factory will produce the same warning/error, but that will be - # difficult to debug for code that needs upgrading, so we produce the - # warning here too. This logic is reproducing logic inside - # modelform_factory, but it can be removed once the deprecation cycle is - # complete, since the validation exception will produce a helpful - # stacktrace. meta = getattr(form, 'Meta', None) if meta is None: meta = type(str('Meta'), (object,), {}) if (getattr(meta, 'fields', fields) is None and getattr(meta, 'exclude', exclude) is None): - warnings.warn("Calling modelformset_factory without defining 'fields' or " - "'exclude' explicitly is deprecated", - RemovedInDjango18Warning, stacklevel=2) + raise ImproperlyConfigured( + "Calling modelformset_factory without defining 'fields' or " + "'exclude' explicitly is prohibited." + ) form = modelform_factory(model, form=form, fields=fields, exclude=exclude, formfield_callback=formfield_callback, diff --git a/django/views/generic/edit.py b/django/views/generic/edit.py index d581279f28a..abc8b99ce29 100644 --- a/django/views/generic/edit.py +++ b/django/views/generic/edit.py @@ -1,9 +1,6 @@ -import warnings - from django.core.exceptions import ImproperlyConfigured from django.forms import models as model_forms from django.http import HttpResponseRedirect -from django.utils.deprecation import RemovedInDjango18Warning from django.utils.encoding import force_text from django.views.generic.base import TemplateResponseMixin, ContextMixin, View from django.views.generic.detail import (SingleObjectMixin, @@ -112,9 +109,10 @@ class ModelFormMixin(FormMixin, SingleObjectMixin): model = self.get_queryset().model if self.fields is None: - warnings.warn("Using ModelFormMixin (base class of %s) without " - "the 'fields' attribute is deprecated." % self.__class__.__name__, - RemovedInDjango18Warning) + raise ImproperlyConfigured( + "Using ModelFormMixin (base class of %s) without " + "the 'fields' attribute is prohibited." % self.__class__.__name__ + ) return model_forms.modelform_factory(model, fields=self.fields) diff --git a/docs/ref/class-based-views/mixins-editing.txt b/docs/ref/class-based-views/mixins-editing.txt index dbe1c915473..cfa343c9370 100644 --- a/docs/ref/class-based-views/mixins-editing.txt +++ b/docs/ref/class-based-views/mixins-editing.txt @@ -129,15 +129,18 @@ ModelFormMixin .. attribute:: fields - .. versionadded:: 1.6 - A list of names of fields. This is interpreted the same way as the ``Meta.fields`` attribute of :class:`~django.forms.ModelForm`. This is a required attribute if you are generating the form class automatically (e.g. using ``model``). Omitting this attribute will - result in all fields being used, but this behavior is deprecated - and will be removed in Django 1.8. + result in an :exc:`~django.core.exceptions.ImproperlyConfigured` + exception. + + .. versionchanged:: 1.8 + + Previously, omitting this attribute was allowed and resulted in + a form with all fields of the model. .. attribute:: success_url diff --git a/docs/ref/forms/models.txt b/docs/ref/forms/models.txt index 1d2b468553f..9700c1ed384 100644 --- a/docs/ref/forms/models.txt +++ b/docs/ref/forms/models.txt @@ -34,16 +34,16 @@ Model Form Functions See :ref:`modelforms-factory` for example usage. - .. versionchanged:: 1.6 - You must provide the list of fields explicitly, either via keyword arguments ``fields`` or ``exclude``, or the corresponding attributes on the form's inner ``Meta`` class. See :ref:`modelforms-selecting-fields` for more - information. Omitting any definition of the fields to use will result in all - fields being used, but this behavior is deprecated. + information. Omitting any definition of the fields to use will result in + an :exc:`~django.core.exceptions.ImproperlyConfigured` exception. - The ``localized_fields``, ``labels``, ``help_texts``, and - ``error_messages`` parameters were added. + .. versionchanged:: 1.8 + + Previously, omitting the list of fields was allowed and resulted in + a form with all fields of the model. .. function:: 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, widgets=None, validate_max=False, localized_fields=None, labels=None, help_texts=None, error_messages=None) diff --git a/docs/topics/class-based-views/generic-editing.txt b/docs/topics/class-based-views/generic-editing.txt index f12672df691..44680148abb 100644 --- a/docs/topics/class-based-views/generic-editing.txt +++ b/docs/topics/class-based-views/generic-editing.txt @@ -128,16 +128,15 @@ here; we don't have to write any logic ourselves:: We have to use :func:`~django.core.urlresolvers.reverse_lazy` here, not just ``reverse`` as the urls are not loaded when the file is imported. -.. versionchanged:: 1.6 +The ``fields`` attribute works the same way as the ``fields`` attribute on the +inner ``Meta`` class on :class:`~django.forms.ModelForm`. Unless you define the +form class in another way, the attribute is required and the view will raise +an :exc:`~django.core.exceptions.ImproperlyConfigured` exception if it's not. -In Django 1.6, the ``fields`` attribute was added, which works the same way as -the ``fields`` attribute on the inner ``Meta`` class on -:class:`~django.forms.ModelForm`. - -Omitting the fields attribute will work as previously, but is deprecated and -this attribute will be required from 1.8 (unless you define the form class in -another way). +.. versionchanged:: 1.8 + Omitting the ``fields`` attribute was previously allowed and resulted in a + form with all of the model's fields. Finally, we hook these new views into the URLconf:: diff --git a/docs/topics/forms/modelforms.txt b/docs/topics/forms/modelforms.txt index bd312562cbe..a55e7a5ced3 100644 --- a/docs/topics/forms/modelforms.txt +++ b/docs/topics/forms/modelforms.txt @@ -430,13 +430,11 @@ In addition, Django applies the following rule: if you set ``editable=False`` on the model field, *any* form created from the model via ``ModelForm`` will not include that field. -.. versionchanged:: 1.6 - - Before version 1.6, the ``'__all__'`` shortcut did not exist, but omitting - the ``fields`` attribute had the same effect. Omitting both ``fields`` and - ``exclude`` is now deprecated, but will continue to work as before until - version 1.8 +.. versionchanged:: 1.8 + In older versions, omitting both ``fields`` and ``exclude`` resulted in + a form with all the model's fields. Doing this now raises an + :exc:`~django.core.exceptions.ImproperlyConfigured` exception. .. note:: diff --git a/tests/generic_views/test_edit.py b/tests/generic_views/test_edit.py index d7982685c01..86a02603f6d 100644 --- a/tests/generic_views/test_edit.py +++ b/tests/generic_views/test_edit.py @@ -1,6 +1,5 @@ from __future__ import unicode_literals -import warnings from unittest import expectedFailure from django.core.exceptions import ImproperlyConfigured @@ -8,7 +7,6 @@ from django.core.urlresolvers import reverse from django import forms from django.test import TestCase from django.test.client import RequestFactory -from django.utils.deprecation import RemovedInDjango18Warning from django.views.generic.base import View from django.views.generic.edit import FormMixin, ModelFormMixin, CreateView @@ -151,33 +149,23 @@ class CreateViewTests(TestCase): ['name']) def test_create_view_all_fields(self): + class MyCreateView(CreateView): + model = Author + fields = '__all__' - with warnings.catch_warnings(record=True) as w: - warnings.simplefilter("always", RemovedInDjango18Warning) - - class MyCreateView(CreateView): - model = Author - fields = '__all__' - - self.assertEqual(list(MyCreateView().get_form_class().base_fields), - ['name', 'slug']) - self.assertEqual(len(w), 0) + self.assertEqual(list(MyCreateView().get_form_class().base_fields), + ['name', 'slug']) def test_create_view_without_explicit_fields(self): + class MyCreateView(CreateView): + model = Author - with warnings.catch_warnings(record=True) as w: - warnings.simplefilter("always", RemovedInDjango18Warning) - - class MyCreateView(CreateView): - model = Author - - # Until end of the deprecation cycle, should still create the form - # as before: - self.assertEqual(list(MyCreateView().get_form_class().base_fields), - ['name', 'slug']) - - # but with a warning: - self.assertEqual(w[0].category, RemovedInDjango18Warning) + message = ( + "Using ModelFormMixin (base class of MyCreateView) without the " + "'fields' attribute is prohibited." + ) + with self.assertRaisesMessage(ImproperlyConfigured, message): + MyCreateView().get_form_class() class UpdateViewTests(TestCase): diff --git a/tests/model_forms/tests.py b/tests/model_forms/tests.py index 58ed7cc0fb2..c19461ac259 100644 --- a/tests/model_forms/tests.py +++ b/tests/model_forms/tests.py @@ -4,10 +4,9 @@ import datetime import os from decimal import Decimal from unittest import skipUnless -import warnings from django import forms -from django.core.exceptions import FieldError, NON_FIELD_ERRORS +from django.core.exceptions import FieldError, ImproperlyConfigured, NON_FIELD_ERRORS from django.core.files.uploadedfile import SimpleUploadedFile from django.core.validators import ValidationError from django.db import connection @@ -15,7 +14,6 @@ from django.db.models.query import EmptyQuerySet from django.forms.models import (construct_instance, fields_for_model, model_to_dict, modelform_factory, ModelFormMetaclass) from django.test import TestCase, skipUnlessDBFeature -from django.utils.deprecation import RemovedInDjango18Warning from django.utils._os import upath from django.utils import six @@ -202,24 +200,16 @@ class ModelFormBaseTest(TestCase): self.assertEqual(instance.name, '') def test_missing_fields_attribute(self): - with warnings.catch_warnings(record=True): - warnings.simplefilter("always", RemovedInDjango18Warning) - + message = ( + "Creating a ModelForm without either the 'fields' attribute " + "or the 'exclude' attribute is prohibited; form " + "MissingFieldsForm needs updating." + ) + with self.assertRaisesMessage(ImproperlyConfigured, message): class MissingFieldsForm(forms.ModelForm): class Meta: model = Category - # There is some internal state in warnings module which means that - # if a warning has been seen already, the catch_warnings won't - # have recorded it. The following line therefore will not work reliably: - - # self.assertEqual(w[0].category, RemovedInDjango18Warning) - - # Until end of the deprecation cycle, should still create the - # form as before: - self.assertEqual(list(MissingFieldsForm.base_fields), - ['name', 'slug', 'url']) - def test_extra_fields(self): class ExtraFields(BaseCategoryForm): some_extra_field = forms.BooleanField() @@ -2329,11 +2319,12 @@ class FormFieldCallbackTests(TestCase): def test_modelform_factory_without_fields(self): """ Regression for #19733 """ - with warnings.catch_warnings(record=True) as w: - warnings.simplefilter("always", RemovedInDjango18Warning) - # This should become an error once deprecation cycle is complete. + message = ( + "Calling modelform_factory without defining 'fields' or 'exclude' " + "explicitly is prohibited." + ) + with self.assertRaisesMessage(ImproperlyConfigured, message): modelform_factory(Person) - self.assertEqual(w[0].category, RemovedInDjango18Warning) def test_modelform_factory_with_all_fields(self): """ Regression for #19733 """ diff --git a/tests/model_formsets/tests.py b/tests/model_formsets/tests.py index c1e01cd6bee..4c7d2a535e5 100644 --- a/tests/model_formsets/tests.py +++ b/tests/model_formsets/tests.py @@ -6,6 +6,7 @@ from datetime import date from decimal import Decimal from django import forms +from django.core.exceptions import ImproperlyConfigured from django.db import models from django.forms.models import (_get_foreign_key, inlineformset_factory, modelformset_factory, BaseModelFormSet) @@ -131,6 +132,15 @@ class DeletionTests(TestCase): class ModelFormsetTest(TestCase): + def test_modelformset_factory_without_fields(self): + """ Regression for #19733 """ + message = ( + "Calling modelformset_factory without defining 'fields' or 'exclude' " + "explicitly is prohibited." + ) + with self.assertRaisesMessage(ImproperlyConfigured, message): + modelformset_factory(Author) + def test_simple_save(self): qs = Author.objects.all() AuthorFormSet = modelformset_factory(Author, fields="__all__", extra=3)