ENGINE_render_template() and ENGINE_extension() are undocumented

Thank you @tim.plunkett! This has been on my list of issues to create.

Adding docs tag.

Attached a patch that adds documentation for those hooks into system.api.php

Please review

Thank you @pixelmord! Here's an initial review:

1. +++ b/core/modules/system/system.api.php
   @@ -1230,6 +1230,89 @@ function hook_template_preprocess_default_variables_alter(&$variables) {
   + * Declare a template file extension to be used with a theme engine
   + *
   + * This hook is used in a theme engine implementation in the format of
   + * ENGINE_extension()
   ...
   + * @return string
   + *   The output generated by the template. In most cases this will be a string
   + *   containing HTML markup
   
   +++ b/core/themes/engines/twig/twig.engine
   @@ -18,6 +18,9 @@ function twig_theme($existing, $type, $theme, $path) {
   + * @return string
   + *  The file extension the Twig theme engine will recognize
   

   All documentation needs to be complete sentences (usually ending in a period) per https://drupal.org/node/1354#general.

2. +++ b/core/modules/system/system.api.php
   @@ -1230,6 +1230,89 @@ function hook_template_preprocess_default_variables_alter(&$variables) {
   + * @return string
   + *  The file extension the theme engine will recognize
   

   Need one more space of indent for the @return description (https://drupal.org/node/1354#return) and the description should be a complete sentence (ending in a period) per https://drupal.org/node/1354#general.

3. +++ b/core/modules/system/system.api.php
   @@ -1230,6 +1230,89 @@ function hook_template_preprocess_default_variables_alter(&$variables) {
   +  // Extension for template base names in Twig
   +  return '.html.twig';
   ...
   +  $twig_service = \Drupal::service('twig');
   +  $output = array(
   +    'debug_prefix'    => '',
   +    'debug_info'      => '',
   +    'rendered_markup' => $twig_service->loadTemplate($template_file)->render($variables),
   +    'debug_suffix'    => '',
   +  );
   

   Let's please not duplicate twig_extension() and twig_render_template() for these docs. It might actually make sense to copy these from PHPTemplate instead if we're going to copy+paste :)

4. +++ b/core/modules/system/system.api.php
   @@ -1230,6 +1230,89 @@ function hook_template_preprocess_default_variables_alter(&$variables) {
   + * This hook is used in a theme engine implementation in the format of
   + * ENGINE_render_template()
   + * Here the theme engine takes care of rendering the contents of a template with the
   + * provided variables.
   

   To make a new paragraph add a blank line (https://drupal.org/node/1354#general) and the first line is not a complete sentence. The phrasing "Here the theme engine" is a bit strange, maybe we can rephrase this line a bit? Here's a shot:

   This hook is used in a theme engine implementation in the format of ENGINE_render_template(). It takes in an array of variables and returns a string containing the rendered template.

5. +++ b/core/modules/system/system.api.php
   @@ -1230,6 +1230,89 @@ function hook_template_preprocess_default_variables_alter(&$variables) {
   + * @param $template_file
   + *   The file name of the template to render.
   

   Can we specify whether or not the extension should be included? I think that would be helpful. Also this needs a @param type per https://drupal.org/node/1354#param.

6. +++ b/core/modules/system/system.api.php
   @@ -1230,6 +1230,89 @@ function hook_template_preprocess_default_variables_alter(&$variables) {
   + * @param $variables
   + *   A keyed array of variables that will appear in the output.
   

   The variables won't necessarily be included in the output. I think we should just say that the variables will be passed to the template. This should also document the @param type (array).

@Cottser: thanks for your review and reminding me of those things. DOT ;)

Attached is another patch with updated doc.

I disagree with you on the point to use the PHPtemplate implementations as an example, because even if the implementation for Twig might still be changed in the future, they are the better example, because here a 'real' template engine is used and if anybody wants to use this documentation for adding some other template engine, the code will be much more similar. See the linked sandbox with the mustache engine implementation.

Also it makes much more sense to me to find documentation to something that is actually used ;)

Update status

Ok, I'm fine with including Twig as an example but can we please take out the twig_debug stuff? That makes it really big. It could be one line of code I think without twig_debug.

/me wants to create an issue to pull out all that twig_debug logic into at least a separate function…

Please link up this mustache sandbox! And interdiffs are everyone's friend when updating patches :)

Ok here's the interdiff for convenience for the two previous patches 5-7.

@Cottser: the link to the mustache code is in the summary.

Here is a new patch with interdiff that uses simple code as a twig example

Fairly easy reroll, so here it is. I found some more minor changes that I will roll in but I think this is looking pretty good now. Thanks @pixelmord (and thanks for bumping @anavarre), I should have come back to this sooner.

Attached patch incorporates these fairly minor changes. This is ready to go in my opinion.

1. +++ b/core/modules/system/system.api.php
   @@ -1241,6 +1241,47 @@ function hook_template_preprocess_default_variables_alter(&$variables) {
   +  // Extension for template base names in Twig
   

   Missing period.

2. +++ b/core/modules/system/system.api.php
   @@ -1241,6 +1241,47 @@ function hook_template_preprocess_default_variables_alter(&$variables) {
   + * @param string template_file
   

   Missing $ in front of variable name.

3. +++ b/core/modules/system/system.api.php
   @@ -1241,6 +1241,47 @@ function hook_template_preprocess_default_variables_alter(&$variables) {
   + *   drupal root directory.
   

   s/drupal/Drupal/

4. +++ b/core/modules/system/system.api.php
   @@ -1241,6 +1241,47 @@ function hook_template_preprocess_default_variables_alter(&$variables) {
   + *   If all or just a subset of those variables are used in the template may
   + *   vary depending on the code in the template.
   

   I just reworded this, I found it a bit hard to read.

5. +++ b/core/themes/engines/twig/twig.engine
   @@ -20,6 +20,9 @@ function twig_theme($existing, $type, $theme, $path) {
   + *
   + * @return string
   + *  The file extension the Twig theme engine will recognize
   

   The line after @return needs to be indented by one more space. But actually I don't think these docs are necessary, the docs in hook_extension() can be referred to instead. I removed these lines.

To go, it is!

I have a few concerns with this patch:

a) When declaring hooks, we normally want verbs like "Declare" not "Declares". See
https://www.drupal.org/node/1354#hooks

b)

+ * This hook is used in a theme engine implementation in the format of
+ * ENGINE_render_template(). It gets passed a template name and an array of
+ * variables and returns a string containing the rendered template.
...
+function hook_render_template($template_file, $variables) {

The second sentence of the description paragraph is not necessary, since the @param and @return sections give you this information. Please omit. Also I think it's a bit misleading the way it's worded; the @param/@return are better IMO.

c)

+ * @param string $template_file
+ *   The relative path to the template to be rendered including its extension in
+ *   the format 'path/to/TEMPLATE_NAME.EXT'. The path should be relative to the
+ *   Drupal root directory.

Um... "should be"??!? You are describing a hook here. You need to tell hook implementers what they can count on being passed in, not what they *should* expect. Also I think maybe these two sentences can be combined?

Looks good other than that!

Thanks for the review @jhodgdon, I will take a look at those points.

New patch incorporating feedback from #18.

Oh sorry, one more thing: Those hooks should be in theme.api.php I think, not system.api.php? All the hooks related to the Theme system are normally put into theme.api.php.

The rest looks great, thanks!

Oh I definitely should have picked up on that when reviewing.

Actually I think upon closer inspection that either these belong in system.api.php, or (some of?) the following hooks should maybe be moved to theme.api.php as well:

hook_theme
hook_theme_registry_alter
hook_template_preprocess_default_variables_alter

Or maybe there is some special consideration needed because these are purely "theme engine" hooks and not valid for use by modules or themes.

They all belong in theme.api.php. See #2307859: Move theme/render hooks from system.api.php to theme.api.php -- which needs review -- for the existing hooks that are in the wrong place.

Excellent, thanks for that, I wasn't aware of the other issue. I will review that and update the patch here.

Moved the hook docs to theme.api.php. No other changes.

Back to RTBC, thank you @Cottser and @jhodgdon

Committed be15db1 and pushed to 8.0.x. Thanks!