In insert_insert_content(), there's an array of file types that will insert as an image, everything else is assumed to be something else and will get inserted as a link or an icon link.

I believe adding 'svg' to this array allows it to work for me. But honestly my local setup for the site I'm working on is complex enough. I'd love further confirmation and testing.

Patch coming.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vordude created an issue. See original summary.

vordude’s picture

FileSize
545 bytes

Something like this? An alter hook would probably be better. And D8?

Snater’s picture

As far as I am aware (please correct, if I am wrong), there is no native svg support in core: #2368485: Allow .svg file uploads. I suppose you are using another module to enable svg upload? Having svg file types recognized as images would impose a dependency on such a module.

The best way would be to check whether uploading of svg is supported in the Drupal instance and add the svg extension to the Insert module on that check. However, I am not sure there is a proper way to do that as the basic extensions are already hard-coded into the core image module: https://api.drupal.org/api/drupal/modules%21image%21image.field.inc/func..., so other modules probably use customized validation.
An alternative would be introducing an option for the Insert module allowing to configure the file extensions that shall be recognized as images. Yet, that would make things complicated for users as they might expect that enabling file extensions here would immediately allow them to upload such files.
Eventually, I tend to declare this won't fix pointing to https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func... for overwriting the whole Insert module hook as a quirky solution. But, maybe, there is some more input on the issue.

The Drupal 8 version works differently. Image and file fields receive specific Insert functionality. There is no need to white-list specific file types on module level.

Snater’s picture

Status: Active » Closed (won't fix)
q0rban’s picture

Status: Closed (won't fix) » Needs review

As far as I am aware (please correct, if I am wrong), there is no native svg support in core: #2368485: Allow .svg file uploads

That issue is actually for drupal.org, not Drupal core. I'm pretty sure Drupal core works with SVGs. Reopening. :)

Snater’s picture

OK, thanks, I got some wrong reference there. Still, I do not get the proper picture. The issue is not about core not being capable of processing svg files in general. I am a bit mixed up by image.field.inc, line 324, which, basically, is mirrored in the Insert module.

Would adding svg to the Insert module's line not make the assumption svg is natively supported by core's image module, which it does not seem to be as that particular line is lacking svg? How do I enable svg files on image fields without altering core's image module? Just adding svg to the allowed file extensions on an image field does not seem to have an effect.

juanolalla’s picture

Would adding svg to the Insert module's line not make the assumption svg is natively supported by core's image module, which it does not seem to be as that particular line is lacking svg?

Personally, I don't see why recognizing a .svg file as an image in this module would make an assumption about core, and why this improvement would be bad. If this is a good way to solve it I would include it, and if you think there is a better way to do this, let's implement it that better way.

Snater’s picture

OK, maybe I am being too theoretical on that -- please, do not hate me for that, I just wanted to help out on reviewing the module's D7 issues. Yet, my personal issue with the change is that it works around a problem that, as far as I am aware, is to be solved in core's Image module. The change makes a work-around even dirtier.

First off, why is the list of supported image file types hard-coded into core's Image module at all, instead of being configurable, for example, in the UI on an Image module configuration page? There might very well be an important reason, but I have not found the answer to that yet. More related to the issue: Why doesn't it seem to be possible to query core's Image module for enabled file types?

In my opinion, there is an assumption: Why does the Insert module, which is, in some way, just piggybacking on the Image module, have to maintain a list of supported image file types assuming these are supported by (enabled in) the Drupal instance the module is installed in? Someone else might want to have support for bmp or some other file type… Personally, I do not see the point in maintaining such info in the Insert module. That is one reason I dropped the "auto" insert option (which the change is related to) on the Insert module's D8 version -- personally, I would not want to maintain such code as it is prone to errors and promotes a state violating the Separation of Concerns principle that requires a change upstream.

And, finally, someone who applies a local alteration to core (Image module) code for enabling a file type (which, still, is the only way I currently know to enable SVG) should be fine with altering the Insert module code as well. That said, I would be perfectly fine with having that tweak documented.

(In the end, it is your guys' company having created the D7 module and you probably can just have the change merged -- I am not going to revert it, that is for sure.)

q0rban’s picture

Status: Needs review » Needs work

Snater, I think you raise fantastic questions, and I appreciate your thoughtfulness on the issue. You're right, it does seem odd that this is hard-coded in core. I did find #2539478: [D7] Allow image fields to use any extensions the current image toolkit supports (instead of hard-coding jpg, png and gif only) for Drupal 7 and #1014816: Allow image fields to use any extensions the current image toolkit supports (instead of hard-coding jpg, png and gif only) for Drupal 8 (which has been committed to 8.5.x). I think it's fine to leave this open until such a time as we get some traction on the issue for Drupal 7. We're using the patch with success, so it might be helpful for others in the interim, but that doesn't necessarily mean we need to commit it.

Snater’s picture

The change in 8.5 is a great improvement. I have already implemented it in Insert for D8 - it will be part of 8.x-2.x.

As for D7, while having a look into #1431322: Working with other media formats (insert <video> and <audio> tags), I thought it might make sense to have some Insert setting(s) anyway, since audio and video files would need to be detected as well. Personally, I am still not perfectly sure about implementing #1431322: Working with other media formats (insert <video> and <audio> tags) at all; But if, there would need to be a way to determine which type's of audio and videos files may be embedded using the automatic option and I would not want to limit that to hard-code values as well. Best might be global config options on a dedicated Insert configuration page (like it exists in Insert for D8). I am not sure about #2539478: [D7] Allow image fields to use any extensions the current image toolkit supports (instead of hard-coding jpg, png and gif only) getting resolved in the foreseeable future. So having a global Insert config option (or, alternatively, a hook) for supported image file extensions would be an alternative to having the extensions purely hard-coded (might call it a workaround to #2539478: [D7] Allow image fields to use any extensions the current image toolkit supports (instead of hard-coding jpg, png and gif only)). Although the default values would still be hard-coded, they could be extended individually. For some reason, someone might want to insert bmp or ico files, which could also be activated easily having a config setting.

Snater’s picture

As per last comment, the attached change set adds a global module configuration page that allows specifying file extensions for detecting image files. This would be an easy way to individually add extensions that should be treated as images. The change is planed to go in hand with #1431322: Working with other media formats (insert <video> and <audio> tags).

  • Snater committed 5a383e8 on 7.x-1.x
    Issue #2918451: Allow setting image file type extensions
    
Snater’s picture

Status: Needs work » Fixed

Eventually committed the slimmed down the change after resolving #1431322: Working with other media formats (insert <video> and <audio> tags). This is the last remaining issue for the D7 version of the module, so I would like to get it off the table. File type extensions may be set on an Insert module config page now, which, in my opinion, is a reasonable solution for the time being. While working on top of #2539478: [D7] Allow image fields to use any extensions the current image toolkit supports (instead of hard-coding jpg, png and gif only) would be the perfect solution, I do not see that issue getting resolved any time soon. Whenever that gets resolved, a new issue may be opened to adapt Insert.

Status: Fixed » Closed (fixed)

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