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
Comment | File | Size | Author |
---|---|---|---|
#23 | interdiff-19-22.txt | 963 bytes | aboros |
#22 | 2011128-theme_disable-bug-22.patch | 2.56 KB | aboros |
#21 | interdiff.txt | 778 bytes | jsbalsera |
#21 | 2011128-theme_disable-bug-21.patch | 2.56 KB | jsbalsera |
#19 | 2011128-theme_disable-bug-19.patch | 2.68 KB | jsbalsera |
Comments
Comment #0.0
Devin Carlson CreditAttribution: Devin Carlson commentedAdded logic.
Comment #1
Devin Carlson CreditAttribution: Devin Carlson commentedWhile attempting to write a test to demonstrate the bug, I ran:
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:
Comment #2
Devin Carlson CreditAttribution: Devin Carlson commentedA patch to demonstrate the issues and a fix.
Comment #4
pplantinga CreditAttribution: pplantinga commentedThe 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.Comment #6
pplantinga CreditAttribution: pplantinga commented#4: disable-correct-themes-2011128-4.patch queued for re-testing.
Comment #6.0
pplantinga CreditAttribution: pplantinga commentedMade results more specific.
Comment #7
star-szrThis 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).
Comment #8
jsbalseraComment #9
jsbalseraComment #10
jsbalseraComment #11
aboros CreditAttribution: aboros commentedThe 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.
Comment #12
mr.baileysI 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):
Comment #14
mr.baileysIt 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).
Comment #15
jsbalseraChanging state to test #14
Comment #17
jsbalseraThe test wasn't working
Comment #18
star-szrPlease take these changes out ;)
Thanks for jumping in on this @jsbalsera, @aboros, and @mr.baileys!
Comment #19
jsbalseraOh my! I thought that I changed everything back, but only did it for the interdiff. My bad :(
Comment #20
mr.baileysI 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).
Comment #21
jsbalseraSomething like that? It's more clear...
Comment #22
aboros CreditAttribution: aboros commentedI 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.
Comment #23
aboros CreditAttribution: aboros commentedSorry 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.
Comment #24
mr.york CreditAttribution: mr.york commented#22 applied cleanly.
Comment #26
alexpott