Problem/Motivation

Once #1503464: Automatically add theme hook suggestion for node view mode is in, would be great to have a generic mechanism that adds the view mode based suggestions to all entity types, not only for nodes.

Proposed resolution

Create a generic system that generates theme hook suggestions for entities based on entity type, bundle and view mode.

Remaining tasks

Create the patch.
Testing.

User interface changes

None

API changes

None

Comments

plopesc’s picture

Status: Active » Needs review
FileSize
4.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch entity_suggestions_view_mode-2270883-1.patch. Unable to apply patch. See the log in the details link for more information. View

Hello

Attaching first approach for this issue, creating a functions that includes theme_suggestions based on the entity view modes in case we are theming an entity with view modes.

There could be another better approach. Any feedback is welcomed here.

Thank you!

Status: Needs review » Needs work

The last submitted patch, 1: entity_suggestions_view_mode-2270883-1.patch, failed testing.

plopesc’s picture

Status: Needs work » Needs review
Manuel Garcia’s picture

Status: Needs review » Needs work

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

plopesc’s picture

Status: Needs work » Needs review
FileSize
4.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 40,112 pass(es), 25,698 fail(s), and 6,550 exception(s). View

Hello Manuel
I found the bug, it was my mistake because getDefinition() threw an exception because I didn't add the correct parameter.

New patch including this change:

+++ b/core/includes/theme.inc
@@ -2248,6 +2251,33 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+  $entity_definition = Drupal::entityManager()->getDefinition($entity_type_id);

Changed to getDefinition($entity_type_id, FALSE) to avoid exceptions.

And changing file structure after #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4

Regards.

plopesc’s picture

FileSize
4.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,641 pass(es). View

Patch in #6 was only the re-roll of #1

Here is the patch including the bug fixed.

The last submitted patch, 6: entity_suggestions_view_mode-2270883-6.patch, failed testing.

Schnitzel’s picture

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

plopesc’s picture

FileSize
3.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch entity_suggestions_view_mode-2270883-10.patch. Unable to apply patch. See the log in the details link for more information. View
4.78 KB

Addressing comments in #9

Regards

andypost’s picture

Status: Needs review » Needs work

The last submitted patch, 10: entity_suggestions_view_mode-2270883-10.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,326 pass(es), 2 fail(s), and 0 exception(s). View
3.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,330 pass(es). View

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

andypost’s picture

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

The last submitted patch, 15: entity_suggestions_view_mode-2270883-15-tests.patch, failed testing.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Yes yes!
Gave this a test:

Still good:




And moar.







YIPPEEE



  
    
BOOOO YAH
Cottser’s picture

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?

joelpittet’s picture

Status: Reviewed & tested by the community » Needs work

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

Cottser’s picture

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.

joelpittet’s picture

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

Status: Needs work » Needs review
FileSize
3.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,286 pass(es). View
2.01 KB

Hello
New version that I think improves performance a bit.
Firstly checks if $variables['elements']['#view_mode'] is set, given that this property is set by EntityViewBuilder for Content Entities and that's what we are looking for here.
Then it checks the other variables and uses local variables for entity_type, bundle and entity_id.

I know this hook approach can hit the performance, other option could be do something more "special" that could convert something inserted in EntityViewBuilder automatically into theme suggestions.

Thank you for your reviews.

joelpittet’s picture

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

plopesc’s picture

FileSize
6.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,550 pass(es). View
2.79 KB

Hello

All content entity types provide at least "default" view mode, so there is no possibility to get one without ['#view_mode'].
On the other hand, config entities don't allow bundles, bundle() method returns the entityTypeId.

So, I think we are OK now...

New patch adding support for custom blocks, given that they are embedded into regular blocks and needs specific support.

Anyway, maybe would be a good idea to get more feedback from the entity team.

Regards

joelpittet’s picture

Assigned: Unassigned » Berdir

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

Berdir’s picture

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

joelpittet’s picture

Assigned: Berdir » Unassigned

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

plopesc’s picture

FileSize
9.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,748 pass(es). View

Hello

Here is a new approach for this patch following some of the directions provided by Berdir (Thank you for your time).

This uses a specific implementation for blocks, given that its architecture is different.

Let' see if that approach is better than previous :)

Regards

Manuel Garcia’s picture

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

Manuel Garcia’s picture

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

Status: Needs review » Needs work
  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’s picture

Status: Needs work » Needs review
FileSize
12.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch entity_suggestions_view_mode-2270883-33.patch. Unable to apply patch. See the log in the details link for more information. View
4.25 KB

Thank you for your review @manuel and @Cottser

Here is a new patch addressing comments in #32

Also includes a new test file.

Regards

SteffenR’s picture

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

Berdir’s picture

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

joelpittet’s picture

Status: Needs review » Needs work
Issue tags: +Amsterdam2014

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

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.22 KB
12.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,797 pass(es), 85 fail(s), and 55 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 37: entity_suggestions_view_mode-2270883-37.patch, failed testing.

Berdir’s picture

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

The last submitted patch, 1: entity_suggestions_view_mode-2270883-1.patch, failed testing.

plopesc’s picture

The last submitted patch, 33: entity_suggestions_view_mode-2270883-33.patch, failed testing.

Cottser’s picture

Issue tags: +Needs reroll

Tagging for reroll.

Manjit.Singh’s picture

FileSize
13.01 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch entity_suggestions_view_mode-2270883-46.patch. Unable to apply patch. See the log in the details link for more information. View

re-rolling #37

Manjit.Singh’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

Status: Needs review » Needs work

The last submitted patch, 46: entity_suggestions_view_mode-2270883-46.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 46: entity_suggestions_view_mode-2270883-46.patch, failed testing.

SteffenR’s picture

I'll reroll the patch against latest core version.

SteffenR’s picture

Status: Needs work » Needs review
SteffenR’s picture

FileSize
13.03 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,610 pass(es), 132 fail(s), and 105 exception(s). View

Attached rerolled patch against latest core.

Status: Needs review » Needs work

The last submitted patch, 53: entity_suggestions_view_mode-2270883-52.patch, failed testing.

The last submitted patch, 53: entity_suggestions_view_mode-2270883-52.patch, failed testing.

SteffenR’s picture

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

SteffenR’s picture

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

joelpittet’s picture

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

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.

Status: Needs review » Needs work

The last submitted patch, 58: automatically_add_theme-2270883-58.patch, failed testing.

joelpittet’s picture

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'];

Berdir’s picture

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

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.

andypost’s picture

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

andypost’s picture

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

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.

Berdir’s picture

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.

jibran’s picture