Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pfrenssen’s picture

Title: Convert the Default Theme variable to CMI » Convert the 'theme_default' variable to CMI
FileSize
29 KB

Here's a first patch.

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

pfrenssen’s picture

Status: Active » Needs review

Status: Needs review » Needs work

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

leschekfm’s picture

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.

pfrenssen’s picture

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.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
28.04 KB

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

pfrenssen’s picture

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 "{}"'
pfrenssen’s picture

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

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

alexpott’s picture

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

Tagging and setting status.

mbrett5062’s picture

@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 do not have 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.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned

Unassigning myself.

lirantal’s picture

Assigned: Unassigned » lirantal
Status: Postponed » Needs review
FileSize
25.39 KB

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.

lirantal’s picture

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.

vijaycs85’s picture

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.

vijaycs85’s picture

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

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

vijaycs85’s picture

Status: Needs review » Needs work
vijaycs85’s picture

Status: Needs work » Needs review
FileSize
23.97 KB

Re-rolling...

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

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

vijaycs85’s picture

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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
42.76 KB

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

alexpott’s picture

Forgot sites/default/default.settings.php

EclipseGc’s picture

This looks very sane to my eyes.

alexpott’s picture

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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
42.51 KB
1.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';
gdd’s picture

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!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

sun’s picture

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 do not have an installation status...

Status: Fixed » Closed (fixed)

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

effulgentsia’s picture

Status: Closed (fixed) » Active

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

ianthomas_uk’s picture

Assigned: lirantal » Unassigned
Status: Active » Needs review
FileSize
1.4 KB

Here's the fix for the tests.

catch’s picture

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.

ianthomas_uk’s picture

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

Status: Needs review » Reviewed & tested by the community

Re-applying RTBC by catch in #37

alexpott’s picture

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.