This is essentially a duplicate of #61528: Private files show open/save prompt for PDFs, etc. while public files automatically open inline, but since that issues relates to 4.7.x-dev I'll open a new one.

Issue:

When visiting an uploaded file, the presentation in the browser depends on the storage protocol. This is especially important for PDF files as they may either be displayed in the browser or downloaded/saved/opened in a different application.

The method of specifying the desired browser action is through the Content-Disposition header which is either:

  • inline, meaning showing the file in the browser, if possible; and
  • attachment, meaning prompt the user to save the file.

By default, Drupal installations serve up files in two different ways, depending on the storage method:

public://...
These files are generally served directly by the webserver without Drupal bootstrapping. Many web servers do not send any content disposition headers and so the file is generally shown inline in the browser.
private://...
These files are served by Drupal. Drupal injects a Content-Disposition header which depends on MIME type. Currently file_inline_types (i.e. text/*, image/*, and *flash) are served as inline and remaining files are served as attachments.
In the private://.. storage method, other MIME types, like PDFs, are sent as attachments making their behavior different than if they were stored using public://.

History

Upload Module

Looking through the history (via git log -Sdisposition), it's clear that the Content-Disposition header was removed from the original upload module in #61528: Private files show open/save prompt for PDFs, etc. while public files automatically open inline in version 4.7.x.

CCK FileField

In CCK FileField, the commit

commit a7da1cd39f893cdf629f9b9a2cd423f43b06c5d3
Author: Darrel O\'Pry <dopry@22202.no-reply.drupal.org>
Date:   Thu Feb 1 09:27:38 2007 +0000

    backporting some fixes, untested... use at your own risk!!

copied upload_file_download() (from a version before the upload module patch) into filefield_file_download().

The list of inline mime types was made configurable in #485336: Make Content-Disposition configurable for private downloads (using the variable 'filefield_inline_types').

When CCK FileField entered core (#391330: File Field for Core) this was preserved (the variable being renamed to 'file_inline_types'). The code was later split out to a separate function (file_get_content_headers) but the logic preserved in #943112: file_file_download should delegate header checks to a separate function.

Proposed Solutions

  1. Remove the content disposition header entirely. Is it necessary?
  2. Reconfigure the list of MIME types which should show up as inline. Perhaps add a configuration page, or default to a larger list of MIME types.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bfroehle’s picture

Status: Active » Needs review
FileSize
996 bytes

The attach patch removes the content disposition header, but will fail the test suite because of an assertion (that the content disposition header was sent correctly) added in #898036: Private images broken.

Status: Needs review » Needs work

The last submitted patch, 61528-remove-content-disposition.patch, failed testing.

bfroehle’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs backport to D7
FileSize
2.28 KB

The attached patch removes all of Drupal handling of the Content-Disposition header. Read the issue summary for reasons why this is a good idea.

dvcroftarb’s picture

This seems to be exactly the issue I am experiencing on Drupal 7. Any chance of patching it too?

claudiuv’s picture

I hope that this isn't off topic, but i suspect this bug is related to my problem - when downloading private files, I get a generic filename in the Save As dialogue, that is "download.pdf" instead of the original filename. I am running Drupal 7 with Filefield Sources module installed.

haydeniv’s picture

I just manually applied this patch to D7 and it fixed my behavior problem as well. Bot's happy so I'd say this is RTBC but I don't think I have enough community mojo to flag it as such.

jackbravo’s picture

Status: Needs review » Reviewed & tested by the community

I think this can be marked as RTBC. I also don't have community mojo =P (http://drupal.org/node/156119), but in my opinion the patch is not too complicated and the only reason against it could be that the filename is set on the Content-Disposition header (http://stackoverflow.com/questions/1741353/how-to-set-response-filename-...), but private file URLs are rightly named so I don't see any problem there.

bfroehle’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, Content-Disposition-header-1229014-3.patch, failed testing.

jackbravo’s picture

Status: Needs work » Needs review
FileSize
2.95 KB

Here is a re-roll of the patch.

haydeniv’s picture

Status: Needs review » Reviewed & tested by the community

I've been running fix on a site for a couple months now and it works well. RTBC.

haydeniv’s picture

Tagging

tim.plunkett’s picture

Issue tags: +Needs backport to D7

Fixing tags

jackbravo’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs review
FileSize
1.72 KB

So PATCH #10 is for D8. Has that one been applied already?

And here is a PATCH for D7.

tim.plunkett’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Reviewed & tested by the community
catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs review

Makes sense to just remove this. Committed/pushed to 8.x.

jackbravo’s picture

Status: Needs review » Reviewed & tested by the community

It is the same change for Drupal 7, and all tests pass already. Seems RTBC.

pattip’s picture

Hello-
Can I get help?
D7 patch (located in drupal root folder) is failing
(Drupal 7.16) (Environment: hostgator-Linux shared hosting w/SSH via PuTTY)
error:
patch -p1 < Content-Disposition-header-D7-1229014-14.patch
patching file includes/file.inc
Hunk #1 FAILED at 2478.
1 out of 1 hunk FAILED -- saving rejects to file includes/file.inc.rej
patching file modules/image/image.test
Hunk #1 FAILED at 749.
1 out of 1 hunk FAILED -- saving rejects to file modules/image/image.test.rej

I am grateful for any help..thank you!

pattip’s picture

jackbravo’s picture

@pattip the patch applies cleanly to drupal 7 dev version. Drupal 7.16 probably is a little behind. You could try to apply this patch by hand, it only changes to files.

haydeniv’s picture

haydeniv’s picture

Just applied this patch again and tested for good measure. PDFs now follow default browser handling. Works good on file downloads. If bot is happy then this is good to go.

haydeniv’s picture

Bot is happy. Hopefully this gets committed before next version.

chrisarusso’s picture

As of 7.20 patch is as attached:

Status: Reviewed & tested by the community » Needs work

The last submitted patch, Content-Disposition-header-D7-1229014-25.patch, failed testing.

David_Rothstein’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.72 KB

The patch in #15 still applies. Reuploading that and assuming it's still RTBC (haven't reviewed it myself yet though).

iva2k’s picture

minor note: line $name = mime_header_encode($file->filename); can be removed as $name use is removed.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.22 release notes

Yeah, it seems like this is the right thing to do. Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/b927d87

I'll add it to the release notes also, just to be safe (in case someone out there is actually expecting this header to be there).

Maybe we can get a followup to remove the useless $name variable mentioned above (affects Drupal 8 too, I believe)?

Automatically closed -- issue fixed for 2 weeks with no activity.

David_Rothstein’s picture

Maybe we can get a followup to remove the useless $name variable mentioned above (affects Drupal 8 too, I believe)?

Happened in #2853160: Variable $name is defined but not used in file_get_content_headers() for Drupal 7, and somewhere else for Drupal 8.