Form fields cannot have names that are present as attributes on the form

[gc@…]

This was broken in #8620. Fixing this is impossible, unless shadowing is only enabled for None, at which point the fix becomes trivial ( ​https://github.com/gcbirzan/django/commit/f62f991444610ebadad809981d9a037298a660db ).

[Tim Graham]

Loic, could you take a look at this?

[loic84]

At the time I suggested we only allowed shadowing with None, @mjtamlyn convinced me that we should follow normal Python inheritance/shadowing and I think that was a good decision.

I see value in matching as closely as possible the MRO behavior, other than the fact that Field magically remove themselves from the class (whether shadowed or not), we are pretty good on that front.

Inheritance makes this kind of clash possible, but a single form couldn't define both a Field and an attribute anyway.

To expose a media=42 attribute like in the test of the proposed patch, one can set self.media in __init__(), or ensure that it appears first in the MRO. By ensuring it appears first in the MRO it won't shadow any field, and it will not be shadowed itself because fields are removed from the class.

We document:

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.

Which is exactly the case described in the test of the proposed patch.

[gcbirzan]

Replying to loic84:

At the time I suggested we only allowed shadowing with None, @mjtamlyn convinced me that we should follow normal Python inheritance/shadowing and I think that was a good decision.

True, but the problem is that the builtin Form fields also have this property. It's made worse by, for example, media being set by magic via the previous meta class, so it's set on each of the classes that inherit from Form.

I see value in matching as closely as possible the MRO behavior, other than the fact that Field magically remove themselves from the class (whether shadowed or not), we are pretty good on that front.

Inheritance makes this kind of clash possible, but a single form couldn't define both a Field and an attribute anyway.

To expose a media=42 attribute like in the test of the proposed patch, one can set self.media in __init__(), or ensure that it appears first in the MRO. By ensuring it appears first in the MRO it won't shadow any field, and it will not be shadowed itself because fields are removed from the class.

To be honest, I don't remember why media = 42 is in there, it doesn't really matter, it happens either way.

As for the proposed solution, with __init__() is ugly and, at the very least, needs documentation explaining that things you don't manually set might have this effect.

We document:

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.

Which is exactly the case described in the test of the proposed patch.

True. But even so, there's another bug that's fixed by my patch. Reordering the shadowing and collecting means that, if nothing else, defining media as a field on a form will work, if there's no other inheritance.

[loic84]

@gcbirzan, I don't really understand, could you provide a set of examples (in documentation format rather than test)?

[loic84]

I've discussed this with @gcbirzan on IRC, there is indeed a regression on attributes called media.

Since MediaDefiningClass monkeypatches each subclass with a media attribute, it effectively shadows any field called media.

Moving the shadowing before the parent's fields discovery as suggested above would allow a single Form to have a media field, but that wouldn't work when inheritance is involved anyway.

The problem could also manifest itself with overridden methods that would clash with a field name:

Form(forms.Form):
    clean = forms.BooleanField()

Form2(Form):
    def clean(self):
        pass

Options I can think of:

• Backward incompatible change, care must be taken when subclassing a form not to accidentally shadow a field with a method, and a special mention for media.
• Change the shadowing logic to use a sentinel value (e.g. None, or a ShadowField object).
• A declarative class attribute to list the fields to shadow:

  Form(forms.Form):
      something = forms.CharField()
  
  Form2(Form):
      shadow_fields = ['something']
  

For reference the ticket for field shadowing is #8620.

[loic84]

POC for the shadow_fields option: ​https://github.com/django/django/pull/2615/.

[loic84]

I've renamed shadow_fields to ignore_fields on the premise that "overloading" (which forms support) is also a form of shadowing, whereas this feature is only about opting out from previously declared fields.

@apollo13 commented on the PR "Shadow_fields should at least be on meta to not clutter the form namespace any more". The issue is that normal Form don't support Meta, and I'm not convinced we should start introducing it. We already have a bunch of fields related attributes, namely fields, base_fields, declared_fields, IMO ignore_fields fits nicely.

I like this implementation better because there is now an obvious parallel between the handling of declared_fields and ignore_fields, it also does away with fiddling with __dict__.

To move this ticket forward we need to make a design decision, but if this solution is retained the patch on the PR is ready for review.

[Marc Tamlyn]

I am strongly in favour of loic's option number 1 - change no code and document the problems with naming fields after things which would be on the form anyway. In the spirit of the fix for #8620 (making forms behave more like "normal" python objects), this makes sense anyway.

As a side note, it would be much more incompatible (but imo better) to not remove the field definitions from the form object at all - i.e. MyForm().foo == MyForm['foo']. This may have serious implications for the use of forms in DTL though. However, it would potentially give us the option of using the metaclass simply to define fields after the class is resolved normally by python. This is likely a step too far.

[anonymous]

Replying to mjtamlyn:

I am strongly in favour of loic's option number 1 - change no code and document the problems with naming fields after things which would be on the form anyway. In the spirit of the fix for #8620 (making forms behave more like "normal" python objects), this makes sense anyway.

This is, in no way similar to how normal python objects behave. In particular, normal Python objects do not silently discard attributes you put on the class definition. Also, normal objects, as you mention, do not magic attributes into __getitem__() stuff.

[loic84]

Allowing MyForm().foo == MyForm['foo'] would require additional magic. Fields defined declaratively are stored in Form.declared_fields, those fields should be considered immutable, they are then cloned into Form.fields in Form.__init__ where they can be freely modified by form instances. If we just stuck to python inheritance and didn't remove fields, we'd expose a big footgun where altering self.fieldname would alter the form globally. We could mitigate the problem by shadowing declared fields by their clone in __init__, but that's just additional magic, and it doesn't offer much benefit over Form.fields. All in all, I'm -0 on this option.

[Marc Tamlyn]

Ok fair point I now see why MyForm.foo !== MyForm['foo'].

Having reviewed the available options, I'm happy to make a design decision to harden the current recommendation to use None into a requirement to use None assuming that solves this ticket (which I believe it does). If there are no objections, I can commit this at the sprints on Friday.

[loic84]

My preference still goes to ignore_fields but I wouldn't stand in the way of using None as a sentinel value since it's a decent solution that would indeed fix the regression on Form.media.

[Marc Tamlyn]

Ugh, so much for getting things done at sprints, sorry. I discussed with Russ and although part of his argument was "just use del" he decided he was happy enough with None as a sentinel, and preferred it to ignore_fields. This is still on my list, but if anyone else gets time then feel free.

[Aymeric Augustin]

With reference to the list in comment 6, my order of preference is 1, 3, 2.

• It seems quite natural that clashes between field names and form method names cause errors. +0 to just documenting that.
• Adding ignored_fields isn't too bad, but it's one more API that our users may encounter. +0 to adding this API.
• Requiring that shadowing only works with a None is seriously idiosyncratic. -0 to that solution.

I hope this helps!

[loic84]

I guess my preference for 3 comes down to ignored_fields being an API vs. None as a sentinel value being a trick.

Regarding the documentation-only fix, I suspect it'll be an issue to quite a few people. I can imagine it's pretty common to have a model field called media or clean, and since DTL gives more precedence to dict lookups than it does to attribute lookups I suspect most never noticed that they were shadowing form attributes in their templates.

[Marc Tamlyn]

Pull request for the None approach: ​https://github.com/django/django/pull/2706

[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