Yesterday I did an update to the new beta7-version. I did a new import of my previously defined CSV-Mapping in which I have filefields with a path. After running the import with beta7 all sourcefiles on the server where deleted.

Comments

Status:Active» Needs review
StatusFileSize
new807 bytes

I can confirm that.

These files are local to the site, right? You have placed them in your sites directory, referenced them from your CSV file, then mapped them to a filefield. After the import they are gone?

This bug is an interaction between two "improvements" to Feeds, one deleting temporary files, the next one allowing local files to be references. The latter one actually referencing files that should be permanent.

#863494: Delete temporary enclosures file
#755556: Support saving local files in filefields

This patch reverses #863494.

StatusFileSize
new1.46 KB

This approach still removes temporary files, but keeps files living in site's files directory alive.

Your are right. The sourcefiles are stored in my sites directory and after import they have been vanished.
I have downgraded to beta5 and everything works as expected.

#3: Care to give me a quick review on #2?

Sorry, but I do not find the time for it right now.

IMHO, getFile and getContent are looking quite messy right now (actually partly my fault from #755556: Support saving local files in filefields). They should be rewritten...though that is probably a separate less urgent issue.

After disagreeing with the use of __destruct here initially and thinking about every way we could avoid it, I finally came around to realizing it should be OK and it logically makes sense if you think of FeedsEnclosure as the only exposed access to the file for feeds. Ideally getFile should be named getTempFilename if that filename is going to be deleted as soon as the FeedsEnclosure is unset. But that's for another issue.

I haven't gotten around to actually testing the patch but it looks good to me.

#6

Please take a look at the D7 version of FeedsEnclosure: http://github.com/lxbarth/Feeds/blob/master/plugins/FeedsParser.inc

D7's FeedsEnclosure:: getFile() takes $destination parameter and returns a permanent file object. This has the following implications:

1. Caller of getFile() can specify where it expects the file to be placed. In many cases, this avoids an additional copy operation on the file system.
2. The file returned by the enclosure is permanent. Due to the fact that we have the $directory is fine for all use cases right now, we MAY want to change that.
3. If the original resource is a file (not a URL like in most cases) and the file is NOT in the location requested via $destination, it will be copied, but not deleted. I think this behavior is fine, we shouldn't delete these files.

Alex, that does indeed seem like a nicer model, although I'm confused about this line in the D7 getFile:

<?php
if (strpos($this->getValue(), $directory) === 0) {
?>

I don't see $directory set before this line...

Anyway, for this D6 issue, what do we need to do to get this patch in? Have one person test it?

Status:Needs review» Reviewed & tested by the community

#8: you got it: http://github.com/lxbarth/Feeds/commit/d570ee01b6c902c08db1eaf88a214fa62...

Updated #2 with a more concise comment for getFile():

<?php
 
/**
   * @return
   *   A path pointing to a file containing the resource referenced by the
   *   enclosure. This method downloads the resource if it is not local. The
   *   file path must be considered temporary and is only valid for the current
   *   page load.
   *
   * @todo Get file extension from mime_type.
   * @todo This is not concurrency safe.
   */
 
public function getFile() {
   
// ...
 
}
?>

Will commit shortly.

StatusFileSize
new2.03 KB

Title:csv import deletes sourcefiles Fix enclosure source files deleted
Status:Reviewed & tested by the community» Fixed

Committed, thank you for reporting and reviewing.

http://drupal.org/cvs?commit=427320

Status:Fixed» Closed (fixed)

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