This is similar to #1229014: Content-Disposition header makes private files show open/save prompts, but public files show inline, which is fixed in both D7 and D8. This module forces files to have "Content-Disposition: attachment" header, thus making it impossible to view files in browser, which is often not the desired behavior (especially for PDF files) and differs from Drupal default behavior. I will attach a patch fixing this to the first comment.

CommentFileSizeAuthor
#9 1933774-storage_api-headers_alter.patch1.99 KBjonhattan
#1 storage_api-content-disposition_header_removal-1933774-1.patch458 bytesShevchuk

Comments

Shevchuk’s picture

Shevchuk commented
StatusFileSize
new storage_api-content-disposition_header_removal-1933774-1.patch458 bytes
Shevchuk’s picture

Shevchuk commented
Issue summary: View changes

Fixed similar issue link

jbrown’s picture

jbrown commented
Version: 7.x-1.4 » 7.x-1.x-dev
Status: Active » Needs work

I think there should be a setting as it is often desirable to force download.

Ideally this setting would have eventual consistency where instances already created are modified to serve as configured, but Storage API doesn't yet have the infrastructure to handle this.

Shevchuk’s picture

Shevchuk commented

With this patch module is not forcing content disposition header, so it is up to the web server to output file properly, and this can be configured in the web server config (per location or server-wide). Of course, it would be nice to have an option within Storage API module, but for the meantime it seems better to me to behave consistently with Drupal itself.

Shevchuk’s picture

Shevchuk commented
Issue summary: View changes

Spelling

jbrown’s picture

jbrown commented

Note that with some cloud services, e.g. S3 it is not possible to configure force download container wide (to my knowledge).

jbrown’s picture

jbrown commented
Issue summary: View changes

Updated D7 patch status.

perignon’s picture

perignon commented
Issue summary: View changes
Status: Needs work » Fixed

Is this still an issue. At over 2 years ago it's rather old. If this is still valid please reopen. The Storage API is under a lot of rework at the moment so contributions are welcome!

Shevchuk’s picture

Shevchuk commented

Didn't test dev version, but from source code it seems this is still an issue, see http://cgit.drupalcode.org/storage_api/tree/storage.inc#n481 Probably module should provide administrator an option to force files download or not.

Shevchuk’s picture

Shevchuk commented
Status: Fixed » Needs work
perignon’s picture

perignon commented
Assigned: Unassigned » perignon
jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented
Status: Needs work » Needs review
StatusFileSize
new 1933774-storage_api-headers_alter.patch1.99 KB

Alternative proposal: allow others to alter the headers to their own taste.

perignon’s picture

perignon commented

Reviewing this. I think that the header "Content-disposition" is widely argued and not included in some of the HTTP specifications so it would be best to remove it. Thanks to @jonhatten for pinging this issue.

  • Perignon committed df1f342 on 7.x-1.x authored by jonhattan 
    Issue #1933774 by Shevchuk, jonhattan: No need to force Content-...
perignon’s picture

perignon commented

I have committed @jonhattan modified patch based off the patch provided by @Shevchuk. Please test dev branch.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented 
+++ b/class.inc
@@ -28,7 +28,7 @@ class StorageClass {
-        throw new StorageException();
+        throw new StorageException(t('Unable to load class !class', array('!class' => var_export($class, TRUE))));
       }

Sorry I added this unrelated change inadvertently.
It is part of another patch I'm preparing for raising meaningful exceptions.

perignon’s picture

perignon commented

Oopsie. I wondered for a second what that was for but was in a hurry and didn't see that it caused harm.

perignon’s picture

perignon commented

I just reverted that line back to what it was.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented

Thanks Perignon. Note two things:

* you also commited the patch file (actually there're two patch files in the repo)
* my patch in #9 doesn't include the change in #1, so content-disposition header is still there (in case you wanted to remove it -- i don't have an opinion here)

perignon’s picture

perignon commented

Removed the patch files.

In regards to the header. Being it can be removed now via an alter, I am going to leave it in for now.