I've the following URL of image file which I want to import it:

http://media.resales-online.com/live/ShowImageXML.asp?SecId=ckzbxwfvludjkrl&Id=P1&ImgId=X1014122&z=1446224711&.jpg

However the problem is that this file doesn't have content-disposition (so I can't use the patch from #1104378-8: Allow passing custom filename to FeedsParser.inc::getFile()), so my filename is get saved as ShowImageXML.asp which fails with:

notice feeds Invalid enclosure http://media.example.com/live/ShowImageXML.asp?SecId=x&Id=P2&ImgId=y&z=z...
error Feeds exception 'PDOException' with message 'SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'public://images/ShowImageXML.asp' for key 'uri''

I think this is related to #1482530: Support remote files in file field., but I'm not sure.

So I guess the solution would be ASP extension to allowedExtensions, but it didn't work. It seems it works for other feeds which have proper JPG extension (like this).

My FeedsEnclosure looks like:

FeedsEnclosure::__set_state(array(
   'mime_type' => 'image/jpeg',
   'allowedExtensions' => 'png gif jpg jpeg asp',
   'safeFilename' => 'ShowImageXML.asp',
   'value' => 'http://media.resales-online.com/live/ShowImageXML.asp?SecId=ckzbxwfvludjkrl&Id=P1&ImgId=X1014122&z=1446224711&.jpg',
))

I've tried to use tamper plugin, but upon changes, the URL gets broken and I've got 404.

So I think the solution would be to save the file based on its mime type?

CommentFileSizeAuthor
#29 feeds-mime-type-detection-2611014-29.patch7.25 KBsamerali
#28 feeds-mime-type-detection-2611014-28.patch7.24 KBsamerali
#27 feeds-mime-type-detection-2611014-25.patch7.25 KBsamerali
#25 feeds-mime-type-detection-2611014-24.patch7.24 KBroman.haluska
#23 feeds-mime-type-detection-2611014-23.patch6.96 KBroman.haluska
#19 feeds_feeds-parser_match-extension-to-mime-type.patch6.4 KBjrusse27
#15 feeds-mime-type-detection-2611014-15.patch5.32 KBtwistor
#6 FeedsParser.inc-2611014-3.patch1.08 KBkenorb
#4 FeedsParser.inc-2611014-2.patch1.06 KBkenorb
#2 FeedsParser.inc-2611014.patch891 byteskenorb

Comments

kenorb created an issue. See original summary.

kenorb’s picture

kenorb commented
Status: Active » Needs review
StatusFileSize
new FeedsParser.inc-2611014.patch891 bytes

I've attached patch which compares the file mimetype with actual mime_type from the headers, if not match, it adds the missing extension based on Drupal MIME extension mappings.

So for example ShowImageXML.asp is application/octet-stream, however mimetype from URL is image/jpeg, therefore we're appending jpeg extension to the original filename, so it becomes ShowImageXML.asp.jpeg.

megachriz’s picture

megachriz
Primary language Dutch
commented
Related issues: +#2510788: Remove query string from path in FeedsEnclosure.

#2510788: Remove query string from path in FeedsEnclosure. looks related, adding for reference.

kenorb’s picture

kenorb commented
StatusFileSize
new FeedsParser.inc-2611014-2.patch1.06 KB

Corrected hardcoded mimetype. And when multiple extensions are available, choose one which is in allowedExtensions list (png gif jpg jpeg asp).

For example image/jpeg mime type can have jpeg, jpe, jpg extensions, so we take the first one which is allowed, so in this case it's jpeg, but it won't work for jpe.

So basically if we've no correct extension to be applied, nothing would be actually applied.

kenorb’s picture

kenorb commented
Title: Feeds doesn't import files which have non-image extension. » Feeds doesn't import remote images which have no proper extension (such as ASP)
kenorb’s picture

kenorb commented
StatusFileSize
new FeedsParser.inc-2611014-3.patch1.08 KB

A small fix:

                 $extensions = array_keys($mapping["extensions"], $ext_id);
                 foreach ($extensions as $extension) {
-                  if (array_search($extension, $this->allowedExtensions)) {
+                  if (in_array($extension, explode(' ', $this->allowedExtensions), TRUE)) {
                     $filename .= ".$extension";

as $this->allowedExtensions isn't an array, but string.

Tested Feed import and it works fine.

megachriz’s picture

megachriz
Primary language Dutch
commented

@kenorb Since this issue, #2510788: Remove query string from path in FeedsEnclosure. and #1104378: Allow passing custom filename to FeedsParser.inc::getFile() look so closely related, maybe you want to write a few automated tests that covers all three cases?

kenorb’s picture

kenorb commented

Good idea, I could check when I can, but I can't promise anything.

ckidow’s picture

ckidow
he/his
Primary language German
commented

subscribing

jschrab’s picture

jschrab commented

I don't think this will fully work.

Deep down underneath it all, file_get_mimetype() calls getMimeType(), which just looks for an extension to determine a file type - and that is what get's feed into the constructor for FeedsEnclosure(), which is how $this->mime_type is set.

I'm not seeing where the Content-Type is being used as a source for MIME type. Am I missing something?

(Addendum)

Under time pressure, for my case, I had to hack getSafeFilename() and put a magic number sniffer in before the extension existence test that can throw a RuntimeException. If recognized, the proper extension would be added to $filename and $extension would be set.

Doing a exif_imagetype() would be best - but for non-local files, we would have to fetch it and write it to disk first before we test it. Changes would have to be made so we don't fetch the file twice when $this->getContent() is called.

This is doable but there would be some regression testing to be sure.

Sigh. The best solution is to say "Feeds requires image files to contain accurate filename extensions."

djschoone’s picture

djschoone commented

The content-disposition is missing with the service we are using. And the mime type is determined as application/octet-stream.

On the other hand, the headers of showed me the content-type. I added a few lines

if(isset($file_download->headers['content-type'])) {
  $this->mime_type = $file_download->headers['content-type'];
}

I'm not sure if this can be adopted. If a patch is wanted, I can provide it.

joos’s picture

joos commented

Someone capable of porting this to the D8 version?
Please

megachriz’s picture

megachriz
Primary language Dutch
commented
Issue tags: +needs port to Drupal 8

@joos
I've put the issue on the todo list for Feeds D8. I'm in the process of gathering a few people for continuing the port of Feeds to D8. If would like to help with the port, let me know :).

Anyone who wants to write an automated test for this issue, for the D7 version?

joos’s picture

joos commented

@MegaChriz
Sure, I would love to help, but right now Im working on getting up to speed on how to do stuff "the right way" in D8, soo, im not sure im capable of helping yet.

twistor’s picture

twistor commented
StatusFileSize
new feeds-mime-type-detection-2611014-15.patch5.32 KB

Here's a quick example of how this should go. Not sure if all the logic is correct, but it's a start.

twistor’s picture

twistor commented

getMimeTypeMap() will need more work, but it illustrates the point.

Status: Needs review » Needs work

The last submitted patch, 15: feeds-mime-type-detection-2611014-15.patch, failed testing.

nwom’s picture

nwom commented

#15 appears to need a re-roll.

jrusse27’s picture

jrusse27 commented
StatusFileSize
new feeds_feeds-parser_match-extension-to-mime-type.patch6.4 KB

I've got an updated patch which fixes a logical bug with earlier patches by passing the response->headers through to this->sanitizedUri().

Patched against 7.x-2.0-beta4, needs to be tested against 7.x-2.x-dev

madelyncruz’s picture

madelyncruz commented

Has anyone encountered the same issue with 8.x?

megachriz’s picture

megachriz
Primary language Dutch
commented
Related issues: +#2968671: File target: add support for local file path as source

@madelyncruz
I haven't tested it, but based on the code for the file target \Drupal\feeds\Feeds\Target\File I expect this to be in an issue in the D8 version as well.
In #2968671-10: File target: add support for local file path as source @gkaas posted a patch that contains code that could probably help fix this issue for D8. Look specifically at the following line there:
$filemime = mime_content_type($filepath);

guypaddock’s picture

guypaddock commented

Any consideration to exposing an alter hook instead of trying to make the logic work for all cases? We're trying to get this to work with the "Media: Wistia" module and so far it's been tough-going. Feeds won't accept URIs formatted like "wistia://v/MEDIA_ID".

roman.haluska’s picture

roman.haluska commented
Status: Needs work » Needs review
Issue tags: -needs port to Drupal 8
StatusFileSize
new feeds-mime-type-detection-2611014-23.patch6.96 KB

I updated the patch with a few fixes

Status: Needs review » Needs work

The last submitted patch, 23: feeds-mime-type-detection-2611014-23.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

roman.haluska’s picture

roman.haluska commented
Status: Needs work » Needs review
Issue tags: +-needs port to Drupal 8
StatusFileSize
new feeds-mime-type-detection-2611014-24.patch7.24 KB

Patch against 7.x-2.0-beta4

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Issue tags: --needs port to Drupal 8 +needs port to Drupal 8

Thanks Roman, I reviewed this internally at jobiqo and looks good to me.

Tests are failing, not sure if they are related to this change? I see different fails at https://www.drupal.org/node/583948/qa so not sure.

samerali’s picture

samerali commented
StatusFileSize
new feeds-mime-type-detection-2611014-25.patch7.25 KB

Updated the patch against the latest 7-x-2.x-dev branch and added webp support.

samerali’s picture

samerali commented
StatusFileSize
new feeds-mime-type-detection-2611014-28.patch7.24 KB

Sorry uploaded the wrong file earlier, this is the new patch: Updated the patch against the latest 7-x-2.x-dev branch and added webp support.

samerali’s picture

samerali commented
StatusFileSize
new feeds-mime-type-detection-2611014-29.patch7.25 KB

Updated the patch against the latest 7-x-2.x-dev branch and added webp support.

The last submitted patch, 27: feeds-mime-type-detection-2611014-25.patch, failed testing. View results

bluegeek9’s picture

bluegeek9 commented
Status: Needs review » Closed (outdated)
Issue tags: -needs port to Drupal 8

Drupal 7 reached end of life and the D7 version of Feeds is no longer being developed. To keep the issue queue focused on supported versions, we’re closing older D7 issues.

If you still have questions about using Feeds on Drupal 7, feel free to ask. While we won’t fix D7 bugs anymore, we’re happy to offer guidance to help you move forward. You can do so by opening (or reopening) a D7 issue, or by reaching out in the #feeds channel on Drupal Slack.

If this issue is still relevant for Drupal 10+, please open a follow-up issue or merge request with proposed changes. Contributions are always welcome!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.