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)

14039_r13760.diff (3.5 KB ) - added by Carl Meyer 14 years ago.
14039_r13760_with_note.diff (4.2 KB ) - added by Carl Meyer 14 years ago.
add a docstring note to MultiValueField
14039_1.4.3_with_note.diff (4.5 KB ) - added by bradleyayers 12 years ago.
Updated to apply cleanly to 1.4.3

Download all attachments as: .zip

Change History (17)

comment:1 by Carl Meyer, 14 years ago

Looking into perhaps fixing #7048 and taking care of this as a side effect.

comment:2 by Carl Meyer, 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 Richard Terry, 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 Carl Meyer, 14 years ago

Attachment: 14039_r13760.diff added

comment:4 by Carl Meyer, 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

in reply to:  4 ; comment:5 by Richard Terry, 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 Carl Meyer, 14 years ago

Attachment: 14039_r13760_with_note.diff added

add a docstring note to MultiValueField

in reply to:  5 comment:6 by Carl Meyer, 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 Russell Keith-Magee, 14 years ago

Needs documentation: set
Triage Stage: UnreviewedAccepted

MultiValueField is documented.

(and even if it wasn't... wouldn't this be a grand opportunity to fix the situation :-)

comment:8 by Julien Phalip, 13 years ago

Severity: Normal
Type: Bug

comment:9 by Carl Meyer, 13 years ago

Easy pickings: unset
UI/UX: unset

#6405 closed as duplicate of this.

comment:10 by Vasily Alexeev, 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?

in reply to:  10 comment:11 by Carl Meyer, 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.

comment:12 by Vasily Alexeev, 13 years ago

Cc: mail@… 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.

in reply to:  12 comment:13 by Carl Meyer, 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 bradleyayers, 12 years ago

Cc: bradleyayers added

by bradleyayers, 12 years ago

Attachment: 14039_1.4.3_with_note.diff added

Updated to apply cleanly to 1.4.3

Note: See TracTickets for help on using tickets.
Back to Top