[drupal-devel] [task] improve theme system

stefan nagtegaal drupal-devel at drupal.org
Sun May 1 08:02:49 UTC 2005


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:   stefan nagtegaal
-Status:       fixed
+Status:       patch

Well, I added the wrapper drupal_get_help() to get more inline with the
rest of the theming function like drupal_get_title() and
drupal_get_breadcrumb()..
To get rid of the most PHP-logic inside the theme function itself, was
also a decision to make the warapper function. That way it is just one
call to the drupal_get_help() function to get the help, without having
to much attention to PHP at all..


I still think that my code is a little better when we look at
consistency and usability..


I set the status of this post to 'Patch' again because I would like to
improve the theme system more, by eliminating most of the PHP
code/logic out of the themable functions (by replacing the code in
wrapper functions), and so this is e-mailed to the drupal developers
list.




stefan nagtegaal



Previous comments:
------------------------------------------------------------------------

April 28, 2005 - 18: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 - 18: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 - 18: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 - 19:00 : killes at 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 - 20:38 : adrian

I completely +1 this.




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

April 28, 2005 - 23: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 - 23:35 : walkah

looks good to me. +1




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

April 28, 2005 - 23:45 : chx

+1.




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

April 29, 2005 - 10:53 : Bèr Kessels

and another +1




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

April 29, 2005 - 21: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 30, 2005 - 04: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 - 11:29 : Dries

Isn't it amazing that everyone nods '+1' without actually reviewing the
code?




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

April 30, 2005 - 12:05 : TDobes

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.




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

April 30, 2005 - 19:47 : Dries

Committed TDobes' latest version!  Thanks.







More information about the drupal-devel mailing list