regression from 4.6: hook_file_download gets called on at most one module

In what cases would you want to send multiple headers before a file download?

Could module weighting be used to solve your problem instead?

Hmm thats a plus one, because I can definately think of places I would like caching for files and not for the rest of drupal.org, and allowing other modules to set headers would be cool.

Some behavior to note that you are modifying...

1) allowing/denying of file downloads...
Before your patch, which ever module answered first with an allow or deny won...

Now we either need to select a defaul behavior, allow, deny or deny,allow
You patch is exhibits a deny, allow behavior...

ie) if any module denies the download, it won't be sent, where as before if a module returned headers before another module could deny the transfer was allowed.

2) You also introduce the possibility of several contribs adding conflicting headers, or duplicate headers....

A simpler approach may be to....

1) add allow,deny && deny,allow behavior settings
2) add private files cache control settings
3) modify file_download to just be a permissions check(false blocks access, true allows access)
4) augment file_transfer to send the mime content type and the appropriate cache setting.

As it is file_download looks badly broken... fortunately there is only one hook_download in core so the problems that can arise from this probably aren't noticed often...

He's another patch with an explicit deny,allow that will allow multiple headers to be passed to file_transfer.

I'm gonna go ahead and assign this to myself... I've got the rest of the rc file isssues anyway.

Code looks good to me. Needs some testing.

I can test this with the standard modules. But to be comprehensive I should also install a module which also uses that hook.

wtanaka, would it be feasible for you to provide a stable version of your custom module for testing?

I testd this one with organic groups module and upload module. attachments to protected nodes were blocked as expected. i rerolled with a tweak to upload_file_download() so that it takes advantage of the new API and shows an access denied message instead of 404.

RTBC

Committed to HEAD. Thanks.

While your patch addresses the issue of your module's hook getting ignored, it still leaves a fundamental weakness in file_download() that has persisted until at least the latest version of Drupal 5 and even exacerbates it by indiscriminately piling up all headers returned.

It seems to me that a critical core function such as this should at least allow modules greater control over handling file downloads, instead of leaving it to, well, to chance almost, how a file's headers are set.

Attached is a patch that solves the problem by allowing the situation you introduced, i.e. calling module_invoke_all for hook_file_download, to coexist with a weighted solution (as proposed earlier in this thread) that at least allows module developers to keep control over such an essentially straightforward thing as setting file headers without having it messed up by other modules.

Also attached an example module that implements the new proposed hook_file_download_weight.

After some more testing, another patch with a small addition (setting of $headers to an empty array if none of the modules with lowest weight returned anything).

New features should be added to the development version, currently 7.x. I would recommend opening a new issue for this.

Yes, please post the patch in a new issue from http://drupal.org/node/add/project-issue/drupal