Make 'template' the default output option for hook_theme()

Trying to make the issue summary a bit more verbose :) Thanks for jumping in on this @euphoric_mv!

Oops, I must have been looking at D7 when I updated the issue summary. Updated to point to the correct method.

I think we should start by changing this code:

// If function and file are omitted, default to standard naming
// conventions.
if (!isset($info['template']) && !isset($info['function'])) {
  $result[$hook]['function'] = ($type == 'module' ? 'theme_' : $name . '_') . $hook;
}

This is setting the default to 'function' and generating a default function name. We want to do change this to do something very similar but for 'template'.

This definitely looks like the direction we want to go in! Sending to testbot (needs review).

+++ b/core/includes/theme.inc
@@ -615,6 +615,9 @@ function _theme($hook, $variables = array()) {
+    if (!file_exists($template_file)) {
+      return;
+    }

I think file_exists() is a pretty expensive (slow performance) check, if we're doing this check at all it should likely be in the theme registry rebuild and not in _theme() which could easily be called hundreds of times per request. But we might be able to just let PHP handle this case :)

I would lean towards no file_exists() check and just checking to see what happens when the file doesn't exist. I still think the built-in error handling of PHP and/or Twig will probably be sufficient here.

The patch looks like it's failing just because we're missing some lines where we need to specify the function name, for example in aggregator_theme(), book_theme(), etc.

We should also leave all the 'template' lines alone, let's do that in a follow-up issue please :)

Looks like this needs some more love, probably more 'function' lines need to be added. Need to search all hook_theme() implementations.

+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -426,7 +426,9 @@ protected function processExtension(&$cache, $name, $type, $theme, $path) {
-          $result[$hook]['function'] = ($type == 'module' ? 'theme_' : $name . '_') . $hook;
+          $template_name = strtr($hook, '_', '-');
+          $info['template'] = $template_name;
+          $result[$hook]['template'] = $template_name;

I don't think the $info['template'] line is needed based on the existing code.

Here's an example of an error message via testbot when a template file doesn't exist (this means we need to add a 'function' line where the 'username' theme hook is defined):

[30-Mar-2014 10:36:32 UTC] Uncaught PHP Exception Twig_Error_Loader: "Unable to find template "core/modules/user/templates/username.html.twig" (looked into: /var/lib/drupaltestbot/sites/default/files/checkout)." at /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/twig/twig/lib/Twig/Loader/Filesystem.php line 202

Edit: And so far that's the only template-related error I'm seeing so likely just updating user_theme() will improve the testbot outcome significantly.

Adding the 'function' lines step to the proposed resolution. Yes, this will break conversions in #1757550: [Meta] Convert core theme functions to Twig templates but the rerolls will be easy and I think this is an important change.

Draft change record: https://drupal.org/node/2231673

Some thoughts from @sun in IRC for making verification of this patch easier so we don't break anything:

I assume it could be scripted tho... basically a diff against HEAD and HEAD+patch, and separately on code bases, run a script that (1) scans for all .module files + include them, (2) calls hook_theme(), (3) records hook name + template/function + default, (4) produces a list. That would yield two lists that can be compared next to each other

Great, thanks @mgbellaire! If you have any questions along the way post them here or you can ping me in IRC if I'm online.

@mgbellaire Thanks! I wasn't able to respond to you in IRC the other day (traveling) but it looks like we lost the changes to theme.inc. You can compare or diff your patch against the one in 18 to see what I mean.

@mgbellaire thanks! I'd really encourage you again to include interdiffs. The changes to system.api.php aren't necessary here since the body of hook_theme() is just an example. I don't think we should update hook_theme() in this issue, but rather create a separate issue to replace 'forum_display' with the 'forums' declaration from forum_theme().

Looking at the test failures it looks like we're missing a 'function' line for more_link and views_theme() will need some special treatment because there is a foreach in there.

@mgbellaire - I think in views_theme() we want to tweak the logic to add 'function' lines to the Views plugins that have theme functions, almost the programmatic version of what we've been doing with the other patches to date on this issue.

Setting to needs review to try and get the last patch tested and see where we're at.

+++ b/core/modules/system/tests/modules/theme_suggestions_test/theme_suggestions_test.module
@@ -12,6 +12,7 @@ function theme_suggestions_test_theme() {
   $items['theme_suggestions_test_include'] = array(
...
+    'function' => 'theme_suggestions_test_include',

Testing theme APIs gets confusing with all the theme_theme action ;)

'function' should be 'theme_theme_suggestions_test_include' here.

Green! I'm going to work on that verification script and I realized tonight that the script could be useful for contrib module/theme developers as well so all the more reason to make it happen.

In the meantime, this needs some test coverage.

Rerolled.

Follow-up already existed (check child issues ;)) updating issue summary to point to it.

I want to review this again soon. This does need a reroll after some Twig conversions got in though, any takers? If so, feel free to reassign while you're working on it.

Thanks @LinL, here's some feedback on the reroll:

+++ b/core/includes/theme.inc
@@ -2551,6 +2551,7 @@ function drupal_common_theme() {
       'template' => 'mark',
+      'function' => 'theme_mark',

@@ -2596,23 +2604,31 @@ function drupal_common_theme() {
       'template' => 'input',
+      'function' => 'theme_input',

@@ -2652,6 +2669,7 @@ function drupal_common_theme() {
       'template' => 'form-element-label',
+      'function' => 'theme_form_element_label',

Can't do both 'template' and 'function' in hook_theme(). These were recently converted to Twig so we should keep the 'template' line that's in HEAD and not add a 'function' line.

There may be others like this in the patch.

This is looking great overall, I have a couple questions that are probably more for @Mark Carver so I'm assigning to him.

1. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -439,20 +436,36 @@ protected function processExtension(&$cache, $name, $type, $theme, $path) {
   +        // Templates are the default implementation for a theme hook. If a
   +        // theme hook provides a theme function callback, check to ensure that
   +        // it actually exists.
   +        if (isset($info['function']) && !function_exists($info['function'])) {
   +          throw new \BadFunctionCallException(sprintf(
   +            'Theme hook "%s" refers to a theme function callback that does not exist: "%s"',
   +            $hook,
   +            $info['function']
   +          ));
   +        }
   +        // Provide a default naming convention for 'template' based on the
   +        // hook used. If the template does not exist, the theme engine used
   +        // should throw an exception at runtime when attempting to include
   +        // the template file.
   +        elseif (!isset($info['template'])) {
   +          $result[$hook]['template'] = strtr($hook, '_', '-');
   +        }
   

   I guess we are not handling the case where neither 'function' or 'template' is specified in hook_theme() and no template exists, because it seems like that will only surface when trying to actually use the theme hook/output. Is there any way we can handle that without doing a file_exists()? Off the top of my head I can't think of a way but maybe I'm missing how we are catching these cases now.

2. +++ b/core/modules/views/views.module
   @@ -173,35 +174,44 @@ function views_theme($existing, $type, $theme, $path) {
   -      if (isset($def['theme_file'])) {
   -        $hooks[$def['theme']]['path'] = $module_dir;
   +      elseif (!empty($def['theme_file'])) {
   

   Making this an elseif seems questionable to me because it's modifying a different array key than the code above, what's the reasoning?

Small conflict in simpletest.module after #1898452: simpletest.module - Convert theme_simpletest_result_summary functions to Twig got in, here's a reroll that just removes this now unnecessary hunk:

diff --git a/core/modules/simpletest/simpletest.module b/core/modules/simpletest/simpletest.module
index 96ab21b..9cb2e6c 100644
--- a/core/modules/simpletest/simpletest.module
+++ b/core/modules/simpletest/simpletest.module
@@ -56,6 +56,7 @@ function simpletest_theme() {
     'simpletest_result_summary' => array(
       'render element' => 'form',
       'file' => 'simpletest.theme.inc',
+      'function' => 'theme_simpletest_result_summary',
     ),
   );
 }

Thanks @Mark Carver!

So this patch looks good code-wise, but I don't think it's ready to be committed yet. I tried experimentally removing the 'template' => 'node', line from node_theme() and cleared cache to rebuild the theme registry. Things mostly seemed to work okay, however Views UI (and possibly other things) complain. Here is a screenshot of editing the frontpage view with one sample node added:

Removing all the 'template' lines (regular expression I used to bulk search & replace added to the issue summary of #2246675: Remove all unnecessary 'template' lines in hook_theme() declarations) resulted in a WSOD. So this needs some more work before it can be committed.

And if it helps this is what I saw after resetting everything to HEAD and clearing caches:

Unable to find template "status-messages.html.twig" (looked into: /Users/Scott/Sites/d8.dev).
Unable to find template "breadcrumb.html.twig" (looked into: /Users/Scott/Sites/d8.dev).
Unable to find template "region.html.twig" (looked into: /Users/Scott/Sites/d8.dev).
Unable to find template "feed-icon.html.twig" (looked into: /Users/Scott/Sites/d8.dev).
Unable to find template "input.html.twig" (looked into: /Users/Scott/Sites/d8.dev).
Unable to find template "form-element.html.twig" (looked into: /Users/Scott/Sites/d8.dev).
Unable to find template "container.html.twig" (looked into: /Users/Scott/Sites/d8.dev).
Unable to find template "form.html.twig" (looked into: /Users/Scott/Sites/d8.dev).
Unable to find template "html.html.twig" (looked into: /Users/Scott/Sites/d8.dev).

Yup that sounds right, thanks a bunch @m1r1k!

Putting this patch up to see what else might break (this includes all the 'template' line removals – but I'm not proposing we include them in this issue) but overall this seems much much closer to working well. Thank you @m1r1k and @dawehner!

With the attached patch applied I see this on the default homepage, which if I had to guess looks like something is not going through template_preprocess() that was before:

Notice: Undefined index: is_front in template_preprocess_html() (line 1791 of core/includes/theme.inc).
template_preprocess_html(Array, 'html', Array)
_theme('html', Array)
drupal_render(Array)
Drupal\Core\Page\DefaultHtmlPageRenderer->render(Object)
Drupal\Core\EventSubscriber\HtmlViewSubscriber->onHtmlPage(Object, 'kernel.view', Object)
call_user_func(Array, Object, 'kernel.view', Object)
Symfony\Component\EventDispatcher\EventDispatcher->doDispatch(Array, 'kernel.view', Object)
Symfony\Component\EventDispatcher\EventDispatcher->dispatch('kernel.view', Object)
Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.view', Object)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)
Notice: Undefined index: logged_in in template_preprocess_html() (line 1793 of core/includes/theme.inc).
template_preprocess_html(Array, 'html', Array)
_theme('html', Array)
drupal_render(Array)
Drupal\Core\Page\DefaultHtmlPageRenderer->render(Object)
Drupal\Core\EventSubscriber\HtmlViewSubscriber->onHtmlPage(Object, 'kernel.view', Object)
call_user_func(Array, Object, 'kernel.view', Object)
Symfony\Component\EventDispatcher\EventDispatcher->doDispatch(Array, 'kernel.view', Object)
Symfony\Component\EventDispatcher\EventDispatcher->dispatch('kernel.view', Object)
Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.view', Object)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)

Thanks a ton for working on this @lauriii!

+++ b/core/modules/update/update.module
@@ -176,24 +176,20 @@ function update_theme() {
-      'template' => 'update-last-check',
...
-      'template' => 'update-report',
...
-      'template' => 'update-project-status',
...
-      'template' => 'update-version',

Super minor but these should be left as is if possible (not removed) in the "non-testing" patch.

@lauriii, you rock! A couple nice green patches. So I was about to write a change record, turns out I already did that: https://www.drupal.org/node/2231673.

I was going to update it to add the note about the exception that would be thrown if your theme function doesn't exist (BadFunctionCallException, can be seen in #107 patch). But it seems like we lost that somewhere in the past 24 hours :) Can we add that back please?

I see what you mean. Need to think about that some more, both ways have their pros and cons. Right now I'm leaning a bit towards declaring the dependencies because that way if other modules want to use those theme functions then they don't need to worry about including the (I'm assuming .inc) files.

In the meantime can we update the docs for hook_theme() to reflect this change?

Thanks @lauriii! Confusing interdiff but I managed to scroll down :) great start on the docs update. Here's some feedback:

1. +++ b/core/modules/system/system.api.php
   @@ -1098,16 +1098,16 @@ function hook_help($route_name, \Drupal\Core\Routing\RouteMatchInterface $route_
   + *     For example, if a module registers the 'node' theme hook, 'node'
   

   Can we use an example with an underscore please? To show that for example the 'search_result' theme hook gets converted to a 'search-result' template name I think is pretty important. Plus I think node is actually a bad example to use here. If we can come up with a non-foo_bar example to use here that isn't used in core I think that might be even better, that would apply to the below as well.

2. +++ b/core/modules/system/system.api.php
   @@ -1098,16 +1098,16 @@ function hook_help($route_name, \Drupal\Core\Routing\RouteMatchInterface $route_
     *   - function: If specified, this will be the function name to invoke for
   - *     this implementation. If neither 'template' nor 'function' is specified,
   - *     a default function name will be assumed. For example, if a module
   - *     registers the 'node' theme hook, 'theme_node' will be assigned to its
   - *     function. If the chameleon theme registers the node hook, it will be
   - *     assigned 'chameleon_node' as its function.
   + *     this implementation. If the chameleon theme registers the node hook,
   + *     it will be assigned 'chameleon_node' as its function.
   

   To me this still sounds like function is auto registered (at least for themes). "will be assigned". I think it should be clearer that whether it's a theme or not registering the theme hook, the function name needs to be manually specified.

   Here's a bit of an attempt:

   function: If the theme implementation is not a template but a theme function, you must specify the function name. For example, a theme hook of 'search_result' would by convention use a theme function of 'theme_search_result'. If you are building a theme called 'chameleon' that registers a new 'search_result' hook and theme function, you would usually use 'chameleon_search_result' as the theme function name.

Regarding the exception and dependencies, I'm still thinking manually specifying the dependencies in hook_theme() is a more solid solution, and keeping the exception.

Here's the testing patch with all the possible 'template' lines removed from #136 with the relevant interdiffs since then applied. Just want to make sure that one is still green with the recent changes.

This is looking very promising though, thanks @rteijeiro! The only thing is I think there may be a few too many commas in the revised docs :)

+++ b/core/modules/system/system.api.php
@@ -1100,14 +1100,17 @@ function hook_help($route_name, \Drupal\Core\Routing\RouteMatchInterface $route_
+ *     'theme_search_result'. So, if you are building a theme called 'chameleon'
+ *     , that registers a new 'search_result' hook and theme function, then you
+ *     would usually use 'chameleon_search_result' as the theme function name.

I think here we can remove all commas but this one on the second line:

"'search_result' hook and theme function, then you"

Here's the interdiff that's more than 0 bytes. Forgot --staged :)

Needs a reroll now that theme_aggregator_page_opml() is gone.

#2154781: Convert aggregator/sources and aggregator/opml to views

Thanks! Just updated the change record a bit to talk about the BadFunctionCallException and reworked the first paragraph so it makes more sense.

Thanks for taking a look @skwashd! Looking better, the docs for 'template' look good now.

This part is not correct though, under 'function':

If neither 'template' nor 'function' is specified, a default function name will be assumed.

That's the key change here. If neither 'template' nor 'function' is specified, a default template name will be assumed/inserted/whatever.

A possible way to solve this would be to use stronger language for 'function' like "…you must specify the function name…".