Entity system invokes non-existing theme hooks: "Theme hook $entity_type_id not found."

Total lack of proper error reporting. Most likely this code/logic bug exists in HEAD for 1-2 years already (i.e. since the original rewrite for entities/fields).

Currently testing whether the following stone-age issue would have prevented this: #674108: ThemeManager::theme() does not trigger an error when a theme hook is not found

Closely related, same conceptual situation: #2051877: Log error when people use invalid route parameters

Suppose we need a template to render message, also integration with quickedit module would be nice

We can explicitly set #theme => NULL to prevent the default element info for #theme to get applied.

A generic entity template (see related issue) still makes sense to me, but perhaps that might not make much sense for a contact message entity, because it is primarily rendered into an email body. Any additional wrapping HTML markup will produce at least superfluous white-space in the html2text conversion.

This issue blocks me in massive ways, so bumping priority. Also re-categorizing to entity system + adjusting title.

Removing "Needs tests" tag, because (1) there's no clean/simple/isolated way to test this, and (2) the only necessary change to prevent this from happening in the future is to replace the log message in _theme() with trigger_error(); cf. #674108: ThemeManager::theme() does not trigger an error when a theme hook is not found

The complete list of broken theme invocation attempts is:

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.

I can see multiple options to fix this though:

A) Inject the Theme\Registry (actually the runtime Utility\ThemeRegistry) into EntityViewBuilder. Automatically check in getBuildDefaults() whether there is a theme hook for the entity type ID before assigning #theme.

B) Because it's an edge-case to begin with, require specialized EntityViewBuilder classes of affected entities (i.e., EntityTestViewBuilder + BlockContentViewBuilder + not yet existing Contact\MessageViewBuilder) to override getBuildDefaults(), so as to manually remove or adjust #theme after calling the parent.

I think I'd prefer B), because less magic, less interdependencies, and more explicit.

Thoughts?

"entity_test" should not appear in test failures.

This should theoretically come back green.

#2152217: Remove theme_form_required_marker() from the theme system - use CSS instead forgot to remove one instance of 'form_required_marker'.

+++ b/core/includes/theme.inc
@@ -2449,10 +2449,7 @@ function template_preprocess_field_multiple_value_form(&$variables) {
-      $header[0]['data']['required'] = array(
-        '#theme' => 'form_required_marker',
-        '#element' => $element,
-      );
+      $header[0]['data']['#prefix'] = '<h4 class="label form-required">';

Missing #suffix closing the /h4 tag?

@joelpittet: Missing diff context: http://cgit.drupalcode.org/drupal/tree/core/includes/theme.inc#n2437

Ah yeah thanks for clearing that up. How about this then? Interdiff against #9

Instead of this, can we do something like #theme => array('entity', $this->entityTypeId)? (or array('entity', 'entity--' . $this->entityTypeId) but that would require to rename the existing templates)

This template and the corresponding template preprocess hook is one of the last magical pieces you need to provide to properly display an entity, I'm still trying to generalize what I can of that, see #2023571: Support preprocessing in EntityViewBuilder. would be nice if we could provide something out of the box, most of these templates (node, user, comment, term) are now very similar.

Related exposes idea to implement suggestions like #13 said for all entity types

@Berdir: In general, that sounds like a good idea, and reminds a lot of #2186653: $build['#attributes'] added in hook_entity_view_alter() are not output for entity types that provide neither a #theme nor #theme_wrappers (breaks Quick Edit module for those entity types)

However, AFAICS, in the specific cases here (at least Contact\Message + BlockContent), the entity intentionally has no larger/wrapping template — i.e., the inner content of a BlockContent block gets wrapped into a block template already, so if the inner entity would use an own entity template, then that would duplicate at least the subject/title/label in the output. Likewise, a Contact\Message is only supposed to render the inner entity fields.

Specifying an array of theme hook suggestions instead of a single would still cause _theme() to render a generic entity template (or trigger an error, until a generic template exists) for an entity that doesn't want a template.

So the difference is that automation/generalization is great, but doesn't help code to opt-out. :)

@joelpittet: Yes, that interdiff looks like a good idea.

Ok cool here it is.

Thanks @joelpittet!

This looks ready to me.

@Berdir et al, are you OK with moving forward here?

Thoughts, anyone? Is this good enough to move forward for now?

The proposed solution respects the current API and does not involve API changes. Entities that do not have or want a template simply adjust the render array to not have a #theme.

We can look into improvements and better ideas, but as a first step it would be great to resolve the issue based on status quo of HEAD.

Just throwing this out there, a generic entity theme hook fallback?

OR maybe we don't dynamically add theme hooks to entities and they are manually set as a $theme property of the builder?

The fix for contact message and block_content looks fine to me, agreed that it is by design there to not have a template, can we maybe add a quick inline comment why we are doing this?

The other parts seem somewhat unrelated to the current issue title at least, and the issue summary also needs an update.

What I think is not correct is the change to EntityTestViewBuilder, the only reason we are removing it there is that it currently doesn't exist, I think it would be more correct to add it there and maybe unify it on entity_test instead of removing, because we have a bunch of almost identical entity types that would all require a different template.

Sorry, I didn't recognize your comment, @Berdir - thanks for having a look.

What I think is not correct is the change to EntityTestViewBuilder, the only reason we are removing it there is that it currently doesn't exist, I think it would be more correct to add it there and maybe unify it on entity_test instead of removing, because we have a bunch of almost identical entity types that would all require a different template.

Wondering whether we can move that into a separate issue? Right now, those test entities do not have a wrapping template. I can only assume that adding a template would cause a change in behavior and output, which in turn will most likely cause test failures.

All of the affected test entities are based on the entity_test entity, so that is why adjusting that single entity view builder is sufficient to resolve the errors.

Ok, I'm fine with a separate issue, can we add a comment with @todo and issue reference? i doubt that would cause test failures, but it's fine.

Yes, I know that a single override is enough, what I meant is that we'd still need an override to set the #theme always to entity_test and not entity_test_mul/rev/... as it would be by default. Once we add

Created #2323461: Missing test coverage for entity template output of entity_test* entities

The separate issue should be sufficient, so I did not add a @todo.

#2260059: Title in template_preprocess_item_list() is typecasted as string added a new, bogus '#theme' => 'markup' call to Drupal\system\Tests\Theme\FunctionsTest.

1. +++ b/core/includes/theme.inc
   @@ -462,7 +462,7 @@ function _theme($hook, $variables = array()) {
          // Only log a message when not trying theme suggestions ($hook being an
          // array).
          if (!isset($candidate)) {
   -        \Drupal::logger('theme')->warning('Theme hook %hook not found.', array('%hook' => $hook));
   +        trigger_error(sprintf("Theme hook %s not found.", $hook), E_USER_ERROR);
          }
   

   Do we need to update the comment here? We're not longer logging it, we're triggering an array (which might then be logged again, but we don't know that or have to)

2. +++ b/core/includes/theme.inc
   @@ -2468,12 +2472,6 @@ function template_preprocess_field_multiple_value_form(&$variables) {
   -    if (!empty($element['#required'])) {
   -      $header[0]['data']['required'] = array(
   -        '#theme' => 'form_required_marker',
   -        '#element' => $element,
   -      );
   -    }
   

   Is this actually resulting in visible bugs, in case of multiple value fields or something?

I'm OK with not adding a @todo to entity_test if we have the issue, but I still think we should document block_content/contact and explain why we have that unset() ?

Adjusted/added comments.

The required marker in the multiple field widget table header is simply not output in HEAD and silently ignored, because the missing theme hook is only logged. This patch makes the corresponding test fail, because a proper error is triggered. I don't think we need additional test coverage beyond the existing.

Thanks, yes, I just wanted to be sure to understand what is going on there.

This looks good to me. I don't think there is something that needs a change record or so, as written above already.

This was deliberately an error message log, and not an exception trigger. See #412730: Theme system should optionally report when a theme key is not found for backstory. Short version: 1) pushing errors that only devs can do something about to user space, 2) makes it extremely frustrating to build out a module/theme because you'll get exceptions before you can actually clear the cache.

Could we please reduce this issue to whatever's needed to actually fix the bug, which seems to only be the unset('#theme') parts?

Btw, I'm not opposed to revisiting that decision at all, but it doesn't make sense to lump it in with an actual bug fix, since it's an optional functionality change for better DX.

please reduce this issue to whatever's needed to actually fix the bug, which seems to only be the unset('#theme') parts?

OK, sounds like you're fine with committing this fix without exposing the error (also in tests). Happy to move the trigger_error() change into a separate issue.

Same as #30, sans change to _theme().

Wicked, thanks. And yep, let's discuss what we want to have happen. I can definitely see the argument for finding out at the time (not 8+ months later) that you have a bug, but at the same time we can't make theme/module development overly painful.

Committed and pushed to 8.x. Thanks!

Thanks!

Re-opened #674108: ThemeManager::theme() does not trigger an error when a theme hook is not found for the trigger_error() change (which was previously marked as duplicate in favor of this issue).