Opened 11 years ago
Closed 11 years ago
#22510 closed Bug (fixed)
Form fields cannot have names that are present as attributes on the form
Reported by: | Owned by: | nobody | |
---|---|---|---|
Component: | Forms | Version: | 1.7-beta-2 |
Severity: | Release blocker | Keywords: | |
Cc: | loic84 | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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 ).
Change History (19)
comment:1 by , 11 years ago
Cc: | added |
---|---|
Severity: | Normal → Release blocker |
follow-up: 3 comment:2 by , 11 years ago
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.
comment:3 by , 11 years ago
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 setself.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.
comment:4 by , 11 years ago
@gcbirzan, I don't really understand, could you provide a set of examples (in documentation format rather than test)?
comment:5 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:6 by , 11 years ago
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 aShadowField
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.
comment:7 by , 11 years ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
POC for the shadow_fields
option: https://github.com/django/django/pull/2615/.
comment:8 by , 11 years ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | unset |
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.
follow-up: 10 comment:9 by , 11 years ago
Component: | Forms → Documentation |
---|
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.
comment:10 by , 11 years ago
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.
comment:11 by , 11 years ago
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.
comment:12 by , 11 years ago
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.
comment:13 by , 11 years ago
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
.
comment:14 by , 11 years ago
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.
comment:15 by , 11 years ago
Component: | Documentation → Forms |
---|
comment:16 by , 11 years ago
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!
comment:17 by , 11 years ago
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.
comment:18 by , 11 years ago
Pull request for the None
approach: https://github.com/django/django/pull/2706
comment:19 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Loic, could you take a look at this?