convert color variables to config/state

After investigating this issus we've found that themes don't require a default config and a variable is only set if it is overridden in the UI.

A new config file will be required in the theme if it wants to include default config named color.[theme name].yaml

Here's the patch, includes a TODO as it would be better for all themes to define their default configuration in YAML rather than the current method but this is not in scope of this patch.

Just reviewed and realised I broke logic in color_get_palette function when trying to fix for test.

+++ b/core/modules/color/color.moduleundefined
@@ -332,21 +341,14 @@ function color_scheme_form_submit($form, &$form_state) {
-    @drupal_rmdir($file);
-  }
-
-  // Don't render the default colorscheme, use the standard theme instead.
-  if (implode(',', color_get_palette($theme, TRUE)) == implode(',', $palette)) {
-    variable_del('color_' . $theme . '_palette');
-    variable_del('color_' . $theme . '_stylesheets');
-    variable_del('color_' . $theme . '_logo');
-    variable_del('color_' . $theme . '_files');
-    variable_del('color_' . $theme . '_screenshot');
-    return;
+      @drupal_rmdir($file);

Indendation looks wrong here, that drupal_rmdir() line shouldn't be changing?

#3: color_variables_to_config-1804380-3.patch queued for re-testing.

Berdir thanks for the review.

Fixed up indentation.

#7: color_variables_to_config-1804380-7.patch queued for re-testing.

#7: color_variables_to_config-1804380-7.patch queued for re-testing.

Where is the yml file? And where is the update function?

The problem is the config is unique to each theme that implements the color module so the yml file should really be in the theme and therefore each theme would need it's own update function.

See comment #1 & #2 for more info

Passing a first try on how the upgrade path should migrate the previous settings.
Anything addigional / custom missing?

Still no .yml file(s) present.

We could add a yml file to the color module, but wouldn't it be better placed in each of the themes that use the color module?

That way they would each have a yml file named color.[theme_name].yml inside their config.
'color/color.inc' is currently where default configuration is stored per theme.

A different thing is upgrade test coverage.
I tried adding howto add upgrade test coverage to the example howto:
http://drupal.org/node/1667896

What's the proper location to do a upgrade test? Seems we need to write a separate color upgrade test from scratch.

The config upgrade test class is currently incorrectly named "SystemUpgradePathTest".

Providing a version that should fix the upgrade path.

Tests for upgrade path missing.
Since the theme doesn't provide anything by default, i think we don't need any default .yaml config in the themes.

Switching upgrade to plain $config objects. No more update_variables_to_config().

Grr :-)

Great!
Note that the upgrade now doesn't do anything.
Thus, to test if upgrading really works, we need to prefill a theme with color settings and check if upgrad works.

Added test coverage. :-)
Indeed found a bug!

Added a real upgrade path for bartik, plus a faked for seven with optional screenshot.

The last testfail there is a random one, I've seen that before, opened #1833928: Random test failure in Drupal\user\Tests\UserAccountLinksTests.

Will trigger a re-test.

#29: 1804380_color_variables_to_config_upgrade_29.patch queued for re-testing.

#29: 1804380_color_variables_to_config_upgrade_29.patch queued for re-testing.

Reroll to apply against head.

Rerolled.

Grrrr, merged buggy :-)

+++ b/core/modules/color/color.moduleundefined
@@ -132,8 +132,14 @@ function color_get_palette($theme, $default = FALSE) {
+  //TODO - default color config should be moved to yaml in the theme.

should be @todo (and a space before the @todo), yes we have coding standards for todo's. See http://drupal.org/node/1354

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/SystemUpgradePathTest.phpundefined
@@ -89,12 +89,56 @@ public function testVariableUpgrade() {
+    // screenshot is optional ¶

Trailing whitespace

Fixed up todo & trailing white space

#42: color_variables_to_config-1804380-42.patch queued for re-testing.

#42: color_variables_to_config-1804380-42.patch queued for re-testing.

Reroll.

Good to go!

+++ b/core/modules/color/color.installundefined
@@ -37,3 +37,24 @@ function color_requirements($phase) {
+function color_update_8001() {

list_themes() isn't safe to use during updates, but it looks like this could be replaced by a LIKE query on the variables table?

+++ b/core/modules/color/color.moduleundefined
@@ -132,8 +132,14 @@ function color_get_palette($theme, $default = FALSE) {
+  $palatte_config = config('color.' . $theme)->get('palette');
+  return $palatte_config ? $palatte_config : $palette;

Should be $palette_config

fixes for review comment #49.

Misplacement of upgrade code on patch merge conflict. here is the locally working patch

Looks pretty good, a bit niggly but in $themes = db_query('SELECT name FROM {variable} WHERE name like :name', array(':name' => 'color_%_palette'))->fetchAllAssoc('name'); like needs to be capitalized.

capitalised like - comment #53

updating query to escape underscore.

Re-rolling...

#57: 1804380-57-color_variables_to_config_upgrade.patch queued for re-testing.

Can't find any problems...

Found a few minor issues, and there's one hunk that's removed with no configuration system equivalent that I'm not sure can just go away.

+++ b/core/modules/color/color.installundefined
@@ -37,3 +37,32 @@ function color_requirements($phase) {
+function color_update_8001() {
+  $themes  = db_select('variable', 'v')
+    ->fields('v', array('name'))
+    ->condition('v.name', db_like('color_') . '%' . db_like('_palette'), 'LIKE')
+    ->execute()
+    ->fetchAllAssoc('name');
+
+  foreach ($themes as $theme_palette => $theme) {
+    if (update_variable_get($theme_palette)) {
+      // get theme name from color palette variable.
+      preg_match('/color_(.*)_palette/', $theme_palette, $matches);

Why is the update_variable_get() check necessary? It's not going to be different to the direct query that was just run. Also nitpicking but the comment should start with a capital letter.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/SystemUpgradePathTest.phpundefined
@@ -99,12 +99,56 @@ public function testVariableUpgrade() {
+    // color module for theme bartik
+    // screenshot is optional

Again the comments don't meet code style.

+++ b/core/modules/color/color.moduleundefined
@@ -332,23 +340,16 @@ function color_scheme_form_submit($form, &$form_state) {
+  if(isset($files)) {

Should be a space after the if before the parenthesis.

+++ b/core/modules/color/color.moduleundefined
@@ -332,23 +340,16 @@ function color_scheme_form_submit($form, &$form_state) {
-  // Don't render the default colorscheme, use the standard theme instead.
-  if (implode(',', color_get_palette($theme, TRUE)) == implode(',', $palette)) {
-    variable_del('color_' . $theme . '_palette');
-    variable_del('color_' . $theme . '_stylesheets');
-    variable_del('color_' . $theme . '_logo');
-    variable_del('color_' . $theme . '_files');
-    variable_del('color_' . $theme . '_screenshot');
-    return;
-  }

This isn't replaced at all. Is that OK?

Fixed all comments in #61, except last one

This isn't replaced at all. Is that OK?

Hopefully we are saving them as config.

sorry wrong ext for difftext.

#62: 1804380-color_variables_to_config_upgrade-62.patch queued for re-testing.

Seems patch in #61 (1804380-color_variables_to_config_upgrade-62.patch) was failing because of testbot. Now it is ready for review.

+++ b/core/modules/color/color.moduleundefined
@@ -332,23 +340,16 @@ function color_scheme_form_submit($form, &$form_state) {
-  // Don't render the default colorscheme, use the standard theme instead.
-  if (implode(',', color_get_palette($theme, TRUE)) == implode(',', $palette)) {
-    variable_del('color_' . $theme . '_palette');
-    variable_del('color_' . $theme . '_stylesheets');
-    variable_del('color_' . $theme . '_logo');
-    variable_del('color_' . $theme . '_files');
-    variable_del('color_' . $theme . '_screenshot');
-    return;

I don't quite understand this either. Yes, it's config, but we need to delete that too?

Not really visible from the patch context when this is triggered exactly and the comment doesn't tell me much either. Might require some manual testing...

Hi Berdir,

I just did a test around this and here is my findings:

1. In color.inc, we have default color scheme for all default themes has been defined
2. Whenever we save the color settings form of a theme, we create list of variable to save this changes
3. However, if we hit save without any change in color settings form, this code avoids the creation/existence of this set of variables and fetch colors from default (i.e. color.inc).
4. This has been handled in color_get_palette().

  // @todo Default color config should be moved to yaml in the theme.
  return config('color.' . $theme)->get('palette') ?: $palette;

where is $palette is from $theme/color.inc. So adding this code back to remove $config.

When @todo (to move $theme/color.inc variables to config system) fixed, we will have two configs for each color scheme under a theme and after this change the above code would become:

return config('color.' . $theme)->get('palette') ?: config('color.' . $theme)->get('default.palette');

#68: 1804380-color_variables_to_config_upgrade-68.patch queued for re-testing.

#68: 1804380-color_variables_to_config_upgrade-68.patch queued for re-testing.

#68: 1804380-color_variables_to_config_upgrade-68.patch queued for re-testing.

Did a visual review, looks good apart from the following minor issues:

+++ b/core/modules/color/color.moduleundefined
@@ -556,7 +567,9 @@ function _color_render_images($theme, &$info, &$paths, $palette) {
-      variable_set('color_' . $theme . '_screenshot', $image);
+      config('color.'.$theme)
+        ->set('screenshot', $image)

Should have spaces around the .

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/SystemUpgradePathTest.phpundefined
@@ -66,7 +66,7 @@ public function testVariableUpgrade() {
-      // simpletest overrides this configuration with simpletest@example.com.
+      // Simpletest overrides this configuration with simpletest@example.com.

Looks like an unrelated change?

Thank you @Berdir. Updating the patch with fix for #74.

I think that looks good now, @catch's review has been dealt with and the delete/default handling fixed.

Looks good to me now too. Committed/pushed to 8.x.