Issue status update for http://drupal.org/node/29002 Post a follow up: http://drupal.org/project/comments/add/29002 Project: Drupal Version: cvs Component: theme system -Category: tasks +Category: bug reports Priority: normal -Assigned to: drumm +Assigned to: TDobes Reported by: drumm Updated by: TDobes -Status: fixed +Status: patch (code needs review) Attachment: http://drupal.org/files/issues/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 [1]), 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. See also: another issue in which the same problem was discussed [2] [1] http://cvs.drupal.org/viewcvs/drupal/drupal/includes/theme.inc?view=markup [2] http://drupal.org/node/26302 TDobes Previous comments: ------------------------------------------------------------------------ Tue, 16 Aug 2005 22:48:16 +0000 : drumm Attachment: http://drupal.org/files/issues/list_themes.patch (2.77 KB) 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. ------------------------------------------------------------------------ Thu, 18 Aug 2005 22:07:45 +0000 : Dries Committed to HEAD. Less is more.