Opened 5 years ago

Last modified 3 years ago

#14039 new Bug

FileField special-casing breaks MultiValueField including a FileField

Reported by: carljm Owned by: carljm
Component: Forms Version: master
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 carljm 5 years ago.
14039_r13760_with_note.diff (4.2 KB) - added by carljm 5 years ago.
add a docstring note to MultiValueField
14039_1.4.3_with_note.diff (4.5 KB) - added by bradleyayers 3 years ago.
Updated to apply cleanly to 1.4.3

Download all attachments as: .zip

Change History (17)

comment:1 Changed 5 years ago by carljm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 Changed 5 years ago by carljm

  • 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 Changed 5 years ago by radiac

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

Changed 5 years ago by carljm

comment:4 follow-up: Changed 5 years ago by carljm

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

comment:5 in reply to: ↑ 4 ; follow-up: Changed 5 years ago by radiac

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

Changed 5 years ago by carljm

add a docstring note to MultiValueField

comment:6 in reply to: ↑ 5 Changed 5 years ago by carljm

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 Changed 5 years ago by russellm

  • Needs documentation set
  • Triage Stage changed from Unreviewed to Accepted

MultiValueField is documented.

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

comment:8 Changed 4 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:9 Changed 4 years ago by carljm

  • Easy pickings unset
  • UI/UX unset

#6405 closed as duplicate of this.

comment:10 follow-up: Changed 4 years ago by valexeev

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 in reply to: ↑ 10 Changed 4 years ago by carljm

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 follow-up: Changed 4 years ago by valexeev

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

comment:13 in reply to: ↑ 12 Changed 4 years ago by carljm

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 Changed 3 years ago by bradleyayers

  • Cc bradleyayers added

Changed 3 years ago by bradleyayers

Updated to apply cleanly to 1.4.3

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