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..

CommentFileSizeAuthor
#12 cleaner_themable_help-20050430.patch4.88 KBTDobes
#5 phptemplate_drupal_get_help.patch816 bytesStefan Nagtegaal
#3 cleaner_themable_help_0.patch1.99 KBkilles@www.drop.org
#1 cleaner_themable_help.patch1.99 KBStefan Nagtegaal

Comments

Stefan Nagtegaal’s picture

Stefan Nagtegaal commented
StatusFileSize
new cleaner_themable_help.patch1.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:

-  if ($help = menu_get_active_help()) {
-    $output .= '<small>'. $help .'</small><hr />';
-  }
+  $output .= theme('help', drupal_get_help());

To change your PHP-Template based theme:
if ($help):

print $help;

endif;

becomes a simple:
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.

dikini’s picture

dikini commented

+1 from me
it makes help text themeable
it uses semantic markup, which is good practice, or as some say the Right Way

killes@www.drop.org’s picture

killes@www.drop.org commented
StatusFileSize
new cleaner_themable_help_0.patch1.99 KB

The patch had DOS line endings and wouldn't apply.

I fully support this patch. +1

adrian’s picture

adrian commented

I completely +1 this.

Stefan Nagtegaal’s picture

Stefan Nagtegaal commented
StatusFileSize
new phptemplate_drupal_get_help.patch816 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...

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,
walkah’s picture

walkah commented

looks good to me. +1

chx’s picture

chx commented

+1.

Bèr Kessels’s picture

Bèr Kessels commented

and another +1

dries’s picture

dries commented

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;
  }
}
TDobes’s picture

TDobes commented

Why do we need a drupal_get_help function at all? Why could the theme_help function not look like this:

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());.

dries’s picture

dries commented

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

TDobes’s picture

TDobes commented
StatusFileSize
new cleaner_themable_help-20050430.patch4.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.

dries’s picture

dries commented

Committed TDobes' latest version! Thanks.

Stefan Nagtegaal’s picture

Stefan Nagtegaal commented

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.

dries’s picture

dries commented

Feel free to provide more patches. I'll review them when they hit the patch queue. Set the status back to 'patch' when there is a new patch to be reviewed.

coreb’s picture

coreb commented
Version: x.y.z » 6.x-dev

Moving out of the x.y.z queue into 6.x-dev. Technically, the theme system has been improving with each version, so it is probably safe to close this, but I'd rather have others decide.

kkaefer’s picture

kkaefer commented
Status: Active » Fixed

This patch has already been comitted.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.