From 9fb0f5dddc4cf7f2d294af1bcde2c359cffd90a5 Mon Sep 17 00:00:00 2001 From: Marc Tamlyn Date: Sat, 24 May 2014 10:52:18 +0100 Subject: [PATCH] Fixed #22510 -- Harden field removal to only None. Refs #8620. If we allow any value to remove form fields then we get name clashes with method names, media classes etc. There was a backwards incompatibility introduced meaning ModelForm subclasses with declared fields called media or clean would lose those fields. Field removal is now only permitted by using the sentinel value None. The docs have been slightly reworded to refer to removal of fields rather than shadowing. Thanks to gcbirzan for the report and initial patch, and several of the core team for opinions. --- django/forms/forms.py | 4 ++-- docs/ref/forms/api.txt | 18 ++++++++++++++---- docs/releases/1.7.txt | 4 ++-- docs/topics/forms/modelforms.txt | 6 ++---- tests/model_forms/tests.py | 20 +++++++++++++++++++- 5 files changed, 39 insertions(+), 13 deletions(-) diff --git a/django/forms/forms.py b/django/forms/forms.py index f8681659775..b5cf17f4eb1 100644 --- a/django/forms/forms.py +++ b/django/forms/forms.py @@ -92,8 +92,8 @@ class DeclarativeFieldsMetaclass(MediaDefiningClass): declared_fields.update(base.declared_fields) # Field shadowing. - for attr in base.__dict__.keys(): - if attr in declared_fields: + for attr, value in base.__dict__.items(): + if value is None and attr in declared_fields: declared_fields.pop(attr) new_class.base_fields = declared_fields diff --git a/docs/ref/forms/api.txt b/docs/ref/forms/api.txt index 950bd3a0889..10e85b40194 100644 --- a/docs/ref/forms/api.txt +++ b/docs/ref/forms/api.txt @@ -982,10 +982,20 @@ classes:: .. 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. +* It's possible to declaratively remove a ``Field`` inherited from a parent + class by setting the name to be ``None`` on the subclass. For example:: + + >>> from django import forms + + >>> class ParentForm(forms.Form): + ... name = forms.CharField() + ... age = forms.IntegerField() + + >>> class ChildForm(ParentForm): + ... name = None + + >>> ChildForm().fields.keys() + ... ['age'] .. _form-prefix: diff --git a/docs/releases/1.7.txt b/docs/releases/1.7.txt index a8e600a04b0..0b43b97d73b 100644 --- a/docs/releases/1.7.txt +++ b/docs/releases/1.7.txt @@ -544,8 +544,8 @@ Forms 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. +* It's now possible to remove a field from a ``Form`` when subclassing by + setting the name to ``None``. * The new :meth:`~django.forms.Form.add_error()` method allows adding errors to specific form fields. diff --git a/docs/topics/forms/modelforms.txt b/docs/topics/forms/modelforms.txt index e96345f1554..4ac4dcea905 100644 --- a/docs/topics/forms/modelforms.txt +++ b/docs/topics/forms/modelforms.txt @@ -666,10 +666,8 @@ There are a couple of things to note, however. .. 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. +* It's possible to declaratively remove a ``Field`` inherited from a parent class by + setting the name to be ``None`` on the subclass. 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 diff --git a/tests/model_forms/tests.py b/tests/model_forms/tests.py index c3b4c339c08..de96246c6fd 100644 --- a/tests/model_forms/tests.py +++ b/tests/model_forms/tests.py @@ -2227,7 +2227,7 @@ class ModelFormInheritanceTests(TestCase): self.assertEqual(list(ModelForm().fields.keys()), ['name', 'age']) - def test_field_shadowing(self): + def test_field_removal(self): class ModelForm(forms.ModelForm): class Meta: model = Writer @@ -2250,6 +2250,24 @@ class ModelFormInheritanceTests(TestCase): self.assertEqual(list(type(str('NewForm'), (ModelForm, Form, Mixin), {})().fields.keys()), ['name', 'age']) self.assertEqual(list(type(str('NewForm'), (ModelForm, Form), {'age': None})().fields.keys()), ['name']) + def test_field_removal_name_clashes(self): + """Regression test for https://code.djangoproject.com/ticket/22510.""" + + class MyForm(forms.ModelForm): + media = forms.CharField() + + class Meta: + model = Writer + fields = '__all__' + + class SubForm(MyForm): + media = None + + self.assertIn('media', MyForm().fields) + self.assertNotIn('media', SubForm().fields) + self.assertTrue(hasattr(MyForm, 'media')) + self.assertTrue(hasattr(SubForm, 'media')) + class StumpJokeForm(forms.ModelForm): class Meta: