Issue status update for http://drupal.org/node/21517 Project: Drupal Version: cvs Component: theme system Category: tasks Priority: normal Assigned to: stefan nagtegaal Reported by: stefan nagtegaal Updated by: TDobes Status: patch Attachment: http://drupal.org/files/issues/cleaner_themable_help-20050430.patch (4.88 KB) I've attached a "counter-patch" using the code I suggested. It also includes patches to update the core themes, which were missing from the other patch. I also fixed the call to theme('status_messages'), which was being called as the (non-themable) theme_status_messages. TDobes Previous comments: ------------------------------------------------------------------------ April 28, 2005 - 10:21 : stefan nagtegaal The theme system is very inconsistent and not clean. In a steady stream of patches i'll try to make the theme system clean and let most of the theme functions behave the same.. One of the main goals is, that when writing themes in PHPTemplate for example, the way how things are being returned is different from theme function to theme function.. Let's try to unify this, so theming will be more efficient, simpler and more consistent.. ------------------------------------------------------------------------ April 28, 2005 - 10:40 : stefan nagtegaal Attachment: http://drupal.org/files/issues/cleaner_themable_help.patch (1.99 KB) The attached patch does: - remove unused function theme_help() and theme_error(); - added a wrapper function drupal_get_help() inside common.inc (for consistency with drupal_get_title(), drupal_get_breadcrumb(), etc); - reimplemented theme_help(), only this time for a more consistent way for themers; This patch makes it easier to theme the page specific helptexts for drupal. To change your PHP-based themes, you can do: <?php -  if ($help = menu_get_active_help()) { -    $output .= '<small>'. $help .'</small><hr />'; -  } +  $output .= theme('help', drupal_get_help()); ?> To change your PHP-Template based theme: <?php if ($help): ?> <?php print $help; ?> <?php endif; ?> becomes a simple: <?php print $help; ?> If this patch is accepted i'll try to make the functions for returning the local tasks and the status messages behave the same.. Overriding the <div class="help">...</div> could be done through deifinig your own theme_help() function in your theme or template.php file. ------------------------------------------------------------------------ April 28, 2005 - 10:44 : dikini +1 from me it makes help text themeable it uses semantic markup, which is good practice, or as some say the Right Way ------------------------------------------------------------------------ April 28, 2005 - 11:00 : killes@www.drop.org Attachment: http://drupal.org/files/issues/cleaner_themable_help_0.patch (1.99 KB) The patch had DOS line endings and wouldn't apply. I fully support this patch. +1 ------------------------------------------------------------------------ April 28, 2005 - 12:38 : adrian I completely +1 this. ------------------------------------------------------------------------ April 28, 2005 - 15:07 : stefan nagtegaal Attachment: http://drupal.org/files/issues/phptemplate_drupal_get_help.patch (816 bytes) Because Adrian +1's this, his reward is a proper patch against PHPTemplate, sorry I forgot this one... here it is, a really simple one liner which does the job... <?php Index: phptemplate.engine =================================================================== RCS file: /cvs/drupal/contributions/theme-engines/phptemplate/phptemplate.engine,v retrieving revision 1.24.2.1 diff -u -F^f -r1.24.2.1 phptemplate.engine --- phptemplate.engine    23 Apr 2005 01:04:53 -0000    1.24.2.1 +++ phptemplate.engine    28 Apr 2005 21:03:59 -0000 @@ -219,7 +219,7 @@ function phptemplate_page($content) {      'tabs'                => theme('menu_local_tasks'),      'messages'            => theme_status_messages(),      'layout'              => $layout, -    'help'                => menu_get_active_help(), +    'help'                => theme('help', drupal_get_help()),      'styles'              => theme_get_styles(),      'mission'             => $mission,      'is_front'            => $frontpage, ?> ------------------------------------------------------------------------ April 28, 2005 - 15:35 : walkah looks good to me. +1 ------------------------------------------------------------------------ April 28, 2005 - 15:45 : chx +1. ------------------------------------------------------------------------ April 29, 2005 - 02:53 : Bèr Kessels and another +1 ------------------------------------------------------------------------ April 29, 2005 - 13:12 : Dries Any particular reason we can't do: function drupal_get_help() { return menu_get_active_help(); } instead of the proposed: function drupal_get_help() { if ($help = menu_get_active_help()) { return $help; } } ------------------------------------------------------------------------ April 29, 2005 - 20:12 : TDobes Why do we need a drupal_get_help function at all? Why could the theme_help function not look like this: <?php function theme_help() {   if ($help = menu_get_active_help()) {     return '<div class="help">'. $help .'</div>';   } } ?> This would be far more consistent with what we've done with status messages (theme_status_messages). It would also ask less of theme authors as they would only need $output .= theme('help'); instead of $output .= theme('help', drupal_get_help());. ------------------------------------------------------------------------ April 30, 2005 - 03:29 : Dries Isn't it amazing that everyone nods '+1' without actually reviewing the code?