Support preprocessing in EntityViewBuilder

Discussed this today with @fago, @yched and @Cottser, here's what we came up with.

All that field_attach_preprocess() does is make the raw values of all field items available as top level variables in a template, in the right language. (And invokes a alter hook. Just because it can).

That means that in Drupal 7, you could write something like

print $some_field[0]['value'];

Instead of having to write.

print $node->some_field[LANGUAGE_DEFAULT][0]['safe_value'];

(Actually, you would have to call field_get_items() and then use that)

That was useful. Fast-forward to today, you would print this like this:

{{ some_field.0.value }}

instead of
{{ node.some_field.value }}

Because the node object has an active language now and gives you the right translation and also allows you to skip the delta 0 if you want to. So, that shorthand really isn't useful anymore. We could put the field objects in there and allow to access them, but I don't think we should.

So, we can just drop this. Note that these are the raw, unformatted values. To print formatted fields, you use content.some_field. We don't loose anything when removing it, clean up the available variables and it's much easier to separate raw field values and formatted fields (and document them).

Second, we can do the following.

In EntityRenderController::getBuildDefaults(), add '#render_controller' => $this, to the $return array. Then in template_preprocess, add the following snippet:

  if (isset($variables['elements']['#rnder_controller'])) {
    $variables['elements']['#render_controller']->preprocess($variables);
  }

Then implement that method in EntityRenderController and document it in EntityRenderControllerInterface.

Tagging as Novice for the first steps:

1. Remove field_attach_preprocess() and all calls to it.
2. Add the above snippets as described.
3. Add the empty methods as described.

Optional, not 100% sure if we want to do it in this issue:
4. Find preprocess functions for entity and move them into the corresponding render controller, which you can find in the entity type annotation. e.g., move template_preprocess_node() into NodeRenderController::preprocess($variables)
5. Find common patterns (or things that should be common) between all these functions and move it into the base implementation. For example: $variables['view_mode'] = $variables['elements']['#view_mode'];

Just in terms of the printing of the field variables, I "think" i'd be fine with printing them from the node and without having them in the template root.

Though I'd prefer:
{{ node.some_field }}
As being the formatted/escaped value of the field

And the raw value being at.
{{ node.some_field|raw }}

And swap languages out with something like:
{{ node.some_field|lang('fr') }}

That way if for example there was a date field that was formatted, I could reformat it something like:
{{ node.createdDate|raw|date("F jS, Y") }}

Sorry if this 'value' bit needs to be there or the requests above are pie in the sky but just wanted to put a couple cents in. If you want I can split this off to a follow-up for DX.

The only thing that worries me slightly is putting the RenderController in the $render array that gets cached by the render cache. That's adding another case of "we serialize a business object", and those are nasty:
- they can contain random references other "big" objects --> serialization chain of hell
- render controllers are supposed to be "singleton-like" ($entity_manager->getRenderController() returns the same object shared throughout a request), but unserialization breaks objects identity.
Also, render controllers are *not* services, meaning #2004282: Injected dependencies and serialization hell does not cover them.

We could also have a standardized property like #entity_type, that would be enough to get the controller/builder/whatever?

yep, +1 on #entity_type / get the render controller yourself if you need it.

@jeolpittet: We're talking about the raw field values here. And specifically, we're talking about a list of field items where each consists of an array of values (although many only have one). It can't be simplified more. {{ node.some_field|raw }} would probably be something like array(0 => array('value' => 'raw string')) for the simplest fields, that doesn't help at all.

{{ content.some_field}} prints the formatted version of a field. There are only few cases to access the field values, e.g. when doing a conditional check on a checkbox.

The other things seem unrelated and to be honest, I wouldn't be sad if you're limited in what you can do. It is usually very confusing if you have configured your formatters to do something but the template uses e.g. a different date format.

@Berdir Sorry, probably the wrong issue to jump in on for that. Thanks for addressing the concern. I'll bow out and slink back to front-end land...

I agree it would be confusing if you're formatters didn't do as you expect through the UI because I know that happens in the menu's kinda right now (turn on 'expanded' in the menu UI and primary_links only shows one level, WTF). Through in some cases it would be nice to hide and enforce some of the formatters per theme and simplify UI settings for content admins hopefully at the same time(pipe dream atm).

Occured to me that I'm doing two things here, so might want to just drop the field attach function separately.

It's also a bit late to really unify those templates (e.g, it's weird that node has node_url and others url) but experimenting a bit. This will fail on terms because of #1857336: Use entity variable in $build for taxonomy_term entity.

Thanks for reviving this, @Berdir.

So in the end, it's not clear to me why we need a dedicated preprocessing step at all ?
Raw field values are automatically available with {{ node.field_foo }}, and it #3 is saying that we don't need to have {{field_foo }} available in addition ?

Yes, that's why I said there are two separate things happening here :)

field_attach_preprocess() can be dropped without any replacement. The idea of the new preprocess() function is to minimize code duplication on those preprocess methods and e.g. make sure that content is always there and so on. But I'm not sure how much we can actually unify at this point in the development cycle :)

Opened #2157153: Remove field_attach_preprocess() for that part.

Re-roll after field_attach_preprocess() has been removed now and also added the #view_mode check.

Completely wrong patch..

Wondering whether the current "smart guess" triggers (presence of #entity_type & #view_mode) are selective enough, though.
Shouldn't we at least check for #{sentity_type} too ? Or maybe an explicit #entity_preprocess = TRUE opt-in trigger ?

More generally, wondering about a unique, generic #theme = 'entity' hook.
- Would still need to resolve to node.tpl & friends rather than "entity.tpl"
- I guess each entity type still needs its own "tpl suggestions" logic...
Well, might be tricky / unintuitive magic.

First time I see this issue.
Whatever we decide we definitely need something predictable to work with. The current D7 situation is kinda annoying. You need to add several switches in your code if you want to build something generic.

#1857336: Use entity variable in $build for taxonomy_term entity removes all the crazy exceptions in core but it doesn't fore contrib to be "nice". Maybe we can throw an exception if the entity doesn't have the required structure.

Re-roll, added a check if #$entity_type exists, no other changes yet.

Ups.

I still think this would be quite useful, it is currently relatively hard to render a completely custom content entity.

Questions:
- This currently renames term to taxonomy_term in the template. Not sure if we want that or not.
- Should we move custom preprocess stuff in the preprocess method now?
- Should we unify more? There are different names for the label, title/name/feed_title/... And different names for the url, node_url/url/... This currently provides a default for url.
- If so, what should be the default label name? title?
- Alternatively, should we provide a default for title but still override it to keep the current behavior?
- Gven we do provide defaults for title and url, can we find a way to provide a default template? Right now, we just default to #theme => $entityTypeId. So you need to figure this out, copy and example from somewhere (for example node) and adjust it. In 7.x entity.module, it defaulted to #theme => entity and that was often enough. Not sure about accessing the entity within that template then. Or should we simply document this?

Ah, patch would help.

Adding Twig tag for template changes.

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilderInterface.php
@@ -84,4 +84,12 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
+   * Preprocess variables.

Preprocesses

Quick review:

1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -265,6 +267,29 @@ public function resetCache(array $entities = NULL) {
   +    $entity = $variables['elements']['#' . $this->entityTypeId];;
   

   double ;; at the line end.

2. +++ b/core/modules/taxonomy/taxonomy.module
   @@ -396,26 +396,15 @@ function taxonomy_theme_suggestions_taxonomy_term(array $variables) {
   -  $term = $variables['term'];
   +  $term = $variables['taxonomy_term'];
   

   Although the magic that is 'render element' is still a sore point for me and I could be wrong. The taxonomy_term variable still needs to be set. $variables['taxonomy_term'] = $variables['elements']['#taxonomy_term'];

3. +++ b/core/modules/taxonomy/taxonomy.module
   @@ -396,26 +396,15 @@ function taxonomy_theme_suggestions_taxonomy_term(array $variables) {
      $variables['name'] = check_plain($term->label());
   

   This could be moved to the template as {{ term.label|e }}

Thanks for the reviews. Reroll and addressed them.

#30
Fixed the docblock comment.

31:
1. Fixed.
2. Not necessary, that happens in the preprocess() method now. The only difference is that it now defaults to the entity_type ID, so we have to rename the code. Forgot to update rdf_preprocess_taxonom_term(), therefore the test fails.
3. Not a fan of that as long as we don't do check_plain() by default, this is too easily forgotten IMHO.

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -273,6 +275,29 @@ public function resetCache(array $entities = NULL) {
+
+    $variables['attributes']['class'][] = $this->entityTypeId;

I really like that this is DRY'ing up the building of entity content variables!

For the classes however, I'd rather it be explicit that these are added. The extra classes could cause some themeing issues if a entity type is named 'description' for example.

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -273,6 +275,29 @@ public function resetCache(array $entities = NULL) {
+    $variables['url'] = $entity->url();

Taking over the 'url' variable automatically could cause us some issues as well, is this needed? Term is the only one that is doing that it seems so that pattern doesn't have much to hang it hat on.

Related #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)
Suppose we need a basic wrapper|template for entities

+++ b/core/includes/theme.inc
@@ -2001,6 +2001,12 @@ function template_preprocess(&$variables, $hook, $info) {
+  // If this is an entity being rendered, call the preprocess() method on the
+  // view builder.
+  if (isset($variables['elements']['#entity_type']) && isset($variables['elements']['#view_mode'])) {

Special casing for entities in template_preprocess() feels … dirty. IDK the theme system well enough to suggest an alternative, but can't this be improved?

Just a re-roll for now.

Awesome clean-up of boilerplate code, so fix failures and removes URL #33.2 - not all entities provides it

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -18,6 +18,7 @@
 use Drupal\Core\Render\Element;
 use Drupal\entity\Entity\EntityViewDisplay;
+use Drupal\Core\Render\Element;

Element already there

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -360,6 +362,29 @@ public function resetCache(array $entities = NULL) {
+    $variables['attributes']['class'][] = $this->entityTypeId;

suppose it would be more useful to provide data-entity-type attribute

Rerolling...

Rerolled, please have a look as there have been a lot of changes since the last patch.

core/modules/user/user.pages.inc was removed, template_preprocess_user now lives in user.module, and with the modifications from previous patch it is now an empty function.

Several classes were being removed from preprocess functions, these are now removed from the appropiate twig files, although I'm not sure that's what we want to do: For example, node class was being removed from template_preprocess_node, it's now removed from core/themes/classy/templates/content/node.html.twig.

Sorry double upload...

+++ b/core/themes/classy/templates/content/comment.html.twig
@@ -69,7 +69,6 @@
 {%
   set classes = [
-    'comment',
     'js-comment',
     status != 'published' ? status,

no, this is not correct.

Instead, remove the additional of that class from the new preprocess.

It's the job of the template now to add it, so we can't generalize it.

Yeah I didn't think this was the point of the patch... here is the new one with the comment and node class left in place.

There was another error:

+++ b/core/modules/taxonomy/taxonomy.module
@@ -250,11 +248,8 @@ function template_preprocess_taxonomy_term(&$variables) {
+  $variables['attributes']['class'][] = drupal_html_class('vocabulary-' . $term->bundle());

This is already done in classy's taxonomy-term.html.twig, so I've removed it from there.

Did some sanity checking with the patch applied, this is affecting the node pages on all themes. Tested on bartik and stark:

Without the patch:

With the patch applied:

#46:

Instead, remove the additional of that class from the new preprocess.

Addressing this now.

Getting the taxonomy vocabulary css class working again on classy's taxonomy term template.

Hopefully this will help with some failing tests... else I could really use a hand on those.

1. +++ b/core/modules/rdf/rdf.module
   @@ -543,7 +543,8 @@ function rdf_preprocess_taxonomy_term(&$variables) {
      // (e.g., schema:Thing, skos:Concept, and so on).
   -  $term = $variables['term'];
   +  /* @var \Drupal\taxonomy\TermInterface $term */
   +  $term = $variables['taxonomy_term'];
   

   this is an API change that's no longer acceptable. We need to ensure that the old key is set as well.

2. +++ b/core/modules/taxonomy/taxonomy.module
   @@ -239,22 +239,14 @@ function taxonomy_theme_suggestions_taxonomy_term(array $variables) {
    function template_preprocess_taxonomy_term(&$variables) {
   -  $variables['view_mode'] = $variables['elements']['#view_mode'];
   -  $variables['term'] = $variables['elements']['#taxonomy_term'];
      /** @var \Drupal\taxonomy\TermInterface $term */
   -  $term = $variables['term'];
   +  $term = $variables['taxonomy_term'];
   

   By not removing the old line that assigns to 'term'

Thanks @Berdir for the review, posting here our brief conversation on IRC for future reference:

berdir, so we should have both $variables['term'] and $variables['taxonomy_term'] ?
manuee: yes, I think so. then both generic code that requires #$entity_type_id and existing code will continue to work
manuee: for the templates, we should probably document both, deprecate the old key and switch the core templates to use the new key

OK, adding back the term variable for taxonomy term templates.

Not entirely sure about the comments on the deprecated var in the templates, please have a look.

Did a bit of digging, looks like NodeCreationTest fails are due to what you can see on my screenshots above, that the submitted by is being displayed there twice for some reason.

New approach: #2808481: Introduce generic entity template with standard preprocess and theme suggestions. This isn't going to happen I guess.

Closing based on #62