I've done some testings with this and it looks like after you try to parse a media url, and in case it's another type of file you have a fall over to verify if the file actually exists by uri. The query you used would be nice in case we import just one type of files per run. But I wanted to map file type in mappings and import all my files with just one feeds importer.

  $fid = db_query("SELECT fid FROM {file_managed} WHERE uri = :uri AND type = :type", array(':uri' => $value, ':type' => $this->config['file_type']))->fetchField();

Since 'uri' is really unique in database you don't have to verify the file type, let's just get the file object like you did before:

if (!$fid) {
  $file = file_uri_to_object($value);
  $fid = isset($file->fid) ? $file->fid : NULL;
}
CommentFileSizeAuthor
#1 feeds_files-existing_fid_by_uri_fix-1933490-1.patch984 bytesasgorobets

Comments

asgorobets’s picture

asgorobets commented
StatusFileSize
new feeds_files-existing_fid_by_uri_fix-1933490-1.patch984 bytes

Here is a patch for that.

asgorobets’s picture

asgorobets commented
Status: Active » Needs review
rwohleb’s picture

rwohleb
he/him
Primary language English
commented

I avoid loading the full file object on purpose. I don't want to load more than I have to for performance reasons.

You are correct about the situation with URI being a unique key. I'm now trying to remember why I did it that way. I think I did it to prevent a lookup of a URI that exists under a different file type, such that an update operation wouldn't trash it.

Your patch doesn't fix anything, other than running the URI through file_stream_wrapper_uri_normalize().

I included the type as a settable field to be well-rounded, but using it is very delicate. Unless you have a good way of setting one of the limited number of correct type values, this is the wrong way to handle things.

Correct me if I misunderstood any of your comments.

asgorobets’s picture

asgorobets commented

As i said, your query could work only for the selected file type, I have PDFs imported along with JPG and others and it's importing a new file (instead of updating existing one) since it's pulling the setting from feeds importer configuration, and it's set to Images.
You avoid loading a file object, but you do the same in lines above here:

  $uri = media_parse_to_uri($value);
  if (isset($uri)) {
    $file = file_uri_to_object($uri);
    if (isset($file->fid)) {
      $fid = $file->fid;
    }
  }

Since $uri is returning false for me, it's not going throw your file_uri_to_object, but instead it would go throw the fall over that contains basically the same code. It would be the same execution time, no?

I wasn't aware why you used this query and just proposed a way to fix that.
Is there a way to select multiple file types for my case?

rwohleb’s picture

rwohleb
he/him
Primary language English
commented

I'm only loading a file object when I have to. If there is a place where I'm not, then it is a bug. The specific line you mentioned is to support the way the media module handles things, IIRC.

Are you possibly confusing file type with filemime? The file type comes from the file_entity module. If you are injecting the mimetype into the file type field, that won't work. The file type will be the file entity bundle, which by default is image, video, audio, or document. Each file type can specify a list of mimetypes that it contains. I don't yet support mapping of type to mimetype, as you've noticed :)

Also, this module doesn't yet support actually moving/copying the files, so make sure that won't be an issue. I plan to support it, but haven't gotten around to it yet.

asgorobets’s picture

asgorobets commented

Sorry if my comments were confusing.

I'm not mapping mimetype to file type (bundle). I'm mapping bundle to bundle. I'm moving files from another D7 site and I have the same bundles (file types) there.
And you have a mapping for type (bundle) that's perfect for my case.

But here is the problem:

$fid = db_query("SELECT fid FROM {file_managed} WHERE uri = :uri AND type = :type", array(':uri' => $value, ':type' => $this->config['file_type']))->fetchField();

When searching for existing file_entity by unique uri target it will fail if in my feed_importer the selected bundle is 'Image' and it's importing a PDF file, since PDF's type would be "application".

The query would be "SELECT fid FROM file_managed WHERE uri = 'public://test.pdf' AND type = 'image', which will never return me the fid.
This results in creating a new entity instead of updating existing one.

Let's say we don't need to load/init a file object, can we just look up for unique uri without 'type'?
$fid = db_query("SELECT fid FROM {file_managed} WHERE uri = :uri", array(':uri' => $value))->fetchField();

OR
Go throw $this->config['mappings'] and verify if we have 'type' target there, use it instead of feed_importers file_type property.

Sorry again If I've confused you. Let me know if you need more details.

Thanks,
Alexei.

rwohleb’s picture

rwohleb
he/him
Primary language English
commented

Ahh, ok. I'm glad you are using type correctly. Saves a support headache :)

You have the same ideas as I've been mulling over since you opened this issue:
* The existingEntityId() function needs to be modified to only check against URI when target is URI.
* The entity entitySave() function needs to be modified to to see if type is set. If not, then it tries to match type based on mimetype. If that fails, it falls back on the type set in the config form.

This should cover all the bases.