Problem/Motivation

There are documentation standards for "Themeable functions" (theme_*() functions), but not preprocess functions. Part of the Twig conversion process is removing themeable functions and in most cases, replacing them with new preprocess functions, so there will be many more preprocess functions in core.

Proposed resolution

Template preprocess functions (template_preprocess_container(), for example) are documented using these standards:

  • The first line should be "Prepares variables for [description of foo] templates."
  • The second line should be "Default template: foo-bar.html.twig" where foo-bar.html.twig is the filename of the default template file.
  • All components of the $variables array need to be documented.

Syntax example:

/**
 * Prepares variables for container templates.
 *
 * Default template: container.html.twig.
 *
 * @param array $variables
 *   An associative array containing:
 *   - element: An associative array containing the properties of the element.
 *     Properties used: #id, #attributes, #children.
 */
function template_preprocess_container(&$variables) {

Remaining tasks

User interface changes

n/a

API changes

n/a

From the Twig sandbox: #1825464: [meta] Documentation blocks for preprocess functions (duplicate)

Other suggestions

Implements template_preprocess_HOOK() for container.html.twig.
Preprocess variables for templates like container.html.twig.

Comments

jhodgdon’s picture

Title: Standardize preprocess function documentation » [policy] Standardize preprocess function documentation
Issue tags: +Coding standards

I think that the first line should be:
Prepares variables for node.twig.
(or whatever the file name is that contains this template). That will make a link to the filename on api.drupal.org, and is closer to what we were doing for d7.

Actually, looking at the documentation for theme(), we should probably make the first line say:

Implements template_preprocess_HOOK() for node.twig.

That would be even closer to what we're doing for d7.

star-szr’s picture

Thanks @jhodgdon!

So if the first line contains the name of the template file would the @see be unnecessary, or should we have both?

jhodgdon’s picture

If the first line has the template file, the @see would be unnecessary. I also think the wordy paragraph in your sample currently in the issue summary is totally unnecessary. And @ingroup themeable is incorrect -- that should be in the template file, not this function, right?

jhodgdon’s picture

Issue summary: View changes

Add code tags.

star-szr’s picture

Issue summary: View changes

Better example, removed @see.

star-szr’s picture

Issue summary: View changes

Updated bullet points

star-szr’s picture

template_preprocess_node() probably wasn't the best example to use, I agree most preprocess functions shouldn't have that paragraph at all.

Because theme_ functions are largely going away in favour of preprocess functions and templates, I think the @ingroup themeable makes sense so these functions show up at http://api.drupal.org/api/drupal/core%21modules%21system%21theme.api.php....

Updated the proposed resolution in the summary to show a better example, removed the @see, updated the bullet points.

star-szr’s picture

Issue summary: View changes

Consistency, just stick with template_preprocess_container()

jhodgdon’s picture

RE #4 - I disagree. What we want to show up in the themeable group is the twig templates, which are the primary thing that a theme would be overriding, and our standard for twig templates indeed says they need @ingroup themeable in them (as we used to do for theme_* functions and *.tpl.php in D7).

We don't want two entries for each themeable item, which is what you would get if you put @ingroup themeable in the preprocess functions too.

jhodgdon’s picture

Issue summary: View changes

Add missing period.

star-szr’s picture

Issue summary: View changes

Remove @ingroup themeable

star-szr’s picture

Okay, that makes sense to me, updated the issue summary. Thanks for your continued feedback on this!

jhodgdon’s picture

The only other suggestion I had is at the end of #1 for the first line, which would make it more like what we were doing for D7.

Implements template_preprocess_HOOK() for node.twig.

If you look at the documentation for theme(), this "hook" is defined there. However, we don't actually have a "hook" definition for it in theme.api.php, so it doesn't turn into a link... we could though!

star-szr’s picture

Issue tags: +Twig

(Adding Twig tag.)

Re: #7, is the idea to add a template_preprocess_HOOK() function to theme.api.php so that it turns into a link? Would we (eventually) add all the possible preprocess/process functions (listed in the theme() documentation) to theme.api.php so they are all linked?

jhodgdon’s picture

We could definitely do what is suggested in #8, and yes it would be a good idea. Let's file a separate issue on that though.

jhodgdon’s picture

Issue summary: View changes

Add missing parenthesis.

star-szr’s picture

Okay, that makes sense to me. Updated the issue summary, for the other issue I did a quick search and found #1025528: Need documentation for the theme process/preprocess functions, can we maybe re-open that?

star-szr’s picture

Issue summary: View changes

Revise example per #7.

star-szr’s picture

I just added 'array' to the @param, missed that before.

jhodgdon’s picture

Status: Active » Needs review

Good idea on #10, did that. And good addition to the summary.

I think the proposal here is ready for general review. I am +1 on adopting it, especially since it's pretty much what we're doing now.

jhodgdon’s picture

Issue summary: View changes

Add array to @param.

star-szr’s picture

Issue summary: View changes

Removing quote, moving issue link to "Related issues" section

star-szr’s picture

Issue summary: View changes

Removing comment # from related issue link

jenlampton’s picture

Just noting here what we'd previously decided on to get feedback from jhodgdon :)

In the first line of docs for a preprocess function, we deliberately left out the template file's name, since in D8 preprocess functions would continue to work for theme engines other than Twig (we expect there will be a number of stickers who won't give up on PHPTemplate). Because of that, we decided not include the .html.twig or the .tpl.php in the docs at all, but instead reference the template by name or description. For example:

/**
 * Preprocess variables for a list of recent content.
 */
function template_preprocess_node_recent_block(&$variables) {
/**
 * Preprocess variables for select form elements.
 *
 * @param $variables
 *   An associative array containing:
 *   - element: An associative array containing the properties of the element.
 *     Properties used: #title, #value, #options, #description, #extra,
 *     #multiple, #required, #name, #attributes, #size.
 */
function template_preprocess_select(&$variables) {

If we don't include the file name in the first line, do we then need to add the @see?

jhodgdon’s picture

The problem with #13 is that it doesn't link you to the template that is actually in use in Core. Sure, you can use other theme engines and maybe people will do that, but there should be a link to the default template file in Core whether it is tpl.php or twig. It is OK with me if we follow the ideas in #13 and put in an @see to the file/function that (again, by default) implements it... and I see the logic somewhat of not putting this in the first line, but I still think the first-line solution is more streamlined?

star-szr’s picture

@jenlampton - Thanks for chiming in, not trying to step on any toes here :)

Consider this example: if we take a theme function and convert it to a Twig template, the first line from the theme function will be moved to the template file docblock, maybe with some minor changes. I prefer the idea of linking directly to the template from the first line in the preprocess docblock rather than repeating the description again. PHP template files won't be in core anyway, so I think I'm missing how this style of documentation would adversely affect PHPTemplate users, an example might help understand that scenario.

jenlampton’s picture

Issue summary: View changes

Add missing word

jenlampton’s picture

The problem I have with Preprocess variables for container.html.twig. is that it's explicit.

In reality we are also preprocessing variables for a template provided by another theme engine, and every override either one provides, as well as any other theme hook suggestion, like container--foo.html.twig. Maybe the problem here is the English we're using around the template file name and not the inclusion of the name itself.

How about...

/**
 * Preprocess variables for templates like container.html.twig.
 *
 * @param array $variables
 *   An associative array containing:
 *   - element: An associative array containing the properties of the element.
 *     Properties used: #id, #attributes, #children.
 */
function template_preprocess_container(&$variables) {
jhodgdon’s picture

I see your point (#16)... but I am not too fond of that wording either. So, how about this?

/**
 * Preprocesses variables for theming a [description of foo].
 *
 * Theme hook: theme('foo'); default core template: foo.html.twig.
 *
 * @param array $variables
 *   An associative array containing:
 *   - element: An associative array containing the properties of the element.
 *     Properties used: #id, #attributes, #children.
 */
function template_preprocess_foo(&$variables) {

I think this would accomplish:
- Clear language in the first line
- Tells you exactly what the theme call is
- Tells you what the default core template is and links to it
- Does not imply erroneously that Twig is the only option for templating
- Concise and easy to replicate

Thoughts?

jenlampton’s picture

I like this, but are we okay with the extra lines in the docs? :)

jhodgdon’s picture

RE #18 - given the (quite valid and important) objections you raised above, and the information I think we need to convey in the docs, I am OK with the one extra line added to #17. We have similar standards for adding a special line like this for a few function-docs special cases already on:
http://drupal.org/node/1354#functions
(mostly for the "callbacks" section)

star-szr’s picture

I'm mostly on board with #17, a few thoughts:

  1. I don't think we need the theme hook in each of these functions, or the word "core".
  2. The prefix text in the summary seems too long at 37 characters including the trailing space given the 80 character summary limit.

So for the summary line I'm proposing we go back to the original proposal. This gets us down to 33 characters for the prefix and suffix text including whitespace and punctuation.

Something like this:

/**
 * Prepares variables for database log message template.
 *
 * Default template: dblog-message.html.twig.
 *
 * @param array $variables
 *   An associative array containing:
 *   - element: An associative array containing the properties of the element.
 *     Properties used: #id, #attributes, #children.
 */
function template_preprocess_dblog_message(&$variables) {
star-szr’s picture

Issue summary: View changes

update issue summary with new proposed resolution?

jenlampton’s picture

Issue summary: View changes

add new suggestion

jenlampton’s picture

I love it, let's see what jhodgdon thinks :)

star-szr’s picture

@jhodgdon - One question - does the leading * count towards the 80 character limit on the summary line? I'm guessing yes but want to be sure.

jhodgdon’s picture

I like #20 almost -- can we make it "templates" at the end of the first line, or else get a "the" in there? As it is, it's not quite grammatical. OK on omitting the theme(xyz) reference and the word "core", especially since this standard will/should apply to contrib templates as well.

And yes, the 80 character limit includes the * and any leading whitespace.

steveoliver’s picture

To @jhodgdon's point in #23: let's do 'templates', as we're preparing variables for one of a number of different possible base or suggested templates.

/**
* Prepares variables for database log message templates.
*
* Default template: dblog-message.html.twig.
*
* @param array $variables
*   An associative array containing:
*   - element: An associative array containing the properties of the element.
*     Properties used: #id, #attributes, #children.
*/
function template_preprocess_dblog_message(&$variables) {
steveoliver’s picture

Issue summary: View changes

update steps

star-szr’s picture

Thanks, sounds good to me. Updated issue summary for new proposed standard.

star-szr’s picture

Issue summary: View changes

Update proposed resolution per #20 through #24.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

OK, I think all 4 of us who have so far shown interest in this issue by commenting on it are in agreement on #25 (which is also in the issue summary now, thanks Cottser!). So, I'm setting this to RTBC. If no one objects in 3-5 days, we'll consider this adopted and I'll update the standards docs.

As a note, I think we should update node/1354 (API docs standards), as well as... wasn't there a page somewhere describing how to convert foo.tpl.php and theme_foo() into Twig -- that might need an update too.

jenlampton’s picture

love it!

The conversion instructions actually live in this google doc, I'll update it.

star-szr’s picture

In the near future I'll be working on creating a d.o handbook page based on that Google doc, because I think it will be valuable during conversion and in the longer term (i.e. for contrib).

jhodgdon’s picture

Somewhere under
http://drupal.org/update/theme
would be a good location for this.

star-szr’s picture

Awesome, thanks @jhodgdon!

jenlampton’s picture

Cottser++

Fabianx’s picture

Late to the game: +1 RTBC - great work!

star-szr’s picture

Friendly bump, no objections in over a month :)

jwilson3’s picture

Late to party and I know this is splitting hairs, but what if we frontload that first line description so that the copy/paste/edit/replace between templates is easy, and so people that look at docs everyday can scan to the end of the line to see the important stuff, which is consistent with "Implements [foo]" used elsewhere.

Instead of:

* Prepares variables for [description of foo] templates.

Maybe:

* Prepares template variables for rendering [foo]s.

This works cleanly for various examples:

Prepares template variables for rendering nodes.
Prepares template variables for rendering comments.
Prepares template variables for rendering comment wrappers.
Prepares template variables for rendering database log messages.
Prepares template variables for rendering tables.
Prepares template variables for rendering unordered lists.
Prepares template variables for rendering generic HTML containers.

steveoliver’s picture

jwilson, #34, sorry but I don't see anything gained in this, less natural language.

-1

joelpittet’s picture

@jwilson3 to add to steveoliver's point the summary line is to fit on one line and I am already running into a few where it's starting to become difficult to fit the summary on one line, so I am going to have to say no to making it longer.

tim.plunkett’s picture

We had standards for this. They were (accidentally) deleted. See #1443202: Apply documentation standards for hook_preprocess_HOOK() and https://drupal.org/node/1354/revisions/view/2415402/2546068, searching for hook_preprocess_HOOK

star-szr’s picture

Title: [policy] Standardize preprocess function documentation » [policy] Standardize template preprocess function documentation

Thanks @tim.plunkett, that's great to know and I think those should be re-added to 1354.

Re-titling this issue to clarify intent. I think modules/themes implementing preprocess hooks can use the existing hook_preprocess_HOOK() standards, but IMO template_preprocess functions need their own format. The issue summary already refers to "template preprocess functions".

jhodgdon’s picture

Due to all of the discussion above, I do not think we want to put the previous standard back into node/1354, and this issue is proposing a new standard (see issue summary). It has been at RTBC for a while but hardly anyone has commented on it. Any thoughts?

tim.plunkett’s picture

The old standard covered both hook_preprocess_HOOK() and template_preprocess_HOOK(). The former made sense, while the latter did not.

This just covers template_preprocess_HOOK(), and is a great improvement.

So RTBC+1 for this dictating the method for documenting template_preprocess_HOOK(), but we should also restore the old docs standard for hook_preprocess_HOOK().

jhodgdon’s picture

Um... So you think the old standard for hook_preprocess_HOOK should be left alone? Shouldn't it be updated similar to the new standard for template_preprocess_HOOK()?

tim.plunkett’s picture

Well it can be updated to refer to template_preprocess_HOOK. But if my module is adding onto the preprocessing, I shouldn't follow the full doc standard and @param and duplicate all of that.

And by "old standard", I mean the one that is gone because it was deleted.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

This has been sitting here for a long time. I just updated the standards doc:
http://drupal.org/coding-standards/docs#themepreprocess

star-szr’s picture

Thank you @jhodgdon!

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

Anonymous’s picture

Issue summary: View changes

Denoting duplicate

neardark’s picture

Issue summary: View changes

Fixed typo.