Release download returns "Page Not Found"

ahh, only if the download method is set to private, which i never tested. *tee hee*. ;)

i'll look more closely at this and figure out if your solution is the best, or if there's another way.

thanks for the report and initial solution.
-derek

p.s. in the future, please submit real patches. see http://drupal.org/patch or http://drupal.org/diffandpatch for details.

Attached is a slightly modified version of his function in diff form. I added content-disposition so that the filename appears correctly.

Yes, this is probably still required for IE. imho, there should be a standard download function which also detects the browser type.

ahh, only if the download method is set to private, which i never tested. *tee hee*. ;)

Yes. But it can be set to private. So someone should test it.

'Content-disposition: attachment'

then IE will force a dialog and prevent the problem. This of course will always force a dialog but at least you have a choice and IE works.

That's not a problem, imo, for the project module.

What I meant was : +1 for 'Content-disposition: attachment' :)

Have you tried this :

'Content-disposition: inline'

Yes, this is probably still required for IE. imho, there should be a standard download function which also detects the browser type.

Not a good idea, imo.

Alternatively to implementing the file_download hook here, one can add the file info to the upload module {files} table, and let the upload module deal with returning a header.

That way, the download_count module would be able to keep a record of project downloads : http://drupal.org/node/131342

This bug has been open for what, six weeks now, with a working patch, but not yet committed. Let's just get it reviewed and checked in already.

As for comment #8, let's not worry about whether to hook in here or in upload; we have code that works reasonably well. So let's use it and get this bug closed. We can worry about where the hook should be later on.

sorry, i've just been swamped with too many things. the non-d.o users of project* have been getting poor service, and i apologize for that.

i'm still not thrilled with duplicating all this code, so i'd like to get a 2nd opinion from someone who really understands drupal's private file handling better than i do.

that said, the patch in #2 needs to be re-rolled to follow the drupal coding standards: http://drupal.org/node/318

also, the reuse of the variable called $file all over the place in that function makes it unnecessarily complicated to follow, so i'd prefer to see that slightly cleaned up before i commit this. what's the $file we get as an argument, vs. the file_create_path() results, vs. the db query results... ? i'm more concerned about clarity than whatever minor performance gain we get by re-using an ambiguous variable name.

thanks,
-derek

makes it unnecessarily complicated to follow

This is an implementation of file_download. Called whenever a file is downloaded with 'system/files' in its URL, hence when the download method is set to private :

function project_release_file_download($file) {

Actually, what's received here, $file, is a relative path from the System Path files folder to the file itself.

$file = file_create_path($file);

Getting the actual file path : no more, no less than a concatenation of the File System Path, a slash, and $file.

Now that we have the path... we'll ckeck if that's a file uploaded with the project module :

$result = db_query("SELECT f.* FROM {project_release_nodes} f WHERE file_path = '%s'", $file);
  if ($file = db_fetch_object($result)) {

If it is, then we check if the user can view uploaded files and if the file is attached to a node the user is allow to view :

if (user_access('view uploaded files')) {
      $node = node_load($file->nid);
      if (node_access('view', $node)) {

If it is, we prepare a header and return it so that the download begins...

$type = mime_header_encode($file->filemime);
        return array(
          'Content-Type: '. $type,
          'Content-Length: '. $file->filesize,
        );

That code is based on the upload module implementation of the file_download hook.
If you think it's hard to read, then the code in upload.module is just as hard to read, it's the same code anyway.

thanks for the clarification. sorry i wasn't more clear about what i was asking for. i actually could follow the code in the patch, but it just seemed unnecessarily cryptic regarding the reuse of $file, and project* is full enough of cryptic code as it is. ;) i was just asking for slightly more self-descriptive variable names in a few places, and perhaps a comment (in the code), not the blow-by-blow reply for this issue.

and yes, the core maintainers are (for obvious and understandable reasons) absolutely obsessed with performance, and sacrifice code clarity for performance all the time. i don't have the same attitude for project*, where the biggest problem is lack of people willing to work on it, not how slow it is. the cryptic code that pervades these modules is a big part of why so few people are willing to try to fix things or add features, so as i generate and review patches, i try to keep an eye out for code clarity.

hope that helps clear up where i'm coming from...

thanks again for your effort on this,
-derek

thanks for continuing to work on this. 2 problems:

1. still not a patch. ;) http://drupal.org/patch
2. $type = mime_header_encode($file->filemime); -- there's no "filemime" column in {project_release_nodes}, so you're just passing a NULL into mime_header_encode(). :(

Is this still not in? As far as I can tell the only thing missing is documentation for the new function, and that issue is now remedied with my patch. Is it can be commit tiem now please?

i'd say this needs some work still.

i think the function's PHP doc comment could be simply:

/**
 * Implementation of hook_file_download().
 */

'view uploaded files' is probably not the correct permission to be checking. that's provided by upload_perm() in upload.module. project doesn't depend on upload.module so it's not safe to assume that permission will be available.

The file_create_path() is unnecessary. This is called from file_download() which calls file_create_path(). Uses of the $path variable should probably be replaced with $filename. And $filename should probably be $filepath which is more accurate...

The returned file name should be the basename: 'Content-disposition: attachment; filename="'. basename($filename) .'"';

I'd also say you could remove both of those else { return -1; }s and just return -1; if it's our file but something doesn't work out.

I'd really like to get a version of this patch in before the 5.x-1.0 release (http://drupal.org/node/150278). Anyone available to re-work the patch given drewish's excellent feedback and review?

I could do it, but no earlier than Monday, unfortunately. I can't do any work on the computer I'm using today.

i'll see if i can get to it.

I followed my suggestions above. Also changed the query to only return the nid which is all we use.

Did a little research on the headers and found RFC 2045. Based on that and what's been discussed in this issue I think that mime_header_encode() is the least bad way to specify a file name for the purposes of the 'Content-Disposition' header.

I couldn't find documentation for the 'application/force-download' content-type but RFC 2046 seemed to suggest that 'application/octet-stream' was a good default for binary data.

Sweet, thanks for the thorough job, drewish. However, what happened to checking user_access('view uploaded files')? Don't we want that, too?

dww, no we shouldn't. from my comments in #17:

'view uploaded files' is probably not the correct permission to be checking. that's provided by upload_perm() in upload.module. project doesn't depend on upload.module so it's not safe to assume that permission will be available.

Ahh, right. Sorry, I had forgotten that already (juggling too many issues to have a clear head these days)...
This looks great. It only needs testing and then it's RTBC.

This doesn't work. ;)
- test site with 5.2 core
- configured for private downloads
- latest project + project_release (no cvs.module)
- created a release node and attached a tar.gz file
- tried to download the tar.gz from the release node

still get "Page not found".

dvm() tells me the following:
- project_release_file_download() is getting invoked with with $filepath == "drupal-5.2.tar.gz"
- this query fails to find anything:
db_query("SELECT f.nid FROM {project_release_nodes} f WHERE file_path = '%s'", $filepath);
because the file_path column of {project_release_nodes} really contains the full path, "/tmp/drupal-5/files/drupal-5.2.tar.gz", not just the file name.

So, what's the right thing to do here?
A) is {project_release_nodes} storing the wrong thing in that field? that seems like a bigger change... :(
B) should this query use a '%' wildcard and LIKE?
C) should project_release_file_download() use file_create_path() after all?

Seems like C is really the way to go...

I took a stab at C. With local testing, it all appears to work. However, drewish is "mr. files" around here, so I'd like to get his review and input before I commit. Any other testing and reviews would be welcome. Thanks!

ah, okay that'd make sense. i did my testing on a d.o db dump which had the files as: files/project/* so i put in some 0-length files for testing. for non files/* paths you'll run into exactly the problem you describe. i think you're correct, C is the way to go.

It looks like all you did was change the parameter name to $filename and add $filepath = file_create_path($filename);. I gave it a test with paths both inside and outside of Drupal's root. I'd say it's RTBC.

The only other change is that we filesize() the $filepath, but we advertise the $filename in the header.

Tested on 4.7.x-2.*, too, just to be sure, then committed to HEAD and DRUPAL-4-7--2. Yay! ;)

Thanks to everyone who helped get this fixed -- this was one of the major blockers for the 5.x-1.0 release:
http://drupal.org/node/150278
http://groups.drupal.org/node/5448