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 :) )

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Island Usurper’s picture

Status: Active » Needs review

Setting 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.

roderik’s picture

Oh, yeah, thanks.

But we have a different problem. There was no 'message' for me. There was a PHP runtime error -> execution halted.

quicksketch’s picture

Status: Needs review » Fixed
FileSize
1.96 KB

I 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().

roderik’s picture

Makes perfect sense, AFAICS.
Happy to see an extra node_load() call disappear from a theme function again.

Status: Fixed » Closed (fixed)

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

Jody Lynn’s picture

Status: Closed (fixed) » Needs review
FileSize
1.46 KB

This 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?

xjm’s picture

I agree with #6. This change broke my file access control. I'll give the patch a test.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Patch in #6 resolved this issue for me.

roderik’s picture

I 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?

quicksketch’s picture

Version: 6.x-3.5 » 6.x-3.7
Category: bug » task
Status: Reviewed & tested by the community » Fixed

Hm, 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.

Status: Fixed » Closed (fixed)

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