ModelForm.Meta.exclude only excludes model fields, not form fields

[levity]

If I have a ModelForm that inherits from another form, and I define exclude and/or fields, these only affect fields on the model. I cannot use them to exclude any non-model fields that I may have added to the ancestor form. E.g.:

class AForm(ModelForm):
  extra_non_model_field = forms.CharField()

class BForm(AForm):
  class Meta:
    model = Foo
    exclude = ('extra_non_model_field',) # this doesn't work, due to the way ModelFormMetaclass.__new__ is written

Sure that's a pathological example, but there are more sensible situations in which this is an issue. Seems to me it's more intuitive to expect the exclude/fields options to affect all fields, not just model fields.

A fix would be to add this at django/forms/models.py line 180:

        [declared_fields.pop(f) for f in declared_fields.keys() if f not in opts.fields or f in opts.exclude]

[levity]

Tthis also fails to work for model fields that are modified in the ancestor form, e.g.:

class Foo(models.Model):
  bar = models.IntegerField()

class Form1(forms.ModelForm):
  bar = forms.ChoiceField(choices=((1,'one'),(2,'two')))
  class Meta:
    model = Foo
  
class Form2(forms.ModelForm):
  class Meta(Form1.Meta):
    exclude = ('bar',) # won't work

[marcinnowak]

1. patch should be changed a bit to avoid "NoneType is not iterable" error.

  [declared_fields.pop(f) for f in declared_fields.keys() if f not in (opts.fields or []) or f in (opts.exclude or [])]

2. this patch needs improvement - if fields aren't set in Meta class, overriding the default field types does not work.

[marcinnowak]

Better patch

[allanw]

Is there any chance we could get this fixed in trunk sometime soon?

[shimonrura]

updated patch with tests based on r15800

[shimonrura]

I've attached an updated patch that includes tests for the new behavior. However, this change in behavior breaks several other tests. One newly-failing test is also in the modelform tests (tests/regressiontests/forms/tests/models.py), but several are elsewhere in the test suite.

That suggests to me that there may be lots of code built on the assumption that the Meta.exclude attribute doesn't affect fields explicitly declared in a superclass. While changing this behavior seems useful to me, it probably merits more consideration, to either OK the change and accept the consequences for users, or to provide an alternate way to achieve exclusion of fields inherited from superclasses (which may arguably be useful outside of ModelForms).

The patch still needs work (changes to several other tests that fail as a result of the change).

[anonymous]

The problem still occurs in Django 1.3
Also, if you set form fields to Required=False, they are still required in form validation.
Here is an example :

class RegistrationForm(forms.ModelForm):
    #---------------------------------------------------- Extra form fields
    email_confirm = forms.EmailField(widget = forms.TextInput(attrs = {'size':20}))
    password_confirm = forms.CharField(required=False, widget = forms.PasswordInput(attrs = {'size':20}))
    eula = forms.BooleanField(required=False, label=u"CGU")
    #===========================================================================
    # Metadata
    #===========================================================================
    class Meta:
        model = User
        fields = ('username', 'email', 'email_confirm', 'password', 'first_name', 'last_name')

[Koen Biermans]

patch with test and doc for 8620

[Koen Biermans]

Added an updated patch, that only implements leaving out an explicitly declared field with Meta.exclude, and does not require adding explicitly declared fields into the Meta.fields list like the previous patch did (which was backwards incompatible).

Added a note to the documentation also.

[Cody A.W. Somerville]

See duplicate ticket:13971 for additional discussion/patches on this issue.

[Tim Graham]

I think this should be fine backwards compatibility-wise, but a posted a ​PR just to give greater visibility to the patch before I commit it.

[Marc Tamlyn]

At the moment, I'm definitely -0 on this change in functionality. I believe that fields and exclude are shortcuts and are secondary to actually defining the fields - i.e. the field definitions on the form should *always* take precedence. I believe that the subclassing issue can be solved by setting username = Noneon the subclass.

In particular this introduces a disparity between Form and ModelForm - I don't want to recommend this as a subclassing route if it doesn't actually work in the general case.

[loic84]

This may or may not be the right solution to this issue, but this PR allows shadowing a declarative field and would enable the username = None example from the previous comment: ​https://github.com/django/django/pull/1382.

[Tim Graham <timograham@…>]

In 54cd930baf49c45e3e91338a9f4a56d19090ff25:

Clarfied the ModelForm docs with respect to generated vs. declared fields.

The ModelForm docs suggested that fields defined declaratively override
default fields generated from the form Meta.

This is conceptually wrong, especially with inheritance in mind. Meta is
usually defined on the topmost ModelForm subclass, while fields can come
from anywhere in the MRO, especially base classes; therefore we suggested
that something defined in a base class override something from a subclass.

This patch rephrases the docs around the idea that Meta is used to generate
*missing* fields.

Refs #8620, #19617.

Thanks @mjtamlyn and @timgraham for the review.

[Tim Graham <timograham@…>]

In 8222a48253e61f4d3c1d30156a5f9e0d59a7c56c:

[1.6.x] Clarfied the ModelForm docs with respect to generated vs. declared fields.

The ModelForm docs suggested that fields defined declaratively override
default fields generated from the form Meta.

This is conceptually wrong, especially with inheritance in mind. Meta is
usually defined on the topmost ModelForm subclass, while fields can come
from anywhere in the MRO, especially base classes; therefore we suggested
that something defined in a base class override something from a subclass.

This patch rephrases the docs around the idea that Meta is used to generate
*missing* fields.

Refs #8620, #19617.

Thanks @mjtamlyn and @timgraham for the review.

Backport of 54cd930baf from master

[Loic Bistuer <loic.bistuer@…>]

In b16dd1fe019b38d0dcdf4a400e9377fb06b33178:

Fixed #8620 -- Updated the Form metaclass to support excluding fields by shadowing them.

[Marc Tamlyn <marc.tamlyn@…>]

In be733bf672eebb7d03061a293a2b0b03c727ab44:

[1.7.x] 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.

Backport of 9fb0f5dddc4cf7f2d294af1bcde2c359cffd90a5 from master