Theme functions group needs some updates

Me!

Waiting for #610408: Create theme.api.php to help consolidate theme-related docs. to be committed before rolling a patch.

I've been working on a bunch of changes. Maybe 30% done at this point.

One point to discuss is the one line descriptions of the theme functions and template files are worded in several different ways. We should use a consistent phrasing like we use "Implements hook_*" for hook functions.

Here's what we currently have in our theme hook one-liners:

1. This function formats blah blah — Yes, we know already know this function is a function.
2. Theme the blah blah — While this might be the most accurate, "theme" is definitely a Drupalism and is a bit jargony.
3. Default theme implementation of blah blah — super, extra jargony! :-p
4. Generate the HTML output for blah blah — verbose
5. Returns a blah blah — What? Functions return stuff? I never knew!
6. Returns code that emits blah blah — wow.
7. Theme callback for blah blah — "theme callback" is a more accurate term then "theme hook", but callback is jargony.
8. Theme function for blah blah — this phrase can't be used for template files.
9. Make a blah blah — meh.
10. Render a blah blah — jargony.
11. Present a blah blah
12. Display a blah blah
13. Format a blah blah

I'm partial to "Format a" since we're not immediately displaying or presenting anything. We're marking it up and returning it for later presentation.

I completely agree with #7. :-)

Some thoughts:

1) Specifying @return is superfluous for most theme functions. It should only be stated if the return value is not HTML like everywhere else. Theme functions returning XML instead of HTML or perhaps even plain-text only, may be able to describe this in the function summary already, so explicitly stating @return really only makes sense for edge-cases.

2) If the return value is HTML, the function summary does not need to specify that it's returning HTML. Again, it's the expected default for all theme functions.

3) The description for @param $variables should be unified and kept as short as possible, since it means that we repeat something on purpose - and normally, we would just skip it instead of duplicating (compare $form, $form_state params for form constructors). But since we need to specify its contents, we can't skip it.

So effectively:

/**
 * Formats a foo.
 *
 * @param $variables
 *   An associative array containing:
 *   - foo: The foo object that is being formatted.
 *   - show_bar: TRUE to show the bar component, FALSE to omit it.
 *
 * @ingroup themeable
 */

/**
 * Formats a foo as XML.
 * ...
 */

/**
 * Formats a foo as neutro-calmic hoover element.
 *
 * @param $variables
 *   An associative array containing:
 *   - foo: The foo object that is being formatted.
 *   - show_bar: TRUE to show the bar component, FALSE to omit it.
 *
 * @return
 *   A neutro-calmic hoover element suitable for rendering via foo_hoover(). :)
 *
 * @ingroup themeable
 */

4) On preprocess and process functions: Those should be listed with theme functions starting with D7. The reason is that we actually have a couple of preprocess and process functions that perform essential data preparations for - sometimes common/generic - theme functions. Partially due to performance reasons, partially due to other reasons. I.e. the only way to achieve a different formatting is to add another preprocessor or processor. Therefore, knowing about preprocess and process functions is an important part of the game.

Both "HTML" and "@return" would be needlessly repeated information all over again. We avoid that in general. Information about defaults can happily live in theme().

On "Formats" - it's a uncommon term, yes, but from all known options it is the most precise.

...

Though, a total alternative - apparently even mentioned above already - would be to use the "abbreviated" summary style...

/**
 * Returns HTML for a foo.
 *
 * @param $variables
 *   An associative array containing:
 *   - foo: The foo object that is being formatted.
 *   - show_bar: TRUE to show the bar component, FALSE to omit it.
 *
 * @ingroup themeable
 */

"@ingroup themeable" is the link to any missing, not duplicated default information about theme functions already...?

well, http://api.drupal.org/api/group/themeable/7 needs a major overhaul anyway. ;) Why not clarify in the very first line(s)?

There's much more to know about default behaviors of theme hooks/functions. You can look up theme_foo() just to find out that theme_foo() is not even invoked on your site, because there is an override or the theme registry has been altered. Not documented for theme_foo(), but instead, once for all theme functions (hopefully) for the group.

That said, we should consider to move some of the existing docs from theme() to the group. Groups are our new topics/chapters, explaining the high-level API, without going into function-level detail. AFAIK, the best example we have is http://api.drupal.org/api/group/ajax/7

Is theme() the proper relation at all?

theme() is for module developers to invoke and trigger formatting. Theme functions just happen to be called by theme(), but if I look up theme(), then I clearly need information about invoking theme() itself -- not information about basics of the theme system and common knowledge about theme functions. Such high-level information belongs into the group.

In other words: Different purpose, different audience?

Actually, from all suggestions I tend to like #11 more and more (which actually was your suggestion, but just happened to slip in one of your examples ;).

The abbreviated form technically kills all 3 known issues in one shot: 1) No "Formats" or other weird term, 2) States that it returns something, and 3) even states what it returns (HTML/XML/etc.)

Of course, I'd still consider this for "standard theme functions" only... the "Returns foo as neutro-calmic hoover element" edge-case should have a @return directive + most probably also requires a more elaborative explanation as function description... ;)

Ok. I've done a light re-write of @themeable, getting rid of a lot of text. This text is supposed to be about themeable functions/templates, not about how the theme system works.

This (137 KB! Holy crap!) patch also:

• Adds several theme functions that weren't in @themeable
• Adds several templates that weren't in @themeable
• Redoes all of the theme function docblock short descriptions to use "Returns HTML for…". Unless the function doesn't return HTML; in which case, both the short description and the @return statement specify what is returned instead of HTML.
• Redoes all of the @file short descriptions for templates to use "HTML for…". I figured the "Returns" bit doesn't work since templates don't return anything, they just are.
• Makes sure that all theme functions that take variables have @params.
• Removes @return statements for theme functions that return HTML.
• Uses "theming hooks" instead of "theme hooks" in any docs. This helps distinguish that these hooks don't belong to themes as "theme hooks" seems to imply; instead they are "theming hooks" that are controlled by the system (and defined by modules).

It only touches docblocks and a few code comments. No code changes at all.

I forgot to mention, there are 2 exceptions to the theme function docblocks, because the theme functions are mis-written, IMO:

theme_menu_local_tasks(): #599706: Allow to alter local tasks/actions
theme_table_select_header_cell(): #768490: theme_table_select_header_cell() is not really a theme function

Oh, looks like I need to re-roll since #767872: theme_node_log_message() has never, ever been used was just committed! So here's the re-rolled patch.

+++ includes/common.inc
@@ -5410,7 +5410,7 @@ function element_get_visible_children(array $elements) {
 /**
- * Provide theme registration for themes across .inc files.
+ * Register theme hooks across .inc files and specify default values for variables.
  */

+++ includes/form.inc
@@ -2502,8 +2472,14 @@ function theme_container($variables) {
+ *     table row's HTML attributes; see theme_table().

+++ includes/theme.inc
@@ -2370,7 +2242,7 @@ function template_preprocess_page(&$variables) {
+ * Process variables for html.tpl.php.
  *

@@ -2417,7 +2289,7 @@ function template_process_html(&$variables) {
-  // Build a list of suggested theme hooks or body classes in order of
+  // Build a list of suggested theming hooks or body classes in order of

+++ includes/theme.inc
@@ -2583,3 +2459,82 @@ function template_preprocess_region(&$variables) {
+  // Set the name to a formatted name that is safe for printing and
+  // that won't break tables by being too long. Keep an unshortened,

+++ includes/theme.maintenance.inc
@@ -226,10 +231,13 @@ function theme_update_page($variables) {
+ * Returns HTML for a report of the results from an operation run via authorize.php.

+++ modules/comment/comment.module
@@ -568,10 +568,8 @@ function comment_new_page_count($num_comments, $new_replies, $node) {
+ * Returns HTML for a list of recent comments to be displayed in the comment block.

+++ modules/dashboard/dashboard.module
@@ -425,13 +419,11 @@ function theme_dashboard_region($variables) {
+ * Returns HTML for a set of disabled blocks, for display in dashboard customization mode.
  *

+++ modules/dashboard/dashboard.module
@@ -447,12 +439,11 @@ function theme_dashboard_disabled_blocks($variables) {
+ * Returns HTML for a disabled block, for display in dashboard customization mode.

+++ modules/field/modules/options/options.module
@@ -366,8 +366,15 @@ function options_field_widget_error($element, $error, $form, &$form_state) {
+ * Returns HTML for the label for the empty value for options that are not required.

+++ modules/node/content_types.inc
@@ -54,6 +54,17 @@ function node_overview_types() {
+/**
+ * Returns HTML for a node type description for the content type admin overview page.

+++ modules/poll/poll-bar--block.tpl.php
@@ -3,8 +3,7 @@
+ * HTML for the results bar for a single choice in a poll when displayed in a block.

Returns HTML .... (I guess)

+++ modules/profile/profile-listing.tpl.php
@@ -3,11 +3,7 @@
+ * HTML for a user and their profile data for member listing pages.

Same..

For the other examples above:
- Your patch needs a lot more wrapping
- Use always 3th person verbs

109 critical left. Go review some!

Thanks for the review, Bram! But I'm not sure what you want me to correct.

Your patch needs a lot more wrapping

Meaning what?

Use always 3th person verbs

Meaning I use verb like "returns"? That's the standard, isn't it? Hooks use "Implements" and we're trying to get theme functions to use "Returns". Or did you mean something else?

As for why the template files use "HTML for a…" instead of "Returns HTML for a…", I explained my reasoning in #21 above. But its up for discussion.

I said all theme functions not returning HTML have @return statements, but I forgot theme_syslog_format(). This patch corrects that.

@jhodgdon: You sounded like you'd agree? I think we've reached a very sensible and solid consensus here.

We therefore should update that documentation chapter. Also, would it make sense to split all the theme hook docblock changes from the main themeable group documentation? The former can probably go in quickly, while the latter usually needs a couple of reviews and re-rolls. We could do both in this very issue, no need for a separate one.

Ok. I've implemented all the suggestions in #26 and #27 in my working copy, but I've also decided to split this patch up into chunks like Sun suggests.

First patch: Fix all the theme function docs.
Second patch: Fix all the template docs.
Third patch: Fix other theme docs.

The first two patches should go quickly. Here's the first patch. It:

1. Adds several theme functions that weren't in @themeable.
2. Removes @themeable from functions that shouldn't need it.
3. Moves the preprocess functions for theme_username to the bottom of theme.inc so that they are no longer inside the @ingroup themeable {@ @} grouping.
4. Changes all of the theme function docblock short descriptions to use "Returns HTML for…" (also makes sure they are on one line). Unless the function doesn't return HTML; in which case, both the short description and the @return statement specify what is returned instead of HTML.
5. Adds @param documentation for all theme functions that were missing it.
6. Removes @return statements for theme functions that return HTML.

That's still a lot to review (86KB down from 137KB), but the patch is more focused and should be easier to review.

Committed to CVS HEAD. Thanks.

Soooo...based on jhodgdon's comment in #37 I'm assigning this to myself for the code sprint day -- I hope I'm not stepping on anyone's toes :)

Here is a patch that updates all the .tpl.php files that weren't already updated.

Setting this back to unassigned since the test passed. Still needs review, of course.

Rerolled patch with the two files I missed.

Awesome! Glad I could help, even if it was with this silly little patch :)

Oops. Fixing status back.

I think I did this correctly...at the very least this little issue is giving me lots of practice with git and core patches!

+++ b/core/modules/comment/comment-wrapper.tpl.php
@@ -33,6 +33,8 @@
+ * @ingroup themable

I'm not a 100% sure, but looking at other modules such as core/modules/user/user.module, I think this should be themeable not themable. This will need fixed for all of the lines.

Re-rolled. Applies cleanly at ea1b955.

Addresses #49.