Color module is broken

Same bug here. Using latest CVS version.

I'm receiveing this error codes while changing colorset.

    * Notice: Undefined index: css in _color_page_alter() (line 96 of Z:\home\drupal7\www\modules\color\color.module).
    * Warning: Invalid argument supplied for foreach() in _color_page_alter() (line 96 of Z:\home\drupal7\www\modules\color\color.module).

I have tried to hack _color_page_alter() function and add $vars['css'] = drupal_add_css(); after

function _color_page_alter(&$vars) {
  global $language, $theme_key;

Error messages disapears but problem exists - only logo.png was changed colorset. The background is not changed. Any suggestions?

I got the same warning message when setting Garland to a different color scheme.

CVS update July 18.

The color module is out of sync with theme..

In the old template_preprocess_page() in theme.inc, it used to have these in D6:

  $variables['css']               = drupal_add_css();
  $variables['styles']            = drupal_get_css();
  $variables['scripts']           = drupal_get_js();

but in the D7 version, these are gone but the color module expects to see ['css']

putting these back in template_preprocess_page() in theme.inc got rid of the error message but color don't change.

Well that's not good!

Sounds like it's time for some Color module tests.

I will take a look at it now and write some tests.

Here is the fix. I will create the tests and then change the status.

The function _color_page_alter() needed to be called as late as possible. I created a garland_process_page(&$vars) function and put the color stuff in there.

Here is the full patch with the fix and a very simple test.

I tried changing the color set of Garland and received the same error as comment #1.

After applying the patch no error shown.

I like how this patch also includes much-needed tests for the color module. Some thoughts on the patch:

- '|'.$stylesheets[0].'|' needs to have the concatenation operator (.) preceded and succeeded by a single space.
- Some weird use of global $theme_key going on the test case. Is global $theme_key updated by SimpleTest? I don't think so, and I'd guess that this test will fail if a site's default theme is not garland/doesn't have garland enabled (?)

Otherwise looks good, keep up the awesome work!

Thank you cwgordon7 for your input.

I've changed the color.test to follow the coding standards for spaces. I also installed coder 7.1-1.x-dev and ran it against the color module. Beside the @file missing (which is also missing from system.test so I presume it is not needed for tests files), nothing else was reported.

I changed my drupal7 config so that garland is not enabled and stark is the only enabled theme and also the default theme.
The color tests works just fine.
I even put $this->pass($theme_key) and it displayed garland.
The color module was the only tests I executed.
I looked in the simpletest module for any hint that garland is put as the default theme but did not find anything.
I do not know why the test works if garland is not the default theme.
I checked the module/simpletest/tests/theme.inc and this file also presume that garland is the default theme it seems.
modules/simpletest/tests/common.test is also using the global $theme_key variable.

Here is the new patch with the concatenation operator (.) preceded and succeeded by a single space as the only changes.

Let me know.

>Some weird use of global $theme_key going on the test case. Is global $theme_key updated by SimpleTest?

global $theme_key is set by init_theme() to the current active theme.

I see it doesn't exist anymore in D7.

@mattyyoung:
The init_theme() function was renamed to drupal_theme_initialize().
Here is the issue: http://drupal.org/node/517542
Here is the commit: http://heine.familiedeelstra.com/node/3887

So the global $theme_key is still being used and being set by drupal.

I think the question was if simpletest was playing with it. I think the question was not about if drupal was setting it.
Drupal is definitly setting this global variable as part of the core.

My question is: Why is the value of global $theme_key back to "garland" in my tests when I've set the default theme to be something else using the admin/structure/themes interface?

So, the conclusion is that my patch will work even if a different theme is being set.

I applied the patch and I still don't see the color module working.

Cache cleared. Reinstall. Admin theme been set to Seven or Garland. Nothing worked.

Here is an updated patch.
Some recent changes #536440: Reconsider whether "Appearance" should be a top-level item in the IA to the themeing interface caused the previous patch to fail.

Note that the color module still does not work 100% even with this patch. Some other recent changes in the file api (streaming) #517814: File API Stream Wrapper Conversion is causing the module to generate warnings/errors. However the color module seem to work even with the warnings. The colors are being changed on the site.

Note also that some people has shown interest in #445990: [META] Refactor color module. I do not mind fixing the color module if 1) we are going to keep it in core 2) Someone can explain which scheme I need to use so that file_unmanaged_copy() is not complaining.

Anyone else wants to remove color module from core?
Anyone want to figure out how to handle the new file api changes for this? I think we might need to create our own streaming class or we simply do not use file_unmanaged_copy(). I have not tried it but maybe if we use the http:// scheme.... The color.module on line 293 might need to be changed to specify a new scheme.

Comments?

Is the color.module being used in D7 garland? I don't see it in the config screen.

@mattyoung - That is because #536440: Reconsider whether "Appearance" should be a top-level item in the IA broke it. If you use my latest patch (#15) you should see the color module appear in the garland config screen.

Fixed the added color.test. This patch doesn't work, but I'd like to see what the test bot says for the test.

Here is an updated patch that fixed the problem caused by the new file api changes done on #517814: File API Stream Wrapper Conversion.
I also changed the color.test which was failing due to the changes to the new file api.

With this patch, the color module works and I get no more warnings/errors.

Thanks to jmstacey for the pointers on the new file api.

Very nicely done, it is working now. I've fixed up the color.test addition in the patch and would like to see what the bot says.

This looks great to me, except for a few extra spaces...

+        $vars['css'][$color_path] = $old;
+        $vars['css'][$color_path]['data'] = $color_path;
       }
+      // ...HERE
     }

Here's a patch with that fixed which looks good as far as I can tell.

Great! Thanks all for your help.

I think this is rtbc.

*damn* this patch would have removed my work done in D6 times. It need to be there! How good that I have seen this patch...

-    // Loop over theme CSS files and try to rebuild CSS array with rewritten
-    // stylesheets. Keep the original order intact for CSS cascading.
-    $new_theme_css = array();
-
-    foreach ($vars['css']['all']['theme'] as $old_path => $old_preprocess) {
-      // Add the non-colored stylesheet first as we might not find a
-      // re-colored stylesheet for replacement later.
-      $new_theme_css[$old_path] = $old_preprocess;
-
-      // Loop over the path array with recolored CSS files to find matching
-      // paths which could replace the non-recolored paths.
-      foreach ($color_paths as $color_path) {
-        // Color module currently requires unique file names to be used,
-        // which allows us to compare different file paths.
-        if (basename($old_path) == basename($color_path)) {
-          // Pull out the non-colored and add rewritten stylesheet.
-          unset($new_theme_css[$old_path]);
-          $new_theme_css[$color_path] = $old_preprocess;
-
-          // If the current language is RTL and the CSS file had an RTL variant,
-          // pull out the non-colored and add rewritten RTL stylesheet.
-          if ($language->direction == LANGUAGE_RTL) {
-            $rtl_old_path = str_replace('.css', '-rtl.css', $old_path);
-            $rtl_color_path = str_replace('.css', '-rtl.css', $color_path);
-            if (file_exists($rtl_color_path)) {
-              unset($new_theme_css[$rtl_old_path]);
-              $new_theme_css[$rtl_color_path] = $old_preprocess;
-            }
-          }
-          break;
-        }

@hass - Thanks for jumping in. Could you tell us more information? For example, why do you think the replacement patch is missing something? Which functionality is now broken because of the new patch?

The only thing I removed is the RTL and the only reason I removed it is because when I choose an RTL language (like Arabic) the color module works fine.

If you explain in more details the reasons then I will be happy to modify my patch accordingly.

Your code does not work as is because of #92877: Add numeric weight to drupal_add_css which changed the way the css array is structured.

Thanks.

If you unset() a theme file in the themes array and than add the color module replacement file to the end of the array the order of CSS files have changed and therefore break the cascading stylesheet order. This will break other themes than Garland that implement color.module support and have more than only one style.css. For e.g. the YAML CSS Framework will completely broken if the order of the CSS files change.

I haven't tested this patch, but it looks like breaking a color module enabled theme for the reasons above. Only as a note - this have all *nothing* to do the with weight feature...

@hass - If only that theme was compatible with D7, I would definitly test it and adjust the patch.
I will try to find some other themes that are color enabled and has more than one style.css file.
If anyone know of any theme that are color enabled and has more than one style.css file, let us know :)

Here is a new patch that take into consideration the order of the stylesheets. Thank you @hass for pointing that out.

I could not test this patch against any other theme than garland because I could not find any other themes that are color-enabled and compatible with D7 and have more than one stylesheet called style.css. However I looked at the color.module more closely and the color_scheme_form_submit() function basename($file) and therefor replace any previous style.css anyway.

I did not put the RTL back because I found out that it is being handled by the locale_css_alter() hook implementation.

@hass - I simply mentionned the weight feature because this is where the structure of the $vars['css'] array was changed and made the color module not work. For example, the $vars['css'] array no longuer identify which css files are from a theme or a module. The theme css files are now stored in $themes[$theme_key]->stylesheets['all']. I hope that is more clear that way.

I do not think we need to support themes that have many stylesheets all named style.css (in the same theme). Theme developer can be told to only specify unique names in their color-enable css file (the ones mentionned in $theme/color/color.inc).

What do you think?

This is not a question of an API as feedback to your email.

Now with an example for your emails... if you have an array of 3 items with the same CSS weight and you remove item #2 and add a color file in array pos 4 back the order of CSS files have changed. You cannot solve this if you are not using the D6 logic.

Example:

Array(
  [0] => ‘/css/foo1.css’,
  [1] => ‘/css/foo2.css’,
  [2] => ‘/css/foo3.css’,
)

After you color module with above patch (broken):

Array(
  [0] => ‘/css/foo1.css’,
  [2] => ‘/css/foo3.css’,
  [3] => ‘/css/foo2-recolored-by-color-module.css’,
)

I hope this makes things clear and the below array is how the array looks like since my D6 patch. You can see the order of CSS files is kept intact as it *must*:

Array(
  [0] => ‘/css/foo1.css’,
  [1] => ‘/css/foo2-recolored-by-color-module.css’,
  [2] => ‘/css/foo3.css’,
)

I cannot test the patch today as the YAML theme has not yet been ported to D7. Attached is an example of the color.inc I've used for testing D6. Maybe it helps understanding...

Compare the re-color array 'css' with the list of YAML files on a typical theme. You can see only a very few files will be re-colored.

YAML CSS files that need to be re-colored.

'css/screen/basemod.css',
'css/screen/basemod_2col_left_13.css',
'css/screen/basemod_drupal.css',
'css/screen/content.css',

Typical theme style sheet list of YAML:

yaml/core/base.css
css/screen/basemod.css
css/screen/basemod_2col_left_13.css
css/screen/basemod_drupal.css
css/screen/basemod_gfxborder.css
css/navigation/nav_slidingdoor.css
css/navigation/nav_vlist_drupal.css
css/screen/content.css
css/modules/yaml-vertical_tabs.css
yaml/core/print_base.css
css/print/print_003.css
css/print/print_drupal.css

@hass - Thank you for the details. I have created a theme based on garland which contains the same list you provided + the standard style.css file. Attached you will find the theme with info file containing the list of css file and color.inc which contains only the files that needs to be recolored.

Using the latest patch found in this issue and this theme, the list of css files remains in the SAME ORDER when the color module is enabled and the colors are changed.

Here is what I did:
1- I checked out the latest Drupal 7 and installed it.
2- I applied my patch
3- I installed the color_theme_test theme (included as attachment).
4- I clicked on "Appearance" and enabled the color_theme_test.
5- I viewed the source of the frontpage and took note of the order of the CSS files.
6- I clicked on configured for the color_theme_test and changed the color for the links to "red" and saved.
7- I viewed the source of the frontpage and took note of the order of the CSS files.
8- I compared the order and it remains the same.

This is the generated style sheets BEFORE the color module is enabled:

<link type="tex.... media="all" href="http://localhost/modules/system/defaults.css?8" />
<link type="tex.... media="all" href="http://localhost/modules/system/system.css?8" />
<link type="tex.... media="all" href="http://localhost/modules/system/system-menus.css?8" />
<link type="tex.... media="all" href="http://localhost/modules/field/theme/field.css?8" />
<link type="tex.... media="all" href="http://localhost/modules/node/node.css?8" />

<link type="tex.... media="all" href="http://localhost/modules/user/user.css?8" />
<link type="tex.... media="all" href="http://localhost/modules/toolbar/toolbar.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/all/themes/color_theme_test/style.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/all/themes/color_theme_test/yaml/core/base.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/all/themes/color_theme_test/css/screen/basemod.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/all/themes/color_theme_test/css/screen/basemod_2col_left_13.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/all/themes/color_theme_test/css/screen/basemod_drupal.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/all/themes/color_theme_test/css/screen/basemod_gfxborder.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/all/themes/color_theme_test/css/navigation/nav_slidingdoor.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/all/themes/color_theme_test/css/navigation/nav_vlist_drupal.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/all/themes/color_theme_test/css/screen/content.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/all/themes/color_theme_test/css/modules/yaml-vertical_tabs.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/all/themes/color_theme_test/yaml/core/print_base.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/all/themes/color_theme_test/css/print/print_003.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/all/themes/color_theme_test/css/print/print_drupal.css?8" />
<link type="text/css" rel="stylesheet" media="print" href="http://localhost/sites/all/themes/color_theme_test/print.css?8" />

This is the generated style sheets AFTER the color module is enabled:

<link type="tex.... media="all" href="http://localhost/modules/system/defaults.css?8" />
<link type="tex.... media="all" href="http://localhost/modules/system/system.css?8" />
<link type="tex.... media="all" href="http://localhost/modules/system/system-menus.css?8" />
<link type="tex.... media="all" href="http://localhost/modules/field/theme/field.css?8" />
<link type="tex.... media="all" href="http://localhost/modules/node/node.css?8" />

<link type="tex.... media="all" href="http://localhost/modules/user/user.css?8" />
<link type="tex.... media="all" href="http://localhost/modules/toolbar/toolbar.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/default/files/color/color_theme_test-0624c93e/style.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/all/themes/color_theme_test/yaml/core/base.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/default/files/color/color_theme_test-0624c93e/basemod.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/default/files/color/color_theme_test-0624c93e/basemod_2col_left_13.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/default/files/color/color_theme_test-0624c93e/basemod_drupal.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/all/themes/color_theme_test/css/screen/basemod_gfxborder.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/all/themes/color_theme_test/css/navigation/nav_slidingdoor.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/all/themes/color_theme_test/css/navigation/nav_vlist_drupal.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/default/files/color/color_theme_test-0624c93e/content.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/all/themes/color_theme_test/css/modules/yaml-vertical_tabs.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/all/themes/color_theme_test/yaml/core/print_base.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/all/themes/color_theme_test/css/print/print_003.css?8" />
<link type="tex.... media="all" href="http://localhost/sites/all/themes/color_theme_test/css/print/print_drupal.css?8" />
<link type="text/css" rel="stylesheet" media="print" href="http://localhost/sites/all/themes/color_theme_test/print.css?8" />

The latest patch is keeping the order of the style sheets intact. Am I missing anything or misunderstanding something?

Let me know. Thanks.

This result looks good, but the patch is missing :-)

@hass - Thanks :) I am happy to see that this patch could do the trick.

See the patch attached to comment #28.

I do not see the need to re-attach it.

Re-rolled patch.

This patch fixes the color module for D7 and introduce the very first and only simpletest for the color module.

How about adding a CSS file order test?

Related (sort of): #558906: Color Module Form Doesn't Work

Just applied this patch and found the color module back. Great!

I'm marking this RTBC as it's an important feature that just disappeared a few months ago! Good thing there's a test now to keep track of it in the future.

CSS tests can come later.

Awesome work folks! Committed to HEAD.

Agreed on a follow-up issue for CSS weighting tests.

You want CSS weight order tests? Isn't this CascadingStylesheetsTestCase::testRenderOrder?

Re-opening as per http://drupal.org/node/365982 "... is a completely different issue..."

While using Garland, if you change the color scheme to anything other than Blue Lagoon then the logo disappears altogether and is replaced with the site name (please see the first pic).

However after playing around with the code a little bit I discovered that in the color.module file, if the following (at ≈ line 305):

variable_set('color_' . $theme . '_logo', $paths['target'] . 'logo.png');

were changed to this:

variable_set('color_' . $theme . '_logo', $paths['source'] . 'logo.png');

then at least a logo starts to appear -- the down-side is that the logo's color scheme (well, actually the logo itself) does not change to match the rest of the theme. It appears to be reusing the Blue Lagoon logo regardless of the selected color scheme (see the second pic).

I also attached a patch for the above code changes, but even with that change applied, a new logo still needs to be rendered. (FTR, test was done using FF so it's not an IE/png issue...)

Attachments from previous post:
---------------------------------------------
Garland logo is gone after changing colors
Garland logo appears, but colors don't change
color.module.patch (which incidentally failed but still).

Here is a patch that address the issue reported by @jerdiggity.

The patch in #43 works fine for me.

Is it possible to get a test for this too?

We created a new issue for creating a test: #606882: Test that image src attributes actually resolve

So the test will be handled separately.

In general, it is a bad idea to handle tests in a separate issue. It's not advised. I'll leave it up to webchick to decide if she wants to commit this without a test.

Yeaaaahhhh, given the focus we have on this issue, I agree that it's best to have the tests here, too.

Let's get a test for this particular bug, and then perhaps expand them out in #606882: Test that image src attributes actually resolve.

With a test.

This might work.

Mmmm. Well. That's not a test for this issue specifically, though it is a good test that says "Something ain't working like it ought to." The problem is that public:// could be output by literally anything dealing with files, so if someone comes across a failing color module test in their file management feature, there will total chaos and confusion.

I've decided to commit the one-liner fix. While I'd like to have tests for this, I can't immediately think of a way to do it. We can't test just for the presence of the logo, since it is printed on the page, just its URL is incorrect. Perhaps the real way to add a test for this is to functionality to SimpleTest to report if any non-asserted 403/404 errors occur during a test run.

Committed to HEAD. Thanks!