Right now, with have field_attach_preprocess() which needs to be moved to the entity system. Also, I guess it makes sense to have preprocessing logic that is tight the buildContent() of the render controller also in there, i.e. it should be moved from e.g. template_preprocess_node() into EntityViewBuilder::preprocessTemplate(). Then, we could just invoke that from there and deal with fields there as well.

Question is what should remain in e.g. template_preprocess_node() or not?
I'd say everything that relates to a special template should stay in template_preprocess_node(), everything that relates to something provided by the render controller, i.e. in buildContent should be moved there.

Files: 

Comments

fago’s picture

Issue tags: +Entity Field API

tagging

Berdir’s picture

Issue tags: +Novice

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

joelpittet’s picture

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.

yched’s picture

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.

Berdir’s picture

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

yched’s picture

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

Berdir’s picture

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

joelpittet’s picture

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

Berdir’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
11.5 KB
FAILED: [[SimpleTest]]: [MySQL] 59,426 pass(es), 40 fail(s), and 46 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 9: preprocess-2023571-1.patch, failed testing.

yched’s picture

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 ?

Berdir’s picture

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

Berdir’s picture

fago’s picture

+++ b/core/includes/theme.inc
@@ -2013,6 +2013,12 @@ function template_preprocess(&$variables, $hook, $info) {
+  }
...
+  if (isset($variables['elements']['#entity_type'])) {
+    \Drupal::entityManager()->getViewBuilder($variables['elements']['#entity_type'])->preprocess($variables);

I could see this check leading to lots of false positives, so we should make it more tight by verifying the view_mode is there as well.

daffie’s picture

Title: Support preprocessing in EntityRenderController » Support preprocessing in EntityViewBuilder
Issue summary: View changes
Berdir’s picture

Status: Needs work » Needs review
FileSize
3.14 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch variable-overrides-2161803-3_1.patch. Unable to apply patch. See the log in the details link for more information. View

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

Berdir’s picture

FileSize
7.42 KB
FAILED: [[SimpleTest]]: [MySQL] 59,242 pass(es), 56 fail(s), and 77 exception(s). View

Completely wrong patch..

The last submitted patch, 16: variable-overrides-2161803-3.patch, failed testing.

yched’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 17: preprocess-2023571-16.patch, failed testing.

aspilicious’s picture

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.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Novice +DX (Developer Experience)
FileSize
7.79 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View
7.79 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View

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

The last submitted patch, 22: preprocess-2023571-22.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 22: preprocess-2023571-22.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
7.79 KB
FAILED: [[SimpleTest]]: [MySQL] 64,158 pass(es), 57 fail(s), and 54 exception(s). View
652 bytes

Ups.

Status: Needs review » Needs work

The last submitted patch, 25: preprocess-2023571-24.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

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?

Berdir’s picture

FileSize
9.88 KB
FAILED: [[SimpleTest]]: [MySQL] 64,325 pass(es), 53 fail(s), and 40 exception(s). View
4.42 KB

Ah, patch would help.

Status: Needs review » Needs work

The last submitted patch, 28: preprocess-2023571-27.patch, failed testing.

jibran’s picture

Issue tags: +Twig

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

joelpittet’s picture

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
10.51 KB
PASSED: [[SimpleTest]]: [MySQL] 64,763 pass(es). View
1.88 KB

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.

joelpittet’s picture

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

Wim Leers’s picture

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

andypost’s picture

Berdir’s picture

FileSize
10.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Entity/EntityViewBuilder.php. View

Just a re-roll for now.

Status: Needs review » Needs work

The last submitted patch, 37: preprocess-2023571-37.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
Related issues: +#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)
FileSize
1.44 KB
10.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch preprocess-2023571-39.patch. Unable to apply patch. See the log in the details link for more information. View

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

Status: Needs review » Needs work

The last submitted patch, 39: preprocess-2023571-39.patch, failed testing.

andypost’s picture

Version: 8.0.x-dev » 8.2.x-dev
Issue tags: +Needs reroll
Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia

Rerolling...

Manuel Garcia’s picture

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.

Manuel Garcia’s picture

Sorry double upload...

Berdir’s picture

Status: Needs review » Needs work
+++ 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.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
10.69 KB
1.48 KB

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.

The last submitted patch, 44: support_preprocessing-2023571-44.patch, failed testing.

The last submitted patch, 44: support_preprocessing-2023571-44.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 47: support_preprocessing-2023571-47.patch, failed testing.

Manuel Garcia’s picture

Issue summary: View changes
Issue tags: -Needs reroll +DrupalCampES
FileSize
17.5 KB
20.17 KB

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:
before

With the patch applied:
after

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
10.62 KB
546 bytes

#46:

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

Addressing this now.

Status: Needs review » Needs work

The last submitted patch, 52: support_preprocessing-2023571-52.patch, failed testing.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
571 bytes
10.72 KB

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.

Status: Needs review » Needs work

The last submitted patch, 54: support_preprocessing-2023571-54.patch, failed testing.

Berdir’s picture

  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'

Manuel Garcia’s picture

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

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
11.38 KB
3.26 KB

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.

The last submitted patch, 58: support_preprocessing-2023571-58.patch, failed testing.

Manuel Garcia’s picture

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.

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

tstoeckler’s picture

Issue tags: +Dublin2016

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.