list_themes() improvements [#29002]

list_themes() currently returns all themes, not just enabled themes. This functionality is only used in one place- configuration for disabled themes. These configuration pages can be removed with a usability improvement since you shouldn't be able to configure things which are disabled. Additionally, this allows us to remove some extra logic in system_user(). And it it more consistent with the module API which only lists enabled modules.

list_themes() sorts the results by name. This uses filesort in MySQL since there aren't any indexes. Sorting is not used except in system_user(). This one use can be handled with ksort since it is not often executed (only on the user edit screen when multiple themes are enabled).

And a one line fix to remove a variable in system_user() is in here too.

Comment	File	Size	Author
#2	user_selectable_instead_of_enabled.patch	3.87 KB	TDobes
#2
	list_themes.patch	2.77 KB	drumm

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Dries CreditAttribution: Dries commented 18 August 2005 at 22:07

Status:

Needs review

» Fixed

Committed to HEAD. Less is more.

Comment #2

TDobes CreditAttribution: TDobes commented 20 August 2005 at 05:21

Assigned:	drumm	» TDobes
Category:	task	» bug
Status:	Fixed	» Needs review

File	Size
user_selectable_instead_of_enabled.patch	3.87 KB

WHOA! This completely broke our ability to use the $custom_theme global to change the theme in different sections of the site. Off the top of my head, this will cause problems for sections.module and blogtheme.module... maybe others too.

Here's the problem. In init_themes() (look in theme.inc), we call list_themes() to determine what themes are available for use. We then make decisions on what theme to use and load the appropriate theme (including whichever .theme file or .engine file, and referencing the appropriate .css file as needed). We load these themes based on the information we obtained using init_themes().

However, with this patch, any theme passed in the $custom_theme global will only work if it's an enabled. While this seems to make sense at first glance, it is very unlikely that this will work for nearly anyone who's using $custom_theme. The reason is that the "enabled/disabled" setting is poorly named. What it really means is "available for users to select / not available for users to select." So... if people want to change the theme in different sections of their site, they now have to make all of those themes available for selection by users. This could work out two ways with very unintended consequences: (a.) users can switch the theme to something inappropriate, resulting in pages that do not display as intended or (b.) users have access to a setting for changing themes which does absolutely nothing because it's been constantly overridden by $custom_theme. Neither of these results are very intuitive... or very usable.

Therefore, I recommend that we do something similar to the attached patch:
* rename "enabled" to "user-selectable"... this should hopefully clear up lots of confusion (this isn't the first time that the terminology has been a problem)
* add an additional parameter to init_theme(), $user_only... this boolean parameter specifies whether we should list all themes or just those available for selection by users
* slightly changed the system_menu() function such that settings pages for non-selected themes are not completely disabled but are just hidden - this will allow contrib modules to link to those pages, but still retain the usability improvement of hiding unselected themes.

Comment #3

Tobias Maier CreditAttribution: Tobias Maier commented 20 August 2005 at 12:12

+1 for the new patch
untested but i need the $custom_theme

Comment #4

Bèr Kessels CreditAttribution: Bèr Kessels commented 21 August 2005 at 08:31

Ahh. So that was why I could not get sections and the admintheme working anymore.
Please roll back, or decide on a new patch.

Or, if we decide to not go for the custom theme feature, we should at least consider removing all the custom theme code from core.

Comment #5

drumm

he/him

NY, US

CreditAttribution: drumm commented 21 August 2005 at 14:31

I don't think "user-selectable" should be hyphenated. Rules I went over to double check: http://en.wikipedia.org/wiki/Hyphenation

I think the order of the arguments for list_themes() should be swapped. I don't see $refresh used anywhere in core so this shouldn't cause problems.

Comment #6

TDobes CreditAttribution: TDobes commented 22 August 2005 at 04:32

Status:

Needs review

» Closed (duplicate)

My patch has been superceded by the idea of moving user-selectable themes out of core. Note that is solves the problem in a different way and should not affect the usability improvements made here.

Slightly off topic: Even after reading the Wikipedia page, I'm not completely convinced on whether I should use "user selectable themes" or "user-selectable themes." When reading the first one, I'm not sure that it adequately conveys the idea that "user selectable" is a phrase modifying "themes". From the Wikipedia entry:

when a compound modifier appears before a term, the compound modifier is generally hyphenated in order to prevent any possible misunderstanding

I think that applies here. I bring this up not to argue, but in the interest of being correct, as I use this term (with hyphen) in the contrib module I'm making to house this functionality. If it still seems like I'm incorrect, let me know.