Closed (fixed)
Project:
FFmpeg Converter
Version:
6.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
22 Apr 2009 at 21:58 UTC
Updated:
3 May 2010 at 15:00 UTC
Jump to comment: Most recent file
Comments
Comment #1
zoo33 commentedFrom what I can tell, the str_replace() in the patch will never actually replace anything, so that line could just as well be written as:
$destination_path = $node->{$field_name}[$field_instance]['filename']Also, I'm not sure how
$node->{$field_name}[$field_instance]['filename']could work as a destination path for this file, as it is a new file being created, not an existing one being overwritten.Please explain.
I can see that here might be a problem with file path tokens not being expanded. There might be some hooks that this module fails to invoke before saving new files. If someone wants to investigate this they are more than welcome!
Comment #2
sndev commented$node->{$field_name}[$field_instance]['filename'] has to do with the original file (input file). I used that information along with the filepath info to get the original files location. Rather then trying to reuse the path tokens set for the file field, which would default to no user since apache is running the conversion during cron.
Comment #3
sndev commentedoops trying to post to a different issue.
Comment #4
zoo33 commentedFirst, I see you point about the file path in the first patch. (I misread the patch before.) This is probably a good change.
The second patch is harder to overview. It seems to deal with a lot of different things at the same time. I would prefer separate issues for each problem/task:
1) Whitespace/coding style changes.
2) Hm, isn't there already a different issue for this:
Also, I think a #description would probably be useful.
3) The changes in
ffmpeg_converter_nodeapi()– are these only related to 4)?4) The split of the conversion and snapshot generation jobs is a good idea, and I really appreciate your efforts on this. I just find the patch a little hard to follow right now, for the reasons mentioned above.
To sum up, let's consider the first patch RTBC, and then create separate issues for 1) through 4), OK?
Comment #5
sndev commented1) http://drupal.org/node/608862
2) You are right I'll adjust patch as necessary.
3 and 4 are related.
I'll create a new patch against HEAD and we can go from there.
Comment #6
sndev commentedThis patch is NOT for this. I am sorry, I'll add this to the different issue.
Comment #7
zoo33 commentedCommitted the patch in the original post, with the addition of a modified code comment. Thanks!