If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Provide ability to specify MIME type of attachment when downloading

VERIFIED FIXED in Bugzilla 2.18

Status

()

Bugzilla
Attachments & Requests
P1
enhancement
VERIFIED FIXED
16 years ago
5 years ago

People

(Reporter: WeirdAl, Assigned: WeirdAl)

Tracking

unspecified
Bugzilla 2.18
Other
Other
Bug Flags:
approval +
documentation +

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

16 years ago
Spun off from bug 111520.

From that bug:

--- quote ---
Currently, Bugzilla supports a straight-forward attachment link:

Created an attachment (id=58900)

However, if I as a Bugzilla user could choose multiple mime-types via link for
this attactioment, it would be invaluable:

Created an attachment (id=58900, application/xhtml+xml) (application/xml)
(text/xml) (text/html)

--- end quote ---

I'm requesting this because a lot of XHTML attachments get filed as text/xml or
text/html.  If we can provide an easier way to examine these attachments from
multiple mime-types, and encourage using a  mime-type which is for XHTML only,
this would make testing of XHTML-based bugs much easier.
(Assignee)

Updated

16 years ago
Keywords: xhtml
I expect you'll find that when you go to download the attachment, you need to
have just one MIME type to tell the browser the attachment is.  What purpose is
there of the multiple MIME types?  Searching?
(Assignee)

Comment 2

16 years ago
The purpose is to help us identify and test bugs which are specific to XHTML
instead of HTML or XML.  The mimetype can have a profound effect, as bug 111397,
bug 111514 and bug 111518 demonstrate.

It'd be nice to have one attachment instead of two, three, or four.
I think here is a case where the cost would outweigh the benefit. Converting our
database structure/values to support multiple mime types per attachment surely
isn't worth it.  And I am not convinced this would benefit users outside of the
bugzilla.mozilla.org install.  As a matter of fact, the number of instances this
would be used properly even on b.m.o would be relatively small.  This is
probably a wontfix as this bug stands.

Perhaps a better solution would be a (undocumented?) feature allowing the URL to
specify the MIME type:  /attachment.cgi?id=1234&mimetype=text/plain for example.
 That would involve considerably less work to implement than the current proposal.

Removing xhtml keyword as this bug has nothing to do with support for XHTML.  We
support it, and AFAIK there is no standard that dictates we provide multiple
content types for a downloaded file.
Keywords: xhtml
OS: Windows 98 → other
Priority: -- → P5
Hardware: PC → Other
Whiteboard: WONTFIX?
(Assignee)

Comment 4

16 years ago
That's all I was really asking for, a way to call a particular mime-type from
the server.  Sending the mime-type via HTTP GET is fine with me.  I was simply
hoping b.m.o could provide four links when it has a file of that mime-type.
Priority: P5 → P4
Summary: Provide multiple XHTML mime-types for application/xhtml+xml → Provide ability to specify MIME type of attachment when downloading
Whiteboard: WONTFIX?
Target Milestone: --- → Bugzilla 2.20
You can now change the MIME type of an attachment later, if it's specified
incorrectly. Does that resolve this bug?

Gerv
Nope, as this bug is about getting an attachment with a content type other than
the one it has been correctly assigned.

Updated

15 years ago
Component: Creating/Changing Bugs → attachment and request management
(Assignee)

Comment 7

15 years ago
Suggested code for patch:

\template\en\default\attachment\list.html.tmpl (line 43 in current CVS)

      <td valign="top">
        [% IF attachment.ispatch %]
          <i>patch</i>
        [% ELSE %]
          [% attachment.contenttype FILTER html %]
          [% IF attachment.contenttype eq "application/xhtml+xml" %]
              <br><a href="attachment.cgi?id=[% attachment.attachid %]
&amp;action=view&amp;mimetype=application%2Fxml">application/xml</a> 
              <br><a href="attachment.cgi?id=[% attachment.attachid %]
&amp;action=view&amp;mimetype=text%2Fxml">text/xml</a> 
              <br><a href="attachment.cgi?id=[% attachment.attachid %]
&amp;action=view&amp;mimetype=text%2Fhtml">text/html</a>
          [% END %]

        [% END %]
      </td>

\attachment.cgi (line 366 in current CVS)

sub view
{
  # Display an attachment.

  # Retrieve the attachment content and its content type from the database.
  SendSQL("SELECT mimetype, thedata FROM attachments WHERE attach_id = $::FORM
{'id'}");
  my ($contenttype, $thedata) = FetchSQLData();

# Begin changes
$mimetype_flag = 0;
$mimetype = $::FORM{'mimetype'};
# Set boolean flag for overriding mimetype, and get whatever mimetype was 
submitted

if (($mimetype)&&($contenttype eq "application/xhtml+xml")) {
# if we have a mimetype submitted, and the mimetype retrieved from MySQL was 
XHTML,
    XHTML_SWITCH: {
        if ($mimetype eq "application/xml") {$mimetype_flag = 1; last 
XHTML_SWITCH;}
        if ($mimetype eq "text/xml") {$mimetype_flag = 1; last XHTML_SWITCH;}
        if ($mimetype eq "text/html") {$mimetype_flag = 1; last XHTML_SWITCH;}
# then check for what the submitted mimetype actually was.
# If the check passes, set the flag to true.
    }
}

# We can similarly override mimetypes for other XML languages such as XUL, 
simply by setting
# the mimetype_flag to 1.  

#if (($mimetype)&&($contenttype eq "application/xul+xml")) {
#    XUL_SWITCH: {
# etc. etc.
#    }
#}

if (!$mimetype_flag) {
# if there is no reason to override a submitted mimetype, go with the 
attachment's mimetype.
    $mimetype = $contenttype;
}
# End of changes
    
  # Return the appropriate HTTP response headers.
  print "Content-Type: $mimetype\n\n";

  print $thedata;
}

I'm not a person who does codes Perl on a daily basis; the above is mostly a 
guess based on context of variable usage and a couple things I picked up from 
perlfaq.  I'd appreciate it if someone could check the code involved here.

I have confirmed, though, that the logic behind my changes to attachment.cgi is 
valid.  (On that one, I'm uncertain of the usage of the $::FORM variable.)
(Assignee)

Comment 8

14 years ago
Patch coming up in a few minutes; I'm going to simplify this and open the
ctype=foo/bar to the same restrictions we have in creating an attachment and
setting its mimetype.
Status: NEW → ASSIGNED
(Assignee)

Comment 9

14 years ago
Created attachment 133994 [details] [diff] [review]
Patch to enable attachment.cgi?id=111522&ctype=foo/bar (untested)
(Assignee)

Comment 10

14 years ago
Taking.
Assignee: myk → ajvincent
Status: ASSIGNED → NEW
(Assignee)

Comment 11

14 years ago
Created attachment 134085 [details] [diff] [review]
Patch to enable attachment.cgi?id=111522&ctype=application/foo (tested)

Heh, good thing myk told me to test my code.  It would not have worked.  This
patch does work.
Attachment #133994 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #134085 - Flags: review?(kiko)

Comment 12

14 years ago
Comment on attachment 134085 [details] [diff] [review]
Patch to enable attachment.cgi?id=111522&ctype=application/foo (tested)

>-    SendSQL("SELECT mimetype, filename, thedata FROM attachments WHERE attach_id = $::FORM{'id'}");
>-    my ($contenttype, $filename, $thedata) = FetchSQLData();
>+    SendSQL("SELECT mimetype, filename, thedata, ispatch FROM attachments WHERE attach_id = $::FORM{'id'}");
>+    my ($contenttype, $filename, $thedata, $ispatch) = FetchSQLData();
>+    
>+    if (($::FORM{'ctype'})&&(!$ispatch))

nit: spacing around the &&

>+    {
>+      $contenttype = $::FORM{'ctype'};
>+    }

Question: why do we want ispatch to be an exception?

Comment 13

14 years ago
Created attachment 134192 [details] [diff] [review]
Version 3

Alex's patch per Kiko's suggestions.
Attachment #134085 - Attachment is obsolete: true

Updated

14 years ago
Attachment #134192 - Flags: review?(kiko)

Updated

14 years ago
Attachment #134192 - Flags: review?(kiko) → review+

Updated

14 years ago
Flags: approval?
(Assignee)

Updated

14 years ago
Attachment #134085 - Flags: review?(kiko)
Flags: approval? → approval+
Target Milestone: Bugzilla 2.20 → Bugzilla 2.18

Comment 14

14 years ago
Comment on attachment 134192 [details] [diff] [review]
Version 3

myk, I reviewed this, and though code-wise it's fine and works, I have a design
question.

This patch splits validation into the if ($action ...) section and then does
another check for $::FORM{'ctype'} inside sub view. I reckon it would be best,
cohesion-wise, to use a single check inside sub view, but since I'm not sure,
it would be nice to have your opinion. 

I'll attach the alternative patch.
Attachment #134192 - Flags: review?(myk)
Attachment #134192 - Flags: review?
Attachment #134192 - Flags: review+

Comment 15

14 years ago
Created attachment 134201 [details] [diff] [review]
alternative 1

Updated

14 years ago
Attachment #134192 - Flags: review? → review+
Comment on attachment 134192 [details] [diff] [review]
Version 3

I agree, this should all be in the view function (as should all other
validation calls), but I'm ok with this version.
Attachment #134192 - Flags: review?(myk)
Comment on attachment 134201 [details] [diff] [review]
alternative 1

This one also works. r=myk
Attachment #134201 - Flags: review+
(Assignee)

Comment 18

14 years ago
Take your pick, check one in to CVS, and obsolete the other one.  :)

Updated

14 years ago
Attachment #134192 - Attachment is obsolete: true

Comment 19

14 years ago
Since myk preferred the alternative, checked it in (but all the credit is yours,
Alex). Thanks a lot for the patch, it's great to get first-time contributions
from mozilla hackers!

/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.47; previous revision: 1.46
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Hang on... this is really confusing. 

Up to this point, the ctype parameter has had a standard meaning in Bugzilla,
which is that it's a short identifier (such as "js" or "xml") which is expanded
to a full content type via a lookup table. It's used this way on a large number
of CGIs. Here, it is instead a full content-type.

It seems to me to make much more sense to have a different name for the
parameter whose value is a full content type. "content-type" seems the obvious
choice. :-)

Gerv
(Assignee)

Comment 21

14 years ago
After discussion with justdave, reopening to comply with Gerv's wish.  Patch
coming up asap (we want this in for 2.17.5, which is being completed now)
Status: RESOLVED → REOPENED
Priority: P4 → P1
Resolution: FIXED → ---
(Assignee)

Comment 22

14 years ago
Created attachment 134639 [details] [diff] [review]
patch to replace ctype with content_type
(Assignee)

Updated

14 years ago
Attachment #134639 - Flags: review?(justdave)
Comment on attachment 134639 [details] [diff] [review]
patch to replace ctype with content_type

r= justdave, a= justdave
Attachment #134639 - Flags: review?(justdave) → review+
Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.48; previous revision: 1.47
done
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED
Cool, thanks Alex :-)

Gerv
This needs to be covered in the docs...
Flags: documention?(gerv)
Flags: documentation?(gerv)
Flags: documentation?(ajvincent)
(Assignee)

Comment 27

14 years ago
bug 232096 filed w/ patch for d=ajvincent@juno.com 
Status: RESOLVED → VERIFIED
Flags: documentation?(ajvincent) → documentation+
(Assignee)

Updated

9 years ago
Depends on: 486685
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.