Opened 14 years ago
Last modified 12 years ago
#14039 new Bug
FileField special-casing breaks MultiValueField including a FileField
Reported by: | Carl Meyer | Owned by: | Carl Meyer |
---|---|---|---|
Component: | Forms | Version: | dev |
Severity: | Normal | Keywords: | sprintSep2010 |
Cc: | mail@…, bradleyayers | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | yes |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
There are a couple places where FileField is special-cased using isinstance
in the forms code: specifically, in http://code.djangoproject.com/browser/django/trunk/django/forms/forms.py#L280 BaseForm._clean_fields, a FileField's clean method is passed the initial value as well as the data, and in http://code.djangoproject.com/browser/django/trunk/django/forms/forms.py#L438 BoundField.as_widget the initial value is used as the rendered data even when the form is bound.
This handling of FileFields is correct, and makes them behave more naturally with redisplayed bound forms. But, as usual, the use of isinstance
makes things more brittle than necessary. Specifically, in http://bitbucket.org/carljm/django-form-utils django-form-utils I've implemented a ClearableFileField via MultiValueField and MultiWidget (because the ClearableFileField contains both a FileField and a checkbox for clearing it). The isinstance special-casing means the ClearableFileField can't get the right behavior to parallel a FileField (because it's not a FileField subclass, it's a MultiValueField subclass containing a FileField).
It would be better if the special FileField behavior were implemented with a class attribute flag or a polymorphic method on Field, rather than an isinstance
check, so that any field could trigger the appropriate behavior regardless of its inheritance ancestry.
(This is obviously low priority, not expecting anyone else to work on it, just recording it here as a I may find some time to work on a patch).
Attachments (3)
Change History (17)
comment:1 by , 14 years ago
comment:2 by , 14 years ago
Has patch: | set |
---|---|
Keywords: | sprintSep2010 added |
The patch for #7048 only ended up needing to fix one of the isinstance checks as a direct side-effect; this patch fixes the others, replacing them with flag class attributes on Field classes. Tests all pass.
comment:3 by , 14 years ago
Patch needs improvement: | set |
---|
I have also had to add clean_takes_initial=True to forms.fields.MultiValueField and modify its clean() function to test the flag again, because that is responsible for cleaning the individual fields (at least in 1.2.3, line 778).
by , 14 years ago
Attachment: | 14039_r13760.diff added |
---|
follow-up: 5 comment:4 by , 14 years ago
Replying to radiac:
I have also had to add clean_takes_initial=True to forms.fields.MultiValueField and modify its clean() function to test the flag again, because that is responsible for cleaning the individual fields (at least in 1.2.3, line 778).
Thanks for the patch review. I'm not sure that the base MultiValueField implementation should add the complexity necessary to support clean_takes_initial "out of the box". Simply testing the flag on member fields is not enough, because without knowing the specific fields in question I don't think it's possible to generically handle the initial value correctly. The initial arg won't be a list, as the value arg is: should the same initial value really be passed into any member field that asks for an initial value? I don't think this is a valid assumption.
Rather, I think MultiValueField subclasses which want to invoke clean_takes_initial behavior will probably have to override clean() anyway and provide the specific initial-handling that makes sense for their case.
I'll leave the needs_better_patch flag on until we can get a third set of eyes on this, but I think this patch is a useful (if minor) improvement as-is: it makes it possible for subclasses to do the right thing, without forcing them to inherit from a specific base class. If you think you can provide a patch for MultiValueField that makes it automatically handle all cases correctly with initial values, please do!
Git branch is at http://github.com/carljm/django/tree/ticket_14039
follow-up: 6 comment:5 by , 14 years ago
Patch needs improvement: | unset |
---|
Replying to carljm:
Rather, I think MultiValueField subclasses which want to invoke clean_takes_initial behavior will probably have to override clean() anyway and provide the specific initial-handling that makes sense for their case.
I think you're right - I must admit to my version not being generic, hence no patch! I could do a workaround for my specific situation, but because the initial value is in compressed format, a generic solution would really need the initial value to be decompressed first, which is currently done by the widget. Leaving it to be implemented by a subclass therefore seems sensible; perhaps noting that in the comments/documentation would be a good idea.
I've removed the needs_better_patch flag - as you say, I think this a useful improvement which makes it possible for people to take the next steps without needing to resort to patching django themselves.
by , 14 years ago
Attachment: | 14039_r13760_with_note.diff added |
---|
add a docstring note to MultiValueField
comment:6 by , 14 years ago
Replying to radiac:
Leaving it to be implemented by a subclass therefore seems sensible; perhaps noting that in the comments/documentation would be a good idea.
That's a good idea; MultiValueField is undocumented and I'm not going to change that, but I added a note to its docstring.
comment:7 by , 14 years ago
Needs documentation: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
MultiValueField is documented.
(and even if it wasn't... wouldn't this be a grand opportunity to fix the situation :-)
comment:8 by , 13 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
follow-up: 11 comment:10 by , 13 years ago
What's the status of this bug? Is there any work needed to be done so it finally makes it in a release?
comment:11 by , 13 years ago
Replying to valexeev:
What's the status of this bug? Is there any work needed to be done so it finally makes it in a release?
The "needs documentation" flag is set. That means the patch needs to include updates to the documentation, and currently doesn't.
follow-up: 13 comment:12 by , 13 years ago
Cc: | added |
---|
I was thinking about working on documentation for it, but I'm not sure where to start, and, first of all, why changes to the documentation are needed at all.
For me it seems like this change concerns itself with lower-level API and so does only matter for someone who is working on a custom form Field. But this topic isn't documented at all at the moment, so there's just no documentation to be updated.
Please correct me if I'm wrong.
comment:13 by , 13 years ago
Replying to valexeev:
I was thinking about working on documentation for it, but I'm not sure where to start, and, first of all, why changes to the documentation are needed at all.
For me it seems like this change concerns itself with lower-level API and so does only matter for someone who is working on a custom form Field. But this topic isn't documented at all at the moment, so there's just no documentation to be updated.
Please correct me if I'm wrong.
It's true that there is no documentation specifically targeted at authors of custom field subclasses. However, as Russell pointed out above, MultiValueField
is documented, even though it must be subclassed to be useful. So I think the documentation that would make sense here would basically be to move (or copy) the docstring addition from the current patch as a note in the MultiValueField
documentation.
comment:14 by , 12 years ago
Cc: | added |
---|
Looking into perhaps fixing #7048 and taking care of this as a side effect.