#1852966: Rework entity display settings around EntityDisplay config entity introduced the EntityDisplay objects,
It also modified the field_attach_[prepare]_view() functions so that they receive EntityDisplay objects as injected parameters (rather than internally fetch the display options to use for the view mode).

The same logic should be applied to all methods and hooks within the entity_view() callstack : get EntityDisplay objects as injected parameters, rather than having to fetch the display options from the outside.

What the patch does :

- Collecting of the EntityDisplay objects involved in viewing entities is moved higher up, from EntityRenderController::buildContent() to EntityRenderController::viewMultiple(). so that all methods and hooks involved from there can receive the $display objects injected instead of having to fetch them.

- new $displays / $display parameter in the following methods and hooks:
EntityRenderControllerInterface::buildContent()
EntityRenderControllerInterface::alterBuild()
hook_entity_prepare_view()
hook_entity_view() / hook_entity_view_alter()
All the hook_ENTITY_TYPE_view() / hook_ENTITY_TYPE_view_alter() for core entities.
hook_view() (for nodes)

- The code that dealt with "extra fields" in those various methods and hooks were doing a mix of
"always add the render element at an arbitrary weight and let the _entity_view_pre_render() callback re-weight and hide if needed"
and
"call field_extra_fields_get_display() to get the configured display options, and if visible, add the component with the right weight".
This is now unified to:
"check visibility with if ($display->getComponent()), don't bother with weight, it will be assigned later on".

- field_extra_fields_get_display() is gone, display options are injected where they need to be accessed.

- ERC::viewMultiple() takes care of assigning weights for all the visible components in the EntityDisplay, after all the methods and hooks above have run.
The _entity_view_pre_render() #pre_render callback is gone (it might be reintroduced of we go for layouts, to assign components to regions sub arrays, but we'll see when we get there).
This also means that field formatters don't need bother with weights either, and thus don't need to receive it anymore. Having the formatters receive the weight of the element within the surrounding elements always felt a bit off.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Status: Active » Needs review

Posting the patch for now, I'll post a summary tomorrow.

yched’s picture

Right, the patch...

dawehner’s picture

Great cleanup! Here are some small points.

+++ b/core/includes/entity.api.phpundefined
@@ -196,12 +199,15 @@ function hook_entity_query_alter(\Drupal\Core\Entity\Query\QueryInterface $query
+      '#markup' => theme('mymodule_addition', $entity->myfield),

This seems to be invalid, as theme() always expects a $var. What about using '#theme' instead and '#field' as example?

+++ b/core/includes/entity.api.phpundefined
@@ -245,17 +254,33 @@ function hook_entity_view_alter(&$build, Drupal\Core\Entity\EntityInterface $ent
+      if ($displays[$entity->bundle()]->getComponent('mymodule_addition')) {

In general, do we know whether the key will be always the bundle, see the views issue as other example.

+++ b/core/lib/Drupal/Core/Entity/EntityRenderController.phpundefined
@@ -26,69 +27,30 @@ public function __construct($entity_type) {
+  public function buildContent(array $entities = array(), array $displays, $view_mode, $langcode = NULL) {

It's odd to have optional parameters first and then requirement parameters. What about dropping the optional part at the beginning?

+++ b/core/lib/Drupal/Core/Entity/EntityRenderController.phpundefined
@@ -26,69 +27,30 @@ public function __construct($entity_type) {
+      if (empty($entity->entity_view_prepared) || $entity->entity_view_prepared != $view_mode) {

Yeah this is an old property but couldn't we camelCase it now, as this code probably is the only part which uses it?

+++ b/core/lib/Drupal/Core/Entity/EntityRenderController.phpundefined
@@ -145,26 +110,68 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
+        // Get the display object for this entity and view mode.

It seems to be more the display object for this bundle.

+++ b/core/modules/comment/comment.api.phpundefined
@@ -73,9 +76,15 @@ function hook_comment_load(Drupal\comment\Comment $comments) {
+      '#markup' => theme('mymodule_addition', $comment->myfield),

See previous comment.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeBuildContentTest.phpundefined
@@ -30,8 +30,7 @@ function testNodeRebuildContent() {
-    $nodes = array($node);
-    $content = entity_render_controller('node')->buildContent($nodes);
+    $content = node_view($node);

It seems to be that this is a bit out of scope, as using the render controller directly seems to be more the D8 way.

+++ b/core/modules/node/node.api.phpundefined
@@ -844,12 +847,15 @@ function hook_node_submit(Drupal\node\Node $node, $form, &$form_state) {
+      '#markup' => theme('mymodule_addition', $node->myfield),

See above.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermRenderController.phpundefined
@@ -18,17 +19,15 @@ class TermRenderController extends EntityRenderController {
+      if (!empty($entity->description) && $display_options = $display->getComponent('description')) {

$display_options Any reason to store the variable if we don't use it later?

yched’s picture

Thanks for the detailed review !

1) True. That code was copied from the current hook_view() sample code, but is incorrect.
Replaced it with something based on the current hook_entity_view() sample code.

2) Yes, the array of displays is going to be keyed by bundle by contract. However I'm pondering having an array keyed by entity id, to make it easier for contrib to provide displays per entity (i.e "hide field_foo in teaser just for node 42").
I'm still mulling over that a bit, not sure whether I'll go there in this issue.

3) True. In current buildContent() all params are optional, but keeping the 1st one optional and not the next ones makes no sense, I forgot to remove the default param value. The method is always called with an array anyway.

4) $entity->entity_view_prepared / camelCase : yeah, dunno. The patch just moves that code around. I think @fago intends to remove the property anyway...

5) True. Kind of connected to 2) though - by bundle or by entity. Currently it's by bundle, so, fixed.

7) Not really out of scope IMO. The patch adds more params for buildContent(), which makes it a bit more convoluted to call directly - which is not a problem, the method only contains one part of the rendering process and leaves out many hooks, so it's not really intended to be called from the outside.
What the testNodeRebuildContent() test is about is checking that a node_view() isn't polluted by whatever might already be there in $node->content. It can test this by calling node_view() rather than NodeRenderController::buildContent(), which only happens to be the current place where $node->content gets reset. It's valid too (if not more), and keeps the test code simpler.
No biggie IMO.

9) Right, we don't need the variable. Patch just does if ($display->getComponent('foobar')) { in the other similar places, I just forgot to remove it in this specific place.

Status: Needs review » Needs work

The last submitted patch, display-hooks-1875970-4.patch, failed testing.

yched’s picture

Issue summary: View changes

Patch summary

yched’s picture

Status: Needs work » Needs review

#4: display-hooks-1875970-4.patch queued for re-testing.

yched’s picture

Posted a patch summary in the OP.

sun’s picture

@@ -18,17 +19,15 @@ class TermRenderController extends EntityRenderController {
-  public function buildContent(array $entities = array(), $view_mode = 'full', $langcode = NULL) {
+  public function buildContent(array $entities, array $displays, $view_mode, $langcode = NULL) {
...
+      $display = $displays[$entity->bundle()];
+      if (!empty($entity->description) && $display->getComponent('description')) {

@@ -583,13 +587,14 @@
-function user_user_view($account) {
+function user_user_view(User $account, EntityDisplay $display) {

Hm. The way the $display is retrieved for a particular entity subtype/bundle in buildContent() looks and feels a bit arbitrary and cumbersome. (I also don't quite understand why we still have a $view_mode in addition to $display, but that's probably off-topic for this issue.)

Did we consider to "attach" the entity display onto the entity object(s) itself?

I.e., something along the lines of: (pseudo-code)

foreach ($entities as $entity) {
  // Retrieve the display for this entity type/subtype + view mode from the factory.
  // ($display is shared)
  $display = entity_get_display($entity_type, $view_mode, ...);

  // Set/attach the (shared) display object on each entity object.
  // (Changes to the $display affect all entities that have it attached.)
  $entity->setDisplay($display);
}
...
$entity->getDisplay();
yched’s picture

Status: Needs work » Needs review

The way the $display is retrieved for a particular entity subtype/bundle in buildContent() looks and feels a bit arbitrary and cumbersome

It's not arbitrary, displays are per bundle. However, it seems to bother people, and as explained in #4, I'm considering passing $displays keyed by entity id. Needs some thought though.

I also don't quite understand why we still have a $view_mode in addition to $display, but that's probably off-topic for this issue.

Because if you look at current implementations of the affected hooks, a lot of code still hardcodes behavior on $view_mode == 'teaser' or 'rss' or whatnot. Not everything is controlled by fields or extra-fields - and not everything can, either.

Did we consider to "attach" the entity display onto the entity object(s) itself? $entity->setDisplay($display); / $entity->getDisplay();

Funny, that was @Stalski & @zuuperman's first take on this in the initial iterations of #1852966: Rework entity display settings around EntityDisplay config entity.
However, big -1 from me, because :
- it introduces more "state" in $entity. $entity->getDisplay() only works when the $entity "is within or has been through an entity_view()". Bleh.
- Those getDisplay() / setDisplay() methods have to live somewhere. We would need to declare a RenderableEntityInterface, and a RenderableEntityBase class for the implementations, and make Node|Term|User... extend RenderableEntityBase... For what other behavior would we then need to stack similar interfaces and base classes, and end up with Node extends FooBase extends BarBase extends... That pattern doesn't scale, and is only doable because we're core and not contrib.
- All in all, it's really a hack to artificially reduce the number of parameters of those methods and hooks: put parameter B inside parameter A and just pass parameter A. But conceptually it makes no sense, the displays are not properties of the entities, they are params for the 'view' operations.

yched’s picture

Issue summary: View changes

typo

yched’s picture

Status: Needs review » Needs work

Setting to NW for "pass $displays keyed by entity id".

yched’s picture

FileSize
748 bytes
54.51 KB

Ok, gave this a try, and in fact passing $displays by entity id defeats the whole "multiple entities" aspect of ERC::buildContent() and field_attach_prepare_view() / hook_entity_prepare_view() that are called in this method.
If any individual entity can use its own $display object, then you can do nothing on $entities as a whole and need to treat them individually. That would be a big performance hit to support what are just edge cases.

So, while I think it would be nice to have a way for some individual entities to opt out and use their own specific displays, this would still have to result in ERC::buildContent() being called successively on subgroups of entities that use the same set of $displays, keyed by bundle.
Thus, the signature and behavior of buildContent() / field_attach_prepare_view() / hook_entity_prepare_view() would be unchanged ($displays keyed by bundle), it's just the way of calling buildContent() within viemMultiple() that would be different.
And thus, I think that's a separate issue and would rather investigate that in a followup.

Updated patch just fixes the phpdoc for ERC::buildContent()'s $view_mode param (not optional any more).

yched’s picture

Rerolled after #1869250: Various EntityNG and TypedData API improvements, that got rid of entity_view_prepared completely...

If green, this should be good to go IMO...

swentel’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/includes/entity.api.phpundefined
@@ -196,12 +199,16 @@ function hook_entity_query_alter(\Drupal\Core\Entity\Query\QueryInterface $query
+  // Only do the extra work if the component is configured to be displayed.
+  // This assumes a 'mymodule_addition' extra field has been defined for the
+  // entity bundle in hook_field_extra_fields().
+  if ($display->getComponent('mymodule_addition')) {

I freaking love this. I always cry when I see people doing this in D7 without even checking whether it's relevant or not.

Let's get this in!

yched’s picture

;-)

Just to be clear for others: that's not a hunk from hook_entity_query_alter(), of course, but from hook_entity_view_alter() - dreditor hiccup :-p

yched’s picture

#12: display-hooks-1875970-12.patch queued for re-testing.

Just checking it still applies, though.

Stalski’s picture

This is a very nice and straight forward cleanup. And as swentel says: lots and lots nicer to work with.

About the entity having a state depending composition of the display, I follow yched since the explanation.

Really great stuff. Good to go for me too

yched’s picture

#12: display-hooks-1875970-12.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, display-hooks-1875970-12.patch, failed testing.

swentel’s picture

Assigned: yched » swentel

#1874300: Remove $entity_type argument from field.module functions that receive a single $entity which probably made this one not apply anymore, I'll do the reroll

swentel’s picture

Assigned: swentel » yched
Status: Needs work » Needs review
FileSize
53.54 KB

Status: Needs review » Needs work

The last submitted patch, display-hooks-1875970-20.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review

#20: display-hooks-1875970-20.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, display-hooks-1875970-20.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
53.55 KB

Urg, note to self: next time, actually test on an install.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Thanks ! Back to RTBC

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

yched’s picture

Slowly getting back to Core Queue land.

This needed a change notification (but was not added to the critical queue, yay...), added one : http://drupal.org/node/1907792

[edit: the patch also modified some methods signatures in EntityRenderControllerInterface, but those are new from D8; the associated change notification in http://drupal.org/node/1907792 does not go into the details of the methods, and thus stays valid and unaffected]

yched’s picture

Issue summary: View changes

hook_entity_prepare_view() was missing in the summary

effulgentsia’s picture

Great work here, but you missed getBuildDefaults(): #2218157: Add $display parameter to EntityViewBuilder::getBuildDefaults()