Files: 
CommentFileSizeAuthor
#36 1829224.convert_theme_default.36.patch1.4 KBianthomas_uk
PASSED: [[SimpleTest]]: [MySQL] 58,603 pass(es).
[ View ]
#30 28-30-interdiff.txt1.71 KBalexpott
#30 1829224.convert_theme_default.30.patch42.51 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 52,264 pass(es).
[ View ]
#28 26-28-interdiff.txt2.48 KBalexpott
#28 1829224.convert_theme_default.28.patch42.5 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 51,702 pass(es), 2 fail(s), and 4,145 exception(s).
[ View ]
#26 25-26-interdiff.txt1.34 KBalexpott
#26 1829224.convert_theme_default.26.patch44.19 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 52,286 pass(es).
[ View ]
#25 1829224.convert_theme_default.25.patch42.76 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 52,240 pass(es).
[ View ]
#21 1829224-convert_theme_default_var-21.patch23.97 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 52,045 pass(es), 21 fail(s), and 648 exception(s).
[ View ]
#16 1829224-convert_theme_default_var-reroll-16.patch22.06 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 49,286 pass(es), 19 fail(s), and 4,231 exception(s).
[ View ]
#16 1829224-convert_theme_default_var-reroll-maintance-16.patch23.73 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 48,721 pass(es), 19 fail(s), and 4,642 exception(s).
[ View ]
#13 convert_theme_default_var-1829224-13.patch25.39 KBlirantal
FAILED: [[SimpleTest]]: [MySQL] 49,887 pass(es), 2 fail(s), and 4,496 exception(s).
[ View ]
#6 1829224-6-cmi-convert_theme_default.patch28.04 KBpfrenssen
FAILED: [[SimpleTest]]: [MySQL] 46,442 pass(es), 3 fail(s), and 3 exception(s).
[ View ]
#1 1829224-cmi-convert_theme_default.patch29 KBpfrenssen
FAILED: [[SimpleTest]]: [MySQL] 46,407 pass(es), 0 fail(s), and 3 exception(s).
[ View ]

Comments

Title:Convert the Default Theme variable to CMIConvert the 'theme_default' variable to CMI
StatusFileSize
new29 KB
FAILED: [[SimpleTest]]: [MySQL] 46,407 pass(es), 0 fail(s), and 3 exception(s).
[ View ]

Here's a first patch.

I want to take a look at _drupal_maintenance_theme() again after the test.

Status:Active» Needs review

Status:Needs review» Needs work

The last submitted patch, 1829224-cmi-convert_theme_default.patch, failed testing.

The changes seem to work. Changing the theme settings reflects in changes to the config files and the db.

I'm not sure why the simpletests fail. When the simpletest are run, the test tries to delete the temporarily set config items from simpletestcache_config which is missing.

Thanks for the review! I'm looking into it with help from fubhy and alexpott. It seems to have to do with the naming of the config object.

Status:Needs work» Needs review
StatusFileSize
new28.04 KB
FAILED: [[SimpleTest]]: [MySQL] 46,442 pass(es), 3 fail(s), and 3 exception(s).
[ View ]

I changed the config name from system.theme.default to system.site.theme_default on alexpotts request.

Status:Needs work» Needs review

I'm going to git bisect this to figure out where it goes wrong. This command puts each changed file in a separate commit to facilitate this. Just a reminder for myself.

$ git status | grep modified | sed 's/^.* //g' | xargs -I {} sh -c 'git add {} ; git commit -m "{}"'

Status:Needs review» Needs work

This is the change that causes the tests to fail:

diff --git a/core/includes/theme.inc b/core/includes/theme.inc
index 0c35b90..d509f03 100644
--- a/core/includes/theme.inc
+++ b/core/includes/theme.inc
@@ -90,7 +90,7 @@ function drupal_theme_initialize() {
   // Only select the user selected theme if it is available in the
   // list of themes that can be accessed.
-  $theme = !empty($user->theme) && drupal_theme_access($user->theme) ? $user->theme : variable_get('theme_default', 'stark');
+  $theme = !empty($user->theme) && drupal_theme_access($user->theme) ? $user->theme : config('system.site')->get('theme_default');
   // Allow modules to override the theme. Validation has already been performed
   // inside menu_get_custom_theme(), so we do not need to check it again here.

Status:Needs review» Needs work

For this patch to work we need #1770902: Theme of parent site executing test leaks into all tests to land...

The change to drupal_theme_initialize() means that the full container is being instantiated in DrupalUnitTestBase :( due to the called to drupal_theme_initialize in TestBase::prepareEnvironment().

Status:Needs work» Postponed
Issue tags:+Configuration system

Tagging and setting status.

@alexpott I have just got #1770902: Theme of parent site executing test leaks into all tests, green, a minor change to @sun's code. If you could review that and mark RTBC, then we can get this unblocked, and I will get #1067408: Themes need an installation status up to scratch with this change. I would like to see the tangled theme issues resolved. Thanks in advance for your help.

Assigned:pfrenssen» Unassigned

Unassigning myself.

Assigned:Unassigned» lirantal
Status:Postponed» Needs review
StatusFileSize
new25.39 KB
FAILED: [[SimpleTest]]: [MySQL] 49,887 pass(es), 2 fail(s), and 4,496 exception(s).
[ View ]

I actually filed #1876534: Convert theme_default setting to configuration system today, after doing a bit of searching and yet still missed this bug so I guess that I can mark it a duplicate of this bug.

See updated patch please.

I think this still might fail as we need to also accommodate for other variables like maintenance_theme etc but let's wait and see.

Status:Needs review» Needs work

The last submitted patch, convert_theme_default_var-1829224-13.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new23.73 KB
FAILED: [[SimpleTest]]: [MySQL] 48,721 pass(es), 19 fail(s), and 4,642 exception(s).
[ View ]
new22.06 KB
FAILED: [[SimpleTest]]: [MySQL] 49,286 pass(es), 19 fail(s), and 4,231 exception(s).
[ View ]

Changes in this patch:

1. Updated patch to work with current code base.
2. Updated maintenance_theme

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

The last submitted patch, 1829224-convert_theme_default_var-reroll-maintance-16.patch, failed testing.

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

Will hopefully take another look at this soon and provide an up to date patch.

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new23.97 KB
FAILED: [[SimpleTest]]: [MySQL] 52,045 pass(es), 21 fail(s), and 648 exception(s).
[ View ]

Re-rolling...

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

The last submitted patch, 1829224-convert_theme_default_var-21.patch, failed testing.

Status:Needs work» Needs review

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

The last submitted patch, 1829224-convert_theme_default_var-21.patch, failed testing.

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

There are loads more usages of the theme_default variable with the blocks to plugins conversions.

Fingers-crossed this patch might be green...

One thing that was interesting is that we need to the default theme config in DrupalUnitTestBase so that any of the many unit tests that use theme functions work. In some ways you could argue that this tests should actually declared this dependency and enable stark and set it to the default theme... maybe @sun can chime in with an opinion?

No interdiff as the patch has doubled in size so reviewers should start again...

StatusFileSize
new44.19 KB
PASSED: [[SimpleTest]]: [MySQL] 52,286 pass(es).
[ View ]
new1.34 KB

Forgot sites/default/default.settings.php

This looks very sane to my eyes.

StatusFileSize
new42.5 KB
FAILED: [[SimpleTest]]: [MySQL] 51,702 pass(es), 2 fail(s), and 4,145 exception(s).
[ View ]
new2.48 KB

In talking with @heyrocker on irc realised that maintenance_theme variable changes have snuck into this patch... the patch attached sneaks them back out again in favour of #1829170: Convert the Maintenance Theme variable to settings

Status:Needs review» Needs work

The last submitted patch, 1829224.convert_theme_default.28.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new42.51 KB
PASSED: [[SimpleTest]]: [MySQL] 52,264 pass(es).
[ View ]
new1.71 KB

Oops... need to ensure bartik default in maintenance theme...

     //   Stark otherwise. Since there is no low-level access to configuration
     //   currently, we only consult settings.php and fall back to Bartik
     //   otherwise, as it looks generic enough and way more user-friendly.
-    $custom_theme = variable_get('maintenance_theme', config('system.theme')->get('default'));
+    $custom_theme = variable_get('maintenance_theme', config('system.theme')->get('default')) ?: 'bartik';

Status:Needs review» Reviewed & tested by the community

I've looked this over and it looks good to me too. So happy to get this in! W00t!

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

Looks great.

re: #25:
I wasn't aware of this separate issue and was only following the existing theme config conversion issues. Feel free to ping me privately or simply tag issues with 'sun'.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
@@ -119,6 +119,8 @@ protected function setUp() {
+    // In order to use theme functions default theme config needs to exist.
+    config('system.theme')->set('default', 'stark');

Hm. So what you're essentially doing is to

1) Install the single system.theme config file.
2) Enable the Stark theme.
3) Configure Stark as default theme.

Apparently, step 2) is missing. Step 1) doesn't happen, and we aren't able to install single config files yet. And step 3) only works, because the current theme extension system is lame.

Also noteworthy: system_list()'s static cache is not guaranteed to be reset. DrupalKernel isn't updated for the new theme.

This will cause us quite some additional fun in #1067408: Themes need an installation status...

Status:Fixed» Closed (fixed)

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

Status:Closed (fixed)» Active

Reopening because in HEAD, BlockTest and MenuTest still have one line each of variable_get('theme_default', 'stark').

Assigned:lirantal» Unassigned
Status:Active» Needs review
StatusFileSize
new1.4 KB
PASSED: [[SimpleTest]]: [MySQL] 58,603 pass(es).
[ View ]

Here's the fix for the tests.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work
Issue tags:-Configuration system

The last submitted patch, 1829224.convert_theme_default.36.patch, failed testing.

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

Status:Needs review» Reviewed & tested by the community

Re-applying RTBC by catch in #37

Status:Reviewed & tested by the community» Fixed

Committed 6ec7f3d and pushed to 8.x. Thanks!

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