theme_disable() can disable the wrong theme

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.

#4: disable-correct-themes-2011128-4.patch queued for re-testing.

Made results more specific.

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

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.

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

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

Changing state to test #14

The test wasn't working

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

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

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

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

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.

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.

#22 applied cleanly.