[drupal-devel] [bug] list_themes() improvements

[drumm]

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:     bug reports
 Priority:     normal
 Assigned to:  TDobes
 Reported by:  drumm
 Updated by:   drumm
 Status:       patch (code needs review)

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.




drumm



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.




------------------------------------------------------------------------

Sat, 20 Aug 2005 05:21:14 +0000 : TDobes

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




------------------------------------------------------------------------

Sat, 20 Aug 2005 12:12:31 +0000 : Tobias Maier

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




------------------------------------------------------------------------

Sun, 21 Aug 2005 08:31:01 +0000 : Bèr Kessels

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.