API page: http://api.drupal.org/api/drupal/modules--system--system.api.php/functio...
Describe the problem you have found:
The sample function body for hook_file_download is totally bogus for D7/8 - it was never updated from the D6 version. Does this hook still exist? If so, the example needs an update to something relevant for D7/8, and if not, it needs to be removed.
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | sample-function-1169564-13.patch | 1.13 KB | FreekyMage |
| #9 | sample-function-1169564-8.patch | 1.13 KB | FreekyMage |
| #4 | sample-function-1169564-4.patch | 1.02 KB | FreekyMage |
Comments
Comment #1
jhodgdonIt does look like hook_file_download() is still valid -- it's invoked from file_download(), which is at least called once in core.
So we just need a different example, taken from one of the core modules that implements it. And the @see to upload_file_download() should be removed as well.
Comment #2
jhodgdonGood project for a novice doc contributor...
Comment #3
FreekyMage commentedGoing to try and solve this to learn how to contribute
Comment #4
FreekyMage commentedThis hook gets used in file_file_download, image_file_download and user_file_download. Because the first 2 are pretty large I used the user one. I don't know if that's the best sollution since it doesn't return -1 when access is denied.
Anyway, hooray for first patch \o/
Comment #5
jhodgdonHooray indeed! Thanks - looks good, and I think you made the right decision to use a simpler example.
Should go into 7.x/8.x.
Comment #6
webchickCongrats, FreekyMage! :D
While you are simply following our documentation standards by copy/pasting an example out of core, the previous example was a bit more complete, since it also showed an example of denying access to the file. So we actually lose some helpful information with this patch. :(
Can we embellish a bit on what user.module supplies and throw in an access check for, say, "user_access('access user profiles')" and deny access in that case?
Additionally, a line or so of comments within the example code here explaining what is going on would be good. We lost the "// Check if the file is controlled by the current module." context from the old text.
Comment #7
jhodgdonGracious, really??? Many of the core hook docs don't have examples that cover all the contingencies -- far from it. I think we should just let this go. Can we reconsider?
Also, we need to change the standards for hook docs ASAP, so we don't have to think about going through this ridiculous exercise of trying to find a core example that covers all the contingencies. We need the API module to just list the functions that implement each hook automatically so we don't have to put in a sample function body. Does anyone use them anyway -- wouldn't you just go look at a core example? Sigh. Anyway, discussion for a different issue...
Comment #8
webchickWell, yeah, really. Denying download access to files is an important function of this hook. If they can't see how to do it in the hook example, how are they supposed to figure out how to do it? Read http://api.drupal.org/api/drupal/modules--file--file.module/function/fil... ? EEEK. :)
Comment #9
FreekyMage commentedThanks for the input. I also thought the example could be better so I made a new patch.
jhodgdon I'm with you that the api module should list the functions as long as they are viewable on the website here so people can look at it without needing opening an editor and locating it.
Comment #10
jhodgdonLooks OK to me. 8.x/7.x...
Comment #11
webchickNice! Looking much better now.
Does this actually work, though? My sneaking suspicion is that this code as written would deny access to all files, not just the files that that module controls, if the requisite permissions were missing. I'm pretty sure that check needs to go inside the
if (strpos(file_uri_target($uri), variable_get('user_picture_path', 'pictures') . '/picture-') === 0) {condition...Sorry to be a PITA on this, but this is the primary way we have to educate developers on how these hooks work, so it's important the docs be correct. :(
Comment #12
jhodgdonAgreed with #11. Sigh. Not so much of a Novice issue any more. :)
Comment #13
FreekyMage commentedYou're right, I should have checked the function better. I'm more of a themer normally and so my logical brain shuts down from time to time :)
Comment #14
jhodgdonAt the risk of webchick once again rejecting the patch, I think it's good (again). :)
8.x/7.x if it's OK...
Comment #15
webchickShoot, sorry! This totally fell off my radar.
This looks great! Thanks for all your work on this.
Committed and pushed to 8.x and 7.x.