Support SVG files for theme logo setting

Drupal supports GIF, JPG and PNG as Drupal 8 used GD2 image manipulation toolkit and GD2 does not supports SVG formats. May be in future Drupal will used ImageMagick.
Follow this links http://www.sitepoint.com/imagick-vs-gd/ for difference between imagick vs gd.
Here is a related issue reported https://drupal.org/node/238678.

Thanks
Sidhartha

There is no restriction from file module. Even we pass the extensions to validate, it will get fail at the toolkit label while manipulating the image.

I experience unnecessary restrictions on logo format for .svg only. I place logo.png in theme folder but {{logo}} print only logo.svg. Why?

Removing unnecessary tags (per [#1023102]).

Tagging for sprint tomorrow and cross linking it as I ran across this while converting logo to be in a block.

Updating title.

Discussed with @webchick and bumping to major.

Thanks jmarkel! I will be working on this at the DrupalCon sprint tomorrow and will take these suggestions into account

I will be working on this at the DrupalConLA mentored sprint.

I have already begun work on this nvahalik. Would you like to work together on it?

The logo file upload form validator now uses the same file validator as the favicon (minus the .ico extension).

I reviewed this and verified that the patch allowed an SVG file to be uploaded to global theme settings. I will wait on testbot and then RTBC.

manually test patch .. working fine.. Attaching screenshot

Adding bmp extension would not be a bad idea.. If its possible !! lets wait for some reviews on #20.

@fietserwin,

I've added the bmp extension since it was valid and wouldn't be anymore.

Regarding #2, both the logo and the favicon work exactly the same now, so I'm inclined to just leave it be.

#3 - updated in the patch.

Examples: core/themes/seven/logo.svg (for a file in the document root) or public://logo.svg for a file in the public files directory.

Updated the test.

I think we have to add tests for this cause this is considered as bug.

uploading SVG files to logo and favicon is working

but uploading BMP file to logo nor favicon is not working as below screenshots.

no manual test needed

Sorry about the BMP not being added to the list. That was my mistake! I'll upload another patch after we get the message straightened out because it's super tedious trying to fix the tests for every message change.

Here are some further options for the description text:

Examples: test.png will look for /logo.png and sites/drupal.dd/files/test.png. Using public://test.png will use an image located at sites/drupal.dd/files/test.png.

Enter a path to an image. Entering test.png will look for /logo.png and sites/drupal.dd/files/test.png. Using public:// before a path (e.g. public://test.png) will use explicitly use an image that has been uploaded to Drupal's public file directory such as sites/drupal.dd/files/test.png.

Maybe it's a little verbose but I'll try with this.

This is not major.

You should change a render (and CSS) to properly display SVG

http://stackoverflow.com/questions/4476526/do-i-use-img-object-or-embed-...

Looking at this issue her at DrupalCon Barcelone

Rerolled patch from #34. The patch needs review.

I'm pretty sure that uploading svg file will not display logo properly, because it's not image to render with IMG tag

+++ b/core/modules/system/src/Form/ThemeSettingsForm.php
@@ -361,7 +361,7 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
-      $validators = array('file_validate_is_image' => array());
+      $validators = array('file_validate_extensions' => array('bmp png gif jpg jpeg apng svg'));

svg is not image

@andypost: if I upload an svg file to sites/default/files thru the file system, and enter path in the theme settings, it is working. So no, SVG files are displayed as images correctly (in all supported browsers)

God point. But according to http://caniuse.com/#feat=svg-img the use of svg in a img tag is widely supportet.

I'm mentoring @nelp, @FMB and @fabio84 on that issue at DrupalCon Barcelona.
The patch has been rerolled and manually tested but automated tests are failing so they are currently working on fixin that tests.

Patch #40 works for me, we have to figure out why some tests on https://www.drupal.org/pift-ci-job/39647 fails

• So it turns out this patch introduces a bug in the tests. We are running them locally to determine what happens.
• Should we really support apng and bmp?
• I think the description is still confusing and could be improved, plus it needs a full stop.

I tested Drupal\system\Tests\System\ThemeTest and I got 15 errors with the patch and 5 errors without the patch

errors with patch:

Value 'public://' is equal to value 'sites/simpletest/763970/files/image-test.png'.	Other	ThemeTest.php	122	Drupal\system\Tests\System\ThemeTest->testThemeSettings()	Fail
Value 'sites/simpletest/763970/files/image-test.png' is equal to value 'public://image-test.png'.	Other	ThemeTest.php	123	Drupal\system\Tests\System\ThemeTest->testThemeSettings()	Fail
Value 'http://localhost:8080/drupal/sites/simpletest/763970/files/image-test.png.pagespeed.ce.HKO7tjhvkE.png' is equal to value 'http://localhost:8080/drupal/sites/simpletest/763970/files/image-test.png'.	Other	ThemeTest.php	133	Drupal\system\Tests\System\ThemeTest->testThemeSettings()	Fail
Value 'public://' is equal to value 'sites/simpletest/763970/files/image-test.png'.	Other	ThemeTest.php	122	Drupal\system\Tests\System\ThemeTest->testThemeSettings()	Fail
Value 'sites/simpletest/763970/files/image-test.png' is equal to value 'public://image-test.png'.	Other	ThemeTest.php	123	Drupal\system\Tests\System\ThemeTest->testThemeSettings()	Fail
Value 'http://localhost:8080/drupal/sites/simpletest/763970/files/image-test.png.pagespeed.ce.HKO7tjhvkE.png' is equal to value 'http://localhost:8080/drupal/sites/simpletest/763970/files/image-test.png'.	Other	ThemeTest.php	133	Drupal\system\Tests\System\ThemeTest->testThemeSettings()	Fail
Value 'public://' is equal to value 'core/themes/classy/logo.svg'.	Other	ThemeTest.php	122	Drupal\system\Tests\System\ThemeTest->testThemeSettings()	Fail
Value 'core/themes/classy/logo.svg' is equal to value 'public://logo.svg'.	Other	ThemeTest.php	123	Drupal\system\Tests\System\ThemeTest->testThemeSettings()	Fail
Value 'http://localhost:8080/drupal/sites/simpletest/763970/files/image-test.png.pagespeed.ce.HKO7tjhvkE.png' is equal to value 'http://localhost:8080/drupal/sites/simpletest/763970/files/image-test.png'.	Other	ThemeTest.php	133	Drupal\system\Tests\System\ThemeTest->testThemeSettings()	Fail
Value 'public://' is equal to value 'core/themes/classy/logo.svg'.	Other	ThemeTest.php	122	Drupal\system\Tests\System\ThemeTest->testThemeSettings()	Fail
Value 'core/themes/classy/logo.svg' is equal to value 'public://logo.svg'.	Other	ThemeTest.php	123	Drupal\system\Tests\System\ThemeTest->testThemeSettings()	Fail
Value 'http://localhost:8080/drupal/core/misc/druplicon.png.pagespeed.ce.4OO_nQPwIR.png' is equal to value 'http://localhost:8080/drupal/core/misc/druplicon.png'.	Other	ThemeTest.php	133	Drupal\system\Tests\System\ThemeTest->testThemeSettings()	Fail
Value 'public://' is equal to value 'core/themes/classy/logo.svg'.	Other	ThemeTest.php	122	Drupal\system\Tests\System\ThemeTest->testThemeSettings()	Fail
Value 'core/themes/classy/logo.svg' is equal to value 'public://logo.svg'.	Other	ThemeTest.php	123	Drupal\system\Tests\System\ThemeTest->testThemeSettings()	Fail
Value SimpleXMLElement::__set_state(array( 0 => 'http://localhost:8080/drupal/sites/simpletest/763970/files/image-test_0.png.pagespeed.ce.HKO7tjhvkE.png', )) is equal to value 'http://localhost:8080/drupal/sites/simpletest/763970/files/image-test_0.png'.	Other	ThemeTest.php	185	Drupal\system\Tests\System\ThemeTest->testThemeSettings()	Fail

errors without patch:

Value 'http://localhost:8080/drupal/sites/simpletest/352600/files/image-test.png.pagespeed.ce.HKO7tjhvkE.png' is equal to value 'http://localhost:8080/drupal/sites/simpletest/352600/files/image-test.png'.	Other	ThemeTest.php	134	Drupal\system\Tests\System\ThemeTest->testThemeSettings()	Fail
Value 'http://localhost:8080/drupal/sites/simpletest/352600/files/image-test.png.pagespeed.ce.HKO7tjhvkE.png' is equal to value 'http://localhost:8080/drupal/sites/simpletest/352600/files/image-test.png'.	Other	ThemeTest.php	134	Drupal\system\Tests\System\ThemeTest->testThemeSettings()	Fail
Value 'http://localhost:8080/drupal/sites/simpletest/352600/files/image-test.png.pagespeed.ce.HKO7tjhvkE.png' is equal to value 'http://localhost:8080/drupal/sites/simpletest/352600/files/image-test.png'.	Other	ThemeTest.php	134	Drupal\system\Tests\System\ThemeTest->testThemeSettings()	Fail
Value 'http://localhost:8080/drupal/core/misc/druplicon.png.pagespeed.ce.4OO_nQPwIR.png' is equal to value 'http://localhost:8080/drupal/core/misc/druplicon.png'.	Other	ThemeTest.php	134	Drupal\system\Tests\System\ThemeTest->testThemeSettings()	Fail
Value SimpleXMLElement::__set_state(array( 0 => 'http://localhost:8080/drupal/sites/simpletest/352600/files/image-test_0.png.pagespeed.ce.HKO7tjhvkE.png', )) is equal to value 'http://localhost:8080/drupal/sites/simpletest/352600/files/image-test_0.png'.	Other	ThemeTest.php	186	Drupal\system\Tests\System\ThemeTest->testThemeSettings()	Fail

Maybe the 5 errors I get also without patch are caused by Google Chrome and PageSpeed?

We think there is some errors in the test. We have to run the tests in a new installation. The sprint here in Barcelona is closing now, but I will continue working on the issue in next week. All input are welcome.

Regarding the problems in the description belongs in its own issue?

I got the same 5 errors on FireFox

Bit of a clean-up

Bit of a clean-up

duh, this should be better...

That seems to work just fine. Support for http://caniuse.com/#search=apng seems pretty weak.

Didn't seem like there was consensus around that. Patch seems fairly straight forward.

@Lendude does the tests test for SVG explicitly?

@joelpittet in #59, the only thing getting tested is the description string. I was only trying to get the current patch in the green. Really haven't looked into testing file uploads.

Really wish I knew what that implicit public file is all about. I'm guessing it was needed to be removed for this to work for some reason?

The only thing that looks like it would be totally needed is the validators change... that is just from reading the code (didn't test)

@joelpittet from the issue summary:

The help text on the field 'Path to custom log', "Examples: logo.png (for a file in the public filesystem), public://logo.png, or core/themes/seven/logo.png." should be rephrased as well, as a plain logo.png refers to the root of the web site, not the public file system

So to paraphrase you are are saying that implicit file path was pointing at webroot and that was just not useful?

Just guessing

I have no idea who put that in the issue summary, but that seems to be the reason the description change is in the patch. But I can totally get behind the argument that that is an unrelated change to this issue.

+++ b/core/modules/system/src/Form/ThemeSettingsForm.php
@@ -358,7 +357,7 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
-      $validators = array('file_validate_is_image' => array());
+      $validators = array('file_validate_extensions' => array('bmp png gif jpg jpeg apng svg'));

This may be the only line necessary to fix this issue. Maybe we can move the other changes to a follow-up if you agree?

And we can improve this line I think so we don't regress by hardcoding the image extensions we can aquire them the way the file_validate_is_image does:

    $image_factory = \Drupal::service('image.factory');
    $supported_extensions = $image_factory->getSupportedExtensions();
    $supported_extensions[] = 'svg';
    $validators = ['file_validate_extensions' => [implode(' ', $supported_extensions)]];

Tested the patch in #59 manually and it is working fine.
Observations
1.Gave the custom logo path in "Path to custom logo" field and clicked on Save Configuration button, success message is displayed and the logo is displayed correctly (tried with SVG and PNG image)
2.Uploaded a image in "Upload logo image" and clicked on Save Configuration button,success message is displayed and the logo is displayed as expected (tried with image with SVG and PNG format)
Attaching snapshot for reference.

I have added the changes from comment 67.

+++ b/core/modules/system/src/Form/ThemeSettingsForm.php
@@ -358,7 +357,10 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+      $supported_extensions = $image_factory->getSupportedExtensions();

Using this is a nice idea but currently only returns 'jpg', 'png' and 'gif' at the moment, so we would need to add more extensions then just 'svg'.

+++ b/core/modules/system/src/Form/ThemeSettingsForm.php
@@ -374,8 +376,6 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
-      $validators = array('file_validate_extensions' => array('ico png gif jpg jpeg apng svg'));

Making both fields use the same validators only works if we add the 'ico' extension.

I also think some text to show that there is a restriction on the type of files that you can upload would be good.

+++ b/core/modules/system/src/Form/ThemeSettingsForm.php
@@ -358,7 +357,10 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+      $supported_extensions = $image_factory->getSupportedExtensions();
+      $supported_extensions[] = 'svg';

why image factory cannot contain all these image types?

@lauriii According to comment #1 due to the use of the GD2 image manipulation toolkit and it only supporting those types. Don't know if that information is still current (but it seems to be).

#72, #73:

yes, the ImageFactory just returns the extensions supported by the toolkit set as default.

Currently the GD toolkit returns 'png gif jpeg' (but see #2477381: GDToolkit::getSupportedExtensions returns incomplete list), and it's unlikely GD will support SVG which is a vector image format any soon. ImageMagick, currently in alpha for D8, can be configured to support more image formats, including SVG.

+++ b/core/modules/system/src/Form/ThemeSettingsForm.php
@@ -358,7 +357,10 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+      $image_factory = \Drupal::service('image.factory');
+      $supported_extensions = $image_factory->getSupportedExtensions();
+      $supported_extensions[] = 'svg';

This would add a duplicate supported extension if svg is already returned by ImageFactory. You'd better merge the values instead.

In general, note that the use of Image objects kind of hints at the fact that you need to manipulate the source image (scale, resize, etc.), and the toolkit just needs to be able to process the image format. If you do not need to do this, but just upload a file and render the image from it, then you probably do not need to get to this - the list of extensions like in #59 would just do.

See also #1014816: Allow image fields to use any extensions the current image toolkit supports (instead of hard-coding jpg, png and gif only) for some background.

@mondrake good points, there's another great issue #2284577: Make image effects smarter, image toolkit operations dumber

Another idea is re-use effects for logo/favicon, would be great to have this issue for 8.1
logo is in "site branding" block so could have image style selection

Doesn't look Novice anymore

More than two years are gone after the issue opening but the bug is still there. I've tested #70 patch and it works great but the question is why it is not commited so far? As I think the reasons are following:

1. The current GD library does not handle .svg images so just allowing to upload these images for a site logo does not automatically resolve the issue of displaying them.

2. Images with .svg extension may have different forms despite looking like **the same**. To see the differences just open those images taken from different resources in a text editor. For example, some of them may be affected by CSS properties (fill, width, height, ...), some can change these properties only in the image code, despite being successfully displayed on a page. Consequently, only a theme author knows what kind .svg they will accept and how they will process them.

If we agree that this issue is not the Drupal core's one then we need a solution on a theme level without touching the core.

Create theme-settings.php file in a theme root folder If you don't have one yet and copy/paste this into the file:

/**
 * @file
 * Provides an additional config form for theme settings.
 */

use Drupal\Core\Form\FormStateInterface;

/**
 * Implements hook_form_system_theme_settings_alter().
 *
 * Form override for theme settings.
 */
function MYTHEME_form_system_theme_settings_alter(array &$form, FormStateInterface $form_state) {
  $form['#validate'][] = 'MYTHEME_form_system_theme_settings_validate';
}

function MYTHEME_form_system_theme_settings_validate(array &$form, FormStateInterface $form_state) {
  if (function_exists('file_save_upload')) {
       // Handle file uploads.
    $image_factory = \Drupal::service('image.factory');
    $supported_extensions = $image_factory->getSupportedExtensions();
    $supported_extensions[] = 'svg';
    $validators = ['file_validate_extensions' => [implode(' ', $supported_extensions)]];

    // Check for a new uploaded logo.
    $file = file_save_upload('logo_upload', $validators, FALSE, 0);
    if (isset($file)) {
      // File upload was attempted.
      if ($file) {
        // Put the temporary file in form_values so we can save it on submit.
        $form_state->setValue('logo_upload', $file);
      }
      else {
        // File upload failed.
        $form_state->setErrorByName('logo_upload', t('The logo could not be uploaded.'));
      }
    }
  }
}

Of course, you need to replace MYTHEME in the code with your theme name.

Some may ask why not declare the core's new HOOK__form_system_theme_settings_validate() and then just implement it on a theme. I think that this is redundant because some months later the core may implement ImageMagick library instead of current GD and therefore allow .svg extension, so the hook will deprecate and require to be deleted.

Thanks @LOBsTerr for the $validators definition snippet.

hi,
still the same format restriction in my 8.2.5
unable to upload a logo.svg from the public system...
in the global theme parameter, as in my pixture reloaded theme:

     Message d'erreur

    Le fichier spécifié logo.svg n'a pas pu être transféré.
        Type d'image non supporté. Types autorisés : png jpeg jpg jpe gif
        Seuls les fichiers se terminant par les extensions suivantes sont autorisés : jpg jpeg gif png txt doc xls pdf ppt pps odt ods odp.
    Le logo n'a pas pu être téléchargé sur le serveur.

beside:

 // File upload failed.
          $form_state->setErrorByName('logo_upload', $this->t('The logo could not be uploaded.'));
        }
      }

      $validators = array('file_validate_extensions' => array('ico png gif jpg jpeg apng <strong>svg</strong>'));

i had to choose a .png format instead

To try to push this in a direction I'm going to ask for product manager review, since it effects the UI.
https://www.drupal.org/governance/core#product-manager

I agree with @fietserwin and would like to remove the toolkit half/feature in favour of SVG.

So the idea is to allow uploading an SVG file for the logo and then there are no problems? I can agree with that :)

Looks like the proposed fix is to make it so that the file uploaded as the site logo can not be interfered with by the imaging toolkit. That makes sense to me. Logos are often subject to strict rules on how to display them and how they should *not* be modified. So it seems ok to me to make the logo upload not available to the toolkit.

Does that help?

Here is the result after applying this path (#70) on 8.3-dev:

patching file core/modules/system/src/Form/ThemeSettingsForm.php
Hunk #1 FAILED at 284.
Hunk #2 FAILED at 358.
Hunk #3 FAILED at 374.
3 out of 3 hunks FAILED -- saving rejects to file core/modules/system/src/Form/ThemeSettingsForm.php.rej
patching file core/modules/system/src/Tests/System/ThemeTest.php
Hunk #1 succeeded at 98 with fuzz 2 (offset -5 lines).
Hunk #2 succeeded at 111 (offset -5 lines).

This is an issue that desperately looks like it's in need of leadership.

• Why hasn't this issue been assigned to anyone to manage?
• I really appreciate the theme level workaround in #81.
• Why hasn't Product stepped in here... and/or any other input?

I'm definitely willing to help with testing on this one.

There might be security issues with SVG files, but then again, the website owner should be able to use SVG files for site logos.

Bumping to 8.5.x.

Hi here,

Please ignore this file (I haven't read the whole thread), I just need a raw patch which adds svg logo upload support.

I'm looking into getting this going again, because it is rather ridiculous Drupal does not accept SVG logo uploads in 2018. To save others maybe some detective work, the main reason #70 is not applying anymore (the last patch that tries to address all issues from the summary) is actually two issues; #2954884: Cannot save theme settings form for themes without logo or favicon features and #2482783: File upload errors not set or shown correctly. Actually, while tracking down the changes breaking the patch, it actually looks like uploading SVGs may have worked at some point, because SVG was part of the allowed file extensions. However, at some point the validation was switched to file_validate_is_image, after which I expect it broke again.

I'll try and get a patch going again.

This is effectively a reroll of #70. I have not touched tests for now, and I've had to move around some stuff due to mainly #2482783: File upload errors not set or shown correctly. Note that for this patch to apply to 8.5.3 you will also need the patch for #2954884: Cannot save theme settings form for themes without logo or favicon features.

Considering it looks like SVG support was actually briefly available somewhere between Drupal 8.0 and 8.5, I think it is a good idea to try and test for that.

Here's a patch that adds an svg upload test. This is expected to fail as it does not contain the actual fix (which I will upload shortly). This does not include the changes to the test to cater for the change in error message, that didn't seem "fair" :)

Here's some updates to the patch, including the above test. I changed the determining of the supported file extensions back to hardcoded, because I don't think it makes a ton of sense to involve supported file formats for the image toolkit, as the logo is not passed through it.

Ported the patch from #98 to 8.7.x

core/modules/system/src/Form/ThemeSettingsForm.php
public function buildForm(array $form, FormStateInterface $form_state, $theme = '')
Line 213 use this code

 $form['logo']['settings']['logo_upload'] = [
        '#type' => 'file',
        '#title' => t('Upload logo image'),
        '#maxlength' => 40,
        '#description' => t("If you don't have direct file access to the server, use this field to upload your logo."),
        '#upload_validators' => [
         //'file_validate_is_image' => [],
	   'file_validate_extensions' => [
            'png gif jpg jpeg apng svg',
          ],
        ],
      ];

New patch

- fix broken test
- changed code lines should not use deprecated calls
- no reason to place branding block in loop cos xpath() & test using first one only

tests are extended to test image & svg

The question - to we need to test exact reworded description?

Revert deprecated constructFieldXpath cos there's no alternative for a while

The question - to we need to test exact reworded description?

We are not yet, right? I think it would make sense to test for the file types mentioned in the description. The exact wording makes less sense, but would be fine too, I guess.

The patch does not apply, so this will need a reroll.

There may by security issues by allowing this as SVGs can contain javascript so the security may need to look at this to check if it could be accepted.

Patch rerolled :-)

Solving test problems.

#111: There may by security issues by allowing this as SVGs can contain javascript so the security may need to look at this to check if it could be accepted.

According to here https://developer.mozilla.org/en-US/docs/Web/SVG/SVG_as_an_Image#Restric...

For security purposes, Gecko places some restrictions on SVG content when it's being used as an image:

It seems that SVG as an Image is safe in Firefox at least.

Meanwhile, there is a rerolled patch, so I change the status to needs review.

Relative to security concerns, see this related issue:
https://www.drupal.org/project/drupal/issues/2868079

Rerolled patch for 9.1

Re-rolled for 9.2.x

Fixed the failing tests.

I don't understand what all the fuss is about in this issue regarding the description. There's a reason why the 'description error' hasn't been addressed before: the description as it is is perfectly right.. Setting the path to 'logo.png' will not refer to a file in the site root, it refers to a file in the public filesystem. I've tested this in numerous instances just now.

Please rework the patch to address exclusively the SVG change, and remove the description changes, please.

Even if for some legacy reason the site root is also an applicable location, I don't think that documenting that behavior should be done in this issue. We should probably discuss if that is a behavior that should be kept (and documented) or removed entirely. And that kind of discussion is very off-topic from allowing a logo.svg file to be uploaded.

Absolutely agreed. I never really got into the reasoning behind that change, but it was already being discussed 5 years ago and looks like concencus was already reached in ~#66. I think this may be the worst issue lifetime vs. lines of code changed ever :)

Last MR is incomplete and won't be committed without tests.

Added back the tests that were removed between #123 and #127.

Hm. How strange... I distinctly recall working on those tests. Must have made a mistake.

The tests needed some adjusting too because of the revert of the description. Redid the failed merge and merged that back into the MR... 🤞

I think I ruined the GitLab MR when I tried to understand why the tests were failing and couldn't understand why there were so many changes.

I've rebased the MR to the latest Drupal 9.2.x branch status. That should fix the problem.

New patch for Drupal 9.2-beta3

@trackleft2, can you try updating the MR in the gitlab issue? It's easier to understand the changes between your changes in #137 and my commit in #136.

@trackleft2, can you provide an interdifff between #137 and the MR in #135, please?

Sorry about the 15:55 re-roll. I rebased on 9.3.x and got all the core changes between 9.2.x and 9.3.x as part of the MR.

The 16:03 re-roll is against 9.2.x. Which doesn't make much sense, but at least the MR is a lot saner.

Apologies for all of the commits/noise from the MRs. I couldn't figure out how to change the destination branch of the existing MR so I ended up creating a new one that is rebased on the latest 9.3.x branch (and includes the recent changes made to core/modules/system/tests/src/Functional/System/ThemeTest.php for #3220255: Convert assertions involving use of xpath on links to WebAssert).

Adding composer patch version of #144 for 9.2.x.

To summarize, the main issue surrounding this patch has to do with the way image validation is done(looks at the selected toolkit for the site, and finds file extensions that are compatible with it), and the fact that you can only use one image toolkit per site, which I've found to be limiting even without SVG.

To fix we could consider a more robust image toolkit options ecosystem.

Step 1 Make image toolkit a per image style setting since that is typically where the processing is done(Does not account for process on upload contrib modules, or settings), or per field setting, or a per file extension setting, and not a per site setting.
Step 2 Allow multiple toolkits
Step 3 Validate against all toolkits
Step 4 Allow processing of uploaded logo images with a toolkit, ha.

Or we could ask the Drupal core team: Theme logo images aren't even processed using an image toolkit, so why are we constraining extensions to a specific toolkit?

If this patch is applied to core before the next release, we could still go back and do all the things laid out in the 4 steps above, since really the only thing that is happening in the patch is a validator swap.

This patch works well considering the scope of the original issue, so RTBC from me.

I should, however, point out something I discovered while testing this. This patch does not check that an uploaded "image file" is actually an image; any arbitrary file can be uploaded as long as it has the correct file extension. This arguably could be considered a regression from core, where if you attempt to upload Text File.jpg as the site logo you will see the following error:

The specified file Text File.jpg could not be uploaded.

    The image file is invalid or the image type is not allowed. Allowed types: png, jpeg, jpg, jpe, gif, webp

I did some research to determine whether this should be considered a security issue and came across #2829048 and #2388749. The tl;dr for these is that the way Drupal core currently works is that if you upload a file with a specific extension it will serve it with the MIME type of that extension. There was a point during the development of Drupal 8.0 that a "mime guesser" was implemented, but it was reverted due to security concerns (see #41 on 2388749).

Additionally, I was able to reproduce this on a File media type by renaming a .jpg file to a .docx and Drupal allowed me to upload it, so this is not something that is limited to the theme logo field. Addressing this should be done with more generalized development in Drupal core in one of the two linked issues.

The suggestions outlined in #147 regarding handling of image toolkits and image files are also outside the scope of this issue and should be split off into new issues.

Rebased MR onto latest 9.3.x branch.

Sorry folks, the Issue Summary needs a bit of work, points below, and adding tag

1. The before and after user interface text change should be in the section 'user interface changes'.
2. Needs up to date screenshots in the IS. AFAICT the screenshots are 6 years old. They also need some text to explain which each screen shot is.

Uploading screenshots for use in the IS.

The issue summary has been updated per @quietone's suggestions.

One thing I noticed is that some file extensions, jpe and webp are now no longer allowed. Should we be including those file types or is that just a list of file formats the image toolkit supports (rather than necessarily a Drupal core-supported list)? The last mention of "webp" in this issue was 7 years ago in #20:

- I miss webp, not widely supported yet, but it seems to be coming (during the lifetime of D8).

Seriously I cannot understand how this issue remains... seriously there were comments on this EIGHT YEARS AGO.

Using an .svg logo OOTB might be something the core team really needs to think about. It's so entirely frustrating as a site builder when you run into stuff like this. User experience anyone?

@holyfire The issue needs review based on #151's suggestions taken by @caesius.

@holyfire: "You cannot understand" is at least an admission or confession of you where others can chime in to explain to you how community momentum and initiatives work and how collective efforts and priorities decide about all task levels what happens in which time. While I kind of see what you want to say, and trust me, even the most hard working core contributors and project leaders here often say "holy... is this issue old" but! I sadly have to turn it back to you: Because in a community it isn't "the others". It is always "you and all others together".

If it was important enough for you and maybe others of you around you, you may should take a careful look at the own history of your efforts in this issue. No complaints. Just a friendly reminder how things work in an Open Source community.

At least you can find really easy work arounds in this issue how to upload an SVG and use it in a theme. So this issue is no deal-breaker at all. Just a 'lil hiccup in the theme settings UI.

Uploading the current state of https://git.drupalcode.org/project/drupal/-/merge_requests/1153.diff in order to use it in composer-patches.

@diqidoq - thank you for the comment, love to talk through this with you, feel free to PM me when you have time.

+1 can't believe this is still an issue...if we show default file as logo.svg, why we don't allow svg upload OOB.

@jacov See comments 157, 158 and 159.

I have successfully applied patch #160 for drupal 9.4.x version.Adding screenshots for the reference.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Updated default theme path in tests, attached patch for same. please review

Leaving the issue summary tag for possible cleanup.

This seems close but think the tests should be expanded to test all the extensions being added.

Also a change record should be written for it.

There's a long-standing concern about allowing SVG uploads to Drupal sites due to the security problems inherent in the format. At the same time, site owners want full control over their sites and should be able to do things that could potentially be harmful to their sites because that's their decision.

There are 3rd party libraries that can filter SVG content to protect a site, but I don't see that being used here. Therefore, this would allow someone with the "administer themes" permission to upload unfiltered SVG files to the site, which would then be viewed by all visitors.

One method of absolving core developers of having to somehow protect every site from every possible security vulnerability, while also allowing individual site owners to do what they want with their own site, has been the use of "restricted access" permissions to protect certain site functionality. For example the permission for controlling field configuration is a "restricted" permission, because you could potentially open a site up to security vulnerabilities based upon how you build fields on an entity type.

The concern with this issue is that (IIRC) this would be the first part of Drupal core that officially allowed uploading SVG files to the site. However, no security tools are being added to protect the site from an unscrupulous user who discovered they'd been given the "administer themes" permission. Furthermore, the "administer themes" permission is not a "restricted" one, therefore some level of protection is warranted.

Therefore, from a security standpoint IMHO this needs further work - either an SVG security tool be added to core and properly implemented, or the "administer themes" permission be changed to a "restricted access" one.

@DemianMcKenna, There are definitely easy workarounds without a patch where you can upload SVG to a server via a file upload, and use via a content block.

This feature request is really just a nice to have, so site owners can use the thing they want where they expect it to be used.

We can't be the SVG police in the same way we can't be the text formatter police. The best thing we can do is to inform users that they should be concerned about arbitrarily uploading files from untrusted sources (This isn't SVG specific advice).

It was mentioned way back in #147 that we should consider rearchitecting how image toolkits are handled, but that did not seem to go over very well.

We can't be the SVG police in the same way we can't be the text formatter police.

But by default Drupal does police HTML content that is rendered on the page via content inserted through text fields. Therefore, we also need to either a) provide filtering for SVG files, or b) change the permission associated with theme administration to one that puts the responsibility squarely on the head of the site owner/builder - that's what the "restricted access" permission flag indicates.

This builds on #168 and changes the "administer themes" permission to be a "restricted access" permission, which IMHO would fulfill the security requirements.

Removing issue summary update which appeared to have been done.

Can a simple change record be written that announce svgs are now allowed?

@DamienMcKenna can we remove the Needs security review tag?

With my "security team" hat on I agree with DamienMcKenna's prior comments that if svgs are allowed to be uploaded then we should mark that permission as "restrict access".

Is this tradeoff worth it to allow uploading of svgs in exchange for marking "administer themes" as a restricted permission?

An option to not have to add "restrict access: true" for the permission would be to filter the SVG similarly to how text formats filter text - an option for that seems to be https://www.drupal.org/project/svg_sanitizer though if we go that route we should try to also do it for core file upload fields.

Removing the "Needs security review" tag since I think Damien and I have provided that perspective, but it might need ongoing review so feel free to add back and ping if needed.

Those tradeoffs and options seem like questions for a framework or product manager.

+++ b/core/modules/system/src/Form/ThemeSettingsForm.php
@@ -236,7 +236,9 @@ public function buildForm(array $form, FormStateInterface $form_state, $theme =
-          'file_validate_is_image' => [],
+          'file_validate_extensions' => [
+            'png gif jpg jpeg apng svg',
+          ],

This will need updating for https://www.drupal.org/node/3363700

Making title more descriptive and changing category. IMO this seems like a feature, to add support for more formats.

Re-rolled #173 and made change suggested in #177.

With patches please include an interdiff. Also if possible MRs are recommended now too.

But moving to NW for the change record.

The patch #179 works fine with latest Drupal 10.2.2 with PHP 8.2

So to clarify, this just needs a change record and the patch in #179 turned into a merge request to move it forward?

Hid some stale files and stale merge requests that were against 9.2 and 9.3.

I don't think that's quite it, but that would be helpful.

I updated the issue summary to remove an outdated remaining task and add some option items from comment 176.

MR Added created from patch in comment 179. Draft change record created.

Hiding the patch as the work is in the MR now.

Unfortunately think the test needs to be tweaked some as it seems to be passing without the update. See https://git.drupalcode.org/issue/drupal-2259567/-/jobs/1161166

Testing manually though when I upload a svg it doesn't appear to render.

Was on standard profile using olivero as frontend theme.

Removing tag for CR.

Edit:

Seems my svg was corrupt. Using a 2nd I am able to upload correctly.

Ah found out the problem. The svg wasn’t saving but that wasn’t being checked if the form threw any errors. And when it went to check the logo_path it had the old image png value which was valid because the form never saved.

I added an assertion for the verification message the form saved

But also cleared the logo at the end of the loop so new value can be added and doesn't fall back to the previous image in the loop.

We now get a test only failure https://git.drupalcode.org/issue/drupal-2259567/-/jobs/1163711

Didn't make any change to the solution so think I'm fine with marking

Back to needs review for that question. I feel like allowing someone to administer themes allows them to deface your site, apart from their ability to upload malicious SVGs, so it probably make sense to add it?

Responded

Trying to sort out issue credit. I've tried to credit everyone who has moved this issue on over years with patches, helpful comments. mentoring at cons, working on it at cons. If I've made a mistake please let me know.

Drupal does provide ample footguns especially behind the restrict access type permissions. I think that this functionality is expected out-of-the-box and therefore we should support it.

Committed and pushed 537f11dcb5 to 11.x and 340845698d to 10.3.x. Thanks!

I'm a little bit amused that we've supported svg upload in favicon since #864938: Can't upload .ico favicon... TIL.