FileField special-casing breaks MultiValueField including a FileField

[Carl Meyer]

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

[Carl Meyer]

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

[Carl Meyer]

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.

[Richard Terry]

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

[Carl Meyer]

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

[Richard Terry]

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.

[Carl Meyer]

add a docstring note to MultiValueField

[Carl Meyer]

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.

[Russell Keith-Magee]

MultiValueField ​is documented.

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

[Carl Meyer]

#6405 closed as duplicate of this.

[Vasily Alexeev]

What's the status of this bug? Is there any work needed to be done so it finally makes it in a release?

[Carl Meyer]

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.

[Vasily Alexeev]

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.

[Carl Meyer]

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.

[bradleyayers]

Updated to apply cleanly to 1.4.3