Download_count no longer works with CCK FileField

kmonty - May 13, 2009 - 16:33
Project:download_count
Version:6.x-1.x-dev
Component:Code
Category:bug report
Priority:critical
Assigned:Unassigned
Status:needs work
Description

This module no longer will track files uploaded with FileField. I assume something changed between 5.x and 6.x which caused the logic to break.

On line 167, the query will fail when searching for a file that was not uploaded with upload.module

$result = db_query("SELECT u.nid, f.filepath FROM {upload} u JOIN {files} f ON f.fid = u.fid WHERE f.filepath = '%s'", $filepath);

The reason is that the there is no row in the upload table. We could ignore the nid and just fetch the f.filepath, however, we cannot run the node access check, which could be important for some users (it is not important for my use case).

Any ideas?

#1

hankpalan.com - May 28, 2009 - 23:18

kmonty -

Did you ever get this worked out on your own? Or are you still waiting for the maintainer to respond?

I'm not getting it to work with cck filefield either. I haven't tried using the upload module.

#2

kmonty - May 30, 2009 - 00:29

I'm slowly working on a workaround but it would be ideal if the maintainer had some thoughts.

#3

Emoreth - June 1, 2009 - 04:56

Is the change on newer versions of FileField or downlaod_count ??

I really do need to get this working fast, so, ill get an older version until it got solved.

I tried to get a workaround, but that node access check is hard to bypass ( i need it and i could not find a way to solve this issue )..

#4

mudd - June 3, 2009 - 14:08

Can I ask, is this module supposed to log anything that is processed by the Private Files subsystem?

I'm using FileField for my node attachments, and as the OP points out, this module doesn't record these. And changing that line to

$result = db_query("SELECT filepath FROM files WHERE filepath = '%s'", $filepath);

doesn't help.

Is the issue that FF used to write to the upload table, and now doesn't?

I also don't understand OP's comment on node access check -- this wouldn't be the function of the counter, would it?

PS, if the maintainer looks at this, I'm hoping it doesn't get hard coded into the path /system/files/... -- I'm looking for a way to nix that path from the download link, as well as remove additional internal filesystem dir's. (I'm sorting uploads based on user, node name, etc, and don't want those paths in the DL link either.)

#5

Emoreth - June 3, 2009 - 20:41

The node access check is soppused to not show to a specific user statistics for a file that he is not allowed to download or access.

I got it to work here, but I lost some functionality, as I completely removed that node access check.

I'm new on drupal core and I could not find how to get information to manually insert info at update table, but I think that in the past FF used to upload via module.upload and know it does somehow different.

Regards,

EMoreth

#6

mudd - June 5, 2009 - 13:10

Thanks EM, I get it.

On a side note, I posted elsewhere (and was encouraged to push the point here) asking if there's a logger for all things downloaded through the Private method (aka Private Files), because what I need is a bit different than what it seems download counter is designed for -- Download Counter seems to provide a block view of download stats for attachments on viewed node. (Pls correct me if I'm wrong) But what I'm looking for is an administrative tool for collecting data on all files downloaded (cuz it's our design spec -- an in-house app and we must keep records) whether it was uploaded via upload.module, CCK/filefield.module, fckeditor.module, etc. Basically anything downloaded through Private method.

Since nid is not being picked up, if I remove && node_access('view', node_load($file->nid)) from:

if (user_access('view uploaded files') && node_access('view', node_load($file->nid)))

... then it starts logging into file_downloads, so nid is going to be needed for DC original block view. (and I'd like to record nid too)

I'm trying to understand how filefield correlates files to nodes and gets the nid, but I'm having some trouble.

@Chill35 -- would you be interested in having me (or someone in my org) take a shot at adding code for this? We just hired a PHP developer who's had some exp w/ drpl. If yes, then the purpose of recording file downloads might strictly be to identify file, user, time, referrer, and perhaps source IP, but we can use the same table and expand the columns for administrative logging. Right now I'm trying to adapt the code from filefield.module for determines the node, but has to be a better way such as using the referrer.

Any help? Thanks!

#7

Emoreth - June 7, 2009 - 12:43

I tried to get this nid for a couple hours too... Unsucessful...

Then i removed that, because I really need it working, and i don't really need this...

After i get my project on time, and delivered to the customer i'll try to do a pretty fix for that...

EMoreth

#8

olio - July 22, 2009 - 10:16
Status:active» patch (to be ported)

Hi,
I changed the download_count_file_download() function and built a patch.

Changes:
1. db_query: The db query now returns a result even if there's no entry in the upload table (LEFT JOIN instead of JOIN). Reason: Files uploaded with the filefield module have no entry in this table.
2. node_access and user_access check: We only check node access permissions and user access permissions if there was an entry for this file in the upload table (= a nid was returned). Reason: These permissions only concern the upload module. I think that view access/ visibility to files provided by the filefield module should be handled by this module. Otherwise there could be the situation where these filefield files are visible and can be downloaded although the user has no permission to 'view uploaded files', as it is in my case because I built my download page with views.

In order to still prevent robots from downloading these files, we could simply disallow access to the directory /system/files with robots.txt (put "Disallow: /system/files/" in robots.txt).

@Caroline: What do you think?

Best regards,
Oliver

AttachmentSize
dowload_count_filefield.patch 1.63 KB

#9

olio - July 22, 2009 - 15:18

Sorry, but in the previous patch I forgot to modify theme_download_count_body($node), download_count_nodeapi() and download_count_view_page()...

With the patch attached to this post, the filefield files will be listed in node_view as well as on the download_count_view_page together with those provided by the upload module.

Best regards,
Oliver

AttachmentSize
dowload_count_filefield_2.patch 4.58 KB

#10

Iritscen - August 1, 2009 - 19:10

I applied the above patch before installing Download Count and switching to private file sharing, but I am getting an error in the log whenever I download a file (simply "failed to download x"), even though the download goes through. DC thus still lists the file as never having been downloaded. Is anyone else getting this issue?

#11

olio - August 3, 2009 - 10:09

Hi Iritscen,
you are right – sorry, my fault. With the attached patch, files uploaded with the filefield module or the upload module should be handled correctly now.
Regards,
Oliver

AttachmentSize
dowload_count_filefield_3.patch 5.23 KB

#12

Iritscen - August 4, 2009 - 20:10

All right, it works fine now! Thanks for your work, olio. Now when Chill35 comes around again, she'll have a patch all ready to slap onto the module code!

#13

kmonty - August 7, 2009 - 15:15
Status:patch (to be ported)» reviewed & tested by the community

#14

Chill35 - August 8, 2009 - 14:54

Looks great!

I had no idea that my module could work along the CCK Filefield module. It was really designed to keep track of files uploaded with the core Upload Module, hence the dependency on it.

This is good news, thanks for the work.

Don't hesitate to use my contact form when you want to grab my attention.

I'll see about committing the patch as soon as I can.

Caroline

#15

mudd - September 11, 2009 - 18:31

This patch doesn't work for me:

# patch -p0 < dowload_count_filefield_3.patch
patching file download_count.module
Hunk #1 FAILED at 129.
Hunk #2 FAILED at 139.
Hunk #3 FAILED at 163.
Hunk #4 FAILED at 217.
4 out of 4 hunks FAILED -- saving rejects to file download_count.module.rej

I'm running it on the latest release:
// $Id: download_count.module,v 1.13.2.5 2008/05/27 04:57:42 chill35 Exp $

...and when I manually integrate the chucks the module doesn't cause any errors but the table {file_downloads} never sees any records.

What am I missing? :(

Note/Edit: Although I did a dos2unix on the original download_count.module and that let me run the patch and update the module, I still don't see any records going into the file_downloads table.

#16

mudd - September 18, 2009 - 17:42

I thought I posted this, so here it is again...

1)
The reason I wasn't getting records written to file_downloads is because I don't set access enabled for upload.module (core), and download_count has this in Line#168:
   && user_access('view uploaded files')

Strictly speaking, should this be removed? If a file is downloaded then it's downloaded, and recording the event should not depend on this setting. Rather, this setting would prevent the download which in turn would not record the event.

2)
Also, each cck filefield field receives it's own permissions setting at the time it's defined, and I believe download_cound needs to perform an access check. I believe this is supposed to be handled by drupal-core's content_permissions module, so no cck-specific functions are needed.

Case in point: I have fields that are visible to admin roles but not by general user roles, yet these files are being listed in the node view for all roles.

I'm struggling on finding the right tidbit, but I /think/ it wants to go in here on Line#249

foreach ($node->files as $file) {
    if ($file->list && <something here>) {

#17

sign - September 22, 2009 - 12:24
Status:reviewed & tested by the community» needs work

The patch in #11 "does work", however it doesn't implement filefield permissions (as @mudd pointed out in #16),
thats why you would probably use download_count module (having a control over who can download filefield files) instead of Public Download Count module (http://drupal.org/project/pubdlcnt - actually I am not sure if this can be used with fielfield)

this can be a bit tricky
mainly, there is no reference in {files} table to which field it belongs

I think the best workaround would be to create another table, which will store this reference, so we can easily check if the file has been uploaded by upload.module or filefield and based on that we can get the proper permission name

anyone got any suggestions?

Marking this issue as needs work

#18

sign - September 22, 2009 - 17:10
Version:6.x-1.3» 6.x-1.x-dev

here is a patch (based on the patch in #11), still work in progress tho

1) if its a filefield, we do check the filefield permissions, eg "view field_name" - there is a new db table called file_downloads_filefield, which stores fid = field name
2) another major change is in db file_downloads, don't know why we should be using filename instead of fid? any reason for that?
3) filefield gets deleted when whole node or file has been deleted (doesn't work for files uploaded with upload module)
4) removed a dependency on upload.module (can work with upload and/or filefield)

TODO:
1) update script - with the db change, we need a simple update script that will convert old entries in file_downloads to new db structure
2) delete files stats uploaded with upload.module when file is deleted
3) user download counter page

AttachmentSize
461654_download_count_cck_17.patch 10.83 KB

#19

mudd - September 23, 2009 - 12:57

I think another high-priority TODO is to call content_permissions, as I described in #16.

As for FID -- I agree that might be the best key/value to work off of.

Note: I originally found my way to Download_count by way of needing a logger to track who has downloaded what files. (due to strict auditing req's.) But this module didn't give me the details I needed (and of course, wasn't intended for my purpose/need; but I also need it's public view feature so I'm liking it!) So I spawned another module for the purpose of only recording which nodes files are download from, by whom, and when. Thus I record file id, node id, filename and path, etc. Also, let's say one of my nodes is deleted -- well, I don't want any records deleted from my dl_log table because this is strictly for record keeping.

That said, I agree with the idea of cleaning download_count's table when a node and corresponding file are deleted. But with only the file name this seems difficult. So I think it's a good idea to change filename to file-id, then the module can either lookup the filename from {files} or store the filename too and get it directly from file_downloads.

As for the deleting records, I guess the assumption you're making is that any given file can be associated with only one node, so deleting is okay? Certainly FID would be easier than filename here. (Also, I don't know what mechanism CCK/filefield's uses for deleting orphaned files, but I believe it does cleanup during cron and waits until at least six hours after last db reference before removing the orphaned files.)

#20

sign - September 23, 2009 - 13:52

@mudd yes you are right, I am assuming that each file IS associated with ONE node.
what content_permissions are you refering here? "view field name" ? thats already implemented in #18

there is a hook filefield is using when file is deleted, which I am using. Not sure about clean up process there. Can't see anything atm.

I have been working on this a bit more and have an updated patch. This time I am including node->nid in table file_downloads. I can then easily pick up node id or all file downloads associated with that nid. Note - it will be stored only if download count module is invoked -> file is being downloaded.

Another thing I have done is a sum downloads per node. New table called file_downloads_sum with two columns, nid and count.

Everything exposed to views, so you can easily create pages like Download counter page with node titles, downloads, files, etc...

I'll clean the code a bit later today and submit the patch

#21

mudd - September 24, 2009 - 14:56

Ah -- cool! I'm trying to roll out a huge update for my site next week, so I'm really appreciative for your help on this access part!

The module "content_permissions" shows up in the User Permissions list. It's where CCK/Filefield sets it's access rights -- presumably the core mechanism for sites that aren't open and have a lot of user/group restrictions. Every time a new filefield (New Field type: file) is created in a content_type, this module lists a new line such as "view field_blah" & "edit field_blah".

(For some of my content_types I'm requiring the user to attach a file which is a sign-off document that tells me their organization has approved their posting. This field is hidden from most roles through the content_permissions module.)

Initially I didn't realize that content_permissions is a CCK component, so I take back what I said before about no CCK-specific hooks.

EDIT: (PS-- Sorry, I missed that you said this addresses the Filefield permission issue.)

I've applied this last patch (461654_download_count_cck_17.patch) to the latest official release, and it still lists filefield files set to no-access in content_permissions.

EDIT:

I'm also not getting any records written to file_downloads or file_downloads_filefield.

Why have two tables?

#22

mudd - October 13, 2009 - 22:10

@sign

From your response in #20, I thought you had an update, but nothing in two weeks... any update? Again, I tried your earlier patch, but it resulted in getting no logging/counting at all. So I stood up a new instance of Drupal, filefield and a download_count with your patch and found the same.

#23

arez - November 5, 2009 - 11:23

iam also very interested in this . any updates?

subscribed

#24

lorcon - November 12, 2009 - 02:11

subscribing...

#25

Cyberwolf - November 25, 2009 - 08:50

Subscribing.

An interesting discussion is going on as well regarding the possible introduction of a new hook that is called right before/after a file actually gets transferred: http://drupal.org/node/233997. If such a hook will be added, it would be better to use that one for counting downloads, as at that time the other modules got their chance already to check access permissions.

 
 

Drupal is a registered trademark of Dries Buytaert.