This is a pretty big one because it makes the module unusable if files are outside of the site root. I am running a media delivery system. We just moved 'files' off to another device, because of disk space and security issues. Files are served by Drupal system, for protection, and the actual location of the files directory is now /media/storage/files, not inside Drupal.

The getcwd() used to build file paths (there are numerous examples) break the module completely in this case. For example, Flashvideo thinks this path:

/media/storage/files/videos/sample.mov

Is actually here:

/var/www/httpdocs//media/storage/files/videos/sample.mov

And the whole thing goes to hell in a hay basket! I don't know why this getcwd() . '/' . stuff is even there? Drupal can be trusted to always give you the FULL system filepath anyway, so why is Flashvideo pre-pending stuff to it?

CommentFileSizeAuthor
#1 675910-getcwd_removal.patch6.34 KBgreg.harvey

Comments

greg.harvey’s picture

greg.harvey
Primary language English
Location Occitanie, France
commented
Status: Active » Needs review
StatusFileSize
new 675910-getcwd_removal.patch6.34 KB

Ok, here's a patch that removes all instances of the offending getcwd() . '/' . bits in the code. I've tested it briefly with the files directory both within site root and not - it seems fine in all cases, as I suspected, but I haven't tested every scenario. That said, the Drupal core will always return full system path, so as I noted before, I have no idea why this code was ever there. (Though because I have no idea why, the maintainers may know of some edge case where this is needed...?)

damienmckenna’s picture

damienmckenna
Location TN, USA
commented

While I'm not using this module, the concept of just using the built-in Drupal functions to obtain the file path is the most logical option.

travist’s picture

travist commented

Thanks for this Greg. I just submitted this patch to CVS, but will need to wait for a lot of testing to occur before the full release comes with it in place. This is a pretty huge change.

One question... do you think we can provide an update path for this change ( using an update function within the *.install file )? This will make sure that this change is reverse compatible. From just looking at this, I can see that it might break old installations of FlashVideo.

I originally wanted to use the drupal_get_path method for most of these, but that was not working well for file system operations when I originally designed this module. Maybe using your code will alleviate this issue once and for all, but I really would like for a lot of people to test it before it becomes official.

Thanks again for your efforts.

Travis.

greg.harvey’s picture

greg.harvey
Primary language English
Location Occitanie, France
commented

Would be straightforward enough to run an update on the flashvideo table, if it's necessary. I confess, I had no old data to deal with, so I didn't pay particular attention to existing paths, but they *seemed* to be ok. Would need to test more carefully though (e.g. install 6.x-1.0, create some content, update to current and apply the patch to see what happens).