Automatically add theme hook suggestions for all entity types

Adding an extremely closely related issue.

The patch applies cleanly, however upon trying to do a fresh install with this patch applied I see this:

Additional uncaught exception thrown while handling exception.
Original

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "install_page" entity type does not exist. in Drupal\Core\Entity\EntityManager->getDefinition() (line 181 of /var/www/drupal8/core/lib/Drupal/Core/Entity/EntityManager.php).

Drupal\Core\Entity\EntityManager->getDefinition('install_page')
theme_get_entity_suggestions(Array, 'install_page')
_theme('install_page', Array)
drupal_render(Array)
Drupal\Core\Page\DefaultHtmlPageRenderer::renderPage(Array, 'Choose language', 'install', Array)
install_display_output(Array, Array)
install_drupal()

Additional

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "install_page" entity type does not exist. in Drupal\Core\Entity\EntityManager->getDefinition() (line 181 of /var/www/drupal8/core/lib/Drupal/Core/Entity/EntityManager.php).

Drupal\Core\Entity\EntityManager->getDefinition('install_page')
theme_get_entity_suggestions(Array, 'install_page')
_theme('install_page', Array)
drupal_render(Array)
Drupal\Core\Page\DefaultHtmlPageRenderer::renderPage(Array, 'Error', 'install', Array)
install_display_output(Array, Array)
_drupal_log_error(Array, 1)
_drupal_exception_handler(Object)

I run rm -rf files/* && rm settings.php && cp default.settings.php settings.php && chmod 777 settings.php before attempting the installation, just in case its me that causing the problem...

I realy dont see why the patch would trigger this error, however if i undo the patch changes the installer shows up fine. Strange...

I really like that, but not sure if theme_get_entity_suggestions() should be in theme.inc, as it is specific to entities and not drupal in general. Maybe this should be in entity Module? And probably there with entity_theme_suggestions_alter() ?

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

I moved hook_theme_suggestions_alter() to system module since there's no entity-module anymore.

+++ b/core/modules/system/system.module
@@ -343,6 +343,24 @@ function system_theme_suggestions_field(array $variables) {
+function system_theme_suggestions_alter(array &$suggestions, array $variables, $hook) {
+  $entity_definition = Drupal::entityManager()->getDefinition($hook, FALSE);
+  if ($entity_definition && $entity_definition->hasViewBuilderClass() && isset($variables['elements']['#view_mode'])) {

+1

Yes yes!
Gave this a test:

Still good:

<!-- FILE NAME SUGGESTIONS:
   * node--1--full.html.twig
   * node--1.html.twig
   * node--article--full.html.twig
   * node--article.html.twig
   * node--full.html.twig
   x node.html.twig
-->

And moar.

<!-- THEME DEBUG -->
<!-- CALL: _theme('comment__comment__article') -->
<!-- FILE NAME SUGGESTIONS:
   * comment--comment--article.html.twig
   * comment--comment.html.twig
   * comment--1--full.html.twig
   * comment--1.html.twig
   x comment--comment--full.html.twig
   * comment--comment.html.twig
   * comment--full.html.twig
   * comment.html.twig
-->
<!-- BEGIN OUTPUT from 'core/themes/bartik/templates/comment--comment--full.html.twig' -->
YIPPEEE

<article class="comment by-node-author clearfix clearfix" role="article" data-quickedit-entity-id="comment/1" data-comment-user-id="1" about="/comment/1" typeof="schema:Comment">

  <header class="comment-header">
    <div class="attribution">
      

<!-- THEME DEBUG -->
<!-- CALL: _theme('user') -->
<!-- FILE NAME SUGGESTIONS:
   * user--1--compact.html.twig
   * user--1.html.twig
   x user--user--compact.html.twig
   * user--user.html.twig
   * user--compact.html.twig
   * user.html.twig
-->
<!-- BEGIN OUTPUT from 'core/themes/bartik/templates/user--user--compact.html.twig' -->
BOOOO YAH

<!-- END OUTPUT from 'core/themes/bartik/templates/user--user--compact.html.twig' -->

Couple questions, leaving at RTBC though. They are pretty minor.

+++ b/core/modules/system/system.module
@@ -343,6 +343,24 @@ function system_theme_suggestions_field(array $variables) {
+  $entity_definition = Drupal::entityManager()->getDefinition($hook, FALSE);
+  if ($entity_definition && $entity_definition->hasViewBuilderClass() && isset($variables['elements']['#view_mode'])) {

1. Shouldn't this be \Drupal?

2. Would checking isset($variables['elements']['#view_mode']) before doing any of the other checks make more sense from a performance perspective?

#re 20.2
I do think it may have a bit of a performance improvement but not a whole lot I'd hazard a guess. The first check is a boolean/truthy/falsey check which IIRC would be fastest, but the second function call would be slower than the isset() so it kind of depends on how often view_mode is not set. Wouldn't hurt to put it first though.

You could also store local variables for $entity->getEntityTypeId() and $entity->bundle(), etc to prevent multiple method calls, though I don't know how much we gain from those changes.

#re 20.1 Yes, it should be, nice catch @Cottser.

That gets me thinking though. Before we had a view_mode with no check. Now some entities may not have one.
Maybe that check for #view_mode should be around the view_mode specific suggestions? Otherwise entity's without view_modes won't get bundle suggestions. What do you think?

I just want us to keep in mind that every theme/template (that is not render cached) is going to be going through this function. So there is potentially a big impact.

Was talking out loud, not meant to dissuade :), I agree, let's do all the things. Good call, thank you for sober second thought.

@plopesc thanks that should be better:)

Wondering your thoughts re #20, if the view mode doesn't exist to still have the other bundle/id suggestions?

Thanks, yeah good call @plopesc maybe @Berdir or @catch could have a quick look, they my have some suggestions on these suggestions:)

+++ b/core/modules/system/system.module
@@ -302,6 +302,29 @@ function system_theme_suggestions_field(array $variables) {
+  if (isset($variables['elements']['#view_mode'])) {
+    $entity_definition = \Drupal::entityManager()->getDefinition($hook, FALSE);
+    if ($entity_definition && $entity_definition->hasViewBuilderClass()) {
+      $entity = $variables['elements']["#$hook"];

We had exactly the same discussion (how to properly identify an entity template) in #2023571: Support preprocessing in EntityViewBuilder. There this was considered as too less specific.

You should probably at least check if #$hook is set as well as you use it unconditionally later on.

I think it would be a good idea to add #entity_type to buildDefaults() in the entity view builder, that would make it a more reliable to identify the entity type. Because it is not mandatory to be the same, just the default.

If you look at the other issue, the approach there was to redirect the call back to the view builder. Doing that here would allow to a) avod duplicting the logic for block_content and give entities an easy way to change those suggestions. so we would add getTemplateSuggestions(EntityInterface $entity) there and use it here.

All you would do then here I think is check #view_mode and #entity_type, and when provided, check if $entity_manager->hasViewBuilder(). we might actually want to pass in $variables instead of $entity, so that we don't have to hardcode #$hook?

Thank you very much for the feedback. We can take a stab at your recommendations.

This looks good to me, here's what I did to test:

1. Applied the patch (clean)
2. Fresh install went fine
3. Enabled custom display mode for full page on the taxonomy vocabulary tags, created a tag.
4. Enabled twig debug mode, which got me:
   

   <!-- THEME DEBUG -->
     <!-- CALL: _theme('taxonomy_term') -->
     <!-- FILE NAME SUGGESTIONS:
        * taxonomy-term--1--full.html.twig
        * taxonomy-term--1.html.twig
        * taxonomy-term--tags--full.html.twig
        * taxonomy-term--tags.html.twig
        * taxonomy-term--full.html.twig
        x taxonomy-term.html.twig
     -->

5. Created the file taxonomy-term--tags--full.html.twig inside the theme, it got picked up and changes were reflected.

I'd set it to RTBC, but I don't understand the patch well enough for that call =)

If it helps, without the patch applied, these are the template suggestions for that same page (taxonomy/term/1)

<!-- THEME DEBUG -->
<!-- CALL: _theme('taxonomy_term') -->
<!-- FILE NAME SUGGESTIONS:
   * taxonomy-term--1.html.twig
   * taxonomy-term--tags.html.twig
   x taxonomy-term.html.twig
-->

1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -321,6 +322,25 @@ public function buildComponents(array &$build, array $entities, array $displays,
   +      $entity_type_id . '__' . $entity_id . '__' . $sanitized_view_mode,
   +
   +    );
   

   Extra blank line can be removed ;)

2. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilderInterface.php
   @@ -36,6 +36,19 @@
      /**
   +   * Preprocesses variables.
   +   *
   +   * @param \Drupal\Core\Entity\EntityInterface $entity
   +   *   The entity to get the template suggestions.
   +   * @param string $view_mode
   +   *   The view mode to render the entity.
   +   *
   +   * @return array
   +   *   The list of template suggestions.
   +   */
   +  public function getTemplateSuggestions(EntityInterface $entity, $view_mode = 'default');
   

   I don't think this is preprocessing variables - summary line should be updated.

Other than that looks good overall to me! Having this on the interface seems quite sensible.

@plopesc: The comment looks more descriptive to me - the suggestions are working fine - tested it for node entities and term entities.
What do you think cottser?

1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -321,6 +322,25 @@ public function buildComponents(array &$build, array $entities, array $displays,
   +    $bundle_name = $entity->bundle();
   ...
   +      $entity_type_id . '__' . $bundle_name,
   +      $entity_type_id . '__' . $bundle_name . '__' . $sanitized_view_mode,
   

   Not all entity types have bundles. User for example doesn't, so we would get template suggestions for user__user and so on.

   I would suggest you add a check for $entity->getEntityType()->getKey('bundle') to see if the entity type supports different bundles and then only add those if it makes sense.

2. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -321,6 +322,25 @@ public function buildComponents(array &$build, array $entities, array $displays,
   +    $sanitized_view_mode = strtr($view_mode, '.', '_');
   

   We have view modes with "." ?

3. +++ b/core/modules/block/src/BlockViewBuilder.php
   @@ -27,6 +27,12 @@ public function buildComponents(array &$build, array $entities, array $displays,
      /**
       * {@inheritdoc}
       */
   +  public function getTemplateSuggestions(EntityInterface $entity, $view_mode = 'default') {
   +  }
   

   I guess this makes this an API change that needs to be documented and accepted to still be possible after beta...

Setting as needs work as per @Berdir's notes in #35

#35.2: isn't it possible to add view_mode with "."?

I don't think that is valid (anymore?), view modes are part of the entity display entity ID, that can not possibly contain ".".

Tagging for reroll.

re-rolling #37

I'll reroll the patch against latest core version.

Attached rerolled patch against latest core.

Looks like the array structure of some parts changed - i'll fix it.

I'm not sure why the tests are breaking here - maybe some one else can help out fixing the issue.

re-rolling based on #37 there was only one failure in the patch which was
EntityViewBuilderInterface.php additions. So I added those in. Let's see if we're failing still.

Bumping to 8.1.x as it's a feature request.

So one error is that not all blocks have a 'block_content' key
block.module:183:

    $view_mode = $variables['elements']['#configuration']['block_content']['view_mode'];

1. +++ b/core/modules/block/block.module
   @@ -177,16 +178,25 @@ function block_theme_suggestions_block(array $variables) {
   +  if ($provider == 'block_content') {
   +    $entity = $variables['elements']['content']['#block_content'];
   +    $view_mode = $variables['elements']['#configuration']['block_content']['view_mode'];
   +    $entity_suggestions = \Drupal::entityManager()->getViewBuilder('block_content')->getTemplateSuggestions($entity, $view_mode);
   +
   +    $suggestions = array_merge($suggestions, $entity_suggestions);
      }
   

   Seems like this should live in block_content.module, not here? Possibly just remove the check and always do the else here?

2. +++ b/core/modules/block/src/BlockViewBuilder.php
   @@ -71,6 +71,12 @@ public function buildComponents(array &$build, array $entities, array $displays,
      /**
       * {@inheritdoc}
       */
   +  public function getTemplateSuggestions(EntityInterface $entity, $view_mode = 'default') {
   +  }
   

   Adding a new method to an interface is an API change. To be backwards compatible, we need to add a new interface for this..

Suppose before making that better to fix #2023571: Support preprocessing in EntityViewBuilder

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilderInterface.php
@@ -33,6 +33,19 @@
+  public function getTemplateSuggestions(EntityInterface $entity, $view_mode = 'default');

any reason to inject theme implementation details into entity layer?

New approach that has a bigger chance of getting in I hope: #2808481: Introduce generic entity template with standard preprocess and theme suggestions

Is this a duplicate of #2808481: Introduce generic entity template with standard preprocess and theme suggestions or postpone on that?

Duplicate of #2808481: Introduce generic entity template with standard preprocess and theme suggestions