EntityViewBuilder uses non-existing #theme hooks

@lauriii can you explain the problem a little?

Ahh, I was very confused at first, because entity types shouldn't be given a view_builder unless they specify.
Except \Drupal\Core\Entity\ContentEntityType forcibly adds \Drupal\Core\Entity\EntityViewBuilder as one!

Unfortunately, removing that line is out of the question. It would be the correct fix, since it blindly adds EntityViewBuilder without ensuring there is a corresponding theme hook.

Since AFAIK we don't actually view shortcuts via that code, I think we should add an alter to unset the view builder...

Could we check somehow in the view builder whether the theme hook exists and otherwise throw an exception?

Let's see how many thing could be broken

Suppose #2270883: Automatically add theme hook suggestions for all entity types should be fixed first

The same way "menu_link_content" missing view builder

I see no reason to implement view builder because root of problem in missing theme functions, see #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)

Ways to solve that
- add missing theme functions (wrappers or real ones) #2270883: Automatically add theme hook suggestions for all entity types
- add preprocessing into \Drupal\Core\Entity\EntityViewBuilder #2023571: Support preprocessing in EntityViewBuilder

Missing builders after applying #7

    [block_content_type] => config
    [comment_type] => config
    [commerce_line_item_type] => config
    [commerce_order_type] => config
    [commerce_order] => content
    [commerce_line_item] => content
    [commerce_currency] => config
    [commerce_product_variation_type] => config
    [commerce_product_attribute] => config
    [commerce_product_type] => config
    [commerce_store_type] => config
    [contact_form] => config
    [editor] => config
    [entity_queue] => config
    [entity_subqueue] => content
    [field_storage_config] => config
    [field_config] => config
    [file] => content
    [filter_format] => config
    [image_style] => config
    [node_type] => config
    [profile_type] => config
    [rdf_mapping] => config
    [search_page] => config
    [shortcut] => content
    [shortcut_set] => config
    [action] => config
    [menu] => config
    [taxonomy_vocabulary] => config
    [user] => content
    [user_role] => config
    [menu_link_content] => content
    [view] => config
    [base_field_override] => config
    [entity_view_display] => config
    [entity_form_display] => config
    [entity_view_mode] => config
    [entity_form_mode] => config
    [date_format] => config

drush @d8 scr theme-entity.php

<?php
  $types = [];
  /** @var \Drupal\Core\Utility\ThemeRegistry $registry */
  $registry = \Drupal::service('theme.registry')->getRuntime();
  foreach (\Drupal::entityTypeManager()->getDefinitions() as $definition) {
    if (!$definition->hasViewBuilderClass()) {
      $types[$definition->id()] = $definition->isSubclassOf('Drupal\Core\Config\Entity\ConfigEntityInterface') ? 'config' : 'content';
    }
  }
  print_r($types);

The core content entities without theme function are

    [block_content] => content
    [contact_message] => content
    [file] => content
    [shortcut] => content
    [menu_link_content] => content

script is

<?php
  $types = [];
  /** @var \Drupal\Core\Utility\ThemeRegistry $registry */
  $registry = \Drupal::service('theme.registry')->getRuntime();
  foreach (\Drupal::entityTypeManager()->getDefinitions() as $definition) {
    if (!$registry->has($definition->id()) && !$definition->isSubclassOf('Drupal\Core\Config\Entity\ConfigEntityInterface')) {
      $types[$definition->id()] = 'content';
    }
  }
  print_r($types);

None of those really have any meaning, you cannot render out a file entity, nor a contact message etc. I would argue that removing the view builder manually is the right solution still.

@dawehner did you mean that way?
Only alter allows to unset that.

@andypost
Yeah something like that ...

Looks fine and has tests. Thanks!

Sorry to mess with such a simple and RTBC patch, but that alter was bothering me.
Specifying NULL in the annotation makes more sense to me, so that we keep the values with the rest of the annotation. Additionally, a custom module can now specify a view builder if they so desire, without needing to worry about hook invocation order.

Also since I was worried about the treatment of NULL, I expanded the tests.

We also have this problem with other entity types, we just have no tests for them. menu_link_content, file and probably more.

The default entity view builder was added in #2446483: Viewing fields requires a view builder., so with this change, it is now longer possible to add shortcut fields to a view. So, I don't think we can just remove it.

Counter-proposal.

This should be better.

Updated the previous test. It's no longer shortcut specific, so we could make it generic as well and use entity_test or so.

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

+++ b/core/modules/shortcut/tests/src/Kernel/ShortcutEntityTest.php
@@ -0,0 +1,38 @@
+class ShortcutEntityTest extends KernelTestBase {

Should we make this a generic entity test instead?

I meant something like this.

@jibran: I would prefer to keep those overrides for forward compatibility with #2808481: Introduce generic entity template with standard preprocess and theme suggestions.

I was thinking about making the hook generic as well but actually, I'm not sure if we need to test every single entity type for a generic implementation.

Ok then NR for patch in #29 again.

Re-uploading the patch from #29 to make sure it still passes.

I think this is ready unless someone feels strongly about converting this test to use entity_test or so. Note that entity_test actually gets a template in #2808481: Introduce generic entity template with standard preprocess and theme suggestions, so it would need to be updated there again.

I think this is ready unless someone feels strongly about converting this test to use entity_test or so.

I think it is fine.

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -159,6 +158,11 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode) {
+    if (\Drupal::service('theme.registry')->getRuntime()->has($this->entityTypeId)) {

We should inject the service - no?

I'm not too keen on using shortcut for this. It is easy to imagine someone adding a shortcut theme and removing the test and then we lose coverage of this change.

Ok. This moves the test to EntityViewBuilderTest and uses test entities. I also injected the theme registry but didn't update all the subclasses, core has 3 that inject additional dependencies.

I also removed the #theme removal in EntityTestViewBuilder. This is different from the others, as that will get to use the generic entity template in #2808481: Introduce generic entity template with standard preprocess and theme suggestions and then I will update the test for the new behavior.

Removed left-over empty test class.

Thank you, that looks good. I agree with not updating the other view builders for now, that would probably double that patch size (or worse...).

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityViewBuilderTest.php
@@ -2,9 +2,11 @@
+use Drupal\shortcut\Entity\Shortcut;

That's not used.

Can be fixed on commit.

Not sure what happened here...

Testbot, be nice again, please :)

Committed and pushed 408f00c to 8.3.x and 0290c9c to 8.2.x. Thanks!

Fixed unused use on commit.

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -159,6 +170,11 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode) {
+      $build['#theme'] = $this->entityTypeId;

This kind of magic naming concerns me. But not changed here...