Part of #1696224: Remove system_settings_form()
system_theme_settings() uses system_settings_form() and needs to use system_config_form().

Files: 
CommentFileSizeAuthor
#95 system_update_8055.patch604 bytesxjm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch system_update_8055.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#92 90-92-interdiff.txt671 bytesalexpott
#92 theme_settings_config_convert-1712250-92.patch33.6 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 55,449 pass(es).
[ View ]
#90 89-90-interdiff.txt2.54 KBalexpott
#90 theme_settings_config_convert-1712250-90.patch33.61 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 55,370 pass(es), 0 fail(s), and 180 exception(s).
[ View ]
#89 theme_settings_config_convert-1712250-89.patch33.24 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 55,406 pass(es).
[ View ]
#89 85-89-interdiff.txt12.32 KBalexpott
#85 theme_settings_config_convert-1712250-85.patch32.6 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 55,295 pass(es).
[ View ]
#83 theme_settings_config_convert-1712250-83.patch32.55 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 54,424 pass(es).
[ View ]
#83 interdiff.txt623 bytesAlbert Volkman
#80 1712250_80.patch32.56 KBchx
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#80 interdiff.txt852 byteschx
#78 1712250-config-theme-settings-78.patch31.62 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#77 1712250-config-theme-settings-77.patch31.52 KBcam8001
PASSED: [[SimpleTest]]: [MySQL] 52,260 pass(es).
[ View ]
#66 1712250-config-theme-settings-66.patch31.52 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#65 1712250-65.drupal8.config-theme.patch31.52 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 49,699 pass(es).
[ View ]
#62 1712250-62.drupal8.config-theme.patch31.46 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1712250-62.drupal8.config-theme.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#59 1712250-59.drupal8.config-theme.patch31.47 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#55 48-55-interdiff.txt5.75 KBalexpott
#55 1712250-55.drupal8.config-theme.patch31.47 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#52 1712250-51.drupal8.config-theme.patch24.93 KBcam8001
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#50 1712250-50.drupal8.config-theme.patch24.93 KBcam8001
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#48 46-48-interdiff.txt7.09 KBalexpott
#48 1712250-48.drupal8.config-theme.patch29.78 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 48,800 pass(es).
[ View ]
#46 1712250-46.drupal8.config-theme.patch29.47 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 48,780 pass(es), 0 fail(s), and 10 exception(s).
[ View ]
#44 1712250-44.drupal8.config-theme.patch29.89 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]
#42 40-42-interdiff.txt8.4 KBalexpott
#42 1712250-42.drupal8.config-theme.patch27.97 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 41,251 pass(es).
[ View ]
#40 1712250-40.drupal8.config-theme.patch30.4 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 41,248 pass(es), 1 fail(s), and 63 exception(s).
[ View ]
#36 1712250-36.drupal8.config-theme.patch27.14 KBn3or
PASSED: [[SimpleTest]]: [MySQL] 40,365 pass(es).
[ View ]
#34 1712250-34.drupal8.config-theme.patch26.55 KBn3or
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1712250-34.drupal8.config-theme.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#34 interdiff.txt8.5 KBn3or
#29 27-29-interdiff.txt626 bytesalexpott
#29 1712250-29.drupal8.config-theme.patch26.56 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1712250-29.drupal8.config-theme.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#27 1712250-27.drupal8.config-theme.patch26.56 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 39,899 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#27 23-27-interdiff.txt8.52 KBalexpott
#23 1712250-23.drupal8.config-theme.patch24.66 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 39,896 pass(es).
[ View ]
#21 19-21-interdiff.txt2.21 KBalexpott
#21 1712250-21.drupal8.config-theme.patch24.66 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#19 17-19-interdiff.patch24.7 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 17-19-interdiff.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#19 1712250-19.drupal8.config-theme.patch27.26 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 39,829 pass(es).
[ View ]
#17 16-17-interdiff.txt1.57 KBalexpott
#17 1712250-17.drupal8.config-theme.patch56.54 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 39,800 pass(es).
[ View ]
#16 15-16-interdiff.txt1.69 KBalexpott
#16 1712250-16.drupal8.config-theme.patch56.3 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 39,799 pass(es).
[ View ]
#15 14-15-interdiff.txt1.42 KBalexpott
#15 1712250-15.drupal8.config-theme.patch56.37 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 39,800 pass(es).
[ View ]
#14 12-14-interdiff.txt589 bytesalexpott
#14 1712250-14.drupal8.config-theme.patch56.39 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 39,798 pass(es).
[ View ]
#12 11-12-interdiff.txt27.78 KBalexpott
#12 1712250-12.drupal8.config-theme.patch56.37 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#11 9-11-interdiff.txt5.19 KBalexpott
#11 1712250-11.drupal8.config-theme.patch27.34 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 39,800 pass(es).
[ View ]
#9 6-9-interdiff.txt18.19 KBalexpott
#9 1712250-9.drupal8.config-theme.patch26.69 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 39,779 pass(es), 16 fail(s), and 0 exception(s).
[ View ]
#6 config-theme.6.patch12.82 KBn3or
FAILED: [[SimpleTest]]: [MySQL] 39,783 pass(es), 16 fail(s), and 0 exception(s).
[ View ]
#6 interdiff.txt5.23 KBn3or
#5 config-theme.5.patch12.87 KBn3or
FAILED: [[SimpleTest]]: [MySQL] 39,786 pass(es), 16 fail(s), and 0 exception(s).
[ View ]
#5 interdiff.txt5.69 KBn3or
#3 config-theme.3.patch12.62 KBn3or
FAILED: [[SimpleTest]]: [MySQL] 40,005 pass(es), 4 fail(s), and 291 exception(s).
[ View ]

Comments

Component:theme system» system.module

Created #1712352: Configuration system does not support themes -- this issue/patch will ultimately be blocked on that, but I think we can already move forward to ~99% in the meantime here. @n3or knows how and actively works on this issue.

Status:Active» Needs review
StatusFileSize
new12.62 KB
FAILED: [[SimpleTest]]: [MySQL] 40,005 pass(es), 4 fail(s), and 291 exception(s).
[ View ]

First patch version.

This patch includes a system.theme.yml containing the global theme configuration, the converted system_theme_settings-form and a upgrade path. That means "theme_settings" and "theme_{$theme}_settings" should be converted to the new configuration system.

My patch contains patch of #1712352: Configuration system does not support themes.

Status:Needs review» Needs work

The last submitted patch, config-theme.3.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new5.69 KB
new12.87 KB
FAILED: [[SimpleTest]]: [MySQL] 39,786 pass(es), 16 fail(s), and 0 exception(s).
[ View ]

Current version. Need a full simpletest-run.

StatusFileSize
new5.23 KB
new12.82 KB
FAILED: [[SimpleTest]]: [MySQL] 39,783 pass(es), 16 fail(s), and 0 exception(s).
[ View ]

Debug code in last patch..

Status:Needs review» Needs work

The last submitted patch, config-theme.6.patch, failed testing.

At the moment we are facing some problems in this issue. Theme settings were managed by the theme_get_settings-function in theme.inc. Originally this function merges settings of different sources:
- hard coded default settings
- global settings (-> admin form)
- settings defined in {$theme}.info-file (-> features and settings option)
- theme settings (-> admin form)

Additionally there are other theme specific settings defined by "theme-extensions" (e. g. color in bartik).

Now there is another source: theme config files (-> #1712352: Configuration system does not support themes). In my patch in #6 I tried to replace the admin form settings with the new config code. But the new configuration format isn't compatible to the old format, so I wrote two converters (new config -> old config and reverse). This could be a solution for the moment but not for D8 in general.

I think we have to rethink D8s theme configuration system: We should unify the different sources into the new configuration system.

Status:Needs work» Needs review
Issue tags:-Config novice
StatusFileSize
new26.69 KB
FAILED: [[SimpleTest]]: [MySQL] 39,779 pass(es), 16 fail(s), and 0 exception(s).
[ View ]
new18.19 KB

This patch makes more than a few changes to theme_get_setting.

The function now caches a clone of the a config object that is builds up from system.theme, the theme's info file settings array, some business logic, and the theme's config file. The reason a clone is used to prevent an code calling ->save() on the config object.

This enables it to act as kind of config wrapper - so calls like this work theme_get_setting('features.comment_user_verification')

EDIT: Interdiff is refusing to produce meaningful output :(

Status:Needs review» Needs work

The last submitted patch, 1712250-9.drupal8.config-theme.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new27.34 KB
PASSED: [[SimpleTest]]: [MySQL] 39,800 pass(es).
[ View ]
new5.19 KB

Patch fixes tests.

StatusFileSize
new56.37 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new27.78 KB

Added conversion of admin_theme, theme_default and maintenance_theme to patch.

Status:Needs review» Needs work

The last submitted patch, 1712250-12.drupal8.config-theme.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new56.39 KB
PASSED: [[SimpleTest]]: [MySQL] 39,798 pass(es).
[ View ]
new589 bytes

Fixed broken menu router test

Status:Needs review» Needs work
StatusFileSize
new56.37 KB
PASSED: [[SimpleTest]]: [MySQL] 39,800 pass(es).
[ View ]
new1.42 KB

+++ b/core/includes/theme.incundefined
@@ -1297,21 +1299,28 @@ function theme_get_setting($setting_name, $theme = NULL) {
-    // To add new global settings, add their default values below, and then
-    // add form elements to system_theme_settings() in system.admin.inc.
-    $cache[$theme] = array(
-      'default_logo'                     =>  1,
-      'logo_path'                        =>  '',
-      'default_favicon'                  =>  1,
-      'favicon_path'                     =>  '',
-      // Use the IANA-registered MIME type for ICO files as default.
-      'favicon_mimetype'                 =>  'image/vnd.microsoft.icon',
+    $config = config('system.theme.global');
+    // Provide defaults as function is used before config installed.
+    $default_config = array(
+      'favicon'  => array('use_default' => 1),
+      'features' => array(
+        'comment_user_picture' => 1,
+        'comment_user_verification' => 1,
+        'favicon' => 1,
+        'logo' => 1,
+        'name' => 1,
+        'node_user_picture' => 1,
+        'main_menu' => 1,
+        'secondary_menu' => 1,
+        'slogan' => 1,
+      ),
+      'logo'     => array('use_default' => 1),
     );
-    // Turn on all default features.
-    $features = _system_default_theme_features();
-    foreach ($features as $feature) {
-      $cache[$theme]['toggle_' . $feature] = 1;

$default_config should continue to use _system_default_theme_features()

StatusFileSize
new56.3 KB
PASSED: [[SimpleTest]]: [MySQL] 39,799 pass(es).
[ View ]
new1.69 KB

Reviewing code has shown that the changes to theme_get_setting() need a bit more work. The order in current HEAD merges:

  1. defaults provided by code
  2. settings provided by theme's .info file
  3. global theme settings
  4. theme's settings

Patch in #14 (and the previous patches) changed this unnecessarily. Patch attached fixes this.

Status:Needs work» Needs review
StatusFileSize
new56.54 KB
PASSED: [[SimpleTest]]: [MySQL] 39,800 pass(es).
[ View ]
new1.57 KB

Improved code comments around the part of this patch that is likely to prove the most contentious... where an unsaved config object is used in theme_get_setting().

ok, um... unfortunately, neither @n3or nor me communicated the results of our discussions into this issue... :-/

We actually wanted to leave the theme_default, admin_theme, and maintenance_theme variables alone for this patch, since there is a chance that these config values will have to go into a completely different "bootstrap" config object -- which is something I'm trying to figure out over in #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config

But yeah, thanks for jumping on that challenge! ;)

What I'm missing in the latest patch are the theme-specific configuration objects/files; i.e., bartik.settings.yml, stark.settings.yml, etc.

It should also be noted that the reason why @n3or went the original route of munging theme setting keys from old to new and vice-versa, because the entire theme_get_setting() function should vanish at a later point in time. There will only be one config object (i.e., THEMENAME.settings.yml) and that's it. That's definitely out of scope for this issue though.

It's a bit surprising that it seems you were able to convert all the calls to theme_get_setting() to the new config object/key layout. We expected that to be a major undertaking... so we might as well want to keep that ;)

StatusFileSize
new27.26 KB
PASSED: [[SimpleTest]]: [MySQL] 39,829 pass(es).
[ View ]
new24.7 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 17-19-interdiff.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Okay I've separated out the theme_default, maintenance_theme and admin_theme parts of the patch and will post it to the other issue :)

The reason why there is no bartik.settings.yml and stark.settings.yml is that currently these themes have no default config other what is exposed in the info file. I think it's probably out of scope to this issue to move that from the settings file to a config directory in the theme - but if @sun thinks that it okay - let's go for it. One less array to merge in theme_get_setting() has to be a plus. The theme specific config objects will be created when you save the corresponding theme specific settings form.

Assigned:n3or» Unassigned

I do not really work on this patch at moment.

StatusFileSize
new24.66 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
new2.21 KB

In light of #1712352: Configuration system does not support themes being committed... removed the duplicate parts of the patch.

Status:Needs review» Needs work

The last submitted patch, 1712250-21.drupal8.config-theme.patch, failed testing.

StatusFileSize
new24.66 KB
PASSED: [[SimpleTest]]: [MySQL] 39,896 pass(es).
[ View ]

Doh! Fix hook_update_N function name...

Status:Needs work» Needs review

+++ b/core/includes/theme.inc
@@ -1296,23 +1297,33 @@ function theme_get_setting($setting_name, $theme = NULL) {
+    // Provide defaults as function is used before config installed.
+    $default_config = array(

I think that this $default_config should not longer exist. system.theme.global covers that already.

OTOH, system.theme.global might go away entirely later.

It's a bit hard to find the proper direction to move forward here. I'm trying to think of parallels in module config. Perhaps the $default_config is even correct. It just feels very weird to have default values for all key/value pairs of a config object hard-coded in PHP -- whereas we do not do that anywhere else. Everywhere else, we rely on the config object to exist and contain the default values.

These theme settings objects are special though in that only a small subset of keys is known upfront. There can technically be an unlimited amount of additional theme settings/features/whatever keys; either originating from modules, or from a particular theme itself.

For now, I think I'd lean towards putting the entire $default_config into system.theme.global and eliminating the hard-coded values in PHP. The theme-specific config as well as .info file stuff is then merged into that.

In a later step, we want to figure out how we can eliminate the global theme settings. (which might mean that we remove the UI, but keep the system.theme.global file, sorta as a "template" or baseline for theme-specific settings; i.e., merged only once during installation of a theme).

+++ b/core/includes/theme.inc
@@ -1329,59 +1340,107 @@ function theme_get_setting($setting_name, $theme = NULL) {
+function theme_convert_variables_to_config($old_config) {

AFAICS, this is only used by the module update function now? If so, let's move the code into the update function.

StatusFileSize
new8.52 KB
new26.56 KB
FAILED: [[SimpleTest]]: [MySQL] 39,899 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

I've removed the defaults in code from theme_get_setting(). This exposed an very interesting issue. We currently have no function for importing a specific config file from an already enabled module - eg. the system module in a Drupal 7 to 8 upgrade. This doesn't matter when you use update_variables_to_config() function because this gets the default. But in the case of global settings for templates or theme specific settings there is really no way to map the existing variable using that function. (Additionally what happens when we create a config file that has no correlation in D7 or a module adds a config file after it has been installed?)

So... this patch adds a new function to update.inc called update_7_to_8_install_default_config() to handle this. Unfortunately we can not use config_install_default_config() because this would install all config files not the one added by an update :). Maybe the solution here is to add the ability to pass a specific config name to that function.

Also...

+function theme_convert_variables_to_config($old_config) {

... this function is used in system_theme_settings_submit() to convert the form values into config keys. In the patch attached I've rewritten this function to be a bit more elegant and have renamed it theme_settings_convert_to_config() because that is what it is doing.

Status:Needs review» Needs work

The last submitted patch, 1712250-27.drupal8.config-theme.patch, failed testing.

StatusFileSize
new26.56 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1712250-29.drupal8.config-theme.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new626 bytes

Fix broken shortcut tests.

+++ b/core/includes/theme.incundefined
@@ -1329,62 +1320,107 @@ function theme_get_setting($setting_name, $theme = NULL) {
       // If the theme does not support a particular feature, override the global
       // setting and set the value to NULL.
       if (!empty($theme_object->info['features'])) {
-        foreach ($features as $feature) {
+        foreach ($cache[$theme]->get('features') as $feature => $enabled) {
           if (!in_array($feature, $theme_object->info['features'])) {
-            $cache[$theme]['toggle_' . $feature] = NULL;
+            $cache[$theme]->set('features.' . $feature, NULL);
           }
         }

The error was caused by replacing $features with the list of features from the theme. This actually is the list of default feature supplied by the global setting.

Status:Needs work» Needs review

Status:Needs review» Needs work

+++ b/core/includes/theme.incundefined
@@ -1296,22 +1297,12 @@ function theme_get_setting($setting_name, $theme = NULL) {
+    // Create a config object that will merge $default_config, settings from the
+    // theme's info file, global theme configuration and the theme's
+    // configuration. This allows the use of CMI to manage the nested array of
+    // keys. The config object is never be saved. Potentially saving it could

Last two sentences don't make sense. "is never be saved". Also "Potentially saving it could offer" should probably read "Saving it could potentially offer..."

+++ b/core/includes/update.incundefined
@@ -1097,6 +1097,40 @@ function update_variables_to_config($config_name, array $variable_map) {
+  // This potentially loads an existing configuration object, in case another

I don't believe you need the comma before "in case" here.

Status:Needs work» Needs review
Issue tags:-API change, -Configuration system

Status:Needs review» Needs work
Issue tags:+API change, +Configuration system

The last submitted patch, 1712250-29.drupal8.config-theme.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.5 KB
new26.55 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1712250-34.drupal8.config-theme.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Made patch compatible to current D8 core and did changes suggested in #31.

Interdiff isn't quite meaningful.

Status:Needs review» Needs work

The last submitted patch, 1712250-34.drupal8.config-theme.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new27.14 KB
PASSED: [[SimpleTest]]: [MySQL] 40,365 pass(es).
[ View ]

Some mismatch in comment.module. Additionally forgot yml-file.

Status:Needs review» Needs work

All in all, it seems like a 1:1 conversion of old stuff to CMI. My one question that I don't have the answer to is if there are some things that could be changed in a way to take advantage of the benefits of CMI. For example, is there a way the theme_get_setting function could be replaced with say a Theme Config Object.

Otherwise, the patch looks pretty good. Here are some comments I have:

+++ b/core/includes/update.inc
@@ -1097,6 +1097,40 @@ function update_variables_to_config($config_name, array $variable_map) {
+function update_7_to_8_install_default_config($type, $config_name) {

I'm wondering if this function belongs in this patch for this issue. It's not a bad idea to have a utility function to do this, but should it be decided in a different issue?

+++ b/core/modules/system/system.admin.inc
@@ -693,8 +693,13 @@ function system_theme_settings_submit($form, &$form_state) {
@@ -728,8 +733,7 @@ function system_theme_settings_submit($form, &$form_state) {
@@ -728,8 +733,7 @@ function system_theme_settings_submit($form, &$form_state) {
     $values['favicon_mimetype'] = file_get_mimetype($values['favicon_path']);
   }
-  variable_set($key, $values);
-  drupal_set_message(t('The configuration options have been saved.'));
+  theme_settings_convert_to_config($values, $config)->save();

I don't think a utility conversion to convert to config should be used in a submit hook.

Thank you for your review!

For example, is there a way the theme_get_setting function could be replaced with say a Theme Config Object.

In fact this is impossible, because we have to merge config properties from different sources. CMI neither supports default values nor merge of different objects (at least at the moment). Or can you imagine some concrete solution? :)

I'm wondering if this function belongs in this patch for this issue. It's not a bad idea to have a utility function to do this, but should it be decided in a different issue?

If I remember correctly you won't get all the tests passed without this function because upgrade path tests fail. Therefore between this issue and the 7to8 update code is a strong dependency. In my humble opinion this issue is the right place.

I don't think a utility conversion to convert to config should be used in a submit hook.

In best case the mentioned utility function won't exist in final D8. But the removal of this function has to be conducted in a follow up issue. We have to think about how far it is really necessary to find an alternative solution to avoid using it within a submit hook.

Thanks everyone for moving forward! :)

Only had a quick look at the latest patch - here's what I think:

+++ b/core/includes/theme.inc
@@ -9,6 +9,8 @@
+use Drupal\Component\Utility\NestedArray;
+use Drupal\Core\Config\Config;
@@ -1319,62 +1309,107 @@ function theme_get_setting($setting_name, $theme = NULL) {
+function theme_settings_convert_to_config(array $theme_settings, Config $config) {

We still need to move this function into system.install, whereas the function name should probably follow the field update function naming; e.g., system_theme_update_settings_to_config_8000()

+++ b/core/includes/theme.inc
@@ -1286,22 +1287,11 @@ function theme_get_setting($setting_name, $theme = NULL) {
+    // keys. Saving it could potentially offer performance improvements.
+    $cache[$theme] = config('cache.theme.' . $theme);

The last sentence has to be removed and could only be replaced with

// @todo Cache that shit.

;)

+++ b/core/includes/update.inc
@@ -1097,6 +1097,40 @@ function update_variables_to_config($config_name, array $variable_map) {
/**
+ * Saves a configuration file into the active store.
+ *
+ * Provide a generalised method to save default configuration object as part of
+ * an update from Drupal 7 to Drupal 8's configuration management system.
...
+function update_7_to_8_install_default_config($type, $config_name) {
...
+  $config->setData($file->read($config_name))->save();

I don't really get what this function is doing :-/

Also, please note that update_variables_to_config() is "re-entrant" by design; i.e., it is possible to call it multiple times to act on the identical target config object.

It is perfectly possible that we're missing a $type differentiation in update_variables_to_config() or have to improve its signature in different ways.

In any case, we shouldn't be using ->setData() in 99% of all cases.

+++ b/core/modules/system/system.admin.inc
@@ -728,8 +733,7 @@ function system_theme_settings_submit($form, &$form_state) {
+  theme_settings_convert_to_config($values, $config)->save();

Oh, I see. We might have to change all the keys in the theme settings form definition then... :-/

If we want to make that a follow-up issue, then I believe it has to be at least major, and we should precisely state a @todo D8: Remove this function after updating the theme settings form to use the new configuration object keys.

Status:Needs work» Needs review
StatusFileSize
new30.4 KB
FAILED: [[SimpleTest]]: [MySQL] 41,248 pass(es), 1 fail(s), and 63 exception(s).
[ View ]

Rerolled this patch :)

Okay... the recent changes to CMI in light of canonical files and the installStorage made the reroll more complex as the idea of the unsaved config object to merge all the theme settings into needed quite a bit of tweaking - the new InstallStorage explicit does not allow you to create config object with no files to load :)

The patch attached continues in the vain of replacing the functionality of the current system and not cleaning up the runtime merge of global settings and theme specific settings. This should be attempted in a followup patch to address @sun's todo of "cache that shit"

I have also removed the update_7_to_8_install_default_config() function and made update_variables_to_config() do this work. I'm not 100% convinved by this change as in this use case update_variables_to_config() is not doing what is claims to. It is just installing the default supplied config for system.theme.global, bartik.settings and seven.settings - this happens in system_update_8020().

Interdiff was impossible due to the reroll :(

One of the trickest parts of the patch to test is when a theme does not support a default feature. There is currently no simpletest in core for this functionality. To test:

  1. Apply this patch :)
  2. Install drupal with standard profile
  3. Edit bartik.info and add line features[] = logo
  4. Clear cache
  5. View home page - should see a lot of stuff removed but the logo still there
  6. Edit bartik.info and add line features[] = wow
  7. Clear cache
  8. View home page - should see a page now with no logo
  9. Edit bartik.info and remove line that start features[]...
  10. Clear cache
  11. View home page - should see a page now with no logo

Status:Needs review» Needs work

The last submitted patch, 1712250-40.drupal8.config-theme.patch, failed testing.

StatusFileSize
new27.97 KB
PASSED: [[SimpleTest]]: [MySQL] 41,251 pass(es).
[ View ]
new8.4 KB

Okay... damn you themes that can be used and not installed or even enabled - come on #1067408: Themes need an installation status!

Rerolled patch to add the shortlink setting back into theme .info files... moving this to the theme's YAML file depends on #1067408: Themes need an installation status.

Re-introduced update_7_to_8_install_default_config() it really is doing something different from update_variables_to_config() - I've updated the docs to try to make this more apparent. And the code to make update_variables_to_config() is fugly.

Also I've moved shortcut_module_link to just a plain setting rather than in the features key. This is because what this is indicating is that the theme supports shortcut links not that it has it enabled. This suggests that we should create a features.supports key in the [theme].settings.yml file which indicates what features are supported and a features.enabled to indicate what features (that the theme supports) are enabled. But this sounds like it is followup territory to me :)

Also discovered that an empty YAML file does no just return an empty config object... it creates an error. Not 100% sure that this is the correct behaviour.

Go testbot go!

Status:Needs work» Needs review

Now go test bot go!

StatusFileSize
new29.89 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]

Rerolled...

Status:Needs review» Needs work

The last submitted patch, 1712250-44.drupal8.config-theme.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new29.47 KB
FAILED: [[SimpleTest]]: [MySQL] 48,780 pass(es), 0 fail(s), and 10 exception(s).
[ View ]

Doh.. htaccess change snuck in...

Status:Needs review» Needs work

The last submitted patch, 1712250-46.drupal8.config-theme.patch, failed testing.

StatusFileSize
new29.78 KB
PASSED: [[SimpleTest]]: [MySQL] 48,800 pass(es).
[ View ]
new7.09 KB

New patch that:

  • Creates a new ThemeSettings object loosely based on the Config object's use of NestedArray so that we don't have to abuse the config object. Doing this means at some point we could consider dumping or caching this object so that we don't have to rebuild the theme setting array on every request. And potentially we could just swap it over to a config object object.
  • Fixes the borked merge that resulted in the exceptions in #46.

Status:Needs work» Needs review

Test please!

StatusFileSize
new24.93 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]

Reroll.

Status:Needs review» Needs work

The last submitted patch, 1712250-50.drupal8.config-theme.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new24.93 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Yikes, needed to bump the system update number.

Status:Needs review» Needs work

The last submitted patch, 1712250-51.drupal8.config-theme.patch, failed testing.

Weird, installer works locally both in drush and via web?

StatusFileSize
new31.47 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
new5.75 KB

Looks like you missed out the new YAML files :)

Patch attached does the reroll and includes the fix from http://drupal.org/node/1864290 but using config instead of variable_set.

Interdiff to last working patch and it's a bit weird as this is a reroll...

Status:Needs work» Needs review

Bot... please :)

Status:Needs review» Needs work
Issue tags:+API change, +Configuration system

The last submitted patch, 1712250-55.drupal8.config-theme.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new31.47 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]

Updating just update_N

Status:Needs review» Needs work
Issue tags:+API change, +Configuration system

The last submitted patch, 1712250-59.drupal8.config-theme.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new31.46 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1712250-62.drupal8.config-theme.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Updating update_N

Status:Needs review» Needs work
Issue tags:+API change, +Configuration system

The last submitted patch, 1712250-62.drupal8.config-theme.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Config novice
StatusFileSize
new31.52 KB
PASSED: [[SimpleTest]]: [MySQL] 49,699 pass(es).
[ View ]

reroll

StatusFileSize
new31.52 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]

Yet another reroll, yet another update_n bump and a tiny change in comment.module. Is this thing good to go?

Issue tags:+Needs tests

I think we need some tests of the upgrade path...

+++ b/core/lib/Drupal/Core/Theme/ThemeSettings.php
@@ -0,0 +1,146 @@
+ * Definition of Drupal\Core\Theme\ThemeSettings.

I do not understand why this ThemeSettings class/object is necessary — it seems to duplicate a Config object?

The reason the ThemeSettings class exists is to manage the results of merging of 3 config objects... system defaults, global theme settings, theme settings... plus also information from the theme's info file. In early patches we just created a pseudo-config object that was never saved... but this felt risky - what happens if somehow another config object gets saved with the pseudo object's name?

I forsee a followup patch where we use the php writer to save the ThemeSettings object so we don't have to do the merge on every single Drupal request.

If this is still an issue on February 27 and needs some help (but why it's tagged config novice then...?) then assign it to me.

Issue tags:-Config novice

Re-testing... Any more thoughts? Or good to RTBC?

Status:Needs review» Needs work
Issue tags:+Needs tests, +API change, +Configuration system

The last submitted patch, 1712250-config-theme-settings-66.patch, failed testing.

Needs reroll for system_update_8049().

Status:Needs work» Needs review
StatusFileSize
new31.52 KB
PASSED: [[SimpleTest]]: [MySQL] 52,260 pass(es).
[ View ]

Updated to system_update_8050().

StatusFileSize
new31.62 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Re-rolling...

Status:Needs review» Needs work

The last submitted patch, 1712250-config-theme-settings-78.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new852 bytes
new32.56 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]

#80: 1712250_80.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs tests, +API change, +Configuration system

The last submitted patch, 1712250_80.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new623 bytes
new32.55 KB
PASSED: [[SimpleTest]]: [MySQL] 54,424 pass(es).
[ View ]

Re-roll against current head.

Status:Needs review» Needs work

Needs a re-roll to mop up the new variable_set in #1972242: User compact view mode is not configured in minimal

Status:Needs work» Needs review
StatusFileSize
new32.6 KB
PASSED: [[SimpleTest]]: [MySQL] 55,295 pass(es).
[ View ]

No problem.

Status:Needs review» Reviewed & tested by the community

I looked through this and it looks good to my eyes, although I don't have an enormous amount of experience with the theme system. However it also appears that a lot of people have looked it over in the last month and there haven't been a significant amount of issues found. So I'm going to RTBC.

Note that this is the last instance of system_settings_form() left in core. When this is committed then #1918684: Remove system_settings_form() and related functions/documentation should be un-postponed.

+++ b/core/includes/theme.incundefined
@@ -1423,62 +1411,111 @@ function theme_get_setting($setting_name, $theme = NULL) {
+ * @todo D8: Move this function to update.inc after updating the theme settings

Is there an issue for this? Why not do it here?

+++ b/core/includes/update.incundefined
@@ -1480,7 +1480,42 @@ function update_config_manifest_add($config_prefix, array $ids) {
+/**
+ * @defgroup update-api-7.x-to-8.x Update versions of API functions
+ * @{
+ * Functions similar to normal API function but not firing hooks.

Shouldn't this be @addtogroup or similar?

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new12.32 KB
new33.24 KB
PASSED: [[SimpleTest]]: [MySQL] 55,406 pass(es).
[ View ]

@catch, #88

  • I've removed the @todo as the re-use of these function in the system theme settings form submit and upgrade path seems quite elegant.
  • the whole @defgroup was spurious so I've removed it.

The patch attached also:

  • Adds commentary to system_update_8054() to explain why we're not deleting the variables... I think this has the potential to be quite complicated... I'm going to have a look at how a theme like Zen or Adaptivetheme (ab)uses system_settings_form in 7.x. Especially as afaik themes can't have hook_update_N functions.
  • Uses Drupal::config over config()
  • Adds a mergeData method to ThemeSettings to reduce code in theme_get_setting()
  • Updates patch for latest coding standards

StatusFileSize
new33.61 KB
FAILED: [[SimpleTest]]: [MySQL] 55,370 pass(es), 0 fail(s), and 180 exception(s).
[ View ]
new2.54 KB

The whole contrib/custom theme issue is even more fun than I thought... mulling this over with @catch and @swentel... has lead me to the following realisation. If you following the major upgrade instructions in UPGRADE.txt then the call to list_themes() in the new system_update_8054() can not possibly return anything other than core themes.. This means that the patch attached is acceptable.

If contrib themes or custom theme want to provide an upgrade path then they will need to provide a helper module to do the upgrade... and solving the existing of what happens to contrib/custom themes during a major upgrade is way beyond the scope of this issue.

Status:Needs review» Needs work

The last submitted patch, theme_settings_config_convert-1712250-90.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new33.6 KB
PASSED: [[SimpleTest]]: [MySQL] 55,449 pass(es).
[ View ]
new671 bytes

How silly of me...

Status:Needs review» Reviewed & tested by the community

Concerns from catch are answered. Was RTBC before, code+comments looks ok. Lets end these config conversion issues :)

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

Title:Convert theme settings to configuration systemHEAD BROKEN: Convert theme settings to configuration system
Category:task» bug
Priority:Normal» Critical
Status:Fixed» Reviewed & tested by the community
StatusFileSize
new604 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch system_update_8055.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

So it looks like this is committed:
http://drupalcode.org/project/drupal.git/commit/31513883d9b15c997c140f16...

But, HEAD is broken because the update hook name is duplicated. Attached fixes.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, system_update_8055.patch, failed testing.

Status:Needs work» Fixed

Just sorted this directly myself, should be OK now.

Title:HEAD BROKEN: Convert theme settings to configuration systemConvert theme settings to configuration system

Reverting title, scared me :)

Priority:Critical» Normal

not critical anymore

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