The theme function looks a bit like this:

  // Generate the output using either a function or a template.
  if (isset($info['function'])) {
    if (function_exists($info['function'])) {
      $output = $info['function']($variables);
    }
  }
  else {
  ..
    // Render the output using the found template file.
    $output = $render_function($template_file, $variables);
  }
return $output;

if $info['function'] doesn't exist, a notice will be thrown:
Notice: Undefined variable: output in theme() (line 932 of /home/nsh/drupal7/drupal/includes/theme.inc).

Files: 

Comments

nvanhove’s picture

Status: Active » Needs review
aspilicious’s picture

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

nvanhove’s picture

FileSize
612 bytes
Passed on all environments. View

Added a space, removed tabs for spaces.

effulgentsia’s picture

Title: theme() not always returns output » If a registered theme function doesn't exist, PHP notice is thrown
FileSize
697 bytes
PASSED: [[SimpleTest]]: [MySQL] 17,887 pass(es). View

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.

aspilicious’s picture

+1 for logging the message

marcvangend’s picture

FileSize
1.26 KB
PASSED: [[SimpleTest]]: [MySQL] 18,730 pass(es). View

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.

neclimdul’s picture

Status: Needs review » Needs work

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

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.

effulgentsia’s picture

tamasd’s picture

FileSize
541 bytes
PASSED: [[SimpleTest]]: [MySQL] 20,542 pass(es). View

My patch from the duplicate thread

marcvangend’s picture

Title: If a registered theme function doesn't exist, PHP notice is thrown » If a registered theme function doesn't exist, PHP notice is thrown and $output is undefined
Status: Needs work » Needs review
FileSize
1.48 KB
PASSED: [[SimpleTest]]: [MySQL] 20,552 pass(es). View

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

tamasd’s picture

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.

effulgentsia’s picture

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

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.

neclimdul’s picture

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.

marcvangend’s picture

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

marcvangend’s picture

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

effulgentsia’s picture

Title: If a registered theme function doesn't exist, PHP notice is thrown and $output is undefined » Need better error handling within theme()
Assigned: effulgentsia » Unassigned
Status: Needs work » Needs review
FileSize
6.25 KB
PASSED: [[SimpleTest]]: [MySQL] 20,591 pass(es). View

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.

Status: Needs review » Needs work

The last submitted patch, theme-error_handling-674108-17.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
Damien Tournoud’s picture

Status: Needs review » Needs work

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.

effulgentsia’s picture

Status: Needs work » Needs review

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

Damien Tournoud’s picture

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

effulgentsia’s picture

FileSize
2.62 KB
PASSED: [[SimpleTest]]: [MySQL] 20,600 pass(es). View

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.

Damien Tournoud’s picture

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

effulgentsia’s picture

FileSize
2.05 KB
PASSED: [[SimpleTest]]: [MySQL] 20,602 pass(es). View

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

effulgentsia’s picture

FileSize
2.05 KB
PASSED: [[SimpleTest]]: [MySQL] 20,598 pass(es). View

Also removed quotes as per #24.

sun’s picture

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

effulgentsia’s picture

FileSize
2.1 KB
PASSED: [[SimpleTest]]: [MySQL] 20,596 pass(es). View

Indeed.

marcvangend’s picture

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

Status: Needs review » Needs work
neclimdul’s picture

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

marcvangend’s picture

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

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
3.2 KB
PASSED: [[SimpleTest]]: [MySQL] 20,930 pass(es). View

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.

sun’s picture

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.

thedavidmeister’s picture

Title: Need better error handling within theme() » Need better error handling within _theme()
Version: 7.x-dev » 8.x-dev
Issue summary: View changes
Status: Needs review » Needs work

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.

sun’s picture

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.

thedavidmeister’s picture

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

sun’s picture

Title: Need better error handling within _theme() » _theme() does not trigger an error when a theme hook is not found
Status: Needs work » Needs review
Issue tags: +DX (Developer Experience)
Related issues: +#2301245: Entity system invokes non-existing theme hooks: "Theme hook $entity_type_id not found."
FileSize
729 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,031 pass(es), 159 fail(s), and 154 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 38: drupal8.theme-trigger-error.38.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

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.

Crell’s picture

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

sun’s picture

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

sun’s picture

Status: Needs review » Closed (duplicate)

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

sun’s picture

Status: Closed (duplicate) » Needs review
Issue tags: +Testing system
Related issues: +#2051877: Log error when people use invalid route parameters
FileSize
1.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,163 pass(es), 0 fail(s), and 790 exception(s). View

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.

andypost’s picture

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

Status: Needs review » Needs work

The last submitted patch, 44: theme.error_.44.patch, failed testing.

sun’s picture

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

sun’s picture

Status: Needs work » Needs review
FileSize
1.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,242 pass(es). View
659 bytes

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.

sun’s picture

FileSize
2.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,241 pass(es). View
747 bytes

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

tim.plunkett’s picture

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

Status: Needs review » Needs work

The last submitted patch, 49: theme.error_.49.patch, failed testing.

sun queued 49: theme.error_.49.patch for re-testing.

sun’s picture

Status: Needs work » Needs review
sun’s picture

FileSize
1.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,685 pass(es). View
702 bytes
+      // 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())

sun queued 54: theme.error_.54.patch for re-testing.

sun queued 54: theme.error_.54.patch for re-testing.

sun’s picture

Assigned: Unassigned » sun
neclimdul’s picture

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.

sun’s picture

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.

jhedstrom’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
sidharrell’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,851 pass(es), 0 fail(s), and 8 exception(s). View
2.84 KB

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

Status: Needs review » Needs work

The last submitted patch, 61: theme_does_not-674108-61.patch, failed testing.

sidharrell’s picture

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?

Cottser’s picture

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

lauriii’s picture

Assigned: sun » lauriii
Status: Needs work » Needs review
FileSize
1.03 KB
2.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,585 pass(es), 12 fail(s), and 12 exception(s). View

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

lauriii’s picture

Assigned: lauriii » Unassigned

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

Status: Needs review » Needs work

The last submitted patch, 65: theme_does_not-674108-65.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 65: theme_does_not-674108-65.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
5.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,851 pass(es), 12 fail(s), and 6 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 71: theme_does_not-674108-71.patch, failed testing.

andypost’s picture

Title: _theme() does not trigger an error when a theme hook is not found » ThemeManager:theme() does not trigger an error when a theme hook is not found
Related issues: +#2388247: Documentation refers to _theme() function, which has been removed

proper title

lauriii’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Needs review
FileSize
4.51 KB

Status: Needs review » Needs work

The last submitted patch, 74: thememanager_theme-674108-74.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
lauriii’s picture

The last submitted patch, 76: thememanager_theme-674108-76.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 78: thememanager_theme-674108-78.patch, failed testing.

andypost’s picture

Cottser’s picture

Title: ThemeManager:theme() does not trigger an error when a theme hook is not found » ThemeManager::theme() does not trigger an error when a theme hook is not found
andypost’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 81: thememanager_theme-674108-81.patch, failed testing.

Wim Leers’s picture

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

joelpittet’s picture

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

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

lauriii’s picture

lauriii’s picture

Status: Needs review » Needs work

The last submitted patch, 81: thememanager_theme-674108-81.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.78 KB

Patch without any test fixes to see failing tests

Status: Needs review » Needs work

The last submitted patch, 91: thememanager_theme-674108-91.patch, failed testing.

lauriii’s picture

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

Patch here proves that this is necessary.

Antti J. Salminen’s picture

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.

lauriii’s picture

Status: Postponed » Needs review
lauriii’s picture

Status: Needs review » Postponed
lauriii’s picture

Status: Postponed » Needs review
FileSize
2.86 KB
1.51 KB

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

Status: Needs review » Needs work

The last submitted patch, 97: thememanager_theme-674108-97.patch, failed testing.

Antti J. Salminen’s picture

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.

andypost’s picture

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

dawehner’s picture

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

lauriii’s picture

Status: Needs work » Needs review
FileSize
2.86 KB
531 bytes

Oops :)

dawehner’s picture

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

lauriii’s picture

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

dawehner’s picture

Status: Reviewed & tested by the community » Needs review

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.