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
- Decide on standards.
- Update http://drupal.org/node/1354 as necessary.
- Update drupal core to match
User interface changes
n/a
API changes
n/a
Related issues
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
Comment #1
jhodgdonI 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.
Comment #2
star-szrThanks @jhodgdon!
So if the first line contains the name of the template file would the
@see
be unnecessary, or should we have both?Comment #3
jhodgdonIf 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?
Comment #3.0
jhodgdonAdd code tags.
Comment #3.1
star-szrBetter example, removed @see.
Comment #3.2
star-szrUpdated bullet points
Comment #4
star-szrtemplate_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.
Comment #4.0
star-szrConsistency, just stick with template_preprocess_container()
Comment #5
jhodgdonRE #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.
Comment #5.0
jhodgdonAdd missing period.
Comment #5.1
star-szrRemove @ingroup themeable
Comment #6
star-szrOkay, that makes sense to me, updated the issue summary. Thanks for your continued feedback on this!
Comment #7
jhodgdonThe 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!
Comment #8
star-szr(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?
Comment #9
jhodgdonWe 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.
Comment #9.0
jhodgdonAdd missing parenthesis.
Comment #10
star-szrOkay, 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?
Comment #10.0
star-szrRevise example per #7.
Comment #11
star-szrI just added 'array' to the
@param
, missed that before.Comment #12
jhodgdonGood 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.
Comment #12.0
jhodgdonAdd array to @param.
Comment #12.1
star-szrRemoving quote, moving issue link to "Related issues" section
Comment #12.2
star-szrRemoving comment # from related issue link
Comment #13
jenlamptonJust 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:
If we don't include the file name in the first line, do we then need to add the @see?
Comment #14
jhodgdonThe 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?
Comment #15
star-szr@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.
Comment #15.0
jenlamptonAdd missing word
Comment #16
jenlamptonThe 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...
Comment #17
jhodgdonI see your point (#16)... but I am not too fond of that wording either. So, how about this?
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?
Comment #18
jenlamptonI like this, but are we okay with the extra lines in the docs? :)
Comment #19
jhodgdonRE #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)
Comment #20
star-szrI'm mostly on board with #17, a few thoughts:
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:
Comment #20.0
star-szrupdate issue summary with new proposed resolution?
Comment #20.1
jenlamptonadd new suggestion
Comment #21
jenlamptonI love it, let's see what jhodgdon thinks :)
Comment #22
star-szr@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.Comment #23
jhodgdonI 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.
Comment #24
steveoliver CreditAttribution: steveoliver commentedTo @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.
Comment #24.0
steveoliver CreditAttribution: steveoliver commentedupdate steps
Comment #25
star-szrThanks, sounds good to me. Updated issue summary for new proposed standard.
Comment #25.0
star-szrUpdate proposed resolution per #20 through #24.
Comment #26
jhodgdonOK, 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.
Comment #27
jenlamptonlove it!
The conversion instructions actually live in this google doc, I'll update it.
Comment #28
star-szrIn 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).
Comment #29
jhodgdonSomewhere under
http://drupal.org/update/theme
would be a good location for this.
Comment #30
star-szrAwesome, thanks @jhodgdon!
Comment #31
jenlamptonCottser++
Comment #32
Fabianx CreditAttribution: Fabianx commentedLate to the game: +1 RTBC - great work!
Comment #33
star-szrFriendly bump, no objections in over a month :)
Comment #34
jwilson3Late 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:
Maybe:
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.
Comment #35
steveoliver CreditAttribution: steveoliver commentedjwilson, #34, sorry but I don't see anything gained in this, less natural language.
-1
Comment #36
joelpittet@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.
Comment #37
tim.plunkettWe 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
Comment #38
star-szrThanks @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".
Comment #39
jhodgdonDue 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?
Comment #40
tim.plunkettThe 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().
Comment #41
jhodgdonUm... 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()?
Comment #42
tim.plunkettWell 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.
Comment #43
jhodgdonThis has been sitting here for a long time. I just updated the standards doc:
http://drupal.org/coding-standards/docs#themepreprocess
Comment #44
star-szrThank you @jhodgdon!
Comment #45.0
(not verified) CreditAttribution: commentedDenoting duplicate
Comment #46
neardark CreditAttribution: neardark commentedFixed typo.