Created from #1775842: [meta] Convert all variables to state and/or config systems.

Not 100% sure if this is config or state, creating the issue so we can work on it at the melbourne sprint tomorrow.

color_ . $theme . _files color.module
color_ . $theme . _palette color.module
color_ . $theme_key . _logo color.module
color_ . $theme_key . _stylesheets color.module

Files: 
CommentFileSizeAuthor
#75 1804380-color_variables_to_config_upgrade-75.patch12.18 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 50,994 pass(es).
[ View ]
#75 1804380-diff-68-75.txt1.41 KBvijaycs85
#68 1804380-color_variables_to_config_upgrade-68.patch12.67 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 50,746 pass(es).
[ View ]
#68 1804380-62-68-diff.txt675 bytesvijaycs85
#63 1804380-difftxt-57-62.txt4.57 KBvijaycs85
#62 1804380-color_variables_to_config_upgrade-62.patch12.75 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 50,486 pass(es).
[ View ]
#62 1804380-difftxt-57-62.patch4.57 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1804380-difftxt-57-62.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#57 1804380-57-color_variables_to_config_upgrade.patch12.34 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 49,436 pass(es).
[ View ]
#56 1804380_color_variables_to_config_upgrade_56.patch12.29 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 49,250 pass(es).
[ View ]
#56 1804380-diff-54-56.txt761 bytesvijaycs85
#54 1804380_color_variables_to_config_upgrade_54.patch12.23 KB8thom
PASSED: [[SimpleTest]]: [MySQL] 49,239 pass(es).
[ View ]
#52 1804380_color_variables_to_config_upgrade_52.patch12.23 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 49,127 pass(es).
[ View ]
#50 1804380_color_variables_to_config_upgrade_50.patch12.31 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 49,251 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
#47 1804380_color_variables_to_config_upgrade_47.patch12.14 KBmiro_dietiker
PASSED: [[SimpleTest]]: [MySQL] 48,812 pass(es).
[ View ]
#42 color_variables_to_config-1804380-42.patch12.2 KB8thom
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch color_variables_to_config-1804380-42.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#39 1804380_color_variables_to_config_upgrade_37.patch12.2 KBmiro_dietiker
PASSED: [[SimpleTest]]: [MySQL] 48,203 pass(es).
[ View ]
#37 1804380_color_variables_to_config_upgrade_35.patch12.19 KBmiro_dietiker
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/tests/upgrade/drupal-7.system.database.php.
[ View ]
#35 1804380_color_variables_to_config_upgrade_35.patch12.04 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 48,199 pass(es), 0 fail(s), and 8 exception(s).
[ View ]
#29 1804380_color_variables_to_config_upgrade_29.patch12.3 KBmiro_dietiker
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1804380_color_variables_to_config_upgrade_29.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#27 1804380_color_variables_to_config_upgrade_26.patch7.21 KBmiro_dietiker
PASSED: [[SimpleTest]]: [MySQL] 47,741 pass(es).
[ View ]
#25 1804380_color_variables_to_config_upgrade_25.patch7.21 KBmiro_dietiker
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
#23 1804380_color_variables_to_config_upgrade_22.patch7.12 KBmiro_dietiker
FAILED: [[SimpleTest]]: [MySQL] 47,340 pass(es), 12 fail(s), and 12 exception(s).
[ View ]
#21 1804380_color_variables_to_config_upgrade_21.patch7.12 KBmiro_dietiker
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/color/color.install.
[ View ]
#16 1804380_color_variables_to_config_upgrade.patch6.86 KBmiro_dietiker
FAILED: [[SimpleTest]]: [MySQL] 47,332 pass(es), 12 fail(s), and 12 exception(s).
[ View ]
#7 color_variables_to_config-1804380-7.patch6.09 KB8thom
PASSED: [[SimpleTest]]: [MySQL] 46,446 pass(es).
[ View ]
#3 color_variables_to_config-1804380-3.patch6.02 KB8thom
PASSED: [[SimpleTest]]: [MySQL] 46,481 pass(es).
[ View ]
#2 color_variables_to_config-1804380-2.patch6.05 KB8thom
PASSED: [[SimpleTest]]: [MySQL] 42,127 pass(es).
[ View ]

Comments

8thom’s picture

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

8thom’s picture

Status:Active» Needs review
StatusFileSize
new6.05 KB
PASSED: [[SimpleTest]]: [MySQL] 42,127 pass(es).
[ View ]

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.

8thom’s picture

StatusFileSize
new6.02 KB
PASSED: [[SimpleTest]]: [MySQL] 46,481 pass(es).
[ View ]

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

8thom’s picture

Assigned:Unassigned» 8thom
Berdir’s picture

+++ 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?

Berdir’s picture

8thom’s picture

StatusFileSize
new6.09 KB
PASSED: [[SimpleTest]]: [MySQL] 46,446 pass(es).
[ View ]

Berdir thanks for the review.

Fixed up indentation.

Status:Needs review» Needs work
Issue tags:-Configuration system, -melb-code-sprint-2012-10

The last submitted patch, color_variables_to_config-1804380-7.patch, failed testing.

8thom’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, color_variables_to_config-1804380-7.patch, failed testing.

aspilicious’s picture

Status:Needs work» Needs review
Issue tags:+Configuration system, +melb-code-sprint-2012-10
aspilicious’s picture

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

heyrocker’s picture

Status:Needs review» Needs work
8thom’s picture

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

8thom’s picture

Status:Needs work» Needs review
miro_dietiker’s picture

StatusFileSize
new6.86 KB
FAILED: [[SimpleTest]]: [MySQL] 47,332 pass(es), 12 fail(s), and 12 exception(s).
[ View ]

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

Still no .yml file(s) present.

8thom’s picture

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.

Status:Needs review» Needs work

The last submitted patch, 1804380_color_variables_to_config_upgrade.patch, failed testing.

miro_dietiker’s picture

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.

Berdir’s picture

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

miro_dietiker’s picture

Status:Needs work» Needs review
Issue tags:+Needs tests
StatusFileSize
new7.12 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/color/color.install.
[ View ]

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.

Status:Needs review» Needs work
Issue tags:-Needs tests

The last submitted patch, 1804380_color_variables_to_config_upgrade_21.patch, failed testing.

miro_dietiker’s picture

Status:Needs work» Needs review
StatusFileSize
new7.12 KB
FAILED: [[SimpleTest]]: [MySQL] 47,340 pass(es), 12 fail(s), and 12 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1804380_color_variables_to_config_upgrade_22.patch, failed testing.

miro_dietiker’s picture

Status:Needs work» Needs review
StatusFileSize
new7.21 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 1804380_color_variables_to_config_upgrade_25.patch, failed testing.

miro_dietiker’s picture

Status:Needs work» Needs review
StatusFileSize
new7.21 KB
PASSED: [[SimpleTest]]: [MySQL] 47,741 pass(es).
[ View ]

Grr :-)

miro_dietiker’s picture

Issue tags:+Needs tests

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.

miro_dietiker’s picture

Issue tags:-Needs tests
StatusFileSize
new12.3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1804380_color_variables_to_config_upgrade_29.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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

Status:Needs review» Needs work

The last submitted patch, 1804380_color_variables_to_config_upgrade_29.patch, failed testing.

Berdir’s picture

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.

Berdir’s picture

Status:Needs work» Needs review
Issue tags:-Configuration system, -melb-code-sprint-2012-10
miro_dietiker’s picture

Status:Needs review» Needs work
Issue tags:+Configuration system, +melb-code-sprint-2012-10

The last submitted patch, 1804380_color_variables_to_config_upgrade_29.patch, failed testing.

swentel’s picture

Status:Needs work» Needs review
StatusFileSize
new12.04 KB
FAILED: [[SimpleTest]]: [MySQL] 48,199 pass(es), 0 fail(s), and 8 exception(s).
[ View ]

Reroll to apply against head.

Status:Needs review» Needs work

The last submitted patch, 1804380_color_variables_to_config_upgrade_35.patch, failed testing.

miro_dietiker’s picture

Status:Needs work» Needs review
StatusFileSize
new12.19 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/tests/upgrade/drupal-7.system.database.php.
[ View ]

Rerolled.

Status:Needs review» Needs work

The last submitted patch, 1804380_color_variables_to_config_upgrade_35.patch, failed testing.

miro_dietiker’s picture

StatusFileSize
new12.2 KB
PASSED: [[SimpleTest]]: [MySQL] 48,203 pass(es).
[ View ]

Grrrr, merged buggy :-)

miro_dietiker’s picture

Status:Needs work» Needs review
aspilicious’s picture

Status:Needs review» Needs work
+++ 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

8thom’s picture

Status:Needs work» Needs review
StatusFileSize
new12.2 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch color_variables_to_config-1804380-42.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixed up todo & trailing white space

miro_dietiker’s picture

Status:Needs review» Needs work

The last submitted patch, color_variables_to_config-1804380-42.patch, failed testing.

8thom’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+Configuration system, +melb-code-sprint-2012-10

The last submitted patch, color_variables_to_config-1804380-42.patch, failed testing.

miro_dietiker’s picture

Status:Needs work» Needs review
StatusFileSize
new12.14 KB
PASSED: [[SimpleTest]]: [MySQL] 48,812 pass(es).
[ View ]

Reroll.

aspilicious’s picture

Status:Needs review» Reviewed & tested by the community

Good to go!

catch’s picture

Status:Reviewed & tested by the community» Needs work
+++ 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

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new12.31 KB
FAILED: [[SimpleTest]]: [MySQL] 49,251 pass(es), 9 fail(s), and 0 exception(s).
[ View ]

fixes for review comment #49.

Status:Needs review» Needs work

The last submitted patch, 1804380_color_variables_to_config_upgrade_50.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new12.23 KB
PASSED: [[SimpleTest]]: [MySQL] 49,127 pass(es).
[ View ]

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

ACF’s picture

Status:Needs review» Needs work

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.

8thom’s picture

StatusFileSize
new12.23 KB
PASSED: [[SimpleTest]]: [MySQL] 49,239 pass(es).
[ View ]

capitalised like - comment #53

8thom’s picture

Status:Needs work» Needs review
vijaycs85’s picture

StatusFileSize
new761 bytes
new12.29 KB
PASSED: [[SimpleTest]]: [MySQL] 49,250 pass(es).
[ View ]

updating query to escape underscore.

vijaycs85’s picture

StatusFileSize
new12.34 KB
PASSED: [[SimpleTest]]: [MySQL] 49,436 pass(es).
[ View ]

Re-rolling...

Status:Needs review» Needs work
Issue tags:-Configuration system, -melb-code-sprint-2012-10

The last submitted patch, 1804380-57-color_variables_to_config_upgrade.patch, failed testing.

Tor Arne Thune’s picture

Status:Needs work» Needs review
Issue tags:+Configuration system, +melb-code-sprint-2012-10
aspilicious’s picture

Status:Needs review» Reviewed & tested by the community

Can't find any problems...

catch’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:-melb-code-sprint-2012-10

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?

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new4.57 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1804380-difftxt-57-62.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new12.75 KB
PASSED: [[SimpleTest]]: [MySQL] 50,486 pass(es).
[ View ]

Fixed all comments in #61, except last one

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

Hopefully we are saving them as config.

vijaycs85’s picture

StatusFileSize
new4.57 KB

sorry wrong ext for difftext.

Status:Needs review» Needs work
Issue tags:-Configuration system

The last submitted patch, 1804380-difftxt-57-62.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
Issue tags:+Configuration system
vijaycs85’s picture

Assigned:8thom» Unassigned

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

Berdir’s picture

+++ 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...

vijaycs85’s picture

StatusFileSize
new675 bytes
new12.67 KB
PASSED: [[SimpleTest]]: [MySQL] 50,746 pass(es).
[ View ]

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().

<?php
 
// @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:

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

Status:Needs review» Needs work
Issue tags:-Configuration system

The last submitted patch, 1804380-color_variables_to_config_upgrade-68.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
aspilicious’s picture

Status:Needs review» Needs work

The last submitted patch, 1804380-color_variables_to_config_upgrade-68.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
Issue tags:+Configuration system
Berdir’s picture

Status:Needs review» Needs work

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?

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new1.41 KB
new12.18 KB
PASSED: [[SimpleTest]]: [MySQL] 50,994 pass(es).
[ View ]

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

Berdir’s picture

Status:Needs review» Reviewed & tested by the community

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

catch’s picture

Status:Reviewed & tested by the community» Fixed

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

Automatically closed -- issue fixed for 2 weeks with no activity.