Reading through the following code was the only way I could see to learn about theme engines.

http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/theme...
https://github.com/damz/mustache-drupal/blob/master/mustache.engine
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/inclu...

From what I see, a theme engine requires implementing two hooks, ENGINE_init() and ENGINE_theme(). They are documented as hook_init() and hook_theme, and mentioned on the docs for theme().

But ENGINE_extension and ENGINE_render_template also exist, aren't available to themes or modules, and aren't documented at all.

Files: 
CommentFileSizeAuthor
#27 interdiff.txt3.21 KBCottser
#27 1332068-27.patch2.73 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,859 pass(es). View
#21 interdiff.txt1.68 KBCottser
#21 1332068-21.patch2.86 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,470 pass(es). View
#15 interdiff.txt1.96 KBCottser
#15 1332068-15.patch3.11 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,316 pass(es). View
#14 1332068-14.patch3.36 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,268 pass(es). View
#11 interdiff-1332068-7-11.txt5.85 KBpixelmord
#11 Drupal-document_theme_engine_hooks-1332068-11.patch3.32 KBpixelmord
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch Drupal-document_theme_engine_hooks-1332068-11.patch. Unable to apply patch. See the log in the details link for more information. View
#10 interdiff-1332068-5-7.txt2.13 KBpixelmord
#7 Drupal-document_theme_engine_hooks-1332068-7.patch5.63 KBpixelmord
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,805 pass(es). View
#5 Drupal-document_theme_engine_hooks-1332068-5.patch5.24 KBpixelmord
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,807 pass(es). View

Comments

tim.plunkett’s picture

Issue summary: View changes
Issue tags: +Twig

Since we had to add a new implementation for Twig, maybe someone from the Twig team can help out.

Cottser’s picture

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

Cottser’s picture

Issue tags: +documentation

Adding docs tag.

pixelmord’s picture

Assigned: Unassigned » pixelmord
pixelmord’s picture

Status: Active » Needs review
FileSize
5.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,807 pass(es). View

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

Please review

Cottser’s picture

Status: Needs review » Needs work

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).

pixelmord’s picture

Issue tags: +sprint
FileSize
5.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,805 pass(es). View

@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 ;)

pixelmord’s picture

Status: Needs work » Needs review

Update status

Cottser’s picture

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 :)

pixelmord’s picture

Status: Needs review » Needs work
FileSize
2.13 KB

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.

pixelmord’s picture

Status: Needs work » Needs review
FileSize
3.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch Drupal-document_theme_engine_hooks-1332068-11.patch. Unable to apply patch. See the log in the details link for more information. View
5.85 KB

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

Status: Needs review » Needs work

The last submitted patch, 11: Drupal-document_theme_engine_hooks-1332068-11.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
FileSize
3.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,268 pass(es). View

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.

Cottser’s picture

FileSize
3.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,316 pass(es). View
1.96 KB

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.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

To go, it is!

alexpott’s picture

Component: theme system » documentation
Assigned: pixelmord » jhodgdon
jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

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!

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Cottser’s picture

Assigned: Unassigned » Cottser

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

Cottser’s picture

Assigned: Cottser » Unassigned
Status: Needs work » Needs review
FileSize
2.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,470 pass(es). View
1.68 KB

New patch incorporating feedback from #18.

jhodgdon’s picture

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!

Cottser’s picture

Assigned: Unassigned » Cottser
Status: Needs review » Needs work

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

Cottser’s picture

Status: Needs work » Needs review

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.

jhodgdon’s picture

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.

Cottser’s picture

Status: Needs review » Needs work

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

Cottser’s picture

Assigned: Cottser » Unassigned
Status: Needs work » Needs review
FileSize
2.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,859 pass(es). View
3.21 KB

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

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC, thank you @Cottser and @jhodgdon

alexpott’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed be15db1 and pushed to 8.0.x. Thanks!

  • alexpott committed be15db1 on 8.0.x
    Issue #1332068 by Cottser, pixelmord | tim.plunkett:...

  • alexpott committed be15db1 on 8.1.x
    Issue #1332068 by Cottser, pixelmord | tim.plunkett:...

  • alexpott committed be15db1 on 8.3.x
    Issue #1332068 by Cottser, pixelmord | tim.plunkett:...

  • alexpott committed be15db1 on 8.3.x
    Issue #1332068 by Cottser, pixelmord | tim.plunkett:...

  • alexpott committed be15db1 on 8.4.x
    Issue #1332068 by Cottser, pixelmord | tim.plunkett:...

  • alexpott committed be15db1 on 8.4.x
    Issue #1332068 by Cottser, pixelmord | tim.plunkett:...