Change default logo filetype to .svg and add an SVG version of Druplicon

+++ b/core/themes/bartik/logo.svg
@@ -0,0 +1,112 @@
+<!-- Generator: Adobe Illustrator 11.0, SVG Export Plug-In . SVG Version: 6.0.0 Build 78)  -->
+<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.0//EN"    "http://www.w3.org/TR/2001/REC-SVG-20010904/DTD/svg10.dtd" [
+	<!ENTITY ns_flows "http://ns.adobe.com/Flows/1.0/">
+	<!ENTITY ns_extend "http://ns.adobe.com/Extensibility/1.0/">
+	<!ENTITY ns_ai "http://ns.adobe.com/AdobeIllustrator/10.0/">
+	<!ENTITY ns_graphs "http://ns.adobe.com/Graphs/1.0/">
+	<!ENTITY ns_vars "http://ns.adobe.com/Variables/1.0/">
+	<!ENTITY ns_imrep "http://ns.adobe.com/ImageReplacement/1.0/">
+	<!ENTITY ns_sfw "http://ns.adobe.com/SaveForWeb/1.0/">
+	<!ENTITY ns_custom "http://ns.adobe.com/GenericCustomNamespace/1.0/">
+	<!ENTITY ns_adobe_xpath "http://ns.adobe.com/XPath/1.0/">
+	<!ENTITY ns_svg "http://www.w3.org/2000/svg">
+	<!ENTITY ns_xlink "http://www.w3.org/1999/xlink">

Do we need all this Adobe lines? I like the idea of adding an svg Druplicon!

Cool! Easy to see the difference when you blow up the 2 images.

Yeah! Getting all of the Adobe tags out and thereby reducing the filesize to 4kb is a really good job.

Here a screenshot from Chrome using a retina display: http://cl.ly/image/1G3F422r3N02.
And here's a screenshot when zooming in Chrome using a retina display: http://cl.ly/image/3e1R2j3C0H3r.

Maybe Lewis himself can give this another review so we can get this in!

The SVG version definitely looks better. However, here are some more issues:

1. The SVG logo is displayed slightly bigger than the regular PNG logo. That also breaks the alignment with the primary navigation.
2. The SVG logo is darker and more saturated. That might be the official specs, but it doesn't blend in as nicely with the Bartik colors.
3. In the preview of the color module in the theme settings, the SVG logo is huge.
   
4. The SVG logo doesn't work with the color module.

It turns out there's only two issues: size/alignment and using a transparent logo instead of a blue one. The attached patch addresses both issues.

Here's a screenshot with a comparison between the PNG logo and the new SVG one.

Reviewed and tested. Looks good to me.

Looks good to me as well. Let's wait for the testbot and get this RTBC!

11: svg-logo-2142653-11.patch queued for re-testing.

Not sure why the tests failed the first time, apparently everything is fine now.

11: svg-logo-2142653-11.patch queued for re-testing.

patch applies nicely, and it works fine. It's so sharp in retina screens!

Couple of minor issues
The svg logo still looks a couple of pixels bigger than original png. So still, It's not totally aligned with the Home Tab. (both in home page and preview page)
(I don't know why I can't embed it. It's a relative url)

With white background, the druplicon doesn't look very good. Maybe adding a bit of grey shadow in the middle of druplicon would make it look better?

@corbacho: Could you maybe add the *.svg you've created in the screenshot above? I don't know how to create that grey shadow in the middle of druplicon? I'd be happy to create the patch, since I'd love a retina-ready druplicon. It's on the front page when Drupal is installed, so it would be really nice to get this in!

@sqndr sorry, the screenshot at #20 were comparing the old *png* with the new svg.
But I've been working on the SVG in the weekend. I will put a patch + screenshots later today :)

Awesome! I'll happily review that one for you! ;)

Here it is.

Aligned.
See image
And I try to balance the opacity of the areas and fill colors, to make nice gradients in all range of colors. Starting from the original Druplicon SVG, I splitted the main region in 2 areas, so they don't overlap and it's easier to control the attributes of both areas independently.

Compared to the previous logo.png
You can play with it here: http://jsfiddle.net/corbacho/ZzLfx/
The SVG was optimized with SVGO tool and now it's only 3.2Kb

About using logo.png as fallback, we don't need to support IE8, but if we want support for Android 2.3, it's quite difficult to detect it. Modernizr svg feature detect seems to be the only reliable way. See Chris Coyier post, and specially this comment.

Adding to Related -> I see an issue about Drupal 8 SVG support #2286601: [policy] Drop support for browsers that don't support SVG

@corbacho: Nice! Looks like a good icon at first sight. Don't forget to change these lines, since they are not in the patch:

+++ b/core/includes/theme.inc
@@ -917,7 +917,7 @@ function theme_get_setting($setting_name, $theme = NULL) {
-          $cache[$theme]->set('logo.url', file_create_url($theme_object->getPath() . '/logo.png'));
+          $cache[$theme]->set('logo.url', file_create_url($theme_object->getPath() . '/logo.svg'));

Haha, couldn't wait so wrote that patch myself. Here we go:

So - yeah, needs another review ;)

Trying to make a side-by-side comparison, I discover a bit extra padding that increases the header size

Otherwise, I think this is ready folks. No PNG fallback needed, since we agreed on it #2286601: [policy] Drop support for browsers that don't support SVG

Looks good! As mentioned by you, no fallback is needed. Awesome!

So why is logo.png hanging around :)

+++ b/core/themes/bartik/css/style.css
@@ -354,11 +354,12 @@ ul.tips {
+  width: 57px;

Doesn't this break uploading of your own logo in the bartik theme?

Right, we're not doing *.png any more, so I removed it. Will look into the other issue (width: 57px;), see if it's needed or not.

Need some opinions here. If we don't include a width, the svg is massive, and takes up almost the entire screen. Since it's a vector, I suppose max-width: 100%; isn't working ;-)

Would it be possible to save the svg with a default width of 57px?

I agree with sqndr, there is nothing in the HTML or CSS classes to differentiate if it's the default druplicon or a custom logo. So the only way to fix it will be to hard-coded it directly on the logo.svg. SVG by default has preserveAspectRatio enabled. So no height needed.
I'm not a fan of hard-coding values, but the svg hardcoded width can be overridden via CSS, in case someone needs:

#logo img {
width: 100px;
}

Sorry I couldn't create an interdiff.. (using the interdiff tool of patchutils), it complains about malformed patch. I think it couldn't digest those PNG strange characters.
Please, test the patch thoroughly.

PS. useful guide about SVGs coordinate systems

Mhh.. Trying again. Now interdiff worked, that's a good sign.

I modified a bit the SVG colors to add more contrast. Druplicon looked too dark with the new header gradient colors committed at #2125621: Contrast between title/slogan and header is too low
Screenshot: Before #32 and After #39

Also, with different background colors: http://jsfiddle.net/GvR3X/1/

Awesome! Let's get this in so the Druplicon can shine on high resolution displays!

correcting amsterdam tag (No space)

I still think that we've not solved the upload of custom logos - automatically scaling them to 57px will be completely unexpected.

I understand the concern of alexpott. For site-point-click-builders with no CSS knowledge, maybe they want to upload a custom logo (from theme settings), and bartik shouldn't dictate the width of the logo. In D7, it's like that.

A solution: We could use a attribute CSS3 selector img[src^="core/themes/bartik/logo.svg"] to check if it's the default bartik logo ?
It should work on IE7+ http://www.quirksmode.org/css/selectors/

What do you think? Other solution is to check from PHP side if it's the default logo, then output some extra class to identify it. But sounds hacky

I think this looks good. I've tested in Android and on various Mac browsers and I can see it's a nice SVG image and everything looks fine. It's senseless to keep this open to nitpick... if there is anything to further improve, that could come in a future issue, but there are bigger fish to fry right now, aren't there?

This is a screenshot from android

I agree that the logo shouldn't be set by default at 57 px. I tried with a custom logo I uploaded on the UI, and if I didn't know how and where I can change the CSS, I couldn't use this logo.

I wanted to test the CSS3 selector method corbacho proposed in #46, but I didn't manage to make it work.

So, some remarks while testing this:

- Why has the page.html.twig in Bartik still has an img tag in it? We have a site-branding block now.
- We should add a width and height to every logo, either svg or a user-uploaded one. Thats an front-end performance issue as the browser won't start redering the image unless it fully downloaded it.
- Why isn't the logo variable in the twig files an img tag, but only its path? If we keep it like this, can we at least name it logo_path? If we make it a variable, we might be able to set the width and height in theme.inc when we use the default logo.

Anyway, the concerns from #44 are not tackled by the patch in #39.

I created the svg with a width of 57px and deleted the css that sets this svg to 57px.

New patch in attach.

Here some screenshots:

This should probably be postponed until this related issue is finalized.

Since Dries has not decided yet and due to the fact that he's more leaning towards not changing the Druplicon, we can continue working on this issue to make sure Drupal ships with a high-res Druplicon. If the discussion in the related issue changes towards changing the Druplicon, we can close this issue and create a new issue to change the Druplicon with the new version - and make sure that it's an SVG.

I minified the *.svg file using SVGO (http://cl.ly/image/3f1r1y073b3D). The rest of the patch looks good to me. Added a new patch, with the minified *.svg file.

Looks great Sander, thanks for minifying. Here's a patch that adds the svg to Classy as well (oddly, it was missing a logo).

arggh, sorry to deliver bad news.
The Preview functionality of Bartik settings, shows a broken image. It's trying to load still the png logo. See file /bartik/color/preview.html
Screenshot of Classy and Bartik here: http://monosnap.com/image/UI9pognUlsmgxAmOPneSibq0IWYUuq It looks perfect in retina.
A bit cleanup of preview functionality, and this is RTBC

Please, announce if you are taking this last task, so we don't step on each other.

Will work on it right away.

Here's a patch that fixed the logo in the Preview too. Also, I found other references to logo.png in the tests and updated these too.

By the way: good catch. Thanks for pointing that out!

Moving this to markup, as it applies to Stark, Classy and Bartik.

Thanks Baris Sander and Guter, I think we got it all right now. :)
I tested Bartik, and the preview. Saving settings with different color configurations. All good.
The logo in Stark looks good, same than screenshot of Classy (attached in #59)

Thumbs up!

Agreed with the evaluation template in the issue summary. Additionally, from the perspective of mobile support, this is really just fixing a bug. I think it's fine to fix this now, rather than block on #2057767: [policy, no patch] Revise the Druplicon logo, since it might be awhile until that is sorted.

Looks like Alex has chimed in here a few times, so assigning to him for sign-off.

So what happens in color module is still a problem. A svg gets created in sites/default/files/color/bartik-COLOR/logo.svg

But this code

  $config
    ->set('palette', $palette)
    ->set('logo', $paths['target'] . 'logo.png')
    ->save();

Saves it in config calling it a png... and...

function color_preprocess_page(&$variables) {
  $theme_key = \Drupal::theme()->getActiveTheme()->getName();

  // Override logo.
  $logo = \Drupal::config('color.theme.' . $theme_key)->get('logo');
  if ($logo && $variables['logo'] && preg_match('!' . $theme_key . '/logo.png$!', $variables['logo'])) {
    $variables['logo'] = file_create_url($logo);
  }
}

Expects logo to be a png but it's an svg and therefore by not working makes color work because the svg has no colour!!!

The function _color_render_images() looks suspiciously like it only supports png's.

The new svg logo is a big imporvement on a retina screen and I look forward to committing this patch but we've got more work to do on the color module.

@alexpott
Ouch, I see. I can replicate.

It seems Color module is copying & hard-coding logo paths to "files/color/theme-xxx/logo.png". Quite bad.

The history
----------
The current behaviour of Color module is due of historic reasons, when Garland in D6 had to generate **and fill** the transparencies of the logo (because of IE6 support).
Later on, when Bartik #683026: Add a new theme to D7 core: Bartik was introduced, it continued the same pattern, even it was not necessary. (the logo.png was not filled, just used as-it-is).

The solution
-----------
Get rid of "copying" the logo when creating a color combination. We don't need to. Bartik in D7 doesn't need that neither (but it continued the historic pattern)
When I got rid of it, also I saw that we can remove the config setting, and preprocess_page hook. No theme in Drupal 8 core need anymore a special logo-per-color-combination. And I believe that no contrib theme needs it neither IMHO. The browsers have changed.

Notice that this is different than overriding logo with a custom one. That is still working.

All Drupal 8 now is free of "logo.png" references :)

I think the solution looks ok to me. We will need a change record to outline this new behaviour.

Also looking at the code I wonder about the "copy", "base_image" and css filepath rewriting functionality. This is all completely untested. Messy. I've asked for markcarver's feedback on this as the person I know with the most expertise in the color module.

I agree with @corbacho, the whole "copying/color-fying" of logo/images is rarely used and is legacy functionality from 6.x when CSS/image techniques were... limiting to say the least; CSS3 was but just a dream.

Removing color_preprocess_page() is, however, technically an API change. I'm not entirely sure that removing something like this so late in the game is very wise, the color module is definitely "... all completely untested. Messy." There is a lot of legacy code I wanted to remove from color but was always pushed back with the fact that we were well past feature/bc breaking freezes.

We can definitely remove the copying of the logo from bartik's color.inc. However, instead of removing color_preprocess_page() entirely, we should only save the color.logo configuration in color_scheme_form_submit() if the "logo.png" file actually exists. Then in color_preprocess_page() the logo should only be replaced if it has been actually been set. Which, in the case of Bartik (and most other themes I suspect), wouldn't happen.

Removing the "Needs change record" tag as the above approach isn't actually changing anything, just fixing a bug in the color module (ie: url/path to logo replaced by color even when it doesn't exist). I'm sure there's a color issue for this exact thing somewhere, I just don't remember or have time to hunt it down :)

Thanks @Mark Carver #69 makes lots of sense.

Thanks Mark! Really good suggestion
I'm on the same page. It's a pity we can't remove some legacy code. The solution you propose is sensitive.

I changed the patch. When saving the theme settings, Color module will check if the theme is using a logo.png in ['copy']. In that case, the logo path will be pointing to the logo copied in the 'files' folder. It will override the default logo. This behavior is exactly as before.

The difference with this patch, is that if there is no logo.png in ['copy'], path won't be saved, and color module won't override the logo.

I didn't have to modify hook_preprocess_page at all.

This is a per-theme setting, so there is no concern about side-effects when user is switching themes.

I tested all possible combinations (with/without custom logo, with/without logo.png in 'copy', with default colors/custom colors) and it behaves as expected.

Awesome work @corbacho :) This is certainly moving in the right direction.

I'll admit though that I was just looking the proposed color module changes in the prior interdiff comment. However, after looking at the full patch now, I still think there definitely needs to be some more work done here.

1. +++ b/core/includes/theme.inc
   @@ -400,7 +400,7 @@ function theme_get_setting($setting_name, $theme = NULL) {
   -          $cache[$theme]->set('logo.url', file_create_url($theme_object->getPath() . '/logo.png'));
   +          $cache[$theme]->set('logo.url', file_create_url($theme_object->getPath() . '/logo.svg'));
   
   +++ b/core/modules/color/color.module
   @@ -134,7 +134,7 @@ function color_preprocess_page(&$variables) {
   -  if ($logo && $variables['logo'] && preg_match('!' . $theme_key . '/logo.png$!', $variables['logo'])) {
   +  if ($logo && $variables['logo'] && preg_match('!' . $theme_key . '/logo.svg$!', $variables['logo'])) {
   
   +++ b/core/modules/system/src/Form/ThemeSettingsForm.php
   @@ -225,7 +225,7 @@ public function buildForm(array $form, FormStateInterface $form_state, $theme =
   -    foreach (array('logo' => 'logo.png', 'favicon' => 'favicon.ico') as $type => $default) {
   +    foreach (array('logo' => 'logo.svg', 'favicon' => 'favicon.ico') as $type => $default) {
   

   Simply changing these from .png to .svg is still an API break. These changes have nothing to do with the color module and are, in fact, part of the theme system "API". Not every contrib/custom theme will have a .svg logo and this will break existing themes that have logo.png files.

   There should be logic in place to actually detect if a logo is present and which extension to use. Technically speaking, this is a feature request and as we are adding the functionality to support SVG logos in themes. We shouldn't be removing PNG logo support.

2. +++ b/core/modules/color/color.module
   @@ -451,10 +451,16 @@ function color_scheme_form_submit($form, FormStateInterface $form_state) {
   +    $config
   +      ->set('logo', $paths['target'] . 'logo.png')
   +      ->save();
   

   No need to use ->save() here, it's called below.

Thank you for the review Mark.

New patch supports themes with logo.png and logo.svg. (In case both are present, the logo.svg will be the default)

I think is appropriated to use by default "logo.svg" in the description of ThemeSettingsForm, and avoid use 'logo.png' there. See screenshot. Anyway this description is collapsed by default. And "logo.svg" is used as an example, it won't affect anything or suggests is the default value. In the moment user uploads a custom logo in that field, the description uses the custom logo filename.

What matters is the values that theme_get_setting('logo.url');, and it now supports both png and svg.

I saw in #61 we renamed colors.css to colors.svg by mistake. And I've dragged that in my patches too.
Corrected now. Please review!, let's give Drupal 8 a retina logo for Christmas.

I've manually tested the patch from #74 with a Retina MacBook. The patch looks good for Bartik. In Bartik, there is a background color and so the color of the Druplicon change accordingly. Using the color module, the Druplicon will change as well. In Stark and Classy, the Druplicon is white/grey because it's displayed on a white background.

Screenshots are attached.

let's give Drupal 8 a retina logo for Christmas.

I'm hoping we can get there as well! Nice work corbacho!

+++ b/core/includes/theme.inc
@@ -378,7 +378,11 @@ function theme_get_setting($setting_name, $theme = NULL) {
+          if (file_exists($favicon = $theme_object->getPath() . '/logo.svg')) {

I assume this is a typo, no need to set $favicon.

+++ b/core/includes/theme.inc
@@ -378,7 +378,11 @@ function theme_get_setting($setting_name, $theme = NULL) {
+          } else {

else on a new line, coding standards.

Thank you sqndr for the manual review.
And I addressed those two issues in #76 by Jeff. Well spotted! Thank you

Ignore this patch. Argh

great Lewis! that explains why I couldn't generate a clean interdiff, I had some problems.

I'm attaching a slightly different patch, see interdiff. It's exactly the same strategy than favicon file path, just some lines below.

Hopefully this is the last patch :)

tests are ok again. thanks sqndr for re-testing

1. +++ b/core/includes/theme.inc
   @@ -378,7 +378,12 @@ function theme_get_setting($setting_name, $theme = NULL) {
   +          if (file_exists($logo_svg = $theme_object->getPath() . '/logo.svg')) {
   +            $cache[$theme]->set('logo.url', file_create_url($logo_svg));
   +          }
   +          else {
   +            $cache[$theme]->set('logo.url', file_create_url($theme_object->getPath() . '/logo.png'));
   +          }
   

   This is problematic - since this will result in an unnecessary file system hit for every request using the default logo.

2. +++ b/core/modules/color/color.module
   @@ -104,7 +104,7 @@ function color_preprocess_page(&$variables) {
   -  if ($logo && $variables['logo'] && preg_match('!' . $theme_key . '/logo.png$!', $variables['logo'])) {
   +  if ($logo && $variables['logo'] && preg_match('!' . $theme_key . '/logo.(svg|png)$!', $variables['logo'])) {
   
   @@ -408,10 +408,14 @@ function color_scheme_form_submit($form, FormStateInterface $form_state) {
   +  // Save logo location only if logo.png is provided.
   +  if (isset($info['copy']) && in_array('logo.png', $info['copy'])) {
   +    $config->set('logo', $paths['target'] . 'logo.png');
   +  }
   

   These statements are contrary as there can never be a color version of the logo with an svg image (or a png actually - but that is an existing bug)... It might be simpler to remove this in a separate issue - I think it is - so I've created #2396789: Color no longer needs to manipulate logos to do this.

@alexpott Thanks for great review again
About 1. I see and I noticed too that file_exists can be expensive. But I don't see why is unnecessary. Is there any other way to know if the theme is providing a SVG or PNG logo? (without adding a new admin setting). The code is inspired on how the favicon.ico file is discovered (some lines below).

On the bright side, if someone has a great idea on keeping the functionality without "file_exists", we could get better performance for the favicon handling also.

About 2. Good bug catching :) always nice to remove dead code. I'll review tomorrow

declare them like we do other assets, i.e. in the info.yml file?

We could always move to only supporting svg as the default logo for a theme.

We could always move to only supporting svg as the default logo for a theme.

I'd rather have the ability to decide whats right for my project, rather than Drupal dictating a single file format, we already had years of that and now we have issues with it, we can solve the problem and allow any format (even WebP if I want). Also I'd really rather not have logo, screenshot and favicon littering my theme root.

So #2396789: Color no longer needs to manipulate logos does not work out. If the logo appears in a css file the path will be rewritten :(

If we want to go down the line of

I'd rather have the ability to decide whats right for my project, rather than Drupal dictating a single file format

We are going to need to implement some key in the theme's info.yml - but this is feeling very feature-ish - atm we only support png logos - it feels far less risky at this point to only support svg logos. Supporting multiple logo types will take plenty of new code - but supporting only one is string replace.

Lets do it then, I think the objections raised in #90 are sufficiently answered. We can always discuss such things in #1507896: Allow theme developers to add the default logo filename to the theme's .info.yml

I agree on supporting only svg. But I would like to reconsider #2396789: Color no longer needs to manipulate logos, if you can have a look. So we can get rid of dead code in color module. (this shouldn't block this issue though)

1. +++ b/core/includes/theme.inc
   @@ -379,7 +379,12 @@ function theme_get_setting($setting_name, $theme = NULL) {
   -          $cache[$theme]->set('logo.url', file_create_url($theme_object->getPath() . '/logo.png'));
   +          if (file_exists($logo_svg = $theme_object->getPath() . '/logo.svg')) {
   +            $cache[$theme]->set('logo.url', file_create_url($logo_svg));
   +          }
   +          else {
   +            $cache[$theme]->set('logo.url', file_create_url($theme_object->getPath() . '/logo.png'));
   

   So if we only support svg the file exists is not needed. Just replace png with svg

2. +++ b/core/modules/color/color.module
   @@ -115,7 +115,7 @@ function color_preprocess_page(&$variables) {
   -  if ($logo && $variables['logo'] && preg_match('!' . $theme_key . '/logo.png$!', $variables['logo'])) {
   +  if ($logo && $variables['logo'] && preg_match('!' . $theme_key . '/logo.(svg|png)$!', $variables['logo'])) {
   
   @@ -424,10 +424,14 @@ function color_scheme_form_submit($form, FormStateInterface $form_state) {
   -  // Save palette and logo location.
   +  // Save logo location only if logo.png is provided.
   +  if (isset($info['copy']) && in_array('logo.png', $info['copy'])) {
   +    $config->set('logo', $paths['target'] . 'logo.png');
   +  }
   +
   +  // Save palette.
      $config
        ->set('palette', $palette)
   -    ->set('logo', $paths['target'] . 'logo.png')
   

   Not needed. Just replace png with svg

3. +++ b/core/themes/bartik/color/color.inc
   @@ -101,11 +101,6 @@
   -  // Files to copy.
   -  'copy' => array(
   -    'logo.png',
   -  ),
   

   Replace .png with .svg

Looks good Lewis!, I reviewed the patch and tested manually.

A space needed after the . in .....getPath() .'/logo.svg'));
I fixed that in the patch below.. but this is RTBC ! :)

I noticed another bug, not blocking this issue. Follow-up: #2396983: Header Logo with Bartik won't change in settings preview

We need to remove the classy png logo.

Classy logo is there for reason; its being used in tests.

Edit: Sorry for messing around. There is .svg logo added already in the patch so .png only needs to be removed!

Good catch Jeff
I removed the logo.png from classy and seven. And I added logo.svg to seven.
Seven theme doesn't output the logo in the template, as you know, but it felt wrong to leave it without logo. What do you think Lewis ?

So all core themes logo.png have been replaced with the new logo.svg
I made a search in all code base for "logo.png", 0 results.
interdiff shows also changes in ThemeSettingsForm.php, only because the lines have changed. (@@ Numbers part of the diff). Nothing to worry

Updated the summary for the sprints.

We need a CR

Sorry if I am late to the party for feedback. While I understand we don't need a png fallback to support old browsers #2286601: [policy] Drop support for browsers that don't support SVG i still wonder if we could have a fallback to a logo.png for theme developers. Now they need specify this via custom theme settings, if the fallback was there it would just pick up logo.svg first or logo.png if the first doesn't exist?

Other than that the change notice looks good :)

@dasjo, if you are looking for an ie8 fallback you could do this in the template with conditional comments, I actually saw this on the toyota.com site the other day, lol.

re #113 nope contrib can't provide this. It is baked into theme_get_setting().

Agreed. This issue has major scope creep.

Considering that theme settings are ultimately cached, could we simply not introduce an alter hook (new simpler issue) instead? This would certainly allow us to provide/flush out this functionality in contrib and then we can postpone this issue to 8.1.x.

Re-titling/categorizing since this is/should be what this issue is really about. Changing the Druplicon in Bartik should really be another separate issue dependent on this one.

I edited a bit the CR to be more clear that core is not supporting a default PNG logo.
Concern of @dasjo is valid
@markcarver. The support of different logo types is open at this issue #1507896: Allow theme developers to add the default logo filename to the theme's .info.yml. Don't change the scope of this one.
As Alex said in #92, multiple logo types will take plenty of new code - but supporting only svg, is basically a string replace.

Support for logo.png can be achieved easily in contrib themes:
In themename/config/themename.settings.yml:

fallback_logo_filename: "logo.gif"

In themename.theme:

function themename_process_page(&$variables) {
  $fallback_logo_filename = theme_get_setting('fallback_logo_filename') ? String::checkPlain(theme_get_setting('fallback_logo_filename')) : 'logo.png';
  if (file_exists($fallback_logo = $variables['directory'] . '/' . $fallback_logo_filename)) {
    $variables['fallback_logo']     = file_create_url($fallback_logo);
  }
}

In page.html.yml:

<img src="{{ logo }}" alt="{{ 'Home'|t }}" onerror="this.src='{{ fallback_logo }}'; this.onerror=null;">

I don't recommend necessarily this type of fallback, it's just an example.
You could use also override the variable "logo" inside the hook_preprocess_page.

Conclusion

There are multiple alternatives, to provide a PNG default logo. It shouldn't block this looong issue IMO.

1. Ultimately, this issue is breaking backwards compatibility WAY too late in the game here. We are well past breaking things unless it's for very, very, valid reasons (of which I DO NOT think this truly qualifies).
2. Many [contrib] themes do not provide a "specified logo" as they know that if they simply replace logo.png, it will be picked up automatically. Changing the "default" will cause more harm than people realize for contrib/custom themes to be ported.
3. Furthermore, this forces (not enhances) maintainers to either A) create a new svg logo (which may or may not be an issue in and of itself) B) Implement a complex hack to do:
   

   Support for logo.png can be achieved easily in contrib themes

   in three different files... that is hardly an "easy" solution.

4. So, to not break BC, we'd have to add the detection of multiple filetypes. I will admit that I did not remember the other issue until just now, but it's still the correct "way" this should be done and it should be done later because it _is_ a feature request.
5. If anything, it should be Bartik/seven that implements a said such "hack" to display a "custom logo" if the real issue here is that we just want an SVG version of the Druplicon to have a clearer retina display until said feature can be implemented.
6. I wasn't "scope creeping". This issue has done that all on it's own. I was just agreeing with others and trying to put this issue back into focus given the current state at where we are at with D8. Believe me, there are a ton of things that I would have wished had gotten in, but that's why we have these different phases for a reason.
7. I'm not going to argue with anyone and really tired of core's ping pong crap. I don't commit the code, ultimately this is up to core committers. I was just trying to point out some very obvious flaws to why this issue's current path is a very bad idea and offer up a better solution that didn't involve breaking everything not core.

edit: p.s. I raised the BC break back in #72 FWIW... it was ignored, it wasn't ignored... it was just replaced again after #86 and #98.

I see where this got derailed again: #86 and #98. @alexpott, yes... file_exists() is technically "expensive", but these settings are cached immediately after are they not? Why would that not be an appropriate place to check if there is one file type or another?

I hear you Mark, but where you see "derailed" I see consensus on a best possible solution, knowing that we are late on the game and the color module is difficult to work-around without introducing a bunch of new code.

As many have said, the "default" logo... is just that, a default. You can specify the logo you want as a custom logo, and it's exportable. This is the way all these theme assets (favicon.ico, logo.png) probably will (should) behave in the future, instead of relying on outdated conventions and expensive guessing with file_exists.

I would like @alexpott to give his opinion here.

PS. I would appreciate Mark if you can add your opinion here, since you are familiar with color module #2396789: Color no longer needs to manipulate logos

So the issue is that the theme settings are statically cached - so this is rebuilt on every request. So a file_exists here represents a something extra on every request. However, this should be cached by the block cache since the logo is displayed as part of the SystemBrandingBlock. However this cache is busted on everything other than a get request. So any request that is probably of value (a node creation, comment or purchase) to your site will result in extra disk access.

Looking at theme_get_setting I think it is simple to add a logo.default_extension setting to allow theme authors to do what they what to. But let's do that in #15. I agree with @LewisNyman's comment in #120 about retina compatibility being important for now and the future.

Committed a2ea305 and pushed to 8.0.x. Thanks!

Thank you for adding the beta evaluation to the issue summary.

yay!

does this task need to be updated for the change notice now?
https://www.drupal.org/node/2410787