Code

Opened 6 years ago

Closed 9 months ago

Last modified 7 weeks ago

#8620 closed New feature (fixed)

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

Reported by: levity Owned by: loic84
Component: Forms Version: master
Severity: Normal Keywords:
Cc: tom@…, tom@…, cody.somerville@…, mmitar@…, loic@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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]

Attachments (3)

django_model_forms_excluding_fields_fix.diff (742 bytes) - added by marcinnowak 5 years ago.
Better patch
django_model_forms_excluding_fields_fix_2.diff (2.4 KB) - added by shimonrura 3 years ago.
updated patch with tests based on r15800
8620_3.diff (3.4 KB) - added by koenb 3 years ago.
patch with test and doc for 8620

Download all attachments as: .zip

Change History (28)

comment:1 Changed 6 years ago by levity

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 Changed 6 years ago by Piotr Lewandowski <django@…>

  • Component changed from Uncategorized to Forms

comment:3 Changed 5 years ago by jacob

  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 5 years ago by 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 [])]
  1. this patch needs improvement - if fields aren't set in Meta class, overriding the default field types does not work.

Changed 5 years ago by marcinnowak

Better patch

comment:5 Changed 4 years ago by allanw

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

comment:6 Changed 4 years ago by kmtracey

  • Has patch set
  • Needs tests set

comment:7 Changed 3 years ago by rizumu

  • Cc tom@… added

comment:8 Changed 3 years ago by shimonrura

  • Owner changed from nobody to shimonrura

Changed 3 years ago by shimonrura

updated patch with tests based on r15800

comment:9 Changed 3 years ago by shimonrura

  • Needs documentation set
  • Patch needs improvement set

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).

comment:10 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to New feature

comment:11 Changed 3 years ago by tomchristie

  • Cc tom@… added
  • Easy pickings unset

comment:12 Changed 3 years ago by anonymous

  • UI/UX unset

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')

Changed 3 years ago by koenb

patch with test and doc for 8620

comment:13 Changed 3 years ago by koenb

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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.

comment:14 Changed 2 years ago by codysomerville

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

comment:15 Changed 2 years ago by codysomerville

  • Cc cody.somerville@… added

comment:16 Changed 2 years ago by mitar

  • Cc mmitar@… added

comment:17 Changed 12 months ago by timo

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.

comment:18 Changed 12 months ago by mjtamlyn

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.

comment:19 Changed 12 months ago by 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.

comment:20 Changed 12 months ago by loic84

  • Cc loic@… added

comment:21 Changed 10 months ago by loic84

  • Owner changed from shimonrura to loic84
  • Status changed from new to assigned

comment:22 Changed 9 months ago by 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.

comment:23 Changed 9 months ago by 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

comment:24 Changed 9 months ago by Loic Bistuer <loic.bistuer@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In b16dd1fe019b38d0dcdf4a400e9377fb06b33178:

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

comment:25 Changed 7 weeks ago by 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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.