ThemeManager::theme() does not trigger an error when a theme hook is not found

+  if(isset($output)) {
+    return $output;
+  }
+  else {
+   return '';
+  }

should be changed to

+  if (isset($output)) {
+    return $output;
+  }
+  else {
+    return '';
+  }

http://drupal.org/coding-standards#controlstruct
Control statements should have one space between the control keyword and opening parenthesis, to distinguish them from function calls.

Am i correct?

Thanks for the bug report. Here's an alternate implementation that logs the condition, since it is an error that the site administrator should be aware of.

+1 for logging the message

I think that simply logging the message and returning an empty string is not enough. I just encountered this problem because seven_fieldset was registered, but not defined. The method to fix this condition is to clear the theme registry.

Without the PHP notice it would have taken longer to find out that something was wrong, so maybe a drupal_set_message is appropriate - I'm not sure.

Even worse: because an empty string was returned, all kinds of fieldsets were not rendered, including the fieldsets on admin/config/development/performance. As a result, the very solution to this problem (the "Clear all caches" button) was not available.

I think it's better to fall back on the default theme function in case the theme override is not available, so there is at least some output generated. If even the default theme function is not defined, an empty string can be returned. See my patch.

Logging would probably be helpful in this case but

• Why do the $info['hook'] = $hook; bit when we go to lengths to preserve $hook through out the function? Maybe there's a reason but I don't see it
• When the function that's failing in the first check is the default theme function we just toss two error messages in the log. Seems like that could be designed better.

You're right, I don't know where my head was, my patch does not totally make sense. I'll try to post a new patch later this weekend.

Marked #812410: $output might be undefined in theme() as a duplicate.

My patch from the duplicate thread

I must admit that I forgot about this issue... Alex, thanks for the wake-up call and Yorirou, thanks for moving your patch here.

I was thinking along these lines: If the missing theme function is a theme override (which will usually be the case), then let's try to fall back on the default theme function before returning an empty string. That will at least provide some basic functionality.

This new patch addresses the two points in #7 and improves the logic. I noticed that Yorirou used trigger_error() instead of watchdog(). I still used watchdog() in this patch, but I'd like to read if there are good reasons to use trigger_error().

I have only a simple reason to prefer trigger_error() in this situation: it is more visible to the developer. It is easier to notice a big green warning on the top of the page than an entry in watchdog.

I disagree with falling back to theme_HOOK(), but I appreciate the use-case described in #6. I will post a patch within the next couple days with my recommendation for how to address this. I also agree with #12 that a drupal_set_message() in addition to a watchdog() is in order, but I'm not sure trigger_error() makes the most sense for achieving that. I'll include my recommendation in the patch I post.

I'm not sure using trigger error to get around a deficiency in the watchdog system is appropriate here. I strongly disagree with a drupal_set_message though as that's a completely public thing and in the case that this where to happen on a live site it should /not/ be displayed to a user what's going on internally. At this point I think I'm of the opinion we should just stick with watchdog.

As far as falling back to base theme function, that makes me nervous too as its almost certainly as bad or worse as displaying nothing in many cases. I'm willing to ride with the consensus though.

@neclimdul: can you give an example where falling back on the default would be worse than displaying nothing at all? Don't get me wrong, I'm not saying that falling back will win beauty contests, but sure would have helped in the case from #6.

[oops, double post, mobile browsers aren't perfect yet!]

Attached patch is my suggestion for how to fix this. It does a drupal_set_message() without giving away internals, and it watchdogs the internals. I agree that the use-case in #6 (a theme gets updated to no longer have a function, but the drupal cache isn't cleared, and if the theme function was THEMENAME_fieldset(), THEMENAME_button(), THEMENAME_submit(), or one of several other possible functions, you lose the ability to clear cache via the admin interface) is a common enough use-case to justify work on a more robust solution, but I think it's too late to work on that for D7 core. So, this patch makes the error handler pluggable so that work on a more robust solution can occur in contrib.

can you give an example where falling back on the default would be worse than displaying nothing at all?

The building of the theme registry involves many steps, including hook_theme_registry_alter(). Falling back to naming convention only and bypassing theme ancestry short-circuits potentially important decisions that got made in building the registry. For example, a site might implement micro-sites with a generic base theme and micro-site-specific themes that are sub-themes of the base theme. The base theme may override a theme_*() function and the site owner may consider this override to be very important. An error in a micro-site theme should not then result in theme() calling the theme_*() function. What needs to be worked on in contrib is a solution whereby the error handler rebuilds the registry and then makes decisions based on that.

#17: theme-error_handling-674108-17.patch queued for re-testing.

I don't see the need for a "pluggeable error handler". Having better error messages is always good, but please simply throw_error() with a correct message, and let the standard error handler do its job.

@Damien: throw() is always fatal, right? So that's not what we want here. trigger_error() would be fine, except the error handler can't control what gets returned from theme(). The idea of a pluggable function here is to enable it to do something like _theme_build_registry() (since the error is likely due to a stale registry) and decide what theme registry key / implementation to fall back to, call that, and return that result back to theme() for theme() to return.

I meant trigger_error(), sorry.

This strikes me as completely unnecessary complexity on top of an already very complex system. A missing theme function is nothing more then a temporary condition. It will go away at the next theme rebuild.

I understand the "developer story" described in #6, but frankly it is a corner case, and there are ways around that (clearing the cache from the MySQL interface, submitting the theme admin page, etc.).

Ok, how about this then? A contrib module can still try to help out the story in #6 by implementing its own global PHP error handler, and do something clever like remove the theme registry from cache and force a page refresh with a drupal_goto() or some such thing. Whether someone chooses to try such a thing out will depend on how much of a corner case #6 really is.

#23 looks good to me.

Only one minor concern, we generally use % placeholders in cases like this:

Theme key "@key" not found.

should be:

Theme key %key not found.

changed @ to % and moved the trigger_error() for missing templates to the render function.

Also removed quotes as per #24.

+++ includes/theme.inc	6 Jun 2010 00:07:11 -0000
@@ -1217,7 +1224,14 @@ function theme_render_template($template
   include DRUPAL_ROOT . '/' . $template_file;   // Include the template file
...
+  $output = ob_get_clean();                     // End buffering and get its contents
...
+  if ($output === '' && !file_exists(DRUPAL_ROOT . '/' . $template_file)) {
+    trigger_error(t('Theme template %template not found.', array('%template' => $template_file)));
+  }
+  return $output;

The include will already throw a PHP warning exposing a fullly-fledged file not found message, no?

Powered by Dreditor.

Indeed.

I think that #28 does a good job at reporting the error, but it's not really helping the user solve the problem. I would like to see an error message like this:

Theme function theme_block not found. It may help to rebuild the theme registry.

This message would be helpful in most cases and also provide a way out of a situation like #6. Unfortunately, there is no such thing as www.example.com/admin/rebuild-theme-registry, and I assume it's too late to add it now.

Something else:

+++ includes/theme.inc	7 Jun 2010 17:32:05 -0000
@@ -866,6 +866,13 @@ function theme($hook, $variables = array
+      trigger_error(t('Theme function %function not found.', array('%function' => $info['function'])));

The %-placeholder doesn't work here because this translated message is escaped in _drupal_log_error(). The resulting message looks like this: Debug: Theme function <em class="placeholder">theme_block</em> not found..

74 critical left. Go review some!

@marcvangend looks like its mostly been dropped but as unfortunate as it is, the theme layer can be used to hide data, ie personal information from the profile page that would otherwise be difficult to remove for what ever reason.

#28 looks pretty reasonable. Agree with the comments in #29. We probably shouldn't be adding any html markup to native error messages. They could be escaped into watchdog, dropped in a log file, etc and having html like that doesn't make sense.

neclimdul: I'm not sure what you mean with the first half of your comment. Can you rephrase?

How about this?

This one replaces trigger_error() with watchdog(). I understand the desire for errors to be reported to the screen, but we have no other Drupal code that calls trigger_error() to accomplish this. I'm not sure we want to be establishing such a pattern here, for the first time, this late in D7, without further community involvement. Instead, I think we need a contrib module like screenlog.module (modeled on dblog.module and syslog.module) that can be used as a generic solution to sending some subset of watchdog() messages to the screen to sufficiently priveleged users. In any case, since such a module isn't in core, administrators should already be quite familiar with checking admin/reports/dblog when something isn't working as they expect.

This removes the use of t() and % as suggested in #29, and uses the standard watchdog() API for variable insertion.

This offers a more complete and hopefully helpful message.

Unfortunately, there is no such thing as www.example.com/admin/rebuild-theme-registry, and I assume it's too late to add it now.

Yeah, I think it's too late. We'd have to debate whether it exposes an unacceptable CSRF risk. On the one hand, rebuilding the theme registry is relatively harmless, so it's not a security risk necessarily, but on the other hand, it's a performance suck, so it potentially exposes DoS risk.

Note that the existing watchdog has been limited to the case when $hook is a string (and not an array of suggestions) via #910572: Command line installations are broken (programmatic form submissions don't need a theme?)

That is, because an array of suggestions is just a list of suggestions. In case none of the suggestion exists, there is no error. This allows forms and other renderable arrays to preemptively specify #theme suggestions, without having to needlessly register them upfront.

wow, i can't believe I haven't seen this issue before. I've come up against errors in theme() making the testbots made multiple times before and this would have saved me headaches there :(

pulling this to d8 as it's still an issue there (although technically now in _theme()).

please return FALSE instead of an empty string in the case that there is some error, as drupal_render() needs to know the difference between a missing hook and an implemented hook that triggers a theme function/template that then returns an empty string.

In D8, we should throw an exception instead, as this situation can only occur when code has changed or when code tries to use a theme hook that isn't available.

#36 is indeed what i was told in IRC when I brought this up some months back.

Let's see whether proper error reporting would have revealed + caught + prevented #2301245: Entity system invokes non-existing theme hooks: "Theme hook $entity_type_id not found."

A range of Contact module tests are expected to fail with this patch.

Victory: https://qa.drupal.org/pifr/test/824123

Theme hook block_content not found.
Theme hook entity_test not found.
Theme hook contact_message not found.
Theme hook form_required_marker not found.

EDIT: This means that various bogus code in HEAD calls theme() for theme hooks that do not exist.

There's a lot of prior art here. I am confused as to why this issue even exists. We went through trying exceptions and various other things before and ended up with Logging being all we could do: #412730: Theme system should optionally report when a theme key is not found

We went through trying exceptions and various other things before and ended up with Logging being all we could do

That was before error reporting was reconfigured to not display messages verbosely by default (in 2014). On production sites, errors will not be displayed, but logged only.

Also, in case you're using a better error handler than Drupal's default, your error handler doesn't even see the error.

In any case, the errors in #40 are real errors (broken code paths) in HEAD that we need to fix. No one noticed them for the past 1-2 years, because _theme() only logs the problem, and no one looks at the logs.

Fixing that is a separate issue, though it's possible that #2301245: Entity system invokes non-existing theme hooks: "Theme hook $entity_type_id not found." will fix all instances (except 'form_required_marker').

#2301245: Entity system invokes non-existing theme hooks: "Theme hook $entity_type_id not found." fixes this once for all. It needs to incorporate this patch in order to break tests. It won't introduce a dedicated integration test, because it would have to account for multiple layers of indirection. Triggering an error is more than sufficient to catch these (developer) bugs.

Aforementioned issue was committed without the trigger_error() change, so re-opening.

Attached patch re-incorporates #33 - even though theme functions are no longer supposed to exist in D8, as all of them are replaced by templates.

Speaking of templates, Twig_Loader_Filesystem goes way beyond of the proposed trigger_error() (not halting execution); it throws a Twig_Error_Loader exception upon any kind of error condition (halting execution).

You can try yourself by changing drupal_common_theme() in /core/theme.inc, manipulating the 'select' theme hook to specify 'template' => 'oops', and simply hitting the installer afterwards. Instead of the installer, you will see:

Twig_Error_Loader: Unable to find template "core/modules/system/templates/oops.html.twig"
  in "core/themes/seven/templates/install-page.html.twig" at line 31.
  in Twig_Loader_Filesystem->findTemplate() (line 202 of core/vendor/twig/twig/lib/Twig/Loader/Filesystem.php).

Compared to that, triggering the error handler for an unknown theme hook is very "relaxed", to put it mildly.

The error is only triggered in case your code factually tries to invoke a theme hook that does not exist; i.e., caused by a real bug in code, which you need to fix anyway, because apparently you're expecting some output to be generated that cannot be generated.

The error is only output as a visible message in the front-end, if the error handler/level is configured to do so. The corresponding error handling configuration already documents very prominently that you should not output errors on production sites.

Without triggering a proper error, the problem goes unnoticed, and not even tests are able to catch it.

In short, a code/developer error should trigger a proper error if the application may continue to operate despite the error. An unrecoverable error must throw an (uncaught) exception. Any attempt to work around that only introduces more problems than it solves.

+1 to throwing error! let's do not hide bugs

790 triggered warnings, 1 single message:

Missing theme function 'theme_toolbar_item' for theme hook 'toolbar_item'. Either rebuild the theme registry and/or correct the definition.

Looks like theme functions are not completely dead yet, and we have a bug somewhere. ;-)

Interesting. Apparently, we have a #type:

function toolbar_element_info() {
  $elements['toolbar_item'] = array(
    '#theme' => 'toolbar_item',
  );
  return $elements;
}

And we have a #theme:

function toolbar_theme() {
  $items['toolbar_item'] = array(
    'render element' => 'element',
  );
  return $items;
}

But the theme hook neither defines a function nor a template. drupal_render() can't know that (doesn't have access to theme registry), so it encounters a #theme and thus it calls _theme().

_theme(), however, only cares for 'function' and 'template', and contains no special processing logic for 'render element' at all (except of some code to prepare theme variables).

The Theme\Registry defaults to this:

        // 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;
        }

→ In turn, _theme() tries to invoke the specified #theme as a function (because there is no 'template'), but there is no theme_toolbar_item() function.

1) I'm not sure why the #type specifies a theme function in the first place?

2) Only in case 1) is legit, then the adjustment in attached patch is required, because #theme isn't used for a function/template, and drupal_render() takes over the rendering already.

Alternatively, here's the reverse - removing the (seemingly bogus) #theme hook.

Yes, that removal is correct. It should have been taken in care of #1898464: toolbar.module - Convert theme_ functions to Twig.

+      // The function is registered but does not exist (yet).
+      trigger_error(sprintf("Missing theme function '%s' for theme hook '%s'. Either rebuild the theme registry and/or correct the definition.", $info['function'], $hook), E_USER_WARNING);

Actually, the state of the theme registry cannot be inconsistent with registered theme hooks in D8. The "rebuild the theme registry" suggestion seems to date back to a time when hook_theme() wasn't cached and thus potentially inconsistent with the cached registry.

Instead, this condition is always an error in a hook_theme() definition. (like theme_toolbar_item())

The theme registry and this function have a lot of moving parts. Does this ensure we'll always rebuild the information for the failure entry because it might be hard to get to the clear cache button and requiring digging in the DB or drush access isn't really better DX most people.

Digging around I know this at least won't write the entry if its resolving a miss since we've got a manual destructor thing on the registry and E_USER_ERROR is fatal, I guess I'm just trying to make sure we won't be stuck with an already written value in the registry cache gumming up a site.

E_ERROR is fatal, E_USER_ERROR is not. This patch here just triggers the actual error handler to properly expose the error.

Unless mission-critical theme hooks (such as 'page') were customized in breaking ways, you are able to navigate to the Appearance or Performance page to adjust your site's configuration/state.

The only difference to HEAD is that all errors are visible in the UI, unless error handling was configured to log only.

reroll of #54
the last part of that patch, to toolbar.module, is no longer applicable, as it's removing code removed in
https://www.drupal.org/node/2226207

I don't know enough yet to say if the exceptions coming from the BooleanFormatterTest are a problem with the patch, or if the patch is revealing a problem with BooleanFormatter that was previously hidden.
Is this still a reroll, or does it need a dev's attention?

Reroll looks good. I can't explain those exceptions at a glance. Thanks @sidharrell!

This should fix the tests. I will write test cases for this later today

I guess I didn't write the tests that day :P

proper title

Combined with #2661470-2: KernelTestBase::render passes wrong values as a parameter for BareHtmlPageRenderer::renderBarePage

+++ b/core/modules/simpletest/src/KernelTestBase.php
@@ -597,7 +597,7 @@ protected function render(array &$elements) {
-      $elements, '', $this->container->get('theme.manager')->getActiveTheme()->getName()
+      $elements, '', 'page'

+++ b/core/modules/system/src/Tests/Common/AddFeedTest.php
@@ -64,7 +64,7 @@ function testBasicFeedAddNoTitle() {
-      $build, '', $this->container->get('theme.manager')->getActiveTheme()->getName()
+      $build, '', 'page'

+++ b/core/modules/system/src/Tests/Common/RenderTest.php
@@ -58,7 +58,7 @@ public function testProcessAttached() {
-      $renderer->renderBarePage($build, '', $this->container->get('theme.manager')->getActiveTheme()->getName());
+      $renderer->renderBarePage($build, '', 'page');

+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -913,7 +913,7 @@ protected function render(array &$elements) {
-      $elements, '', $this->container->get('theme.manager')->getActiveTheme()->getName()
+      $elements, '', 'page'

These seem very wrong. This will result in '#theme' => 'page', which makes no sense.

Why this change?

@Wim Leers. There is no #theme => 'bartik', or bartik.html.twig. Only page.html.twig. Is #theme being used in some other manor here?

Also I thought @lauriii was opening up a new issue for that change.

I opened #2661470: KernelTestBase::render passes wrong values as a parameter for BareHtmlPageRenderer::renderBarePage

#2661470: KernelTestBase::render passes wrong values as a parameter for BareHtmlPageRenderer::renderBarePage is in so this is unpostponed!

Patch without any test fixes to see failing tests

This is now postponed on #2698909: EntityViewBuilder uses non-existing #theme hooks

Patch here proves that this is necessary.

Unless things have changed since I last looked this might require modifications to the form system. Suppressing the error when theme is passed an array is done because forms use it to provide suggestions without giving an implementation at all. I doubt there are any tests that will show that this breaks but I guess there could be indirect failures.

@dawehner suggested that we unset the view builder. This should be green also

Ah, forget what I said in #94. I see that I misread the patch now. The issue title is a bit misleading (to say nothing of the issue summary...) since this still doesn't attempt to fix the WTF of unimplemented theme hooks being completely okay as long as you pass them in inside an array and mentions nothing of the secondary change of causing an error when a theme function itself is missing.

Suppose view builder should be unset for all entities that not "viewable"

+++ b/core/modules/shortcut/shortcut.module
@@ -438,3 +438,12 @@ function shortcut_themes_installed($theme_list) {
+ * Implements hook_entity_type_alter().
...
+function shortcut_entity_type1_alter(&$entity_types) {

s/type1/type

+++ b/core/modules/shortcut/shortcut.module
@@ -438,3 +438,12 @@ function shortcut_themes_installed($theme_list) {
+function shortcut_entity_type1_alter(&$entity_types) {

I think you uploaded the failing patch :) It should be entity_type_alter instead ...

Oops :)

+++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
diff --git a/core/modules/node/src/Tests/NodeListBuilderTest.php b/core/modules/node/src/Tests/NodeListBuilderTest.php
index 2d23604..e4a0d35 100644

index 2d23604..e4a0d35 100644
--- a/core/modules/node/src/Tests/NodeListBuilderTest.php

--- a/core/modules/node/src/Tests/NodeListBuilderTest.php
+++ b/core/modules/node/src/Tests/NodeListBuilderTest.php

+++ b/core/modules/node/src/Tests/NodeListBuilderTest.php
+++ b/core/modules/node/src/Tests/NodeListBuilderTest.php
@@ -20,7 +20,7 @@ class NodeListBuilderTest extends KernelTestBase {

@@ -20,7 +20,7 @@ class NodeListBuilderTest extends KernelTestBase {
   /**
    * {@inheritdoc}
    */
-  public static $modules = ['node', 'user'];
+  public static $modules = ['node', 'user', 'system'];
 

Is this change really needed?

@dawehner It is because that test requires a theme hook from system module and fails for that reason

Gotcha ... so what about writing a kernel test ... note: phpunit should convert that error to an exception, so its doable to test it.

Just a quick reroll of the latest patch.

hah, have we accidentally been introducing bugs into our tests because we don't trigger theme errors? that's hilarious...