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.
This commit is contained in:
Marc Tamlyn 2014-05-24 10:52:18 +01:00
parent f47e226ff7
commit 9fb0f5dddc
5 changed files with 39 additions and 13 deletions

View File

@ -92,8 +92,8 @@ class DeclarativeFieldsMetaclass(MediaDefiningClass):
declared_fields.update(base.declared_fields) declared_fields.update(base.declared_fields)
# Field shadowing. # Field shadowing.
for attr in base.__dict__.keys(): for attr, value in base.__dict__.items():
if attr in declared_fields: if value is None and attr in declared_fields:
declared_fields.pop(attr) declared_fields.pop(attr)
new_class.base_fields = declared_fields new_class.base_fields = declared_fields

View File

@ -982,10 +982,20 @@ classes::
.. versionadded:: 1.7 .. versionadded:: 1.7
* It's possible to opt-out from a ``Field`` inherited from a parent class by * It's possible to declaratively remove a ``Field`` inherited from a parent
shadowing it. While any non-``Field`` value works for this purpose, it's class by setting the name to be ``None`` on the subclass. For example::
recommended to use ``None`` to make it explicit that a field is being
nullified. >>> 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: .. _form-prefix:

View File

@ -544,8 +544,8 @@ Forms
inheriting from both ``Form`` and ``ModelForm`` simultaneously have been inheriting from both ``Form`` and ``ModelForm`` simultaneously have been
removed as long as ``ModelForm`` appears first in the MRO. 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 * It's now possible to remove a field from a ``Form`` when subclassing by
by shadowing it with a non-``Field`` value. setting the name to ``None``.
* The new :meth:`~django.forms.Form.add_error()` method allows adding errors * The new :meth:`~django.forms.Form.add_error()` method allows adding errors
to specific form fields. to specific form fields.

View File

@ -666,10 +666,8 @@ There are a couple of things to note, however.
.. versionadded:: 1.7 .. versionadded:: 1.7
* It's possible to opt-out from a ``Field`` inherited from a parent class by * It's possible to declaratively remove a ``Field`` inherited from a parent class by
shadowing it. While any non-``Field`` value works for this purpose, it's setting the name to be ``None`` on the subclass.
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 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 by a parent class; it won't prevent the ``ModelForm`` metaclass from generating

View File

@ -2227,7 +2227,7 @@ class ModelFormInheritanceTests(TestCase):
self.assertEqual(list(ModelForm().fields.keys()), ['name', 'age']) self.assertEqual(list(ModelForm().fields.keys()), ['name', 'age'])
def test_field_shadowing(self): def test_field_removal(self):
class ModelForm(forms.ModelForm): class ModelForm(forms.ModelForm):
class Meta: class Meta:
model = Writer 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, Mixin), {})().fields.keys()), ['name', 'age'])
self.assertEqual(list(type(str('NewForm'), (ModelForm, Form), {'age': None})().fields.keys()), ['name']) 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 StumpJokeForm(forms.ModelForm):
class Meta: class Meta: