366227 - attachment.cgi could offer UI for viewing filenames of the contents of ZIP attachment

Status: RESOLVED DUPLICATE of bug 385338

(Bugzilla :: Attachments & Requests, enhancement)

Description

[victory <[e-mail]>]

Attachment: patch for tip

this is the first step of the bug 270765 I believe :-)

this patch let action=showarc show
 what files are archived in the zip attachment.

Comment 1

[Frédéric Buclin]

Comment on attachment 250751 [details] [diff] [review]
patch for tip

This should be a method of attachment objects, and so should go into Attachment.pm.

Comment 2

[victory <[e-mail]>]

Attachment: patch for tip

Comment 3

[Max Kanat-Alexander]

Comment on attachment 250758 [details] [diff] [review]
patch for tip

Regardless of anything else, Archive::Zip is not a standard perl module, so you'd have to add it to the optional requirements.

I didn't review anything else about this (LpSolit would be the person to ask).

Comment 4

[:glob ✱]

+    my ($fh, $tempfilename) = tempfile( UNLINK => 0, BINMODE => 1 );
+    my $attachment = Bugzilla::Attachment->get($attach_id);
+    open A,'>',"$tempfilename";
+    binmode A;
+    print A $attachment->data;
+    close A;

this should be:

  my $attachment = Bugzilla::Attachment->get($attach_id);
  my ($fh, $tempfilename) = tempfile(BINMODE => 1);
  $fh->print($attachment->data);
  $fh->close();

Comment 5

[victory <[e-mail]>]

Attachment: patch for tip

Comment 6

[Frédéric Buclin]

Comment on attachment 250936 [details] [diff] [review]
patch for tip

>Index: attachment.cgi

>+sub showarc

It looks like there is a lot of duplicated code. This should be refactored a bit. Moreover, showarch would be more explicit as show_archive.



>Index: Bugzilla/Attachment.pm

>+sub arc{
>+    my ($class, $attach_id) = @_;
>+    use Archive::Zip qw( :ERROR_CODES );
>+    use File::Temp qw/ tempfile /;

This doesn't work. Putting use Foo::Bar doesn't prevent to use it everytime you load Bugzilla::Attachment. And it fails if you don't have this package installed.


>+    my $attachment = Bugzilla::Attachment->get($attach_id);

This should be a method, so you don't have to recreate the object again. Use $self instead.


>+    return $somezip->memberNames();

So all this method does is to display the list of files in the archive? Couldn't this be combined with existing templates, i.e. display the list and offer to download the attachment at the same time?

Moreover, I remember a bug using jar: to view their content (this requires Firefox 3, though). We should avoid to duplicate the features.



>Index: Bugzilla/Install/Requirements.pm

>+    {
>+        package => 'File-Temp',
>+        module  => 'File::Temp',

Isn't this package always available?



>+++ template/en/default/attachment/showarc.html.tmpl	2007-01-07 

This should be combined with existing templates.


>+  style = "
>+    table.attachment_info th { text-align: right; vertical-align: top; }
>+    table.attachment_info td { text-align: left; vertical-align: top; }
>+    #noview { text-align: left; vertical-align: middle; }
>+
>+    table#flags th, table#flags td { font-size: small; vertical-align: baseline; text-align: left; }

No more hardcoded CSS please. Use skins/standard/*.css files instead.


>+        <b>Description:</b><br>
>+          [% attachment.description %]<br>

/me loves XSS! You have to filter this field and some others in this template.

Comment 8

[Frédéric Buclin]

Marking as a dupe of bug 385338 which has a much longer discussion.