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

CommentFileSizeAuthor
#95 system_update_8055.patch604 bytesxjm
#92 90-92-interdiff.txt671 bytesalexpott
#92 theme_settings_config_convert-1712250-92.patch33.6 KBalexpott
#90 89-90-interdiff.txt2.54 KBalexpott
#90 theme_settings_config_convert-1712250-90.patch33.61 KBalexpott
#89 theme_settings_config_convert-1712250-89.patch33.24 KBalexpott
#89 85-89-interdiff.txt12.32 KBalexpott
#85 theme_settings_config_convert-1712250-85.patch32.6 KBAlbert Volkman
#83 theme_settings_config_convert-1712250-83.patch32.55 KBAlbert Volkman
#83 interdiff.txt623 bytesAlbert Volkman
#80 1712250_80.patch32.56 KBchx
#80 interdiff.txt852 byteschx
#78 1712250-config-theme-settings-78.patch31.62 KBvijaycs85
#77 1712250-config-theme-settings-77.patch31.52 KBCameron Tod
#66 1712250-config-theme-settings-66.patch31.52 KBgdd
#65 1712250-65.drupal8.config-theme.patch31.52 KBjibran
#62 1712250-62.drupal8.config-theme.patch31.46 KBvijaycs85
#59 1712250-59.drupal8.config-theme.patch31.47 KBvijaycs85
#55 48-55-interdiff.txt5.75 KBalexpott
#55 1712250-55.drupal8.config-theme.patch31.47 KBalexpott
#52 1712250-51.drupal8.config-theme.patch24.93 KBCameron Tod
#50 1712250-50.drupal8.config-theme.patch24.93 KBCameron Tod
#48 46-48-interdiff.txt7.09 KBalexpott
#48 1712250-48.drupal8.config-theme.patch29.78 KBalexpott
#46 1712250-46.drupal8.config-theme.patch29.47 KBalexpott
#44 1712250-44.drupal8.config-theme.patch29.89 KBalexpott
#42 40-42-interdiff.txt8.4 KBalexpott
#42 1712250-42.drupal8.config-theme.patch27.97 KBalexpott
#40 1712250-40.drupal8.config-theme.patch30.4 KBalexpott
#36 1712250-36.drupal8.config-theme.patch27.14 KBn3or
#34 1712250-34.drupal8.config-theme.patch26.55 KBn3or
#34 interdiff.txt8.5 KBn3or
#29 27-29-interdiff.txt626 bytesalexpott
#29 1712250-29.drupal8.config-theme.patch26.56 KBalexpott
#27 1712250-27.drupal8.config-theme.patch26.56 KBalexpott
#27 23-27-interdiff.txt8.52 KBalexpott
#23 1712250-23.drupal8.config-theme.patch24.66 KBalexpott
#21 19-21-interdiff.txt2.21 KBalexpott
#21 1712250-21.drupal8.config-theme.patch24.66 KBalexpott
#19 17-19-interdiff.patch24.7 KBalexpott
#19 1712250-19.drupal8.config-theme.patch27.26 KBalexpott
#17 16-17-interdiff.txt1.57 KBalexpott
#17 1712250-17.drupal8.config-theme.patch56.54 KBalexpott
#16 15-16-interdiff.txt1.69 KBalexpott
#16 1712250-16.drupal8.config-theme.patch56.3 KBalexpott
#15 14-15-interdiff.txt1.42 KBalexpott
#15 1712250-15.drupal8.config-theme.patch56.37 KBalexpott
#14 12-14-interdiff.txt589 bytesalexpott
#14 1712250-14.drupal8.config-theme.patch56.39 KBalexpott
#12 11-12-interdiff.txt27.78 KBalexpott
#12 1712250-12.drupal8.config-theme.patch56.37 KBalexpott
#11 9-11-interdiff.txt5.19 KBalexpott
#11 1712250-11.drupal8.config-theme.patch27.34 KBalexpott
#9 6-9-interdiff.txt18.19 KBalexpott
#9 1712250-9.drupal8.config-theme.patch26.69 KBalexpott
#6 config-theme.6.patch12.82 KBn3or
#6 interdiff.txt5.23 KBn3or
#5 config-theme.5.patch12.87 KBn3or
#5 interdiff.txt5.69 KBn3or
#3 config-theme.3.patch12.62 KBn3or
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Component: theme system » system.module
sun’s picture

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.

n3or’s picture

Status: Active » Needs review
FileSize
12.62 KB

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.

n3or’s picture

Status: Needs work » Needs review
FileSize
5.69 KB
12.87 KB

Current version. Need a full simpletest-run.

n3or’s picture

FileSize
5.23 KB
12.82 KB

Debug code in last patch..

Status: Needs review » Needs work

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

n3or’s picture

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.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: -Config novice
FileSize
26.69 KB
18.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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
27.34 KB
5.19 KB

Patch fixes tests.

alexpott’s picture

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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
56.39 KB
589 bytes

Fixed broken menu router test

alexpott’s picture

Status: Needs review » Needs work
FileSize
56.37 KB
1.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()

alexpott’s picture

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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
56.54 KB
1.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().

sun’s picture

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 ;)

alexpott’s picture

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.

n3or’s picture

Assigned: n3or » Unassigned

I do not really work on this patch at moment.

alexpott’s picture

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.

alexpott’s picture

Doh! Fix hook_update_N function name...

alexpott’s picture

Status: Needs work » Needs review
xjm’s picture

sun’s picture

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

alexpott’s picture

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.

alexpott’s picture

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.

alexpott’s picture

Status: Needs work » Needs review
kbasarab’s picture

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.

n3or’s picture

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.

n3or’s picture

Status: Needs work » Needs review
FileSize
8.5 KB
26.55 KB

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.

n3or’s picture

Status: Needs work » Needs review
FileSize
27.14 KB

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

disasm’s picture

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.

n3or’s picture

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.

sun’s picture

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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
30.4 KB

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.

alexpott’s picture

Okay... damn you themes that can be used and not installed or even enabled - come on #1067408: Themes do not have 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 do not have 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!

alexpott’s picture

Status: Needs work » Needs review

Now go test bot go!

alexpott’s picture

Rerolled...

Status: Needs review » Needs work

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

alexpott’s picture

Status: Needs work » Needs review
FileSize
29.47 KB

Doh.. htaccess change snuck in...

Status: Needs review » Needs work

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

alexpott’s picture

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.
alexpott’s picture

Status: Needs work » Needs review

Test please!

Cameron Tod’s picture

Reroll.

Status: Needs review » Needs work

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

Cameron Tod’s picture

Status: Needs work » Needs review
FileSize
24.93 KB

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.

Cameron Tod’s picture

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

alexpott’s picture

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

alexpott’s picture

Status: Needs work » Needs review

Bot... please :)

aspilicious’s picture

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

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

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
31.47 KB

Updating just update_N

aspilicious’s picture

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

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

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
31.46 KB

Updating update_N

jibran’s picture

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

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

jibran’s picture

Status: Needs work » Needs review
Issue tags: +Config novice
FileSize
31.52 KB

reroll

gdd’s picture

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

alexpott’s picture

Issue tags: +Needs tests

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

sun’s picture

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

alexpott’s picture

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.

chx’s picture

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.

gdd’s picture

Issue tags: -Config novice
vijaycs85’s picture

vijaycs85’s picture

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

vijaycs85’s picture

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.

jthorson’s picture

Needs reroll for system_update_8049().

Cameron Tod’s picture

Status: Needs work » Needs review
FileSize
31.52 KB

Updated to system_update_8050().

vijaycs85’s picture

Re-rolling...

Status: Needs review » Needs work

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

chx’s picture

Status: Needs work » Needs review
FileSize
852 bytes
32.56 KB
vijaycs85’s picture

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

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
623 bytes
32.55 KB

Re-roll against current head.

alexpott’s picture

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

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
32.6 KB

No problem.

gdd’s picture

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.

alexpott’s picture

catch’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
12.32 KB
33.24 KB

@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
alexpott’s picture

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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
33.6 KB
671 bytes

How silly of me...

aspilicious’s picture

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 :)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

xjm’s picture

Title: Convert theme settings to configuration system » HEAD BROKEN: Convert theme settings to configuration system
Category: task » bug
Priority: Normal » Critical
Status: Fixed » Reviewed & tested by the community
FileSize
604 bytes

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.

catch’s picture

Status: Needs work » Fixed

Just sorted this directly myself, should be OK now.

aspilicious’s picture

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

Reverting title, scared me :)

andypost’s picture

Priority: Critical » Normal

not critical anymore

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