Updated after #86.
Problem/Motivation
Changing the allowed file extensions on an image field doesn't update in the edit form. Other changes to the field are reflected though.
Steps to reproduce -
1. Create an Image Field
2. Set the allowed file extensions to also include svg, etc.
3. Or modify the allowed file extensions after creating the field to include svg, etc.
The current patch tries to solve the fact that the list as defined by a site builder is reduced based on a hard-coded list instead of the list of extensions supported by the actual toolkit.
Proposed resolution
It should also check when defining an image field, not only when showing an image widget (and allow site builders to shoot themselves in the foot).
- When defining a new image field, the default value for 'Allowed file extensions' should be the list of supported extensions.
- When saving field settings, the entered list should be validated against the list of supported extensions.
- If there are no unsupported extensions: save as is
- If there are unsupported extensions, we have the following options (TBD):
- Warn but accept the reduced set.
- Fail with a form validation error.
- If an existing image field is edited: no changes, ie take the currently saved list as value.
- If an existing image field is saved: same validation as above.
- If an image widget is displayed we have 2 options that should align with the options above:
- Take the list as is. Do not make any changes to the list at this point.
- Take the list and intersect with the actual list of supported extensions.
- If an image with an unsupported but allowed extension is uploaded:
- Fail (aligns with the fail option above).
- Fail if dimensions are specified (never logical).
- Fail if dimensions are specified and getimagesize() (does not require a toolkit) indicates that the dimensions fall outside the allowed dimensions. If getimagesize() fails to produce dimensions: TBD.
I am in favor of the warning option, not the hard fail as most error handling on the image widget side will still be necessary for when toolkit configuration has changed (more specifically the list of supported extensions).
If we go for the warning, the warning should indicate which extensions are unsupported and what the consequences are:
- Image styles will not be applied to unsupported extensions.
- Resizing due to min/max resolution will not be possible and thus image uploads might fail.
- [future request] Auto rotating on upload will not be possible, so an upload might fail. To determine if we actually need to rotate, we would not necessarily need an image toolkit, just the EXIF PHP extension.
Remaining tasks
Decide on a path forward, quickly. So we can get rolling on a direction again. This issue has stalled out at comment #86, which bodes poorly for adopting any solution.
User interface changes
API changes
Data model changes
Original Summary
Changing the allowed file extensions on an image field doesn't update in the edit form. Other changes to the field are reflected though.Steps to reproduce -
1. Create an Image Field
2. Set the allowed file extensions
3. Modify the allowed file extensions after creating the field.
Comment | File | Size | Author |
---|---|---|---|
#142 | 1014816-142.patch | 6.65 KB | subhashuyadav |
#141 | 1014816-141.patch | 6.64 KB | TomiMikola |
#140 | 1014816-140.patch | 6.61 KB | venugopp |
#139 | interdiff_allow_svg_image-20200209-139.txt | 3.45 KB | Oscaner |
#139 | allow_svg_image_20200209_139.patch | 6.58 KB | Oscaner |
Comments
Comment #1
guybedford CreditAttribution: guybedford commentedTurns out this is a duplicate of http://drupal.org/node/515152 in D6.
Tiff and other file types are not allowed for Image Fields.
Comment #2
yched CreditAttribution: yched commentedrecategorize
Comment #3
BarisW CreditAttribution: BarisW commentedguybedford: why has this been changed to a feature request? I'd say this is still a bug. If you use other Image libraries (eg GraphicsMagick) it is possible to convert TIFF files and other formats.
Currently you are allowed to change the file types but you don't get a warning/error if certain filetypes are not allowed. This is a Drupal WTF that causes a lot of confusion. So we need to add a message if one tries to change the file extensions on images, or we need to let the user change the extensions to whatever he finds useful.
Comment #4
BarisW CreditAttribution: BarisW commentedAnd for clarification some screenshots:
When entering file extensions in the field definition interface, you are allowed to add any file extension.
But when creating a node, it filters out all extensions except jpg/png/gif. HUH?
Comment #5
rfulcher CreditAttribution: rfulcher commentedWhat is the status on this issue? I am having the same problem. I have people who want to upload tif files and they can't.
Comment #6
webchickClosed #1081060: Image field silently disallows non-web extensions; proclaim it loudly instead as a duplicate, which has a good analysis of the problem from tegneged:
That's indeed what's going on.
Morbus at #1081060-2: Image field silently disallows non-web extensions; proclaim it loudly instead notes:
I tend to agree with this analysis, since image fields come with things like thumbnail generation, inline display, and so on rather than just "a field to put images." A quick gander through http://en.wikipedia.org/wiki/Comparison_of_web_browsers#Image_format_sup... seems to back up the assertion that support for extensions other than JPG, PNG, and GIF are only spottily supported.
Nevertheless, it seems odd that we would hard-code such a whitelist and not allow users to shoot themselves in the foot if they wanted to. Perhaps that array should be swapped out with a variable that could be overridden in settings.php?
Or, if nothing else, we should make the description of that field more clear and set the field to #disabled = TRUE so people don't think they can change it.
Comment #7
webchickAlso, bugs are fixed in Drupal 8 first, then backported.
Comment #8
rfulcher CreditAttribution: rfulcher commentedWebchick,
Thanks very much for the analysis of the situation. I am ok with what you explained, it seems very logical. My suggestion of the situation is to put something in the documentation of the image field so that people know that it is for web image formats not any image format. If I knew that from the get go I would have just converted the images and went on.
Again thinks a lot for looking into this and replying. It is much appreciated.
:)
Comment #9
magnusk CreditAttribution: magnusk commentedI just encountered the same issue. A client requested the addition of the TIFF image format (http://en.wikipedia.org/wiki/Tagged_Image_File_Format) to the upload widget. I changed the field configuration and could not understand why the node edit form would not support the tif and tiff extensions. This is a seriously misleading field configuration option.
If TIFF cannot be supported (they're commonly images obtained from scanners?) then the field configuration interface should indicate as much and prevent entering extensions that will be ignored. Further, for the sake of maintenance and configuration control, the supported extensions should probably not be managed as a hard-coded string in image_field_widget_form(). Which could open up control of the accepted extensions via each image toolkit's admin configuration.
As a practical exploration, if I force the widget to accept TIFF files I see that the thumbnail cannot be generated with the default GD image toolkit. The thumbnail is fine if I install and use the ImageMagick toolkit instead (http://drupal.org/project/imagemagick).
Comment #10
josegaert CreditAttribution: josegaert commentedThere is a lot of talk about Drupal 8 and the html5 initiative.
And html5 includes svg as a major part of its specifications.
Shouldn’t Drupal prepare for html5 and the svg image format by providing support for it in the image module?
True, in the big bad world of the browsers, http://en.wikipedia.org/wiki/Comparison_of_web_browsers#cite_note-FFSVG-171, there is only minor support for it. But it is the buzz of the day, and they are all trying hard to improve support. Well, the major browsers.
For me, I'm going from one pitfall to another trying to write a module that supports svg and also plays well with the image UI.
into another with some extra attributes and maybe some extra options.I think that implementing svg in Image module shouldn't be that difficult. The nice thing about svg is that it doesn't need an image toolkit to transform it. Every transformation can be done easily in php by wrapping one
Comment #11
leschekfm CreditAttribution: leschekfm commentedHi,
I think this would be a good way to go. I have made a patch that removes the restriction to the extensions in the supported_extensions array and adds a warning that unsupported extensions may not be properly displayed. The patch is against d7 for now. If this is the desired behavior I would create another one for d8.
So please give feedback if this would be ok :)
Comment #12
leschekfm CreditAttribution: leschekfm commentedAs I have noticed that #1664844: Convert image toolkits into plugins is being worked on, I would suggest that these plugins define which file types they can handle (I've read this proposal somewhere before). Nonetheless I think it would make sense to add an option to disable the restriction to the supported extensions. This option could be integrated in the image-toolkit form (admin/config/media/image-toolkit).
I think this should be postponed until the above mentioned issue is resolved.
Comment #13
claudiu.cristeaNow, new image types can be added by implementing/extending toolkit plugins. Drupal is shipped with the GD toolkit, handling JPEG, PNG and GIF image formats. A contrib/custom module can easily extend this toolkit with other image formats. Is this enough for your case?
Comment #14
errev CreditAttribution: errev commentedWhat about Drupal 7.
Does it work for D7?
Comment #15
brandy.brown CreditAttribution: brandy.brown commentedthe patch works well for me thank you
Comment #16
DamienMcKennaDon't forget to remove your name from the "Assigned" field after you finish working on a patch.
Comment #17
Dave ReidWe definitely should not hard-code this list of extensions. We should be using the list of supported extensions from the Image toolkit instead.
Comment #18
Dave ReidComment #20
Dave ReidImproved inline docs to help clarify what is going on here.
Comment #21
mondrake#20:
1. There is #2477381: GDToolkit::getSupportedExtensions returns incomplete list aiming at improving the list of extensions actually supported by GDToolkit. Maybe this can be referenced in the @todo.
2. TIFF images are not currently supported by the GD toolkit (see ::supportedTypes), so you'll never get an
$extension === 'tiff'
at the moment. The toolkit needs to be enhanced if we want that, but I'd say that would be a separate issue.In general I think the issue title is out of sync now, we are no longer adding additional file extensions, but rather removing the hardcoded ones with the ones provided by the default toolkit.
Comment #22
Dave ReidComment #23
Dave ReidComment #24
Dave ReidComment #25
Dave ReidComment #26
mgifford@Dave Reid any feedback on #21?
I installed the patch in #25.
I assumed that this would allow me to upload a SVG file to an article (after allowing the svg extension) but it didn't.
The issue summary should be updated to include what the patch is trying to accomplish. .
Comment #27
darol100 CreditAttribution: darol100 as a volunteer and commented1+ for been implemented.
Comment #28
mondrake@mgifford re #26:
That's right but here you need to have a toolkit that does support the SVG format. Core's GD toolkit does not. I tested #25 with ImageMagick 8.x-1.0-alpha1, configured to support SVG, and it works nice, see below:
Before selecting the image (see 'Allowed types')
After uploading the image (see the filename)
(caveat: you may need to add a convert effect to the image styles used for the image widget/formatter and convert to PNG or JPEG to see the derivative picture)
Comment #29
mondrakeI think this needs tests to make sure that we only upload files whose extension is within the intersect of the extensions declared by the field settings and those provided by the toolkit.
Also, the content of the interdiff in #25 is not in the patch :(
Comment #30
m1n0 CreditAttribution: m1n0 as a volunteer commentedRerolled patch from #25, now containing the code from interdiff.
Comment #31
m1n0 CreditAttribution: m1n0 as a volunteer commentedComment #33
mondrakeAdded tests. I think we're pretty close to RTBC now?
Comment #34
claudiu.cristeaNits.
Unrelated. Remove it from the patch.
This is not inheriting anymore the docblock :)
Remove space before "->".
Comment #35
mondrake@claudiu.cristea thanks, fixed.
Comment #36
Dave ReidComment #37
claudiu.cristeaComment #38
mondrakeIn the patch we added 'jpg' and 'jpe' extensions to
::getSupportedExtensions
, but have not adjusted::extensionToImageType
accordingly - this leads to failure in 'create_new' and 'convert' GD toolkit operations.Also - for the image factory - do we go for constructor injection change (will force the patch to go to 8.1.x) like in #35, or shall we better introduce a getter method so that this can still go to 8.0.x?
Comment #39
mondrake#38:
Fixed and added tests.
Done here.
Also, removed 'Needs screenshots' tag as the ones in #28 are OK IMO
Comment #40
mondrakeComment #41
jwilson3Allowing SVG upload to a website is a security risk because an SVG file can contain javascript. One mitigating factor may be that
<script>
tags seem to be ignored in external SVGs attached inside an<embed>
<object>
or<img>
tag. I *think* the SVG has to be added directly into the DOM for the javascript to be executable, in which case, it would be up to other Drupal filtering mechanisms to strip the JS before display.Can someone test this by creating and uploading SVGs containing the following tests, to be sure?
There are more tests, some related to specific browsers, see the full list here:
Source: https://html5sec.org/#svg
Comment #42
jwilson3Comment #43
mondrake#41, thanks @jwilson3.
This could not be tested with core only, as PHP GD2 (and therefore the GD toolkit) does not support vector image formats like SVG.
I tested this with the following environment:
In that configuration, any attempt to upload any of the SVG files in #41 in an image field fails with the following message:
and in the watchdog we have the following failure from ImageMagick detected:
input:
which result in error:
In other words - for the files indicated ImageMagick was unable to identify the files as images, and therefore upload was prevented (again, this requires #2377747-73: Incorrect node create validation error when an invalid image is attached to a field).
Comment #44
claudiu.cristeaWell, I don't think you can change the constructor signature neither in 8.0.x, neither in 8.1.x because in the meantime somebody might have extended the class in a module and that would break his site. IMO it's good to go with a getter.
Anyway, let's add a @deprecated tag with a message like "@deprecated in Drupal 8.0.x, will be removed before Drupal 9.0.0.
Also, I would mark this method as @internal (that is not yet documented, see https://www.drupal.org/node/2550249).
I would add also "@todo Replace this method with proper service injection in 9.0.x."
Comment #45
mondrake#44 done, thanks for feedback.
Comment #46
claudiu.cristeaSee https://www.drupal.org/coding-standards/docs#order for how to order the sections. @return, @deprecated, @todo is the correct order. @internal is not documented yet, let's leave it the last.
I wonder that this doesn't popup a notice when the protected property is not initialised. I would use !isset().
Comment #47
mondrake#46 thanks.
Comment #48
claudiu.cristeaThis is the ultimate sophistication: Introducing an already deprecated method :-)
Good to go, from my point of view.
Comment #49
mondrakeSummarizing here a private conversation with the Security Team about #41, after having received clearance to make it public.
Comment #50
alexpottLet's not deprecate until we actually have service injection. We shouldn't deprecate until we actually can.
Also what about upgrade path issues? Also what happens if you change the toolkit and some image files are no longer supported - in this instance I don;t think you should be allowed to change the toolkit...
Comment #51
mondrake#50, removed the deprecation.
So... I am not sure it makes sense to 'lock' the toolkit if with the change some formats will become unsupported. Such lock would only work for managed files AFAICS, but as pointed above there could be other files that get into the equation. Also note, ImageMagick toolkit allows to define its supported extensions via configuration, so even within the same toolkit we might have the situation of extension previously supported that do not get support any more.
We're not touching any config here, just intersecting the extensions supported by the toolkit with those defined in the field definition, that are two independent lists. Maybe I am missing something?
Comment #52
jwilson3@Mondrake re comment #43:
So were you able to identify definitively that ImageMagick specifically blocking these because of the JS present or could it have been because the SVGs (as written in #41 didn't have the "full" format (with doctype and complete svg signature)? fwiw, most browsers understand the svg files as I've written them in #41, so i assume imagemagick has more strict rules on what it accepts, that may or may not have anything to do with the fact the svg has some embedded JS, thus you may need to specify a more compliant SVG file than the ones in #41 to test this.
Presuming many site builders may not know of the security risks inherent with SVG, this seems like a pretty glaring "self-inflicted" security hole to be acceptable to allow without any further discussion.
Comment #53
claudiu.cristeaI see @alexpott questions from #50 were answered and the deprecation was removed. Also, regarding #52, i see the response clear: By default, .svg files are not in the allowed extensions. So, the upload will fail. Note that this risk exists also for other kind of files if the site admin would allow such uploads.
Moving back to RTBC.
Comment #54
jwilson3Not true.
For regular file fields, the Drupal 8 default extensions list only contains
txt
. If you manually configure the field to accept files with extensionjs
, then the file extension is changed automatically to ".js.txt". See attached screenshot (taken from Simplytest.me instance running Drupal 8.0.2).This same security check needs to be done for SVG. Expecting the image library to do this checking would be akin to allowing JS files be uploaded to the server and then let jQuery test to see if the code is safe or not. (Okay, that's a poor analogy, I admit).
Comment #55
jwilson3A little research: According to the HTML5 spec, SVGs added to the page using the
<img>
tag cannot not execute embedded scripts, but scripts can be run when using the<object>
or<iframe>
tags.Does this make it safe enough then to not have to change file extension? (The assumption being Drupal image fields will always use the
<img
tag)?Comment #56
mondrakeNR seems to me a more appropriate status, given input in #55.
#54
For regular *file* fields, it's out of scope of this issue.
For *image* fields, it seems to me that, based on comment in #55, we need consensus that it is not necessary.
Tagging 'needs security review', I do not know if it's the right thing to do. #49 reflects latest input from Security Team.
Comment #57
mondrakeRe-rolled
Comment #58
mondrakeCopied the GDToolkit + test changes to #2477381-4: GDToolkit::getSupportedExtensions returns incomplete list.
Comment #59
andypostComment #61
mondrakeComment #62
mondrakeRerolled.
Comment #63
Saphyel CreditAttribution: Saphyel as a volunteer commentedthe patch applies well, but when you try to upload a svg file you still see:
And is still showing in the field even after edit the field:
Allowed types: png gif jpg jpeg.
Comment #64
Saphyel CreditAttribution: Saphyel as a volunteer commentedComment #65
mondrake@Saphyel please see #28 and #43, but also be aware about security concerns about allowing upload of SVG files (here in #41 and #49, but more recently in #2718305: Mitigation measures for ImageTragick (CVE-2016–3714/3715/3716/3717/3718)).
Comment #66
Saphyel CreditAttribution: Saphyel as a volunteer commented@mondrake anyway I added other formats like tiff and others random texts and is still only allowing the same 4 formats...
But I'm more concern about SVG to be honest, I'm gonna read all your links, thanks!
Comment #67
mondrakethat's because the default GD toolkit supports only those formats, at least at the moment. it does not matter what the PHP GD library supports.
Comment #69
mondrakeJust a reroll of #62 after commit of #2477381: GDToolkit::getSupportedExtensions returns incomplete list.
Comment #70
Gábor HojtsyRemove duplicate tag.
Comment #71
claudiu.cristeaComment #72
alexpottThis issue still needs a security review according to the comments and issue tags.
Comment #74
robpowellCan someone summarize what the patch does and how to test it? Currently, on simplytest.me, it does not accept .svg - error "The specified file blog.svg could not be uploaded. Only files with the following extensions are allowed: png gif jpg jpeg." Is this expected?
Also, does this patch contend at all with modules/imagemagick?
Comment #75
joelpittetSVG is xml, it can't be acted upon in the same way other images can be (crop, scale, etc) which would be what the image toolkits would support. So yes that is expected.edit: Apparently there is some support for this in imagemagik... sorry for the noise.What do you mean by "contend"?
Comment #76
robpowellI was just looking to see how I could help test if the patch is doing what is supposed to. If you could help me by clarifying what is the patch supposed to do, that would be great :). I was hoping that using this patch with imagemagick would allow me to upload an svg. I meant contention, would enabling imagemagik cause any negative effect to the results of this patch.
Comment #77
joelpittet@robpowell, just a guess but you'd need imagemagick to test out with SVG because GD doesn't support SVG and I'd expect that would be available in the https://www.drupal.org/project/imagemagick module.
So this patch + imagemagick + core 8.3.x should provide that test, wanna give that a try?
Comment #78
mondrakesee comments #28 and #43
Comment #79
joel_guesclin CreditAttribution: joel_guesclin as a volunteer commentedI don't know if this helps or confuses the issue, but I wonder whether it is actually appropriate to use the same field type for matrix and vector images (ie png, jpg, etc on one side, and svg on the other).
I say this for three reasons:
First, a lot of the things you can do with matrix graphics in ImageMagick (resizing them, scaling them, etc) are meaningless for vector graphics (all you do is specify the size)
Second, vector graphics are generally not used for the same things: vector graphics are great for icons, but a no-no for photos for example.
Third, one of the cool things you can do with SVG is actually to include the content directly in the HTML using the
<svg>
tag, which then allows you to do things like change the colour directly in your CSS - and this means not using the<img>
tag at all. This seems to me so different from what is done with the Image type field that I wonder if it should not be handled by something completely differentJust my penny's worth...
Comment #80
mondrakeI feel the discussion here is getting too much biased on "SVG yes" vs "SVG no".
There are other image formats - WEBP, TIFF for example - that could be uploaded in an Image field if a toolkit supports them, and that cannot be now given the hardcoded restriction that this patch removes.
For example, even if #2340699: Let GDToolkit support WEBP image format was solved, it still would not be possible to upload WEBP images in an Image field with the default GD toolkit, because of the hardcoded
$supported_extensions = array('png', 'gif', 'jpg', 'jpeg');
Comment #81
joel_guesclin CreditAttribution: joel_guesclin as a volunteer commented@mondrake - do you think that support for PDF belongs in the Image field type or is this something that should be handled by File Entity?
I don't know anything about TIFF, but my point was more that vector graphics are a very different kettle of fish to matrix graphics - but I confess my knowledge of both is rather sketchy and I will happily bow to the experts on this.
The one thing about SVG that is really different is the ability to refer to it in the CSS files, so for example the same icon can have different colors depending on where in the site it's used, just by changing the CSS
Comment #82
Lukas von BlarerAs mondrake stated, this is rather about supporting any format that the image toolkit can handle. TIFF, PDF, WEBP and SVG being candidates for that. And it is very important that we get support for this.
The security concerns SVG introduces are valid, but currently I create file fields and upload SVGs through that and render them as well. I don't understand how this different from simply supporting SVG in an image field. Could we strip out stuff like
<script>
tags if we render an SVG inline or through an image style? How big are the security issues if SVGs are rendered using a<img>
tag? As far as I am aware this is not a security issue. jwilson3 came to the same conclusion in #55.#79: SVGs are images and they should be supported by image field. Icons are definitely not the only use case for SVGs. An example: I recently created a website for an architecture firm. The want to display plans for their buildings. the old ones are JPEGs or PNGs and the new ones are SVGs. There is no point in creating two different fields for this.
The rendering of the SVGs is a more interesting topic. What I did was using a preprocessor which either renders a responsive image if it is a bitmap or a
<img>
tag if it is a SVG. Should we solve this in the field formatter or in the image style? and how does this affect responsive images? This is going to be difficult to solve... In most cases I would like image styles not to apply anything to SVGs. But for example converting them to a different file format is also a very valid use case which would need an image style to be applied. Any thoughts on this?Comment #83
fietserwinRE #82: If you want to solve this in an image style you probably will need an effect like the aspect switcher effect (D7, currently being ported to D8) that decided what to do based on aspect ratio (landscape or portrait). this new effect would be a "format switcher"or something like that. Not unfeasible, but not currently existing either.
If you want to solve it with an image formatter, you can intercept before the target image (the variable that will be used to render the src attribute) is changed to that of a derivative image url. Your formatter should extend the current image formatter and decide what to do based on format, e.g. vector graphics: just render the image itself, otherwise pass it on to the standard image formatter that can apply an image style for bitmapped images.
Comment #84
Lukas von BlarerHaving to decide what the image style should do based on the format make them too complex. This could live in contrib. But having a default behavior that solves this in the image formatter sound like a reasonable approach.
Comment #85
fietserwinRegarding the needs security review tag.
A quick search on the internet reveals a lot of possible problems with the svg format. WP also doesn't support it out of the box. Using the svg described in https://bjornjohansen.no/svg-in-wordpress, I tested:
FF 49:
Chrome 53:
So, apparently, no direct XSS attacks possible when we restrict ourselves to img tags, but other risks, see e.g. http://security.stackexchange.com/questions/11384/exploits-or-other-secu..., might still be possible, even with img tags.
However, once an image is on the server, it can be referenced via other constructs as well, thus saying that because this is an image field and thus rendering it is restricted to img tags cannot be assured. Example: anonymous people that may upload images and write comments with site internal links only. IMO this means that svg fiiles should be renamed like we currently already do for some other extensions in file_save_upload() : ... preg_match('/\.(php|pl|py|cgi|asp|js)(\.|$)/i', $file->getFilename()) ...
But this should not be part of this issue as it is more of an (already existing) issue with file fields in general and not, or at least less, for image fields.
Therefore removing the "needs security review".
Comment #86
fietserwinRegarding the "Needs issue summary update" tag.
TLDR: Also check when defining an image field, not only when showing an image widget (and allow site builders to shoot themselves in the foot).
Why was this issue posted in the first place and what are we trying to "solve" currently?
The issue started with "I am defining a list of allowed extensions for an image field and without any message or warning the list that users see is a subset of it.". The curent patch tries to solve the fact that the list as defined by a site builder is reduced based on a hardcoded list instead of the list of extensions supported by the actual toolkit.
IMO we are tackling this problem in a wrong manner. I would like to propose the following functionality:
I am in favor of the warning option, not the hard fail as most error handling on the image widget side will still be necessary for when toolkit configuration has changed (more specifically the list of supported extensions).
If we go for the warning, the warning should indicate which extensions are unsupported and what the consequences are:
This proposal (assuming the warning option):
If we can get some agreement on this proposal we can update the issue summary and rework/extend the patch.
Comment #87
heddnComment #88
heddnUpdated the IS. Let's get some reviews on the direction aspects of this issue and some votes on its direction.
Comment #90
anthony.bouch CreditAttribution: anthony.bouch as a volunteer and commentedNot sure how generally helpful this will be, but having just discovered this issue (via https://www.drupal.org/node/2541480) and needing to upload over 100 inline SVGs across several posts for a particular content type, I've created a workaround module here...
https://github.com/58bits/insert_svg
The code is mainly a copy of https://www.drupal.org/project/editor_file, updated to insert an img element with its src attributed set to the uploaded SVG file instead.
After saving a post with an inserted SVG, the editor will return to the built-in image widget, where additional formatting and a caption can be added.
Another option was to use the Paragraphs (https://www.drupal.org/project/paragraphs) module with the newly released SVG file formatter (https://www.drupal.org/project/svg_formatter), and a File field in a paragraph type - but in the end we needed inline text support.
Comment #91
sahaj CreditAttribution: sahaj commented@blue_waters thanks for your solution, I will give it a try on D8. But I still would be grateful to see a fix for D7 (should I open a new issue for that?)
Comment #92
mondrakeJust a re-roll of #69 that no longer applies.
Comment #93
mondrakeAdding related #2868079: Sanitize OR add Content-Security-Policy for SVGs
Comment #94
shmel210 CreditAttribution: shmel210 commentedThis module svg_image_field creates own field type widget and formatter, supports preview and alt attribute.
I am planning to add validation of svg image with unittest.
Please take a look on it, maybe worth to add it to core or left it as is.
Comment #95
larowlanI realize this ugly stuff already exists - is there a reason we don't just use
$this->getSetting('file_extensions');
Comment #96
mondrake#95: changed to remove uglyness + reverted a change in the class use declaration list that is out of scope here.
Comment #97
Wim LeersI had to double-check this
jpe
extension I'd never heard of. Apparently this is a thing!\Drupal\system\Plugin\ImageToolkit\GDToolkit::getSupportedExtensions()
supports it.We don't need manual testing because we have automated tests here.
Comment #98
mondrakeYes, it's probably an exotic one, and there aren't many files with that extension around :) However, it is one of the extensions mapped to the 'image/jpeg' MIME type in
ExtensionMimeTypeGuesser
so for consistency we should support it, I think
Comment #99
catchWhy not change the constructor? We can initialize to NULL and have a fallback still for bc but would allow it to be injected.
What are the implications if image toolkit is changed during the lifetime of a site?
Comment #100
mondrakeRe #99:
1. Done.
2.
See #50/#51.
Comment #101
mondrakeAdding related #2867494: File and image widgets are misleading about the extensions they accept, that tackles some of the points raised in #86.
Comment #103
DuneBLHello,
After applying this patch, is it possible to insert svg with ckeditor?
Comment #104
waynekemp CreditAttribution: waynekemp commentedI'm going to post this here just in case this contributed module is useful for anyone having issues using SVG images in Drupal 8 https://www.drupal.org/project/svg_image. It may help or may not, you decide.
Comment #105
mcaden CreditAttribution: mcaden commentedI just used this patch to save a PDF into an image field, and render the first page of it as a pdf. All I had to do was have imagemagick installed, PDF format enabled, and an image style set to Convert and Scale.
This is extremely versatile and the patch worked without issues. The only improvement I'd suggest is that the file extensions be reflected on the actual upload dialog via the "accept" attribute.
That should simply require this:
$element['#accept'] = empty($extensions) ? 'image/*' : '.' . implode(',.', $extensions);
(Didn't gen a patch as I'm so out of touch with "proper" php dev since I spend 99.9999% of my time in other languages so this "works" but may not be drupal best practice syntax)
Comment #106
dannylamb CreditAttribution: dannylamb commentedI can confirm that this patch works as expected. With the Imagemagick toolkit, I am now able to upload tiffs to image fields!
Comment #107
jpoesen CreditAttribution: jpoesen commentedConfirming that #104 is a perfectly functional workaround witout the need for patching.
1. install svg_image module
2. add svg as allowed extension on any image field
3. done
Thanks, @waynekemp.
Comment #108
mcaden CreditAttribution: mcaden commentedI'm only seeing good things so far on this patch and I think all of the previous concerns have been addressed.
Comment #109
rgpublic#103: No, AFAIK this doesnt do anything for CKEditor. This patch wont magically allow SVGs to be allowed where previouly only PNG/JPG/etc. has been allowed. You need to configure SVG under allowed extensions. For CKEditor, there is no way to configure anything. So this is probably the reason why this fails. Otherwise, the patch works great. We are using it for a long time now successfuly in production. There is another issue (https://www.drupal.org/project/drupal/issues/2650260) which is concerned with the CKEditor aspect but it has (falsely) been duplicated by this one :-(
Comment #110
catchI don't think we can deprecate this because we're calling it above - so with deprecation phpunit integration it should be causing deprecation warnings, which we can't fix because we're using it. What we've been doing in other issues is a ternary in the consturctor calling \Drupal::service() if it's not set.
Otherwise looks good though.
Comment #111
mondrakeThanks @catch, here we go.
Comment #113
mondrakeapcu test failure
Comment #115
mondrakeagain
Comment #117
mcaden CreditAttribution: mcaden commentedLooks good.
Comment #118
catchCommitted/pushed to 8.6.x, thanks!
I think this is backportable to 8.5.x, so leaving it RTBC against that branch, but will give other committers time to object.
Comment #120
catchRetrospectively bumping to major due to the many duplicate issues.
Comment #122
mondrake#121 looks like a random test failure. Back to Rtbc.
Comment #124
catchCherry-picked to 8.5.x.
Comment #125
mondrakeOpened a follow-up issue, #2942160: Rationalize image widget validation error messages.
Comment #127
BramDriesenIs this being backported to D7?
Comment #128
rgpublicGreat that this is now in core. Unfortunately there seems to be a conflict with #2307451, because the accept-attribute of the image input field doesn't care about all the cool new extensions and prevents uploading e.g. MP4 files. Sigh :-(
Comment #129
rgpublicHere my hackish-code to fix this problem in case it is helpful for anyone. IMHO, the image widget module shouldnt use accept=image/* but simply enumerate all the extensions that are available right there in "file_validation_extensions" anyway. For example, currently an ordinary image field will also offer to upload BMP etc. even though there is no support whatsoever.
Comment #130
mondrake@rgpublic may I suggest you open a new issue for #128 / #129, and link to this issue, so folks can take that forward. This issue is closed fixed now and just a few people would check it back.
Comment #131
q0rban CreditAttribution: q0rban at Lullabot commented@BramDriesen, there is #2539478: Allow image fields to use any extensions the current image toolkit supports (instead of hard-coding jpg, png and gif only) for D7.
Comment #132
marcoscanoOpened #3011384: ImageItem::generateSampleValue() and Random::image() should allow all extensions the current toolkit supports (instead of hardcoding a fixed set) as a follow-up.
Comment #133
rimu CreditAttribution: rimu commentedIf this patch never makes it into core (haha, cry cry), you might be able to just use hook_form_alter() to add extra extensions to the fields you want to enhance. e.g. if your field is called 'field_image_preview' and you want to add tiff psd tif eps ai file extensions, do this:
This will get your image uploaded into the field but you will need to use ImageMagick rather than GD for your image processing toolkit (set at admin/config/media/image-toolkit) and use an Image Style that converts to jpg/png to display the image in a browser.
Comment #134
jjwfcd CreditAttribution: jjwfcd commentedshould I reopen this issue since I want to upload svg file even I installed the svg_image module https://www.drupal.org/project/svg_image
and set image filed to support svg
I cannot upload svg files
Comment #135
AnybodyI can confirm exactly the same issue as @jjwfcd in #134. Could you please reopen this?
Did you find any solution, @jjwfcd?
Comment #136
rgpublic@Anybody, @jjwfcd. You don't need a special module. But you need:
1) ImageMagick module installed and enabled as active image toolkit
2) Configure SVG and MVG under Image file formats both as enabled with mime_type: image/svg+xml (Yes, you really need to enable MVG as well even if you only want to upload SVGs)
3) Configure the "thumbnail" image style in such a way that it includes as first effect a conversion to JPEG
4) Your installed ImageMagick version should be able to convert from SVG to JPEG (try it on the command line)
5) Make sure the SVGs you want to upload contain the XML header "<?xml version="1.0" standalone="no"?>" as the first line. If they don't they usually work in other apps or the browser but ImageMagick won't accept/detect them.
Remember as a rule of thumb: You can only successfully upload SVGs if Drupal is successfully able to create a thumbnail from them. We're using SVGs daily in our workflow, but the configuration is in fact quite tricky.
Please also make very sure that only a limited circle of people can upload SVGs, because otherwise you open a huge security hole. SVGs can contain JavaScript etc. If someone can uploaded such a SVG and lure you (e.g. logged in as a Drupal admin) into opening such a URL (you could also be redirected from something innocent-looking like https://go.to/yadayada), then the running script can probably steal your cookies, session and do other evil things you certainly wouldnt want.
Also be aware: All this only works for SVGs uploaded to entity image fields. This won't work for inserting SVG images into a CKEditor text field. This is even more difficult, basically currently not easily possible AFAIK and out of scope for this bug anyway :-)
Comment #137
caspervoogt CreditAttribution: caspervoogt commented@rgpublic Thank you for your detailed rundown. I apologize if I might be going slightly off-topic here, but I agree it appears to be just about impossible to enable SVG insertion in a CKEditor text field. In D7 using Media I was able to insert SVGs and set their dimensions with pixels or percentages. It's a bit tougher in D8.
I have found a workaround of sorts using the Media module alongside File Entity Browser. I used that to define an Entity Browser tailored to images (explicitly allowing SVG). I am using Entity Embed to define an entity embed button that uses the entity browser I set up. With this I am able to insert SVGs into a CKEditor text field.
However, the resulting image dialog only allows me to set image dimensions in pixels. I opened an issue for that here. For the time being I added some CSS to my theme to force any SVGs to display at 100% width; this way I can override it using those pixel values. Not ideal, but better than not having SVGs.
Comment #138
Oscaner CreditAttribution: Oscaner at CI&T commentedBased on this issue, i changed some code to allow SVG upload.
If some guys had better idea, please help to optimize.
Comment #139
Oscaner CreditAttribution: Oscaner at CI&T commentedOptimized #138 patch.
Comment #140
venugopp CreditAttribution: venugopp as a volunteer and at Moonraft Innovation Labs commentedHere is latest patch as of Drupal 8.9.12
Comment #141
TomiMikola CreditAttribution: TomiMikola at Wunder commentedRerolled the patch for version 9.2.8.
Comment #142
subhashuyadav CreditAttribution: subhashuyadav at Specbee commentedRerolled the patch for version 10.2.0