Problem/Motivation

There are currently more templates in core than theme functions, but we still need to define 'template' in each hook_theme() definition. Templates should be the default output option.

For example, in node_theme(), the template for 'node.html.twig' is defined like this:

/**
 * Implements hook_theme().
 */
function node_theme() {
  return array(
    'node' => array(
      'render element' => 'elements',
      'template' => 'node',
    ),
    …
  );

After the changes this issue will make we want it to be able to look like this and still work!

/**
 * Implements hook_theme().
 */
function node_theme() {
  return array(
    'node' => array(
      'render element' => 'elements',
    ),
    …
  );

Proposed resolution

Make 'template' the default output option for hook_theme() (the current default is 'function') and define a standard template name based on the hook name. For example 'item_list' should default to 'item-list'. The default option of 'function' is currently defined in (at least) \Drupal\Core\Theme\Registry::processExtension() and documentation would need to be updated (in at least hook_theme()) accordingly.

We'll also need to add 'function' lines to the hook_theme() definitions for all remaining theme functions in core.

Once the default is changed (with test coverage), we can get rid of all the unnecessary 'template' => 'node', lines in core in #2246675: Remove all unnecessary 'template' lines in hook_theme() declarations.

Remaining tasks

User interface changes

n/a

API changes

Minor API change to hook_theme() for better DX.

Files: 
CommentFileSizeAuthor
#182 make_template_the-2226207-182.patch21.27 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,025 pass(es). View
#162 2226207-161.patch20.9 KBrachel_norfolk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,732 pass(es). View
#136 2226207-136-testing.patch43.6 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,311 pass(es). View
#136 interdiff-2226207-testing-130-136.txt605 byteslauriii
#112 2226207-112-testing.patch44.64 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 46,272 pass(es), 18,415 fail(s), and 11,953 exception(s). View
#107 2226207-default_templates-107.patch21.8 KBm1r1k
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,762 pass(es). View
#101 2226207-101.patch22.64 KBm1r1k
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,659 pass(es), 83 fail(s), and 21,064 exception(s). View
#101 interdiff.txt1.01 KBm1r1k
#97 interdiff.txt682 bytesm1r1k
#94 2226207-92.patch22.41 KBm1r1k
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,399 pass(es), 62 fail(s), and 66 exception(s). View
#90 2226207-90-frontpage-view.png45.4 KBCottser
#88 22262707-88.patch22.54 KBLinL
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,139 pass(es). View
#87 22262707-87.patch23.25 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,113 pass(es). View

Comments

lauriii’s picture

Assigned: Unassigned » lauriii
dawehner’s picture

So +1

lauriii’s picture

Assigned: lauriii » Unassigned
euphoric_mv’s picture

Assigned: Unassigned » euphoric_mv
euphoric_mv’s picture

Status: Active » Needs review
FileSize
17.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
Cottser’s picture

Issue summary: View changes
Status: Needs review » Needs work

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

The last submitted patch, 5: 2226207-5.patch, failed testing.

lauriii’s picture

Do we just want to change the default template loaded to function, or do we first check if there's a function available for the specific case.

Cottser’s picture

Issue summary: View changes

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

lauriii’s picture

FileSize
7.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 60,752 pass(es), 4,189 fail(s), and 461 exception(s). View

Some progress, should we take this path?

Cottser’s picture

Status: Needs work » Needs review

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

euphoric_mv’s picture

@lauriii You can assign task to you, I will unassign from myself.

euphoric_mv’s picture

Assigned: euphoric_mv » Unassigned

Status: Needs review » Needs work

The last submitted patch, 10: template_as_default-2226207-10.patch, failed testing.

jorgegc’s picture

Hey @Cottser,

Do you think something along the line of

// If function and file are omitted, default to standard naming
// conventions.
if (!isset($info['template']) && !isset($info['function'])) {
  $template_name = strtr($hook, '_', '-');
  $template_path = $path . '/' . $template_name . '.html.twig';

  // If template file doesn't exist, skip the override.
  if (!file_exists($template_path)) {
    unset($result[$hook]);
    continue;
  }

  $info['template'] = $template_name;
  $result[$hook]['template'] = $template_name;
}

would work? I want to help but I am not sure whether that is the right direction or how we would implement the file_exists within \Drupal\Core\Theme\Registry::processExtension().

Cottser’s picture

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.

Cottser’s picture

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

lauriii’s picture

Status: Needs work » Needs review
FileSize
15.76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 62,843 pass(es), 3,324 fail(s), and 194 exception(s). View

Sending to testbot

Status: Needs review » Needs work

The last submitted patch, 18: template_as_default-2226207-18.patch, failed testing.

Cottser’s picture

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.

Cottser’s picture

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.

Cottser’s picture

Issue summary: View changes
Issue tags: +API change

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.

Cottser’s picture

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

Cottser’s picture

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

webchick’s picture

Issue tags: +Approved API change

This sounds like a great improvement for "Themer Experience" (TX) in D8.

mgbellaire’s picture

Assigned: Unassigned » mgbellaire

How's it going @Cottser! I'm going to give this one a try. This will be my first issue, but not my first time developing in Drupal. I know most coding standards and how to write in php, but documentation is probably the thing I need most help with.

Cottser’s picture

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’s picture

FileSize
702 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 38,908 pass(es), 20,023 fail(s), and 1,611 exception(s). View

Here is @laurii's patch cleaned up into one line. I'll need a little help with how to check that it needs to be a 'function'.

mgbellaire’s picture

Status: Needs work » Needs review
FileSize
18.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch template_as_default-function_lines_added-2226207-29-8x.patch. Unable to apply patch. See the log in the details link for more information. View

I ran grep to search through all the current "hook_theme" calls and added the proper 'function' lines myself manually. I didn't want to have to diff the patch that @lauriii created against mine. Cross your fingers, boys.

Status: Needs review » Needs work

The last submitted patch, 29: template_as_default-function_lines_added-2226207-29-8x.patch, failed testing.

mgbellaire’s picture

@Cottser My patch was not applied because of menu.module not existing? I'm confused. Am I not creating a patch off of the right codebase? If so, which branch should I be using?

mgbellaire’s picture

Status: Needs work » Needs review
FileSize
18.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 39,436 pass(es), 20,222 fail(s), and 1,555 exception(s). View

Re-creating patch after new pull of 8.x repository.

Status: Needs review » Needs work

The last submitted patch, 32: template_as_default-function_lines_added-2226207-32-8x.patch, failed testing.

mgbellaire’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 32: template_as_default-function_lines_added-2226207-32-8x.patch, failed testing.

The last submitted patch, 28: template_as_default-one_line-2226207-28-8x.patch, failed testing.

mgbellaire’s picture

Status: Needs work » Needs review
FileSize
18.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 39,383 pass(es), 20,283 fail(s), and 1,578 exception(s). View

More function lines.

Status: Needs review » Needs work

The last submitted patch, 37: template_as_default-function_lines_added-2226207-37-8x.patch, failed testing.

joelpittet’s picture

@mgbellaire could you provide an interdiff so we can see what you have changed between the patches you are posting? See instructions https://drupal.org/documentation/git/interdiff or my workflow: http://pittet.ca/drupal/sprint/patch.

If you have ack or ag you can use a command like this to help find things:
ack -Q -C 2 "'render element' => "
ack -Q -C 2 "'variables' => "

Cottser’s picture

@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’s picture

@Cottser: Yes, it looks like we did. I don't know how my grep missed that... I'll have to re-check the command that I use. I'll be looking into ack more! :)

I'm changing that now. Will Create new patch soon.

mgbellaire’s picture

Also, shame on me for not looking thoroughly enough into @lauriii's patch to see my obvious mistake. mgbellaire--

mgbellaire’s picture

Changed the theme.inc file, and it doesn't look like their are any more changes that I'm aware of.

mgbellaire’s picture

Status: Needs work » Needs review
FileSize
21.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,289 pass(es), 305 fail(s), and 33 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 44: template_as_default-function_lines_added-2226207-44-8x.patch, failed testing.

mgbellaire’s picture

Status: Needs work » Needs review
FileSize
22.81 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,484 pass(es), 314 fail(s), and 33 exception(s). View

system.api.php didn't have any templates or functions.

Status: Needs review » Needs work

The last submitted patch, 46: template_as_default-function_lines_added-2226207-46-8x.patch, failed testing.

Cottser’s picture

@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’s picture

FileSize
1.55 KB
23.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,595 pass(es), 221 fail(s), and 31 exception(s). View

This patch is merely to take away the system.api.php changes and add a 'function' => 'theme_more_link' line to the 'more_link' declaration.

mgbellaire’s picture

@Cottser: It looks like views_theme() takes each plugin and creates a template where there isn't a function. Doesn't that cover what we need it to do?

Cottser’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work

The last submitted patch, 49: template_as_default-function_lines_added-2226207-49-8x.patch, failed testing.

mgbellaire’s picture

Status: Needs work » Needs review
FileSize
22.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,139 pass(es), 2 fail(s), and 0 exception(s). View
584 bytes

Tweaked theme_views() to programmatically add 'function' line when it finds a theme function.

Status: Needs review » Needs work

The last submitted patch, 53: template_as_default-function_lines_added-2226207-53-8x.patch, failed testing.

mgbellaire’s picture

@Cottser: Only 2 fails now! I don't know why the test is failing. It's saying something about theme suggestions.

Cottser’s picture

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

mgbellaire’s picture

FileSize
22.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,066 pass(es). View
731 bytes

Changed the line that @Cottser mentioned. Will the test pass now? :D Finger crossed!

mgbellaire’s picture

Status: Needs work » Needs review
Cottser’s picture

Status: Needs review » Needs work

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.

mgbellaire’s picture

FileSize
22.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch template_as_default-function_lines_added-2226207-60-8x.patch. Unable to apply patch. See the log in the details link for more information. View
602 bytes

Just found a line in views.module that didn't comply to Drupal coding standards.

markcarver’s picture

Assigned: mgbellaire » Unassigned
Issue tags: +focus, +sprint
markcarver’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 60: template_as_default-function_lines_added-2226207-60-8x.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
FileSize
21.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,841 pass(es). View

Rerolled.

markcarver’s picture

FileSize
30.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,828 pass(es), 2,261 fail(s), and 6,602 exception(s). View
9.85 KB

@Cottser, here is the added exceptions we talked about recently. This throws a fatal when attempting to build the registry and the theme function doesn't actually exist.

In fact, I was able to find a few errors that were easily fixed by including the path and file the theme hooks were located in.

I also fixed a related sub-issue where we were namespacing our include paths for Twig? This actually prevented proper autodiscovery of templates, which was evident by the hack I removed in Registry.php:

        // The following apply only to theming hooks implemented as templates.
        if (isset($info['template'])) {
          // Prepend the current theming path when none is set.
          if (!isset($info['path'])) {
            $result[$hook]['template'] = $path . '/templates/' . $info['template'];
          }
        }

Status: Needs review » Needs work

The last submitted patch, 65: make_template_the-2226207-65.patch, failed testing.

markcarver’s picture

Assigned: Unassigned » markcarver

Yeah... that didn't go over so well :)

I've already identified the issue and will be working on this later tonight.

markcarver’s picture

Status: Needs work » Needs review
FileSize
32.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,737 pass(es), 1,669 fail(s), and 310 exception(s). View
2.75 KB

Alright, this should clear it up.

Status: Needs review » Needs work

The last submitted patch, 68: make_template_the-2226207-68.patch, failed testing.

markcarver’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Novice, -Needs tests
FileSize
31.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,752 pass(es). View
2.27 KB

Well, even though there's no actual "test" per se, we do know that any test will not pass if it defines a "function" to use but then not actually implement that function (which is far better than a script):
Theme hook "aggregator_page_rss" refers to a theme function callback that does not exist: "theme_aggregator_page_rss"

So, this patch took out that theme hook, which is a relic and should have been removed in #1963544: Convert aggregator/rss to views.

I also talked with @joelpittet about the namespaces in Twig and fixed it so that we're adding paths to the __main__ namespace (for auto-discovery) as well as specific namespaces like: @node/node.html.twig.

This should be green now :)

markcarver’s picture

Status: Needs review » Needs work

Gave it a once over myself, found a couple issues:

  1. +++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
    @@ -48,9 +48,17 @@ public function __construct(\Twig_LoaderInterface $loader = NULL, $options = arr
    +        // is used for fallback auto-discovery. It will load the first template
    

    Typo. It actually loads the last template found (which is why module paths are added before theme paths).

  2. +++ b/core/modules/toolbar/toolbar.module
    @@ -95,7 +94,7 @@ function toolbar_element_info() {
    -    '#theme' => 'toolbar_item',
    +    '#theme' => 'theme_toolbar_item',
    

    Typo. Don't need to include theme_.

markcarver’s picture

Status: Needs work » Needs review
FileSize
29.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,958 pass(es). View
5.62 KB

Despite that it's green and after really thinking about how autoloading works, I realized that it should be a separate issue. Didn't mean to scope creep.

After talking with @joelpittet, I'm also going to make the removal of explicit 'template' definitions in hook_theme() a separate issue.

markcarver’s picture

Also, after talking with @jessebeach, I've removed the (seemingly unused) toolbar_item theme hook.

markcarver’s picture

Cottser’s picture

Issue summary: View changes

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

markcarver’s picture

Sorry, just overlooked it...

Cottser’s picture

Assigned: markcarver » Cottser
Status: Needs review » Needs work
Issue tags: +Needs reroll

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.

LinL’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
28.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: @reason. View

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 78: make_template_the-2226207-78.patch, failed testing.

LinL’s picture

Sorry, missed one, here it is again.

LinL’s picture

Status: Needs work » Needs review
Cottser’s picture

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.

The last submitted patch, 80: make_template_the-2226207-79.patch, failed testing.

LinL’s picture

Status: Needs review » Needs work

Thanks @Cottser. Wasn't sure about that, it did look a bit odd. So, keep 'template', get rid of 'function'. On to it :)

LinL’s picture

Status: Needs work » Needs review
FileSize
23.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,652 pass(es). View
5.25 KB

Here's another try...

Cottser’s picture

Assigned: Cottser » markcarver

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?

Cottser’s picture

FileSize
23.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,113 pass(es). View

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',
     ),
   );
 }
LinL’s picture

FileSize
22.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,139 pass(es). View

And again now that #1987418: user.module - Convert theme_ functions to Twig has been committed.

Removes this:

diff a/core/modules/user/user.module b/core/modules/user/user.module
@@ -103,12 +103,15 @@ function user_theme() {
     'user_permission_description' => array(
       'variables' => array('permission_item' => NULL, 'hide' => NULL),
       'file' => 'user.admin.inc',
+      'function' => 'theme_user_permission_description',
     ),
     'user_signature' => array(
       'variables' => array('signature' => NULL),
+      'function' => 'theme_user_signature',
     ),
     'username' => array(
       'variables' => array('account' => NULL, 'attributes' => array()),
+      'function' => 'theme_username',
     ),
   );
 }
markcarver’s picture

Assigned: markcarver » Unassigned

#86.1:
We don't need to check if a template exists because Twig will throw a fatal error saying that the file does not exist.

edit (didn't answer the other question): No, we need function_exists(). This really isn't too much of a performance impact and is cached once the registry has been built.

#86.2:
This is just some cleanup to only include views.theme.inc for core view template (because the preprocessing doesn't live in an already loaded file). Otherwise it sets the path and file appropriately based on the plugin.

Cottser’s picture

Status: Needs review » Needs work
FileSize
45.4 KB

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.

Cottser’s picture

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).
m1r1k’s picture

Assigned: Unassigned » m1r1k

I will jump on it during #drupalaton.
Seems we need to reroll it because functions theme_menu_tree(), theme_forum_form(), theme_update_manager_update_form(), theme_update_report(), theme_update_version(), theme_update_status_label() are gone after related Twig convert issues.

Cottser’s picture

Yup that sounds right, thanks a bunch @m1r1k!

m1r1k’s picture

FileSize
22.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,399 pass(es), 62 fail(s), and 66 exception(s). View

Functions were removed:
theme_menu_tree(), // #2301317: MenuLinkNG part4: Conversion
theme_forum_form(), // #1987400: forum.module - Convert theme_ functions to Twig
theme_update_manager_update_form(), // #1898466: update.module - Convert theme_ functions to Twig
theme_update_report(), // #1898466: update.module - Convert theme_ functions to Twig
theme_update_version(), // #1898466: update.module - Convert theme_ functions to Twig
theme_update_status_label().// #1898466: update.module - Convert theme_ functions to Twig

Interdiff is absent because previous patch could not be applied.

m1r1k’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 94: 2226207-92.patch, failed testing.

m1r1k’s picture

Status: Needs work » Needs review
FileSize
22.42 KB
682 bytes

Fails occurred because I remove template names from update_theme(). @dawehner helped to find a problem.

dawehner’s picture

Issue tags: +Drupalaton 2014
  1. +++ b/core/includes/theme.inc
    @@ -2657,6 +2664,9 @@ function drupal_common_theme() {
         'menu_link' => array(
           'render element' => 'element',
    +      'function' => 'theme_menu_link',
    +      'path' => 'core/includes',
    +      'file' => 'menu.inc',
         ),
    
    @@ -2664,12 +2674,15 @@ function drupal_common_theme() {
         'menu_local_task' => array(
           'render element' => 'element',
    +      'function' => 'theme_menu_local_task',
         ),
         'menu_local_action' => array(
           'render element' => 'element',
    +      'function' => 'theme_menu_local_action',
         ),
         'menu_local_tasks' => array(
           'variables' => array('primary' => array(), 'secondary' => array()),
    +      'function' => 'theme_menu_local_tasks',
         ),
    

    Should all of those call the path/file bit?

  2. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -439,20 +436,36 @@ protected function processExtension(&$cache, $name, $type, $theme, $path) {
    +          throw new \BadFunctionCallException(sprintf(
    

    Not 100% sure whether this is the proper exception though, tbh.

  3. +++ b/core/modules/toolbar/toolbar.module
    @@ -52,9 +52,6 @@ function toolbar_theme($existing, $type, $theme, $path) {
    -  $items['toolbar_item'] = array(
    -    'render element' => 'element',
    -  );
     
    
    @@ -98,7 +95,6 @@ function toolbar_element_info() {
    -    '#theme' => 'toolbar_item',
    

    Would it make sense to add a followup to put this into a element type?

  4. +++ b/core/modules/views/views.module
    @@ -174,35 +175,44 @@ function views_theme($existing, $type, $theme, $path) {
    -        $hooks[$def['theme']]['path'] = $module_dir;
    -        $hooks[$def['theme']]['template'] = 'templates/' . drupal_clean_css_identifier($def['theme']);
    +        $hooks[$def['theme']]['path'] .= '/templates';
    +        $hooks[$def['theme']]['template'] = drupal_clean_css_identifier($def['theme']);
    +      }
    

    this is really an improvement!

markcarver’s picture

#98.1: Yes, very likely. I probably overlooked those when adding in the first one. Don't know why they live in that file in the first place though, very odd.

#98.2: Not entirely sure. I remember asking @tim.plunkett in IRC one day about this, I believe we came to this conclusion. Not my area of expertise, so if it needs to change to some other exception class, fine by me. Just need to throw some sort of exception ;)

#98.3: No, I think this was just a remnant from the toolbar twig conversion. I remember talking to @jessebeach about this at DC Austin briefly. toolbar_item has no template nor a theme function, it's just a dead theme hook and doesn't do anything as far as I can tell.

#98.4: Cool :) I was hoping someone could review this in better detail though. When I originally did this, it was out of sheer necessity. I'm a little concerned with @Cottser's discovery in #90, but haven't had a chance to look into it too deeply.

dawehner’s picture

#98.2: Not entirely sure. I remember asking @tim.plunkett in IRC one day about this, I believe we came to this conclusion. Not my area of expertise, so if it needs to change to some other exception class, fine by me. Just need to throw some sort of exception ;)

Just read the official documentation again:
Exception thrown if a callback refers to an undefined function or if some arguments are missing.
This is exactly what we want.

#98.4: Cool :) I was hoping someone could review this in better detail though. When I originally did this, it was out of sheer necessity. I'm a little concerned with @Cottser's discovery in #90, but haven't had a chance to look into it too deeply.

m1r1k and I had a small pair-debugging session to figure that out. The interdiff is the fix for that particular problem.

m1r1k’s picture

Status: Needs review » Needs work
FileSize
1.01 KB
22.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,659 pass(es), 83 fail(s), and 21,064 exception(s). View

#98.1: Yes, very likely. I probably overlooked those when adding in the first one. Don't know why they live in that file in the first place though, very odd.

Well now it works only because DrupalKernel directly includes menu.inc: require_once DRUPAL_ROOT . '/' . Settings::get('menu_inc', 'core/includes/menu.inc');

But I think we have to put it into the themes definition as well at least to provide this dependency information in the proper place. Because once someone will remove those lines from DrupalKernel.

Also I'll toggle issue status to resend it to testing.

m1r1k’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 101: 2226207-101.patch, failed testing.

m1r1k’s picture

FileSize
500 bytes
22.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,468 pass(es). View

Well if theme uses twig template but needs file with prerocess to be included we have to include it via 'includes' key otherwise twig template will not be found.

m1r1k’s picture

Status: Needs work » Needs review
LinL’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch no longer applies after changes to image.module and views_ui.module.

Tagging for reroll.

m1r1k’s picture

Status: Needs work » Needs review
FileSize
21.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,762 pass(es). View

Rerolled

Status: Needs review » Needs work

The last submitted patch, 107: 2226207-default_templates-107.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 107: 2226207-default_templates-107.patch, failed testing.

Status: Needs work » Needs review
Cottser’s picture

Issue tags: -Needs reroll
FileSize
44.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 46,272 pass(es), 18,415 fail(s), and 11,953 exception(s). View
29.53 KB

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)

Status: Needs review » Needs work

The last submitted patch, 112: 2226207-112-testing.patch, failed testing.

lauriii’s picture

FileSize
44.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
11.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

The problem is that there's some functions that doesn't exist in certain situations. E.g theme_authorize_message() and theme_authozie_report() are located in theme.maintenance.inc which is included only on maintenance mode I guess. I uploaded patch to test if thats true. I also had to reroll @Cottsers patch so there's no interdiff. :/

lauriii’s picture

Status: Needs work » Needs review

Testbot gogo!

lauriii’s picture

FileSize
44.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 47,006 pass(es), 18,732 fail(s), and 12,219 exception(s). View
8.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Fixed some typos and added views.module changes to the actual patch.

The last submitted patch, 114: 2226207-114-testing.patch, failed testing.

The last submitted patch, 114: 2226207-114.patch, failed testing.

The last submitted patch, 116: 2226207-115-testing.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 116: 2226207-115.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.14 KB
7.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
44.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,195 pass(es), 80 fail(s), and 0 exception(s). View

Found reason why preprocesses are not running..

The last submitted patch, 121: 2226207-120.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 121: 2226207-120-testing.patch, failed testing.

lauriii queued 121: 2226207-120.patch for re-testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.41 KB
43.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,304 pass(es), 7 fail(s), and 0 exception(s). View

The failing tests are because there is some templates named with underscore. So far templates I found using underscore are: config_translation_manage_form_element.html.twig, twig_theme_test.php_variables.html.twig and twig_namespace_test.html.twig

lauriii’s picture

Assigned: m1r1k » Unassigned

The last submitted patch, 121: 2226207-120.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 125: 2226207-125-testing.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
605 bytes
43.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Fixed small typo which caused failing tests.

Status: Needs review » Needs work

The last submitted patch, 129: 2226207-129-testing.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
Issue tags: -Drupalaton 2014
FileSize
43.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,306 pass(es), 7 fail(s), and 0 exception(s). View
369 bytes
26.15 KB
20.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,196 pass(es), 100 fail(s), and 14 exception(s). View

There was some unnecessary code removed in the previous patch. In this comment I have applied patch with all the templates removed from the code and functions (2226207-130-testing.patch). Then there's patch that has only functions added (2226207-130.patch) and there's interdiff between these two (interdiff-testing-2226207-130.txt).

Status: Needs review » Needs work

The last submitted patch, 131: 2226207-130.patch, failed testing.

The last submitted patch, 131: 2226207-130-testing.patch, failed testing.

joelpittet’s picture

Small note:

+++ b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module
@@ -10,7 +10,7 @@
-    'tempalte' => 'twig_theme_test.php_variables',
+    'tempalate' => 'twig_theme_test.php_variables',

These are both spelled wrong.

Cottser’s picture

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’s picture

Status: Needs work » Needs review
FileSize
605 bytes
43.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,311 pass(es). View
1.04 KB
19.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,196 pass(es), 100 fail(s), and 14 exception(s). View

Now it should be right. I was too tired to do something like that.. :D

Status: Needs review » Needs work

The last submitted patch, 136: 2226207-136.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
475 bytes
19.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,311 pass(es). View

There was unnecessary theme function called from the views_ui_theme() which most likely caused breaking tests

lauriii’s picture

Issue tags: +Needs change record
Cottser’s picture

Status: Needs review » Needs work
Issue tags: -Needs change record

@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?

lauriii’s picture

Nice!

I removed the exception since there is cases where we have functions that are not being included. I mentioned that in my comment #114. Another option would be to add all dependencies to theme_hook implementations using function. Which one would we prefer?

Cottser’s picture

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?

lauriii’s picture

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

I started implementing the documentation change to hook_theme

Cottser’s picture

Status: Needs review » Needs work

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.

rteijeiro’s picture

Assigned: Unassigned » rteijeiro

I'll fix lauriii's mess :P

rteijeiro’s picture

Assigned: rteijeiro » Unassigned
Status: Needs work » Needs review
FileSize
22.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 43,130 pass(es), 21,873 fail(s), and 32,586 exception(s). View
3.62 KB

I applied @Cottser suggestions in #144. Also added exception back and included file dependency.

Let's see what does testbot say (https://www.drupal.org/node/2203045)

Status: Needs review » Needs work

The last submitted patch, 146: 2226207-146.patch, failed testing.

rteijeiro’s picture

Status: Needs work » Needs review
FileSize
22.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,618 pass(es). View
685 bytes

Sorry, I forgot the path U_U!

Cottser’s picture

FileSize
46.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,618 pass(es). View
0 bytes

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"

Cottser’s picture

FileSize
3.82 KB

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

lauriii’s picture

FileSize
21.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,824 pass(es), 1,727 fail(s), and 498 exception(s). View

Rerolled the patch and fixed @rteijeiros mess with the comma :P

Status: Needs review » Needs work

The last submitted patch, 151: 2226207-151.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
21.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,599 pass(es). View
344 bytes

Ok for some reason I messed up the patch with 'theme_toolbar'..

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs a reroll now that theme_aggregator_page_opml() is gone.

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

lauriii’s picture

Issue tags: -Needs reroll
FileSize
20.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,587 pass(es). View
Cottser’s picture

Status: Needs work » Needs review

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

skwashd’s picture

FileSize
20.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,587 pass(es). View
2.31 KB

After talking to @rteijeiro and @lauriii the language in the docs could be cleaner. Here is the update patch and interdiff.

Cottser’s picture

Status: Needs review » Needs work

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.

rteijeiro’s picture

Issue tags: +documentation, +Novice

Tagging as documentation and novice and trying to find somebody to fix it.

P.S. What's wrong with my spanglish? :P

Cottser’s picture

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

rachel_norfolk’s picture

Assigned: Unassigned » rachel_norfolk
rachel_norfolk’s picture

Assigned: rachel_norfolk » Unassigned
Status: Needs work » Needs review
FileSize
20.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,732 pass(es). View
1.43 KB

I've changed the language used on the function documentation as per #158. I think it describes how default templates are now assumed when neither a function nor a template is given.

mikl’s picture

Status: Needs review » Reviewed & tested by the community

This looks good, and works even better, great work.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -documentation, -Novice +document

The documentation still needs more work.

lauriii’s picture

Issue tags: -document +documentation
davidhernandez’s picture

Assigned: Unassigned » davidhernandez

I'm currently reviewing the docs lines and change record.

davidhernandez’s picture

Assigned: davidhernandez » Unassigned
Status: Needs work » Needs review
Issue tags: -documentation
FileSize
21.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,730 pass(es). View
5.02 KB
+++ b/core/modules/update/update.module
@@ -204,11 +204,11 @@ function update_theme() {
       'template' => 'update-version',

Shouldn't the template line here be removed?

In the attached patch I just modified some comments.

Cottser’s picture

Status: Needs review » Needs work

Thanks @davidhernandez!

We're removing all the soon-to-be-unnecessary 'template' lines in #2246675: Remove all unnecessary 'template' lines in hook_theme() declarations to keep this patch smaller and easier to review.

Most of the changes from the previous patch look great. The troublesome 'function' section has a repeated sentence now and IMO is not clear enough about the array-PI here. I don't think we should be repeating the stuff from underneath 'template' to tell people how to use 'function'.

I can try to put this into words and update the patch if someone else wants to review. @joelpittet? :D

davidhernandez’s picture

I'm fine chopping out that 'for example' line. Wasn't too keen on it.

markcarver’s picture

+++ b/core/modules/system/system.api.php
@@ -990,11 +990,12 @@ function hook_help($route_name, \Drupal\Core\Routing\RouteMatchInterface $route_
+ *     assigned to its template. Do not put .html.twig on this file,
+ *     that extension will be added automatically by the default
+ *     rendering engine (which is Twig). If 'path' above is specified, the
+ *     template should also be in this path.
Do not put .html.twig

makes no sense here IMO, these are supposed to be function names only. They have nothing to do with templates.

If neither 'template' nor 'function' is specified

This entire block really should probably just say something like:

"If neither 'template' nor 'function' is specified, the theme hook will behave as if a default template was defined. See above for more details."

rachel_norfolk’s picture

Assigned: Unassigned » rachel_norfolk
rachel_norfolk’s picture

Assigned: rachel_norfolk » Unassigned
Status: Needs work » Needs review
FileSize
242.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,730 pass(es). View
1.24 KB

It was me that repeated slightly too much of the template text. I have repaired that.

Also, I looked at the text at #170 but felt that keeping it consistent with the text in the template field description made for easier reading. Hope that's okay.

markcarver’s picture

Status: Needs review » Reviewed & tested by the community

Awesome. Yeah, I wasn't married to exactly what I wrote, was just saying it should be something to that effect ;)

Setting back to RTBC now that the documentation has been iterated on.

Cottser’s picture

That works for me. Thanks everyone!

Cottser’s picture

Status: Reviewed & tested by the community » Needs work

Oh, the patch grew a lot.

lauriii’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.12 KB
21.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch make_template_the-2226207-176_0.patch. Unable to apply patch. See the log in the details link for more information. View
davidhernandez’s picture

I was literally just submitting that and got the "user changed the values" error. Do we have to yell jinx? :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 176: make_template_the-2226207-176.patch, failed testing.

lauriii’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
21.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch make_template_the-2226207-179.patch. Unable to apply patch. See the log in the details link for more information. View

Very minor reroll

Cottser’s picture

Issue tags: -focus

+1, the docblock to twig_render_template was updated in #1332068: ENGINE_render_template() and ENGINE_extension() are undocumented so hunk removed here. Thanks @lauriii!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 179: make_template_the-2226207-179.patch, failed testing.

lauriii’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
21.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,025 pass(es). View

One more time..

Cottser’s picture

+1, looks good. Small conflict with #2201789: Don't print "_theme()" in twig_debug output. Thanks again :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Nice work everyone. Looking forward to the follow up removing all the template info from hook_theme().

Committed d3f105e and pushed to 8.0.x. Thanks!

  • alexpott committed d3f105e on 8.0.x
    Issue #2226207 by lauriii, mgbellaire, Cottser, m1r1k, Mark Carver, LinL...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.