[Html-widget] file uploads and element_type combined patch

Carl Franks fireartist at gmail.com
Fri Nov 17 10:50:13 GMT 2006


John,

Thanks for the patch.
I'm going through it now, but there's a couple things I'm not sure about...

On 15/11/06, John Napiorkowski <jjn1056 at yahoo.com> wrote:
> Hi,
>
> Here's a patch for the two changes I've been talking
> about the last day anf a half.
>
> First it has the new accessor called 'element_type'
> for the element object, which returns the scalar name
> of whatever you typed into the type field when
> creating the element.  It's just a nicer way of doing
> a regex against ref $element to get the type name.
> The only changes between this and the patch I sent
> earlier today is some documentation about the new
> accessor.
>
> The second patch is more involved, and changes the way
> file uploads are handled.  Here's what I did:
>
> If you don't set $widget->uploads or don't set the
> uploads during process ($widget->process($query,
> $uploads) this works exactly like it did before, 100%
>
> If you DO set either the uploads accessor or pass a
> second parameter to process, the following happens:
>
> First, whenever you used for $uploads is tested to see
> if it is a ref and if it has a method called 'upload'.
>  If it doesn't it croaks out and stops.  We might want
> to have this do a warn or something instead, because I
> imagine this might break some people's stuff.  Let me
> know.

I don't think it's necessary to change uploads()/process() to expect a
Catalyst::Request object, rather than the output from
$c->req->uploads()
Doing this /will/ cause the application to die for everyone using
Catalyst::Plugin::HTML::Widget.
It looks like you're only doing it to use the upload() method to get
the list of upload parameter names - which we should be able to get
with just `keys %$uploads`.
If there's something I'm missing, let me know - otherwise I'll change
it back to expecting a hash-ref.

> If everything is good, it will get the uploads and
> merge them into the local params, not the
> catalyst->request->parameters.

I think I might delay this change, until I've managed to experiment
with it in a couple of applications.
I think it's reasonable for the user to have to use
$result->uploads(), as uploaded files are really a special case from
all other parameters.
I'll experiment with Cat a bit more, and see how it handles uploads,
with regards the param() method.

I think it's also reasonable for there to be a special mechanism for
filters and constraints to handle uploads, as again they're a special
case, and the filter/constraint needs to know that it's getting a
file.

> This get's processed as normal for the rest.
> Afterwards, the results object will give you access to
> the upload object (that have been filtered) via the
> $results->param(upload_field) method.  The only that
> that is changed with this is that before the
> $results->param(...) would give you the filename of
> the uploaded file.

As I said, I'll experiment and see what I think about this.

> This is also behavior that could break existing stuff,
> if you are looking for that filename.  Although
> honestly it's pretty useless, so hopefully this won't
> be an issue.
>
> When you make filters or constraints against an upload
> type field, the value is not a scalar, but the upload
> upload that catalyst or a compatible object presents.
> So if you want to do some checking you'll have to call
> those methods on the object.
>
> Here's an example of a filter I wrote that resizes an
> uploaded image:
>
> my $mediafile_filter = $self->form->filter(
> 'Callback', 'mediafile' );
>
>    $mediafile_filter->callback(sub {
>
>       my ($value) = @_;
>
>       if($value)
>       {
>          my $content_type = $value->type;
>          my $mediafile_out;
>
>          if($c->request->parameters->{media_type_id}
> eq 'profile_photo')
>          {
>             my $mediafile_hd = $value->fh;
>
>             ## Convert to the right size and output as
> a jpeg
>
>             my $upload_img = Imager->new;
>
>             if ($upload_img->read(fh=>$mediafile_hd))
>             {
>                my $resized_img   =
> $upload_img->scale(xpixels => 120, ypixels => 90, type
> => 'min');
>
>                my $background   = Imager->new(xsize =>
> 120, ysize => 90);
>                $background->box(filled => 1, color =>
> 'blue');
>                $background->paste(src=>$resized_img);
>
>                $background->write(data =>
> \$mediafile_out, type => 'jpeg');
>             }
>          }
>
>          return $mediafile_out;
>       }
>
>       return '';
>    });

I think it's wrong for the filter to take in a file upload object, and
return a blob/string.
What if you want to run multiple filters on an uploaded file? The
first one will turn it into a normal string variable and the following
filters will choke.
The filter should write back to the $file->fh() file.

> I also added a test for this new behavior.  I had to
> make some changes to the test mock object to support
> uploads.
>
> Now that we have this, I can update the upload element
> so that all those documented constrainsts (accept,
> size, ect) will actually do something.
>
> let me know if you want a more generic form of the
> image filter above.  I can imagine other people might
> want it.  It's good for when you want to allow a
> client to upload an image but you want to normalize it
> to a certain size and file type.
>
> Anyway, let me know if this is the right tack to take.
>  Awaiting your thoughts.

I'll also take a look at how Catalyst handles multiple inputs with the
same name, where one is a file, and another isn't - and make sure the
changes to $widget->process() work appropriate to that too.
In fact, I'll have to see if there's a HTTP/POST spec/rfc which states
how it /should/ be handled, rather than just how Cat handles it.

Thanks again for the patch, but I think you'll have to be patient to
see it getting committed :)
It's important to get it right first time, er, second time!

Cheers,
Carl



More information about the Html-widget mailing list