logo.png copied to files/color/... and will only be updated on new save

This happens because logo is specified in color.inc

  // Files to copy.
  'copy' => array(
    'logo.png',
  ),

There may be some thinking behind this, or its an oversight/legacy code, will await what Jen says.

So color module is taking over the ability to upload custom logos? That sounds bad. Let's test more, and let's fix it.

I need to take a longer look at this, but it's actually a pretty big problem. Couldn't figure it out right away.

Cant we just add a bit of checking to color module?

I didn't had to do this?
Just tried it...
Very recent dev checkout.

@aspilicious ... did you manually just swap out the logo.png (via ftp or whatever) or did you use the appearance settings? The OP is about manually swapping the image.

I manually swap the image on my local computer. Running through localhost. Image gets swapped immediately on the screen with a reload.

I think part of the weirdness is that you have to have the copy array declared in color.inc - you will get an undefined notice if you try to remove it or leave it empty - and that the color module wants to override the logo by default.

The patch...
1) checks that a new logo file exists before overriding the default
2) checks the $info['copy'] isset before trying to copy over neutral images

Which means themes can choose to declare the copy array or not - its just an idea and needs some review from the color module peeps.

The OP is about manually replacing logo.png, but I have related problem. If you set a custom logo, save it, and then recolor the theme, it does not copy the logo to the new color set.

#10 - I see that also - is that really a problem, what would be the purpose of it getting copied to the new color set?

Currently, it is uploaded the active color set. When the color set is changed, it isn't moved, but the new color set assumes it has been, and you get a missing image.

In a custom theme, it normally gets copied straight to the theme's directory, but that's not an option with a core theme, so it puts it with the color set.

It either needs to be copied to the color set, or put somewhere else that the color set can reference.

I can't agree that #12 is a problem - this would be an edge case since it would be silly to load your logo into the color set when you have an upload form on the theme settings page to add your custom logo, which gets saved to the files directory. If you add your logo to the color set and it disappears when you change color schemes - too bad.

Can we look at #5 again, right now Bartik is copying this image (the logo) to the color set simply to avoid an error for something that should not be a hard requirement.

Garland will be doing this also when we change the logo to a transparent png to fix the alignment issues - so both core colorable themes will be hacking their way around color module.

Turns out my problem was separate problem due to PHP 5.2: #924396: _system_theme_settings_validate_path() fails on PHP 5.2.

Trying to make any sense of the major queue currently. This sounds like an annoying bug, but not major.

+++ modules/color/color.module	20 Aug 2010 22:54:30 -0000
@@ -345,12 +345,14 @@
+  if (isset($info['copy'])) {
+    foreach ($info['copy'] as $file) {

Color module should default $info['copy'] to an empty array.

Powered by Dreditor.

I believe this is the issue I ran into today, and if so, it's major.

The symptoms are this:

1. Go to global theme settings (admin/appearance/settings) and upload a custom site logo (annoyingly, you have to uncheck "Use default" to even see that this is an option available to you).
2. Go to Bartik's theme settings (admin/appearance/settings/bartik). Note that you can switch color schemes around and the preview correctly shows them with the custom site logo.
3. Find a suitable colour scheme, then save.

Effect #1: The preview is broken and only shows site name, not site logo.
Effect #2: You also just broke the logo on your site, which is looking in the root directory for the logo, instead of the files directory.

This'll ultimately need to get fixed in D8 first and then back-ported, but leaving it in D7 for now to get some more eyes on it.

Hm. Patch doesn't seem to fix it, so I must have some other problem.

Here's a free re-roll, anyway. Doesn't include sun's suggestion.

I believe this is still relevant.

Personally I think this is part of a bigger issue (ie: having to save the settings for color to re-generate things to begin with... it should be cache controlled in reality).

But I agree... there should be some file watching (maybe even against file mod date?) to detect when a theme changes the logo. It should create a new colored version of the logo (upon cache clear, imho).

Marked #109342: Allow color module to re-color logos as a dup of this issue.

Did a re-roll, tested and working

Tested against the steps in comment #819222-16: logo.png copied to files/color/... and will only be updated on new save
effect 1 and effect 2 were not experinced
screenshots attached

+++ b/core/modules/color/color.module
@@ -94,7 +94,7 @@ function color_preprocess_page(&$variables) {
+   if (file_exists($logo) && $vars['logo'] && preg_match('!' . $theme_key . '/logo.png$!', $vars['logo'])) {

Fix indentation. It seems to have one blank space at the beginning.

using correct coding standards

Tested #24 patch and works like a charm. It looks like a RTBC ;)

1. +++ b/core/modules/color/color.module
   @@ -353,12 +353,14 @@ function color_scheme_form_submit($form, &$form_state) {
   +  if (isset($info['copy'])) {
   

   Per @sun.core in #15, this should always be defined, at a minimum, an empty array. So this check isn't needed and should be removed.

Also, please don't RTBC a patch before it's at least come back from testing green (just to make sure no other regressions have been introduced), thanks! :)

#24: color-819222-24.patch queued for re-testing.

The review I did in #28 still needs to happen.

re-roll with regards to comment #28

@t14, your patch got reversed somehow. This is why the patch failed.

+++ b/core/modules/color/color.module
@@ -94,7 +94,7 @@ function color_preprocess_page(&$variables) {
-  if (file_exists($logo) && $vars['logo'] && preg_match('!' . $theme_key . '/logo.png$!', $vars['logo'])) {
+  if ($logo && $variables['logo'] && preg_match('!' . $theme_key . '/logo.png$!', $variables['logo'])) {

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

Not sure what happened with the last one here is a fresh patch.

does this require something like?

$info['copy'] = '';

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

This should just be:
'copy' => array(),

Go bot go

reroll

I tested this on Drupal 8.9.x and 9.3.x and cannot reproduce the problem. I added a custom logo, went to the site and saw it displayed correctly. Then copied a different logo to the same filename in sites/default/files, reloaded the home page and the logo was changed. The path to the logo is now in config and retrieved from there so there is no 'random' portion.

I have not tested Drupal 7. Moving back to the Drupal 7 issue queue.

I don't think this is quite fixed yet. With 9.2 I after installed color module, and saved the settings if I change the logo in theme folder (not uploading it from the UI) the logo is not updated until I re-save the theme settings again. This is caused because the copy part is done as a submit callback on the theme settings form. I think is an issue because lets say, we start a project we set the theme settings, later during development we choose to change the logo. If we don't save again the settings the logo wil not be copied. Since the color module is altering the core site branding block should check first if the color version of the logo exists, if not leave the block unchanged.