Problem

Page: admin/appearance/settings
Action: I want to set an svg image file as the logo

Situation 1:
- Place the file logo.svg myself on the root of the web site.
- Uncheck "Use the default logo supplied by the theme".
- Fill in logo.svg in "Path to custom logo".
- Click "Save configuration".

result: OK, logo changed and displays as expected.

Situation 2:
- Uncheck "Use the default logo supplied by the theme".
- Browse to the logo.svg on my local computer and select it in "Upload logo image".
- Click "Save configuration".

result: file refused wioth error messages:

    The specified file logo.svg could not be uploaded.
        - Image type not supported. Allowed types: png jpeg gif
        - Only files with the following extensions are allowed: jpg jpeg gif png txt doc xls pdf ppt pps odt ods odp.
    The logo could not be uploaded.

What is wrong:
- The 1st error message comes from the active toolkit in the Image system, but as the logo will not be manipulated by the toolkit, it should stay out of this setting.
- The 2nd error is even stranger as the logo file now suddenly seems to be treated as a regular file upload.

Proposed resolution

  • There should only be a check if the logo is a regular/known image format.
  • Both ways of defining the logo should react the same.
  • 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

User interface changes

Help text change.

API changes

None.

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Instructions
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

Comments

sidharthap’s picture

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

fietserwin’s picture

You are right, but the point here is that a logo is not manipulated in any way by Drupal. So any restrictions due to the active toolkit, and the formats that it (does not) support, should not be applied to the logo, nor should any format restriction from somewhere in the file module be applied.

sidharthap’s picture

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.

itapplication’s picture

Issue tags: +theme logo, +logo file format

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

ckrina’s picture

akalata’s picture

Issue tags: -theme logo, -logo file format

Removing unnecessary tags (per [#1023102]).

joelpittet’s picture

Issue tags: +svg, +Novice, +frontend

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

joelpittet’s picture

Title: Unnecessary restrictions on logo format » Unnecessary restrictions on logo format: Can't upload replacement SVG logo
Issue tags: +mobile

Updating title.

joelpittet’s picture

Priority: Normal » Major

Discussed with @webchick and bumping to major.

mark.labrecque’s picture

Assigned: Unassigned » mark.labrecque
jmarkel’s picture

Issue summary: View changes
mark.labrecque’s picture

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

nvahalik’s picture

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

mark.labrecque’s picture

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

mark.labrecque’s picture

Assigned: mark.labrecque » Unassigned
nvahalik’s picture

Status: Active » Needs review
FileSize
719 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,962 pass(es). View

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

mark.labrecque’s picture

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.

mark.labrecque’s picture

Status: Needs review » Reviewed & tested by the community
Manjit.Singh’s picture

FileSize
54.28 KB

manually test patch .. working fine.. Attaching screenshot

fietserwin’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for picking this up.

Regarding the patch:

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

- According to http://en.wikipedia.org/wiki/Comparison_of_web_browsers#Image_format_sup... we'd better support bmp instead of apng...
- I miss webp, not widely supported yet, but it seems to be coming (during the lifetime of D8).
- This was the only (non-test) use of function file_validate_is_image(), so should we deprecate or even just remove this function?

Regarding how the patch compares to the proposed solution:

This patch solves the 1st point, but does not solve the other 2 points. Dropping the 2nd point is OK for me, but the 3rd point, the help text, remains obscure to me: Examples: logo.svg (for a file in the public file system), public://logo.svg, or core/themes/bartik/logo.svg.. You have to read and interpret very carefully to realize that logo.svg may also point to the root of the Drupal installation (and, absolutely not clear or even deductible, in its current implementation gets precedence over the public:// directory).

So, we should either change the scope of the patch (include point 3) or change the scope of this issue by creating a follow up issue (not my preference as changing help texts used to be a no-go during the lifetime of a major release).

Manjit.Singh’s picture

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

nvahalik’s picture

Status: Needs work » Needs review
FileSize
1.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,317 pass(es), 10 fail(s), and 5 exception(s). View

@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.

Status: Needs review » Needs work

The last submitted patch, 22: unnecessary-2259567-22.patch, failed testing.

nvahalik’s picture

Status: Needs work » Needs review
FileSize
2.29 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch unnecessary-2259567-24.patch. Unable to apply patch. See the log in the details link for more information. View

Updated the test.

lauriii’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

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

lauriii’s picture

Issue tags: +Needs manual testing
gyuhyon’s picture

uploading SVG files to logo and favicon is working

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

gyuhyon’s picture

Issue tags: -Needs manual testing

no manual test needed

fietserwin’s picture

- bmp was not added to the list.
- I'm not sure about the need of testing each and every format. Just testing 1 accepted format and 1 not-accepted format seems enough coverage to me.
- The help text is still wrong: when you enter logo.svg, both /logo.svg and public://logo.svg are tried (in that order) and accepted if found. Nice feature but extremely difficult to describe correctly :(
- It was not that logo and icon behaved differently, but that uploading versus manually entering a file on the server behave differently. But I propose to leave it as is,so admin/editors/themers can circumvent the "arbitrary" limitation to certain image formats. If we decide to leave as is, the issue summary should be updated by removing that point.

nvahalik’s picture

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.

fietserwin’s picture

Or something like:

Enter a path to an image. Drupal will look for the file relative to the Drupal root or the public file system. Explicitly using public:// before the path will only look in the public file system ({actual value of public}).

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
2.44 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,349 pass(es), 10 fail(s), and 0 exception(s). View

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

oriol_e9g’s picture

Priority: Major » Normal

This is not major.

Status: Needs review » Needs work

The last submitted patch, 32: logo_format-2259567-32.patch, failed testing.

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
2.42 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,368 pass(es), 10 fail(s), and 0 exception(s). View
oriol_e9g’s picture

Status: Needs review » Needs work

The last submitted patch, 35: logo_formats-2259567-34.patch, failed testing.

andypost’s picture

Issue tags: +dcuacs2015

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

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

nelp’s picture

Looking at this issue her at DrupalCon Barcelone

nelp’s picture

Status: Needs work » Needs review
Issue tags: +Barcelona2015
FileSize
2.43 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,958 pass(es), 10 fail(s), and 0 exception(s). View

Rerolled patch from #34. The patch needs review.

Status: Needs review » Needs work

The last submitted patch, 40: logo_formats-2259567-40.patch, failed testing.

andypost’s picture

Issue tags: +Needs manual testing

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

valthebald’s picture

@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)

nelp’s picture

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

The last submitted patch, 24: unnecessary-2259567-24.patch, failed testing.

DuaelFr’s picture

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.

fabio84’s picture

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

FMB’s picture

  • 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.
fabio84’s picture

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

fabio84’s picture

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?

nelp’s picture

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?

fabio84’s picture

I got the same 5 errors on FireFox

The last submitted patch, 40: logo_formats-2259567-40.patch, failed testing.

Lendude’s picture

Lendude’s picture

Status: Needs work » Needs review

Bit of a clean-up

Status: Needs review » Needs work

The last submitted patch, 56: logo_formats-2259567-56.patch, failed testing.

Lendude’s picture

Status: Needs work » Needs review
FileSize
798 bytes
3.41 KB

duh, this should be better...

mgifford’s picture

Issue summary: View changes
FileSize
441.59 KB

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.

Screenshot with uploaded SVG image.

joelpittet’s picture

Issue tags: -Needs manual testing

@Lendude does the tests test for SVG explicitly?

Lendude’s picture

@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.

joelpittet’s picture

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)

Lendude’s picture

@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

joelpittet’s picture

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

Just guessing

Lendude’s picture

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.

joelpittet’s picture

+++ 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)]];
Truptti’s picture

FileSize
371.23 KB
405.13 KB
408.08 KB

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.

LOBsTerr’s picture

Assigned: Unassigned » LOBsTerr
LOBsTerr’s picture

Assigned: LOBsTerr » Unassigned
FileSize
3.93 KB

I have added the changes from comment 67.

Lendude’s picture

Status: Needs review » Needs work
+++ 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.

lauriii’s picture

+++ 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?

Lendude’s picture

@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).

mondrake’s picture

#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.

andypost’s picture

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

andypost’s picture

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

valthebald’s picture

Issue tags: -Novice

Doesn't look Novice anymore

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

Version: 8.1.x-dev » 8.2.x-dev
drugan’s picture

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.

fietserwin’s picture

IMO, the site logo is not an image to be processed with the toolkit, so any restrictions coming forth of the active toolkit are out of place.

joelpittet’s picture

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.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

yoroy’s picture

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?

DuneBL’s picture

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).
holyfire’s picture

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.