Allow theme developers to add the default logo filename to the theme's .info.yml

+++ b/core/includes/theme.incundefined
@@ -1414,7 +1414,18 @@ function theme_get_setting($setting_name, $theme = NULL) {
+          if (file_exists($logo = dirname($theme_object->filename) . '/logo.svg')) {
+            $cache[$theme]['logo'] = file_create_url(dirname($theme_object->filename) . '/logo.svg');
+          }
+          elseif (file_exists($logo = dirname($theme_object->filename) . '/logo.png')) {
+            $cache[$theme]['logo'] = file_create_url(dirname($theme_object->filename) . '/logo.png');
+          }
+          elseif (file_exists($logo = dirname($theme_object->filename) . '/logo.gif')) {
+            $cache[$theme]['logo'] = file_create_url(dirname($theme_object->filename) . '/logo.gif');
+          }
+          elseif (file_exists($logo = dirname($theme_object->filename) . '/logo.jpg')) {

Will all those file system hits hurt performance? Maybe we should set default_logo on a per-theme basis explicitly?

-1 days to next Drupal core point release.

Yes !! if i added any .jpg or .gif image in my theme to use as Logo,my site would show a blank screen without adding this patch .After adding the patch the site working fine due to this patch. Its working fine now.
Thanks :)

Do you think that logo.png should be the last option in the case list? It's the default now, so any other file the user drops in will automatically act as an override if it checks for logo.png last.

I'd like to close it as "works as designed". It assumed logo.png is the default logo to all drupal themes. if there're different name or filetypes, custom logo is your choose.

I am interested in this as well, for instance, I would like to use an SVG as my main logo, but I would like it to fall back to a png for older IE browsers.

Could I suggest an alternative approach? How about instead of hard coding the logo filename we instead specify it in the theme .info file.

For example:
features[logo] = logo.png

Then if I want I can change that to:
features[logo] = my-company-logo-200px.jpg

Personally, I find it annoying that I have to name the logo file 'logo.png' even when I am happy to use a .png file.

I guess in some ways this is an additional suggestion to the above, as it could always default to logo.* when not specified.

Thanks,
Ross.

re-reading the thread, I think that was the kind of thing Rob Loach was suggesting as well (at #2).

Thanks for your great post also thanks for that your giving us great information on logo.png

It's been almost a year since there's been any movement on this issue. It would be great if it could be part of D8 and possibly backported to D7.

It seems like adding more SVG support to themes would have lots of benefits - http://net.tutsplus.com/tutorials/why-arent-you-using-svg/

EDIT: Also adding link to http://webdesign.tutsplus.com/tutorials/htmlcss-tutorials/getting-starte... as there are advantages for mobile devices.

There's a reliable technique for falling back from SVG to PNG here.

I think that might be out of scope of this issue though. Let's just recognise other file types and create a follow up to add a fallback method.

If we want to provide a fallback for svg, isn't it better to create an extra variable for a svg logo, instead of allowing more file types?
It would be nicer when a png or jpg version for older browsers is automatically renderd by Drupal out of a svg. Sadly that's not possible with imagemagick nor modernizr. It is possible with nodejs but that's not in core. http://eng.wealthfront.com/2011/12/converting-dynamic-svg-to-png-with.html

So Drupal 8 doesn't support no SVG browsers any more, so we don't need to provide a fallback method in core.

If SVG is supported we could move away from png and switch to vector based logo.svg as a standard?

I think we should still allow people to choose PNG, just for themers that might not have access to the SVG versions. That's definitely happened to me before! But the good news is we don't have to accommodate a fallback method, it would be easy for contrib to implement.

Here's a reroll of the patch from #5

Here's a screenshot of the patch in action, I just added an example logo.svg to Bartik.

+++ b/core/includes/theme.inc
@@ -922,8 +922,21 @@ function theme_get_setting($setting_name, $theme = NULL) {
+        // Search for an svg filetype first, then other common filetypes.
         if ($cache[$theme]->get('logo.use_default')) {
-          $cache[$theme]->set('logo.url', file_create_url($theme_object->getPath() . '/logo.png'));
+          if (file_exists($theme_object->getPath() . '/logo.svg')) {
+            $cache[$theme]->set('logo.url', file_create_url($theme_object->getPath() . '/logo.svg'));
+          }
+          elseif (file_exists($theme_object->getPath() . '/logo.png')) {
+            $cache[$theme]->set('logo.url', file_create_url($theme_object->getPath() . '/logo.png'));
+          }
+          elseif (file_exists($theme_object->getPath() . '/logo.jpg')) {
+            $cache[$theme]->set('logo.url', file_create_url($theme_object->getPath() . '/logo.jpg'));
+          }
+          elseif (file_exists($theme_object->getPath() . '/logo.gif')) {
+            $cache[$theme]->set('logo.url', file_create_url($theme_object->getPath() . '/logo.gif'));
+          }
         }

The problem is that this will possible do upto 4 additional file_exists checks per request. The cache you are setting here is a static cache.

I had a go wrapping the logo url in a cache object but mysql broke before I could test it (again). Here's how far I got.

It's cool but I have no idea why we're making CORE such complexity. We can register a new Logo easily in own theme or through yml settings

Now that I am able to test this patch, it seems like Bartik is loading the Seven logo. So I guess we would need a separate cache for each theme?

#2142653: Change default logo filetype to .svg and add an SVG version of Druplicon has been committed which changes the purpose of this issue slightly

Here's a reroll that should help nudge this along.

Let's not do the whole file exists thing but allow theme developers to add the default logo filename to the theme's .info.yml

Removing unnecessary tags (per [#1023102]).

+++ b/core/includes/theme.inc
@@ -330,7 +330,14 @@ function theme_get_setting($setting_name, $theme = NULL) {
+          if (property_exists($logo, 'info') && isset($logo->info['logo']) && !empty($logo->info['logo'])) {

I don't think the property_exists condition is needed. Otherwise this looks good to go.

@see http://3v4l.org/qVbYK

You'll notice it actually throws notices on line 14 where it doesn't when you don't check the property.

Fixing status because there is a patch.

Setting as needs review for testbot.

Added tests for logo path.

While reviewing this I:
1. Applied the patch.
2. Made changes to .info.yml of bartik theme and specified logo value to screenshot.png.
3. The new logo reflected properly.
4. Ran tests using Testing module interface on Theme/ThemeSettingsTest with zero errors/warnings returned.

+++ b/core/includes/theme.inc
@@ -351,7 +351,14 @@ function theme_get_setting($setting_name, $theme = NULL) {
+          $logo = \Drupal::theme()->getActiveTheme()->getExtension();
+          if (isset($logo->info['logo']) && !empty($logo->info['logo'])) {
+            $logo = $logo->info['logo'];
+          }
+          else {
+            $logo = 'logo.svg';
+          }

Let's add a getLogo method to the activeTheme class and populate it during theme initialisation and then move this logic into that method.

Also I think under the beta evaluation https://www.drupal.org/core/beta-changes this might need to be postponed to 8.1

Made change suggested in #57.

Looks good. RTBC+

Thanks for working on the issue and sorry for the nits :)

1. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
   @@ -177,4 +177,24 @@ public function getBaseThemes() {
   +     * Returns the default logo to be used by the active theme.
   

   This is a getter method so it should be documented starting with "Gets"

2. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
   @@ -177,4 +177,24 @@ public function getBaseThemes() {
   +      return $logo;
   

   Just a nice thing to have would be a extra line change before return :)

3. +++ b/core/modules/system/src/Tests/Theme/ThemeSettingsTest.php
   @@ -65,4 +65,41 @@ function testNoDefaultConfig() {
   +   * Tests logo config.
   

   Maybe we could document this a little bit more accurately

Made changes as per #62.

I don't see how this is in need of a re-roll.

I guess I reacted too soon. Sorry, re-roll it is.

Have re-rolled the patch along with recommendations in #62.
Thanks

Now this looks good to me :) Sorry for the interruption here!

Sorry I didn't review other things yet.. Still something to figure out.

This doesn't match beta evaluation. Pushing to 8.1.x

We would need to do this in a backwards compatible way.

> We would need to do this in a backwards compatible way.

To me that says 'needs work' as in 'needs a rethink' rather than postponed.

I re-rolled the patch for 8.3.x.
I had to adjust unit tests to make them use file_url_transform_relative function, which is now called in theme.inc as well to generate the logo url.
It seems backward-compatible to me, but I may be missing something here.

- applied the patch: allow_theme_developers-1507896-77.patch
- made bartik child theme with these instructions https://www.drupal.org/docs/8/theming-drupal-8/creating-a-drupal-8-sub-t...
- added logo.png to theme folder
- added line "logo: logo.png" to theme.info.yml

I can confirm that the feature is working as intended.

1. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
   @@ -195,6 +195,26 @@ public function getBaseThemes() {
   +   * Use the logo specified in the active theme configuration file, if it exists.
   
   

   Minor nit: This line is one character over the max line length, so we need to wrap another word here.

2. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
   @@ -195,6 +195,26 @@ public function getBaseThemes() {
   +    $config = $this->getExtension();
   +    if (isset($config->info['logo']) && !empty($config->info['logo'])) {
   +      $logo = $config->info['logo'];
   

   Is this logo key a schema addition for the theme info file? We would probably need to add documentation for it somewhere.

   Also, it's not a "configuration" file, is it? As in, it's not simple config or a config entity as managed by the configuration system. We should change the variable names if it's not to avoid confusion.

Other than that, this looks like a good feature addition to me. It appears backwards-compatible and safe for a minor version (8.4.x at this point). I have not yet reviewed the test coverage in full, but I'd like to get one of the theme maintainers' signoff here based on the earlier discussions. So one of @Cottser or @lauriii.

Also, we still need that change record.

Thanks everyone!

Fix for #83-1

Made interdiff.patch by mistake, .txt attached here...

Also, it's not a "configuration" file, is it? As in, it's not simple config or a config entity as managed by the configuration system. We should change the variable names if it's not to avoid confusion.

It's Drupal\Core\Extension\Extension object, so maybe $extension or $file_info would make more sense?

ThemeSettingsTest::testLogoConfig() is indented incorrectly with 3 spaces and is missing an empty line before the closing bracket.

diff --git a/core/tests/Drupal/KernelTests/Core/Theme/ThemeSettingsTest.php b/core/tests/Drupal/KernelTests/Core/Theme/ThemeSettingsTest.php
index 6c2dfb39f0..03c3ee7ecb 100644
--- a/core/tests/Drupal/KernelTests/Core/Theme/ThemeSettingsTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeSettingsTest.php
@@ -60,4 +60,47 @@ function testNoDefaultConfig() {
     $this->assertNotNull(theme_get_setting('features.favicon', $name));
        }
        +     $this->assertIdentical($default, theme_get_setting('logo.url'));
...
        +     $this->assertIdentical(
        +       file_url_transform_relative(file_create_url('public://logo_with_scheme.png')),
        +       theme_get_setting('logo.url')
        +     );
...
        +     $this->assertIdentical(
        +      file_url_transform_relative(file_create_url($theme->getPath() . '/logo_relative_path.gif')),
        +      theme_get_setting('logo.url')
        +     );

assertIdentical() is deprecated and might be good idea to replace them with assertSame() or assertEquals().

+++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
@@ -195,6 +195,28 @@ public function getBaseThemes() {
+    $extension = $this->getExtension();

I was looking at this patch and this seems a big inconsistent with how everything else uses the constructor values from getActiveTheme(), after looking at this I wondered if we could creep this a tad and make logo's inheritable. The reason I ask is that I often work on big projects with multiple skin type themes, all use the same logo etc.

I've whipped up a patch as a PoC, and sorry I don't mean to throw a spanner in this issue as it's coming along nicely and a tad overdue already, it's just a thought that this should at least be passed from getActiveTheme(), and could we do inheritance (which would be a nice feature I think).

This is like only the second patch I've written for D8 so please excuse if I've duffed somewhere here. No test incl.

Nice work everyone!

@Jeff Burnz I think that looks way cleaner, and makes things way more intuitive, while still achieving what we want in this issue.

Patch works and looks great as well. We should probably add tests though. I can whip up some quickly if you don't want to?

Thanks to everyone working on this.

Like many on this thread I am keen to see this issue committed. We are really close now. There has been a lot of work to create the existing patch, pulling in feedback from many people to build a consensus that has been well tested and reviewed. We have got to the stage of reviewing at the level of "minor nits".

Regarding comment #88, I can see that inheritance would be a bonus. Can we creep to add it in? Personally I think no, that should be a separate issue, which validly incrementally builds on what we have here. We are too close to completion here to make significant additions. However even if consensus is that we do creep, it must be done very soon, with a patch that is very closely based on the work so far, with a small addition for inheritance, and otherwise of the same standard of testing and automatic testing.

Apologies @Jeff Burnz, I'm afraid I think you are very much throwing a spanner in the works! You seem to be discarding the established patch and going back to a totally new "proof of concept" currently without tests. Personally I don't even agree with you that the new approach is more intuitive or consistent (libraries is an array, yes, because a theme has multiple libraries, but a theme has just one logo), and I can see some possible flaws in the new approach. However I don't even want to start that debate. We have a simple, clean, working solution with a consensus of acceptance and should stick with it.

I have hidden the new patch, and propose that the next step is for final review of patch #87.

FWIW Here is my suggestion for how to build on what we have to add inheritance in a future issue. I would suggest that getLogo could be extended to included a third case, so it becomes:

1. Check $theme->info['logo']
2. Else check file_exists('logo.svg')
3. Else recursively call getLogo on base theme.

This solution has the following advantages:

• It's **back-compatible**. If an existing theme provides logo.svg, that must still work without mandating every theme update to info.xml. I.e. logo.svg should take precedence over settings in the base theme.
• It's simple and fast as we stop searching as soon as a logo is found and just store that one logo
• Cleaner OO style using polymorphism/encapsulation.

Like many on this thread I am keen to see this issue committed. We are really close now.

None of the theme API sub-system maintainers have chimed in on this for more than 18 months, and none since the patch you assert to be "established" was written. As we can see the issue has taken many turns already, based on maintainer feedback - lets give one of them a chance to review various approaches etc. I think that's pretty fair.

Note that xjm added that tag (needs sub-system maintainer review etc) in her last review, we need one of them to review the whole issue.

We have got to the stage of reviewing at the level of "minor nits".

No, I am calling out the whole approach as inconsistent, this is not a minor nit.

Personally I don't even agree with you that the new approach is more intuitive or consistent

I have not made these claims. What I argue is that the patch from #87 is inconsistent. Please don't put words into my mouth.

I can see some possible flaws in the new approach.

Such as?

And one more thing (sorry...):

I have hidden the new patch, and propose that the next step is for final review of patch #87.

You haven't provided a review, patch or discussion on this issue until now, so I feel a tad blindsided by your harsh critique. I actually argued for something like my patch before we even changed to SVG as default and imposed this limitation (years ago now) - note that I was met with the argument that since it was established that .png was the only file type previously supported, a single file type would be fine. It was viewed as scope creep to allow others, and now years later we'er still here doing this dance. Sometimes a little creep can save a lot of mucking around.

Inheritance could be another issue, granted. It just seems like a grand opportunity to do something missing from Drupal forever.

I'm working on a group of sites for a legal services company that has 40 offices, 40 sites, and a number of sub-themes (different services are provided depending on the office type) but they all have the same corporate logo.

A few years ago on D7 I worked on a similar system that had around 25 sub-themes and thousands of sites. All with the same logo (a very large ivy league college).

My point is this is not out of the ordinary for companies, clubs, institutions etc to have many sub-sites with sub-themes yet all have the same logo.

Thank you for tagging this issue to be reviewed by the Theme API maintainers.

Adding the logo value to the theme info itself is fine. As @xjm pointed out on #83, we will have to ensure that we have proper documentation in place. Besides creating the change record, this should be added at least to the documentation page about creating a theme info.yml file.

The problem with the approach taken on this issue is that we add some logic to a value object. On the latest patch from @Jeff Burnz, we are moving to the right direction. It already removes some of the logic that the original approach introduces to the ActiveTheme value object. Looking at this, I think we should move the default value handling also to the ThemeInitialization.

I agree that the theme inheritance would be a nice feature, but I'd leave it out of the scope of this issue since it opens up some new issues we wouldn't have to deal with otherwise. I strongly believe that the theme inheritance can be done in a non-BC breaking way even after committing this, why I'd like to suggest that we open up a follow-up issue for that.

Thanks everyone and keep up the good work!

OK, opened the follow up for inheritance: #2854740: Allow sub-themes to inherit the base theme logo

WiP patch attached. I think I have this right based on feedback from @lauriii in #94.

Tests omitted at this stage, because I am a test neophyte to be frank :/

Besides creating the change record, this should be added at least to the documentation page...

Should we push this change into a core theme to expose this new feature, e.g. Bartik? I'm not sure about BC with regards to tests if we do that.

Ops, wrong patch.

This one.

Looks good! As mentioned, we will have to add test coverage, tagged and moved back to needs work for that.

Should we push this change into a core theme to expose this new feature, e.g. Bartik? I'm not sure about BC with regards to tests if we do that.

Good idea! Bartik has been marked internal which means we are allowed to make this type of changes.

I agree with this being a better approach.

diff --git a/core/includes/theme.inc b/core/includes/theme.inc
index f180cbd..15a554c 100644
--- a/core/includes/theme.inc
+++ b/core/includes/theme.inc
@@ -345,7 +345,8 @@ function theme_get_setting($setting_name, $theme = NULL) {
+        $logo = \Drupal::theme()->getActiveTheme()->getLogo();
+        $cache[$theme]->set('logo.url', file_url_transform_relative(file_create_url($logo)));

Using ->getActiveTheme() to figure out the logo path will break $theme argument in theme_get_settings().

I wrote some initial tests and replaced getActiveTheme() call with getActiveThemeByName().

Using ->getActiveTheme() to figure out the logo path will break $theme argument in theme_get_settings().

Out of interest what is the breakage here, wrong theme name etc?

Out of interest what is the breakage here, wrong theme name etc?

Yep.

theme_get_settings() allows you to specify theme name as a second argument and load settings for any given theme and getActiveTheme() will always load logo for the currently active theme regardless of given argument.

The novice task is to create a test and write a change record, tagging for DrupalCon Vienna

I'm on DrupalCon, with help of gambry, I've chosen that task and will try to work on it.

While reviewing this I:
1. Applied the patch.
2. Made simple subtheme of classy, and included in info file information about logo.png to use (logo: logo.png)
3. The new logo was used properly.

Still TODO:
1. Run tests.

Working on change record.

Added a change record draft and updated the IS.

Tried to draft a change to the doc page, but that goes live directly? What is the process of drafting a change there? I can update it once it is committed, but not sure what is the best way to draft it in the meantime?

Maybe the documentation should be a different issue, because now the patch is waiting for the documentation, but the documentation cannot be released until the patch is in.

Another option would be that you attached your draft to this issue (either in a comment or as a text file) so that it could be reviewed. I.e. it would not be part of the patch but we could still review it.

Patch looks great, CR looks good. I also just went ahead and added it to https://www.drupal.org/docs/8/theming-drupal-8/defining-a-theme-with-an-... in anticipation of this being committed using a version heading to indicate when it was introduced (which should help alleviate any confusion until this is committed).

Thank you @markcarver for updating the documentation. Thank you @tuutti for adding the test coverage.

The changes look good for me. Committed 018baee and pushed to 8.6.x. I also published the change record. Thanks everyone!