I don't think we're fully done yet with #829856: warning: Missing argument 2 for filefield_view_access()
(though I'll open another issue, since #829856 has already been incorporated in a stable release)
The fix included an addition of node_load($file['nid'])) - but $file is not always filled here. Which means the 2nd argument to filefield_view_access() is FALSE.
And that makes stuff choke. I have a content_access module enabled, and my cron runs were crashing on a drupal_clone(FALSE) call which originated from filefield.
I guess there should just be a proper check for $file['nid']?
And while we're at it -- maybe it's good to change the order of the checks, because filefield_view_access() can be more resource intensive than filefield_file_listed()?
( I don't know what I'm talking about really -- but it seems that way to me :) )
Comment | File | Size | Author |
---|---|---|---|
#6 | filefieldaccess.patch | 1.46 KB | Jody Lynn |
#3 | filefield_view_access2.patch | 1.96 KB | quicksketch |
829856_revisited.patch | 693 bytes | roderik |
Comments
Comment #1
Island Usurper CreditAttribution: Island Usurper commentedSetting this as "needs review" since there's a patch, but I was able to get rid of the message when I flushed the cache. This change may not be necessary, after all.
Comment #2
roderikOh, yeah, thanks.
But we have a different problem. There was no 'message' for me. There was a PHP runtime error -> execution halted.
Comment #3
quicksketchI think generally the addition of $node to filefield_view_access() has been a negative-value overall. It adds more load, increases difficulty of calling the function, and (as it now turns out) doesn't actually give us any benefit that I can tell.
I added $node to filefield_view_access() because we added it to filefield_edit_access(). But as it turns out it's really only important to have the $node on edit, and not so much on view. The reason for this is because editing a node you need to know which CCK widget is being used, so the $node->type makes a big difference when editing. However when *viewing* the content, all field access is the same regardless of what widget was used to upload the file.
So with this in mind I've rolled back part of #696906-14: filefield_edit_access() and filefield_view_access() should use content_access() instead of checking content_permissions specific, which included the changes to filefield_view_access(). I thought it was a good idea at the time, but it doesn't look like that now.
I've committed this patch which rolls back the change to filefield_view_access().
Comment #4
roderikMakes perfect sense, AFAICS.
Happy to see an extra node_load() call disappear from a theme function again.
Comment #6
Jody LynnThis change messed me up because I was relying on filefield_view_access to have that $node object for use with a custom hook_field_access implementation. Basically I need filefield_file_download to send the node object through to content_access so I can set file download permissions according to a node property.
I agree with removing that extra node_load from the theme function, but why not leave in the $node parameter as optional, like it is in content_access, so that it can get passed to content_access when it's already available?
Comment #7
xjmI agree with #6. This change broke my file access control. I'll give the patch a test.
Comment #8
xjmPatch in #6 resolved this issue for me.
Comment #9
roderikI looked this over a bit, since my name's on the original report... though the issue has gone a completely different way since #3...
So, trying to align quicksketch's words with Jody Lynn's:
There doesn't seem to be much of a function for the $node parameter, when viewing the node content...
But there is a function when downloading the file directly. (If the $node parameter is passed, you can use the 'content access' mechanism to pass access restrictions back to filefield_file_download calls, based on the node which the file is attached to. If that wasn't possible, you'd probably have to implement your own hook_file_download() implementation, with lots of duplicated code.)
And that's a valid reason for passing $node only when calling filefield_view_access() from filefield_file_download(), and keep it at NULL otherwise.
(Or alternatively: replace the call to filefield_view_access() with a direct call to content_access()? Idunno...)
Does that sound about right?
Does that need documentation/comments to go along with the patch?
Comment #10
quicksketchHm, okay well we'll add it back again but make it optional. Seems like a general nightmare for access control if you have the node to work with *some of the time*, but not always. But I suppose that's what content_access() gives you and modules have been managing alright with that even though it's only available some of the time also. I've committed Jody's #6 and it'll be in the 3.8 version.