Private Download is a nice little module I have been using on several of my sites with no problem.
Until I installed it on my latest site and noticed that something was going wrong with one of my custom modules that implements hook_file_download.
I then had a look at Private Download's code and found naughty things out there!

- First, PD hijacks path system/files by adding its own access arguments entry.
- Second, PD does not implement hook_file_download the right way, as it does not check any permission, although it is the purpose of this hook (See http://api.drupal.org/api/drupal/developer--hooks--core.php/function/hoo...).
Actually, the access check wrongly added to the system/files entry should be performed inside hook_file_download.
Obviously, PD works flawlessly if it is the only module installed that implements hook_file_download. But as it breaks the way path system/files and hook_file_download are supposed to work, it will conflict with other modules that depend on this hook too.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

anrikun’s picture

Status: Active » Needs review
FileSize
2.19 KB

Below is a patch that fixes this issue:
- it removes the unwanted new system/files entry
- it fixes the implementation of hook_file_download, so that it does what it is supposed to do:

Return value
If the user does not have permission to access the file, return -1. If the user has permission, return an array with the appropriate headers. If the file is not controlled by the current module, the return value should be NULL.

(The last part is the most important part here: PD should only care about files that reside inside the private directory and ignore others.)

This way, Private Download does not conflict with other hook_file_download implementations any more.

On more thing: after applying the patch, don't forget to rebuild your menu cache by visiting update.php.

lucascaro’s picture

Checked the patch and works great! thanks!

anrikun’s picture

Please mark this issue as RTBC then.

anrikun’s picture

Status: Needs review » Fixed

This is now fixed in 6.x-1.x-dev.
Many thanks to @Bacteria Man for letting me commit this patch.

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

Better English(?)