Apply documentation standards for hook_preprocess_HOOK()

It seems to me we might want to add a more explicit standard for how to document these at http://drupal.org/node/1354#themeable. A link to hook_preprocess_HOOK() (which, btw, I think is mislabeled anyway) would be clearer. The information the user really needs is in the documentation for theme(), I think.

Agreed, we need a better standard, and agreed, this function should not say it is implementing a nonexistent hook.

For FORM_ID_alter hooks, I think our standard is something like:
Implements hook_form_FORM_ID_alter() for form_whatever_it_is().

Perhaps we could do that here as well, or maybe something like:

Implements a theme() preprocess function for blocks.

Thoughts?

Assuming we like hook_preprocess_HOOK, I think Implements hook_preprocess_HOOK() for PLURAL_HOOK., like

/**
 * Implements hook_preprocess_HOOK() for blocks.
 */
function mymodule_preprocess_block(&$variables) {
}

#3 -- That's fine for hook_preprocess_HOOK(), but it doesn't do much for the other functions listed on the theme() documentation page that don't have special hook docs pages... Oh actually I guess all of them that belong in modules do, so maybe this is fine...

OK, I'm +1 on #3. Any other opinions?

I think #3 is good also.

The only problem with pluralizing the hook is when the hook isn't just a single word.

In the case of function MYMODULE_preprocess_panels_pane(&$variables) {}

1. Implements hook_preprocess_HOOK() for panels_pane. (just the hook)
2. Implements hook_preprocess_HOOK() for panels_panes. (attempt to pluralize the hook)
3. Implements hook_preprocess_HOOK() for panels panes. (attempt to pluralize and English-ify the hook)

The hook_form_FORM_ID_alter standard is closest to #6.3 from above, so I guess that is the correct way to go?

But that leads to

/**
 * Implements hook_preprocess_HOOK() for FullCalendars.
 */
function MYMODULE_preprocess_fullcalendar(&$variables) {}

Since FullCalendar is a camelCased proper noun (not my doing, btw).

I'd be personally most happy with #6.1, just take the $hook part and no pluralizing at all.

Honestly, I'm not sure how much of a problem this is in the first place.

But anyway, if you guys insist that it's a problem, then I'd go with the following: (similar to #6.1, but more explicit)

/**
 * Implements hook_preprocess_HOOK() for theme_panels_pane().
 */
function mymodule_preprocess_panels_pane(&$variables) {

This

1. automatically links to the general API docs about theme preprocess functions
2. automatically links to the specific theme function the preprocessor is for
   (thus, eliminating a potential @see)

Ohhh! Very good. That makes SO much sense.
#8++

#8 is OK; in the standard we should say to use whatever.tpl.php though if it's a template not a function, right?

#10, I'm not sure, because you still call those template files with theme('whatever', array());.

RE #12, yes, but you can't call theme_whatever(). You need to call theme('whatever') to get either the theme_whatever() or the whatever.tpl.php function.

So probably we should really do instead of #8:

/**
* Implements hook_preprocess_HOOK() for theme('panels_pane').
*/
function mymodule_preprocess_panels_pane(&$variables) {

With the current API module, this should (!!) automatically link to either theme_panels_pane() or panels-pane.tpl.php (whichever actually exists, with a preference for the latter).

In that case, I'd rather go with for theme_foo_bar() or for foo.tpl.php.

Functional code à la for theme('foo') doesn't make much sense for me, as it's only really understood by API module's parser.

I'm happy with #8/#14 or #13. #13 has the advantage of being what you would actually call in code, but #14 has the advantage of letting you know what the actual function is if you are living in the editor rather than on api.drupal.org. So probably a slight preference for #14.

I couldn't find this issue earlier, retitling.

Ah... So the two proposals for documentation headers for preprocess functions are:

a)

/**
* Implements hook_preprocess_HOOK() for theme_panels_pane().
*/
function mymodule_preprocess_panels_pane(&$variables) {

This assumes the theme_panels_pane() function exists; if it's actually a template you would say "for panels-pane.tpl.php".

b)

/**
* Implements hook_preprocess_HOOK() for theme('panels_pane').
*/
function mymodule_preprocess_panels_pane(&$variables) {

So far, I think sun and I have both voted for (a) -- the advantage is it links to the actual function/template where the variables and such are documented. Any dissenting opinions?

I'm wholly in support of #17a.

Tentatively setting the policy to RTBC. If no one objects in the next few days, I'll edit http://drupal.org/node/1354 to put this policy in. Then we might consider a patch...

Travis: Sounds good!

Could the summary please be updated to what the proposal is?

Updated issue summary.

Ah-ha! Yes, that makes total sense. Thanks! :)

OK, the new policy has been added to http://drupal.org/node/1354#hookimpl and I also put links in the theme sections to there.

Next up: a patch is needed. I'll update the issue summary.

Updated issue summary.

hook_preprocess_ENTITY and theme_ENTITY refer to a specific theme_HOOK and hook_preprocess_HOOK where HOOK is one of the ENTITY types. So, comment, node, user, taxonomy_term, etc.

It probably doesn't need to be documented separately, but it doesn't hurt.

What I would do with that comment in #30:
Alternatively, it could also implement hook_preprocess_ENTITY().
==>
Alternatively, it could also implement hook_preprocess_HOOK() for the particular entity type template, if there is one (e.g., node.tpl.php).

This whole line was a bit suspect... For instance, there is no user.tpl.php or taxonomy.tpl.php, so it wouldn't have worked to do hook_preprocess_ENTITY for those entity types.

So... looking at the patch in #30, you have the right idea. However, some of the "theme_" functions are actually nonexistent. For instance, there is no theme_block() -- that line should be
+ * Implements hook_preprocess_HOOK() for block.tpl.php.
You'll need to go through the patch and see which are really functions and which are templates.

Unfortunately, a simple search on api.drupal.org won't help you, because of a point of history that we are *nearly* ready to get rid of. There's this lame file called theme.php in the Documentation repository that has existed for ages to point people from nonexestent theme_xyz() functions to the proper xyz.tpl.php file. theme_block() is one of the fake functions in there. Basically, any "theme function" that is listed on that page needs to be replaced by the tpl.php file in your patch.

As soon as we finish with
#716496: Theme functions group needs some updates
I'll be getting rid of theme.php and we will no longer have this confusion.

Much better! I looked through this and found this little issue:

+ * callback.  Alternatively, it could also implement hook_preprocess_HOOK() for
+ * node.tpl.php. See  * drupal_render() and theme() documentation respectively
+ * for details.

- Should be one space not to after .
- * in the middle of the line

Otherwise, I think this is ready to go. Thanks!

sorry that was in

-- a/core/modules/node/node.api.php
+++ b/core/modules/node/node.api.php
@@ -834,8 +834,9 @@ function hook_node_view($node, $view_mode, $langcode) {

All of the implementations in locale and rdf were using MODULE_preprocess_HOOK, which is close but still wrong. Grabbed those too.

We are unfortunately over thresholds for critical/major issues right now, and I know this will conflict with at least one patch in the queue
#787652: Forum vocabulary alterations possibly obsolete -- possible to delete forum vocab

So I won't be able to commit it for now, and it will need a reroll once that patch lands.

However, on the good side, the patch looks really good. :) I reviewed it carefully and didn't find a single thing wrong with it (as picky as I am).

Interesting; so we don't actually have any theme_foo() for these in core? Templates only?

I agree that #36 looks great.

Ninja-removing some t() on the assertions we're updating. No other changes.

Oops, looks like it needs reroll already.

Rebased.

Moo.

RE #39 - we never have both a theme_* function and a *.tpl.php. In hook_theme() you indicate one or the other.

#43: 1443202-43.patch queued for re-testing.

Still good.

With this many files being touched, committing this will have to wait until we're under thresholds, unless someone wants to volunteer to figure out which needs review / RTBC critical/major issues in 8.x with patches need rerolling, and reroll them all after I apply this patch. :(

OK, we have a new policy on "large" patches. I plan to hit the "retest" link on Thursday May 3rd on this patch (i.e., wait until after the next 7.x release is out, to avoid delaying it, since many 8.x patches go directly to 7.x).

Then, if there are no "avoid commit conflicts" tagged issue in the queue that conflict, I'll give this a final review and commit it. If anyone wants to delay, now's your chance to request a delay (e.g., wait until this issue is in, which should be very soon). Or if there are some "avoid commit conflicts" issues, maybe we can get a volunteer or two to reroll just those patches.

#51: drupal-hook_preprocess_hook-1443202-51.patch queued for re-testing.

The patch still applied, and there were no uncommitted "avoid patch conflicts" patches in the queue... so I *finally* committed this patch. Thanks to all who participated, and for all of your patience in trying to do this kind of cleanup in a fast-moving development environment!

I think we decided we wouldn't try to port this to D7, so I'm just marking it fixed.

https://drupal.org/node/1354/revisions/view/2415402/2546068 removed this standard, and I can't find where it was moved to.

That standard got removed inadvertently (sorry about that!) but a new one is being discussed since that old one was never really formalized and didn't work well with Twig apparently:
#1913208: [policy] Standardize template preprocess function documentation

part 1 has been done, update summary accordingly