Hey Berdir and all,

I've been doing some more testing of fieldable Private Messages in D7 and noticed the following bug: If you create a new Private Message field of type "File" and then use the private upload method, then an incorrect link is created to the file (even though the file upload puts it in the proper place).

More specifically, the link points to the following location:

http://localhost/system/files/message_attachments/filename.jpg

When it should be going here:

/Users/benkaplan/git/private_files/message_attachments/filename.jpg

(Note that in the above examples, I set "message_attachments" as a sub-folder that the file should appear at.)

As a result, when a message recipient clicks on a file he receives a "Page Not Found" error.

Additionally, if you preview the message before sending it, you receive the following notices:

* Notice: Undefined property: stdClass::$uri in theme_file_link() (line 690 of /Users/benkaplan/git/drupal/modules/file/file.module).
* Notice: Undefined property: stdClass::$filemime in theme_file_icon() (line 725 of /Users/benkaplan/git/drupal/modules/file/file.module).
* Notice: Undefined property: stdClass::$filemime in file_icon_path() (line 767 of /Users/benkaplan/git/drupal/modules/file/file.module).
* Notice: Undefined property: stdClass::$filemime in file_icon_map() (line 809 of /Users/benkaplan/git/drupal/modules/file/file.module).
* Notice: Undefined property: stdClass::$filemime in file_icon_path() (line 782 of /Users/benkaplan/git/drupal/modules/file/file.module).
* Notice: Undefined property: stdClass::$filemime in file_icon_path() (line 782 of /Users/benkaplan/git/drupal/modules/file/file.module).
* Notice: Undefined property: stdClass::$filemime in file_icon_path() (line 782 of /Users/benkaplan/git/drupal/modules/file/file.module).
* Notice: Undefined property: stdClass::$filemime in file_icon_path() (line 782 of /Users/benkaplan/git/drupal/modules/file/file.module).
* Notice: Undefined property: stdClass::$filemime in theme_file_link() (line 697 of /Users/benkaplan/git/drupal/modules/file/file.module).
* Notice: Undefined property: stdClass::$filesize in theme_file_link() (line 697 of /Users/benkaplan/git/drupal/modules/file/file.module).
* Notice: Undefined property: stdClass::$filename in theme_file_link() (line 707 of /Users/benkaplan/git/drupal/modules/file/file.module).

Thoughts?

Thanks,
Ben

CommentFileSizeAuthor
#7 fix_bad_drupal_static_call_845838_7.patch784 bytesridgerunner

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

The link is correct, if it doesn't show then something else is wrong. Remember, it wouldn't be very private if the direct link to the file would be printed out, as everyone could then get the file if they know the link. Instead, it goes through a drupal callback that checks if the user is logged in and has access to that specific file. I don't know how that is solved with fields but with privatemsg_attachments in D6 only recipients of the message to wich the field is attached are allowed to see it. I guess there are no additional access checks in fields api.

As a generic rule, if you find something broken regarding fields, try to reproduce it with nodes and if it broken too there, it's not a privatemsg bug.

Back to that bug, I tried it myself and it seems that private downloads are totally broken right now in core. The bug seems to be in file_file_download in file.module. It is able to load the file but then the following line returns nothing "$references = file_get_file_references($file, NULL, FIELD_LOAD_REVISION, $field_type);". To be exact, the whole function looks strange, it only contains access definitions for nodes and users. This issue could be related to #353458: hook_file_references() was not designed for a highly flexible field storage, which is a critical core bug. I'm not sure.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Also, if you choose a specific image style (for example, medium), then it works.

Talked with catch about it, he said we should create a new issue about file_file_download().

BenK’s picture

BenK commented

Thanks for the info. I guess I'm not familiar enough with the bleeding edge of Drupal core to be able to tell what might be a core bug. But I'll take your advice in the future and test on regular nodes, too.

Can you create the core issue about file_file_download()? I think you could probably provide a more descriptive and insightful issue than me.

BenK’s picture

BenK commented

By the way, I've now determined that the notices posted above are occurring for the public file method, too. Also, when using the public method, there are the following additional notices:

    *  Notice: Trying to get property of non-object in file_field_presave()  (line 258 of /Users/benkaplan/git/drupal/modules/file/file.field.inc).
    * Notice: Undefined property: stdClass::$uri in file_save() (line 515 of /Users/benkaplan/git/drupal/includes/file.inc).

Do you know if this related to a core bug? Or should I post a new issue? I suppose I could test this on a regular node too...

--Ben

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: File Added to Message Using Private Upload Method Has Wrong Link » File/Original image attached to private message does not display when using private files.
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Postponed

Opened #846296: file_file_download() only implements access checks for nodes and users. We might not to do something in privatemsg.module, but we need to figure out what first...

ridgerunner’s picture

ridgerunner commented
StatusFileSize
new fix_bad_drupal_static_call_845838_7.patch784 bytes

I found what is probably a pretty nasty bug in file_get_file_references() where it is improperly calling drupal_static(). It is calling it like so...
$references = drupal_static(__FUNCTION__, array());
but it is supposed to call it with the =& reference operator like so...
$references = &drupal_static(__FUNCTION__, array());
I'm not familiar with this end of the drupal code base so I have no idea what the ramifications of this fix might be. Attached is a patch that corrects this error. Let's see if this fix passes automatic testing...

ridgerunner’s picture

ridgerunner commented

It turns out that the drupal_static() call error described in the previous post doesn''t have any real ill effects. As per Berdir's comment #846296-10: file_file_download() only implements access checks for nodes and users, the only effect of the bad call in file_get_file_references() is that references are re-computed over and over rather than being statically cached. Carry on...

BenK’s picture

BenK commented
Status: Postponed » Active

I'm making this issue active again since Berdir got http://drupal.org/node/846296 committed to core... Nice work, Berdir!

So I chatted with him in IRC and he says that we now need to implement a new hook to fix this issue... It's all a bit over my head but I look forward to testing! :-)

--Ben

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Ok, the situation is as following:

In theory, it should be working now because I accidently commited the new hook as part of the validation strings patch. In practice, it works great for files. For images, there seems to be a whole bunch of bugs that break it. I'm currently working on a patch and will post the link as soon as there is an issue.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Fixed

I see that you found it yourself already :)

Anyway, here is the issue for others #898036: Private images broken.

Since I don't think there is anything left from our side and it should just work with the patch there, I'm setting this to fixed. You're of course welcome to test it once the other patch is commited and if there are still issues, re-open this.

Status: Fixed » Closed (fixed)

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