FileField with ModelForm
Brian Rosner
correctly. [1] is the ticket in Trac. The problem, put briefly, is that
when a ModelForm is given an instance it is marked as required and
requires the user to upload a file to bypass the validation. Keep in
mind that if the FileField is explicitly marked as not required then
this is a non-issue.
This is currently a bug in the newforms-admin that I have been working
on for a little while now. After working alot on the formset refactor
patch the problem really seems to lie in newforms itself.
This wasn't a problem with the form_for_* helper functions since
form_for_instance knew the model data when creating the form class.
That is why the FileField model class has the following code in its
formfield method:
# If a file has been provided previously, then the form doesn't require
# that a new file is provided this time.
if 'initial' in kwargs:
defaults['required'] = False
This will no longer work correctly because initial will never be passed
in the kwargs since the form creation is now done in the
ModelFormMetaclass. In looking at the FileInput widget tests it makes
note that its data is useless due to the nature of how file uploads are
handled. That makes complete sense, however, is causing this problem
when the data is coming from the model and not the web browser.
To write a patch I created a new widget named BoundFileInput that
inherits from MultiWidget to display the FileInput and HiddenInput
widgets. The FileInput widget works the same as it does now. The
HiddenInput widget maintains the data that came from the model. As I
understand it this is how it used to work in oldforms, but perhaps just
the admin in trunk.
This is where the problem comes in. To hook BoundFileInput widget into
the correct place I found out that doing so in the newforms FileField
__init__ method is still too soon to check for the initial data. This
then made me realize that perhaps this actually had to be done in the
BaseForm. It wouldn't seem correct to do this only in the ModelForm,
but perhaps I am wrong.
I then figured that changing the widget based on the initial data in
the BaseForm __init__ still seems very hackish, but I may be wrong
again. I then remebered about BoundField which basically defines a
field and its data. Well, that is only used for the display of the
widget as opposed to both display and validation. This seems to be a
gap that might need to be filled to accomplish this fix properly.
So, with all that said I ask:
1. Should FileInput be aware of its data since one use-case is valid.
2. How should the widget be handled when a validation error occurs?
Any ideas/ensight would be greatly appreciated.
[1]: http://code.djangoproject.com/ticket/6302
--
Brian Rosner
http://oebfare.com
Malcolm Tredinnick
On Tue, 2008-01-08 at 18:40 -0700, Brian Rosner wrote:
[...]
> To write a patch I created a new widget named BoundFileInput that
> inherits from MultiWidget to display the FileInput and HiddenInput
> widgets. The FileInput widget works the same as it does now. The
> HiddenInput widget maintains the data that came from the model. As I
> understand it this is how it used to work in oldforms, but perhaps just
> the admin in trunk.
I'd prefer that we worked out a way to get the initial data passed
through. Sticking this in a hidden field would mean that you pretty much
always have to include some kind of security hash as well to avoid the
hidden field value being changed. This is why Russell's improvements for
file uploading in newforms were a slightly better design.
Regards,
Malcolm
--
Plan to be spontaneous - tomorrow.
http://www.pointy-stick.com/blog/
Brian Rosner
<mal...@pointy-stick.com> said:
>
>
> On Tue, 2008-01-08 at 18:40 -0700, Brian Rosner wrote:
> [...]
>
>> To write a patch I created a new widget named BoundFileInput that
>> inherits from MultiWidget to display the FileInput and HiddenInput
>> widgets. The FileInput widget works the same as it does now. The
>> HiddenInput widget maintains the data that came from the model. As I
>> understand it this is how it used to work in oldforms, but perhaps just
>> the admin in trunk.
>
> I'd prefer that we worked out a way to get the initial data passed
> through. Sticking this in a hidden field would mean that you pretty much
> always have to include some kind of security hash as well to avoid the
> hidden field value being changed. This is why Russell's improvements for
> file uploading in newforms were a slightly better design.
Ok, that makes more sense. I had a feeling I was taking the wrong
approach. I hadn't seen his proposal, perhaps I should have done a bit
more research ;) Should FileField be special cased for initial data or
make a global change to all fields?
>
> Regards,
> Malcolm
Malcolm Tredinnick
I'd special-case FileField, since all the related field (ImageField, any
custom upload fields) will be subclasses of that. More specifically,
it's really keyed off the HTML widget, since it's only the file upload
widget that has this particular problem. Without looking at the code, I
couldn't say if it's better to do test the widget type or the formfield
type, but the former is probably more robust, if you can do it.
Regards,
Malcolm
--
Honk if you love peace and quiet.
http://www.pointy-stick.com/blog/
Brian Rosner
<mal...@pointy-stick.com> said:
I have attached a possible patch to the ticket [1]. I would argue that
the widget really doesn't play a role in this issue. The only thing the
widget is doing is rendering the data passed into it which is either
from request.POST or initial data. While it does know how to retreive
its data from request.POST that would only be because of some way it
decided to handle the data in the HTML. Let me know if I am missing
that logic somewhere.
My patch is grabbing the initial data and throwing it at the clean
method of the FileField to ensure it is properly cleaned. Then based on
its clean value save_form_data of the model FileField is able to
determine with a check type check whether or not to save the file. One
thing I want to point out here, which is described in the tests, is
when the file changes there is no garbage collection of the previous
file. I wasn't sure if this should be done by Django, the user or at
all.
Also, fields such as ComboField and MultiValueField probably need to be
made aware of initial data if there is a FileField/ImageField among its
fields.
[1]: http://code.djangoproject.com/ticket/6302
oyvind....@gmail.com
About other fields needing initial data, what about havig a commom
baseclass (for use with isinstance) for those fields or a
takes_initial attribute so it is not only specific to filefields.
On Jan 10, 1:24 am, Brian Rosner <bros...@gmail.com> wrote:
> On 2008-01-09 16:20:39 -0700, Malcolm Tredinnick
>
>
>
>
>
> > On Wed, 2008-01-09 at 14:04 -0700, Brian Rosner wrote:
> >> On 2008-01-08 18:46:02 -0700, Malcolm Tredinnick
Brian Rosner
"oyvind....@gmail.com"
<oyvind....@gmail.com> said:
>
> Looked at the latest patch, great stuff.
>
> About other fields needing initial data, what about havig a commom
> baseclass (for use with isinstance) for those fields or a
> takes_initial attribute so it is not only specific to filefields.
>
I was thinking the same thing. I think adding the takes_initial
attribute would be the best. It would follow the same convention that
needs_multipart_form on the FileInput widget.
oyvind....@gmail.com
About takes_initial, maybe it should just give the field clean a tuple
of (value, initial) to avoid changing the clean arguments?
Not sure if this is a good idea, any core devs have a design decision?
> "oyvind.salt...@gmail.com"
Malcolm Tredinnick
On Thu, 2008-01-10 at 02:12 -0800, oyvind....@gmail.com wrote:
> Looked at the latest patch, great stuff.
>
> About other fields needing initial data, what about havig a commom
> baseclass (for use with isinstance) for those fields or a
> takes_initial attribute so it is not only specific to filefields.
Why is this needed? How many other fields are there that are represented
by an HTML widget which does not allow setting of the initial value?
That is the only case that seems to require this handling. So if there's
another case unrelated to the HTML file upload widget which requires
this, we may have a reason to generalise, otherwise it's not going to be
required. It's not worth over-generalising just because we can; we do it
because we need to.
Cheers,
Malcolm
--
He who laughs last thinks slowest.
http://www.pointy-stick.com/blog/
Brian Rosner
<mal...@pointy-stick.com> said:
>
>
> On Thu, 2008-01-10 at 02:12 -0800,
> oyvind....@gmail.com wrote:
>> Looked at the latest patch, great stuff.
>>
>> About other fields needing initial data, what about havig a commom
>> baseclass (for use with isinstance) for those fields or a
>> takes_initial attribute so it is not only specific to filefields.
>
> Why is this needed? How many other fields are there that are represented
> by an HTML widget which does not allow setting of the initial value?
> That is the only case that seems to require this handling. So if there's
> another case unrelated to the HTML file upload widget which requires
> this, we may have a reason to generalise, otherwise it's not going to be
> required. It's not worth over-generalising just because we can; we do it
> because we need to.
>
To my knowledge there are none. However, there are two fields that may
contain a FileField or ImageField. ComboField and MultiValueField would
need to just pass initial through to the clean method of the FileField.
So a type check needs to be in place for the FileField there as well as
either extending the type check in full_clean to include both
ComboField and MultiValueField or use an takes_initial attribute on the
class. The takes_initial approach may still be overkill for two more
fields, but it would allow users to create some crazy other field that
needs initial or needs to pass it through.
oyvind....@gmail.com
Just want this in quick ;)
On Jan 11, 4:14 am, Brian Rosner <bros...@gmail.com> wrote:
> On 2008-01-10 17:34:11 -0700, Malcolm Tredinnick