While writing an update function for a Drupal distribution I found that calling theme_disable() on a list of themes caused unexpected results.

The update function should disable every theme except for the default theme:

/**
 * Disable unnecessary themes.
 */
function distribution_update_xxxx() {
  $theme_list = array_keys(list_themes());
  // theme_disable() ensures that the default theme will not be disabled.
  theme_disable($theme_list);
  return array();
}

However, instead of the non-default themes being disabled I found that the default theme and all but one other theme were disabled!

Internally theme_disable() checks to see if the default theme is in the array of themes to be disabled and, if it is, removes it. It does this by searching through the array of provided themes, finds the array index of the default theme and then unsets the array element at that index.

  // Don't disable the default theme.
  if ($pos = array_search(config('system.theme')->get('default'), $theme_list) !== FALSE) {
    unset($theme_list[$pos]);
    if (empty($theme_list)) {
      return;
    }
  }

But I found that $pos wasn't getting the correct position:

The default theme is currently: commons_origins

Array
(
    [0] => adaptivetheme
    [1] => adaptivetheme_admin
    [2] => adaptivetheme_subtheme
    [3] => bartik
    [4] => commons_origins
    [5] => garland
    [6] => seven
    [7] => sky
    [8] => stark
)

commons_origins was located at position 1 and was removed

Array
(
    [0] => adaptivetheme
    [2] => adaptivetheme_subtheme
    [3] => bartik
    [4] => commons_origins
    [5] => garland
    [6] => seven
    [7] => sky
    [8] => stark
)

It seems that the conditional gives execution order priority to the !== operator. !== has higher precedence than = so the variable assignment should either occur separately or parentheses should be added around the assignment.

The logic for $pos in the example above:

  • array_search(config('system.theme')->get('default'), $theme_list) returns 4 (the position of commons_origins)
  • 4 !== FALSE is TRUE
  • $pos is assigned the value of TRUE
  • unset($theme_list[TRUE]) converts TRUE to 1 and unsets the array element at position 1
Files: 
CommentFileSizeAuthor
#23 interdiff-19-22.txt963 bytesaboros
#22 2011128-theme_disable-bug-22.patch2.56 KBaboros
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,810 pass(es). View
#21 interdiff.txt778 bytesjsbalsera
#21 2011128-theme_disable-bug-21.patch2.56 KBjsbalsera
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,935 pass(es). View
#19 2011128-theme_disable-bug-19.patch2.68 KBjsbalsera
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,731 pass(es). View
#17 interdiff.txt1.36 KBjsbalsera
#17 2011128-theme_disable-bug-17.patch8.86 KBjsbalsera
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,660 pass(es). View
#14 2011128-theme_disable-bug-11.patch2.57 KBmr.baileys
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,742 pass(es), 2 fail(s), and 6 exception(s). View
#9 disable-correct-themes-2011128-9.patch2.62 KBjsbalsera
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,730 pass(es). View
#4 disable-correct-themes-2011128-4.patch2.49 KBpplantinga
PASSED: [[SimpleTest]]: [MySQL] 56,671 pass(es). View
#2 disable-correct-themes-2011128-2.patch2.56 KBDevin Carlson
FAILED: [[SimpleTest]]: [MySQL] 55,149 pass(es), 8 fail(s), and 10 exception(s). View
#2 disable-correct-themes-2011128-2-tests-only.patch1.64 KBDevin Carlson
FAILED: [[SimpleTest]]: [MySQL] 55,662 pass(es), 4 fail(s), and 0 exception(s). View

Comments

Devin Carlson’s picture

Issue summary: View changes

Added logic.

Devin Carlson’s picture

Priority: Normal » Major

While attempting to write a test to demonstrate the bug, I ran:

  $theme_list = array_keys(list_themes());
  // theme_disable() ensures that the default theme will not be disabled.
  theme_disable($theme_list);
  return array();

on a Drupal 8 site with Bartik, Seven and Stark enabled which resulted in two of the themes becoming disabled. However, when I went to re-enable the themes I found that visiting admin/appearance resulted in a fatal error. Searching through system.admin.inc it looks like administration themes can no longer be disabled in D8 (a change that was done as part of #542828: Do not special case disabled admin theme) but theme_disable() was never updated to also prevent admin themes from being disabled.

So there are two issues here:

  • theme_disable() uses the wrong position when unsetting the admin theme from the list of themes to disable
  • theme_disable() doesn't prevent the administration theme from being disabled
Devin Carlson’s picture

Status: Active » Needs review
FileSize
1.64 KB
FAILED: [[SimpleTest]]: [MySQL] 55,662 pass(es), 4 fail(s), and 0 exception(s). View
2.56 KB
FAILED: [[SimpleTest]]: [MySQL] 55,149 pass(es), 8 fail(s), and 10 exception(s). View

A patch to demonstrate the issues and a fix.

Status: Needs review » Needs work

The last submitted patch, disable-correct-themes-2011128-2-tests-only.patch, failed testing.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
2.49 KB
PASSED: [[SimpleTest]]: [MySQL] 56,671 pass(es). View

The problem with the patch was 2-fold: there are two ThemeTest.php files and this was in the wrong one and the "test bot" user didn't have permission to see the block admin page.

I couldn't create a user with the 'administer blocks' permission, kept getting this error: Invalid permission administer blocks.... I'm not sure what I was doing wrong. Anyway, I re-wrote the test so I didn't need that.

Status: Needs review » Needs work

The last submitted patch, disable-correct-themes-2011128-4.patch, failed testing.

pplantinga’s picture

Status: Needs work » Needs review
pplantinga’s picture

Issue summary: View changes

Made results more specific.

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Twig

This needs to be reworked/rerolled now that theme_disable() has been service-fied into \Drupal\Core\Extension\ThemeHandler::disable(). I was able to confirm that the test does fail on latest 8.x (changed config to \Drupal::config).

jsbalsera’s picture

Assigned: Unassigned » jsbalsera
jsbalsera’s picture

Status: Needs work » Needs review
FileSize
2.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,730 pass(es). View
jsbalsera’s picture

Assigned: jsbalsera » Unassigned
aboros’s picture

The patch in #9 applied cleanly. However, i could not decide if the test function does its job well. After discussing it with others (#drupaldevdays #szeged) i edited the testDisableTheme() function a little bit. Now it also enables Stark to have at least one theme enabled which is neither admin theme nor default. Then it calls the disable function and checks if the admin theme and the default theme is still enabled, but stark is disabled.

mr.baileys’s picture

Status: Needs review » Needs work
+    // Don't disable the default or admin themes.
+    $default_theme = \Drupal::config('system.theme')->get('default');
+    $admin_theme = \Drupal::config('system.theme')->get('admin');
+    $required_themes = array($default_theme, $admin_theme);
+
+    foreach ($required_themes as $theme) {
+      $pos = array_search($theme, $theme_list);
+      if ($pos !== FALSE) {
+        unset($theme_list[$pos]);
+        if (empty($theme_list)) {
+          return;
+        }

I think most of this can be rewritten, dumping the loop and using array_diff() to remove the required themes from theme_list (although make sure to keep the return on empty):

// Don't disable the default or admin themes.
$default_theme = \Drupal::config('system.theme')->get('default');
$admin_theme = \Drupal::config('system.theme')->get('admin');
$theme_list = array_diff($theme_list, array($default_theme, $admin_theme));

The last submitted patch, 11: 2011128-theme_disable-bug-11.patch, failed testing.

mr.baileys’s picture

FileSize
2.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,742 pass(es), 2 fail(s), and 6 exception(s). View

It seems that I cross-posted, which caused the patch from #11 to be deleted, so re-attaching again (please do not give me commit credit for this entry).

jsbalsera’s picture

Status: Needs work » Needs review

Changing state to test #14

Status: Needs review » Needs work

The last submitted patch, 14: 2011128-theme_disable-bug-11.patch, failed testing.

jsbalsera’s picture

Status: Needs work » Needs review
FileSize
8.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,660 pass(es). View
1.36 KB

The test wasn't working

Cottser’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php
@@ -272,11 +272,36 @@ public function testFindThemeTemplates() {
-  function testPreprocessHtml() {
+  function noTestPreprocessHtml() {

Please take these changes out ;)

Thanks for jumping in on this @jsbalsera, @aboros, and @mr.baileys!

jsbalsera’s picture

Status: Needs work » Needs review
FileSize
2.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,731 pass(es). View

Oh my! I thought that I changed everything back, but only did it for the interdiff. My bad :(

mr.baileys’s picture

I still feel that we should rewrite this entire kludgy loop construct with a one-line array_diff(), which would make it more readable (and would have prevented this bug in the first place).

jsbalsera’s picture

FileSize
2.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,935 pass(es). View
778 bytes

Something like that? It's more clear...

aboros’s picture

FileSize
2.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,810 pass(es). View

I think yes, but we have to keep the return on empty. (if we only have the admin and default themes enabled, we return.) Here is my patch for it, i was working on it in parallel already.

aboros’s picture

FileSize
963 bytes

Sorry i just learned how to make an interdiff. Attaching now. However, since while i was working on the one in 22 jsbalsera also submitted one, so this interdiff is between 19 and 22. I hope its not a problem.

mr.york’s picture

Status: Needs review » Reviewed & tested by the community

#22 applied cleanly.

  • Commit 8fa1fdd on 8.x by alexpott:
    Issue #2011128 by jsbalsera, aboros, Devin Carlson, mr.baileys,...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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