Implement an entity render controller

Yes please, I would love to get #1018602: Move entity system to a module in first though.

+1

I've already implemented that by providing a generic implementation in the entity API module. For that it provides a generic "entity" template and a preprocessor, which modules can build upon.

Standardizing on "entity" tag, which will be renamed to "Entity system".

I think we mean that view() should be a method on entity controller. Retitling.

I did work get user_view() to drop all its category nonsense. Hopefully not every entity type will have to override this method.

I think we mean that view() should be a method on entity controller. Retitling.

Please, let's don't put everything in a single, fat controller. That wouldn't help us.
Let's split up separate tasks (viewing vs. storage?) into separated controllers, ala #1302378: Use multiple specialized entity controllers.

Re-titling.

Let's get started with this! :-)

I'm working on this, so assigning for now.

FYI: The issue that standardizes taxonomy term viewing is #1067120: Missing hook_taxonomy_term_view() or hook_entity_view() when viewing a term.

First very raw version:

• All 4 controllers have been introduced, along with their abstract parent
• view() and buildContent() have been implemented
• original procedural functions have not yet been removed

Not tested, this is just to show where we're going: view() looks fine, next steps are:

• refactor *_build_content vs *::buildContent() similarly
• replace *_view and *_build_content by the new methods

Patch applies on top of the one in #1067120: Missing hook_taxonomy_term_view() or hook_entity_view() when viewing a term.

Patch has a lot of tabs in it, should be easy to fix that.

You should also add the @file docblocks, you can copy them from elsewhere, they're standardized for PSR-0 files.

Obviously lots of documentation still missing, but not that relevant right now.

Will do a detailed review later...

Started unification of the buildContent() methods.

Note that all these patches *will* be tested once the issue is set to needs review for the first time, so use do-not-test.patch as the suffix if you don't want that.

Rerolled on current HEAD with unification of the buildContent() methods.

Sorry about the tabs. Looks like my IDE reinstall lost some settings.

Next step: actually using these methods to replace the current procedures.

First testable version. Works manually, but how many tests does it break ?

Still rolled on top of #1067120: Missing hook_taxonomy_term_view() or hook_entity_view() when viewing a term.

Hmmm bot can not apply... maybe with the two patches at once ?

Merging the two patches just for testing. Do not use that patch as such, since it includes #1067120: Missing hook_taxonomy_term_view() or hook_entity_view() when viewing a term

Note that entity_view()-ing vocabularies or files will not cause any error and will return a proper render array, but might possibly something we could want to improve:

• the default render array for a vocabulary renders as an empty string
• the default render array for a file renders as a file upload widget

+++ b/core/modules/comment/comment.module
@@ -977,52 +978,7 @@ function comment_prepare_thread(&$comments) {
+  $build = entity_view($comment, $view_mode, array('node' => $node), $langcode);
   return $build;

Minor, but this could be just one line.

+++ b/core/modules/comment/lib/Drupal/comment/CommentViewController.php
@@ -0,0 +1,84 @@
+  /**
+   * $dependencies msust contain a valid Node as the value for the "node" key.
+   */
+  public function buildContent(EntityInterface $entity, $view_mode, $langcode, array $dependencies = array()) {

Ouch. I'd really prefer to don't have that complex concept of dependencies and normalizeArguments(). Can we do away with it somehow? Let's discuss.

+++ b/core/modules/entity/entity.module
@@ -210,6 +210,40 @@ function entity_load_by_uuid($entity_type, $uuid, $reset = FALSE) {
+  $class = isset($info['view controller class'])
+    ? $info['view controller class']
+    : 'Drupal\entity\EntityViewController';

I think usually we only do that in one line. Also, if no controller is specified the function should default to return FALSE and be documented that way. Maybe, we also want to call it entity_render_controller() and call ->view() on it? So rendering could involved just buildContent() or view().

+++ b/core/modules/entity/lib/Drupal/entity/EntityViewController.php
@@ -0,0 +1,130 @@
+   * Override in descendent classes.
+   *
+   * @var string
+   */
+  protected $entityType;

Override?

+++ b/core/modules/entity/lib/Drupal/entity/EntityViewController.php
@@ -0,0 +1,130 @@
+
+  public function buildContent(EntityInterface $entity, $view_mode, $langcode, array $dependencies = array()) {

Should have comments that it implements the interface.

+++ b/core/modules/entity/lib/Drupal/entity/EntityViewController.php
@@ -0,0 +1,130 @@
+   * @return array
+   *   The build array.
+   */
+  protected function prepareBuild(array $build, EntityInterface $entity, $view_mode, $dependencies = array(), $langcode = NULL) {
+    return $build;
+  }

I think building can be simpler. Just have a look at the entity api module implementation. Add an optional $content parameter being passed in and defaulting to an empty array. Then you can easily override and pass in additional stuff to the parent.

+++ b/core/modules/entity/lib/Drupal/entity/EntityViewControllerInterface.php
@@ -0,0 +1,36 @@
+interface EntityViewControllerInterface {
+  /**
+   * Main Entity view method.
+   *
+   * @param EntityInterface $entity
+   * @param string $view_mode
+   * @param array $dependencies
+   * @param string $langcode
+   *
+   * @throws \InvalidArgumentException
+   */
+  public function view(EntityInterface $entity, $view_mode, $dependencies = array(), $langcode = NULL);
+

Viewing should be implemented as multiple operation, as this is more efficient. Best have a look at the view() implement of the d7 entity api module.
However, I think entity_view() should be singular, while entity_view_multiple() is multiple.

Also, The docs are insufficient - we need to properly describe all parameters and when the exception is thrown and what gets returned. Same for buildContent().

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermViewController.php
@@ -0,0 +1,41 @@
+    // Try to add in the core taxonomy pieces like description and nodes

Misses trailing point.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermViewController.php
@@ -0,0 +1,41 @@
+    // TODO: rename "term" to "taxonomy_term" in theme_taxonomy_term() ?

Makes sense.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/HooksTest.php
@@ -35,7 +35,12 @@ class HooksTest extends TaxonomyTestBase {
-   * Test that hooks are run correctly on creating, editing and deleting a term.
+   * Test hooks on CRUD of terms.
+   *
+   * Test that hooks are run correctly on creating, editing, viewing,
+   * and deleting a term.

Viewing is not a CRUD hook. Thus I guess we should do an entity-view test case that also makes sure the hooks are run.

+++ b/core/modules/taxonomy/taxonomy.module
@@ -569,55 +570,120 @@ function taxonomy_term_delete_multiple(array $tids) {
+function taxonomy_term_show(Term $term) {
+  return taxonomy_term_view_multiple(array($term->tid => $term), 'full');

Why is the single one called show() and not just _view()?

+++ b/core/modules/user/lib/Drupal/user/UserViewController.php
@@ -0,0 +1,23 @@
+    // TODO: rename "theme_user_profile" to "theme_user", and 'account' to 'user' ?

Should be @todo and not exceeed 80chars. Also I don't think we need the question mark ;)

The default render widget for a file should display the file, an upload widget would be the edit form :)

I think it's fine to ignore that for now, there's no path to view them, so it can't be invoked outside of code. You might want to talk with @davereid about file stuff, there is an RTBC issue to move the file entity into file.module and the media initiative plans to bring as much of the file_entity contrib module into core as possible, including separate edit and view pages for each file.

Cross-post.

Addressed most of the "simple" issues: remaining ones are:

• view multiple
• simplify building
• behavior of file rendering, possibly of vocabulary rendering

In detail:

• *_view() functions are now one-liners
• removed the dependencies array from all method calls
• controllers are now *RenderController, not *ViewController
• comment on $entityType property fixed
• phpdoc on ::buildContent() saying it implements EntityRenderControllerInterface
• phpdoc on interface ::buildContent() and ::view() for all parameters
• trailing dot added
• taxonomy_term_show() removed
• Comment entity now has a $nid default property

Regarding the CRUD test: there is already a view test, although it would probaby be better simplified, as it currently build the full term page instead of just caring for the entity view, but that is part of the other issue.

I kept the three-line ternary for readability: on one line, the line is too long, and on two lines it is not visually consistent. But of course that can be changed if it matters.

• Fixed failing test
• Simplified building by removing ::normalizeArguments()

To be addressed today: view_multiple.

OK, ::viewMultiple() is now implemented and ::buildContent() is now a "multiple" method. Test pass locally for entity, comment, node, term, and user. Crossing fingers.

Ahaaa... new kinds of errors ! Fixed the image test.

I'm not sure yet about what's happening with multilingual entities: they appear not to be indexed at all, causing them not to be present in the node_search_execute() results, and I have to leave for Paris. Hopefully this version will only fail on those 2 assertions.

Oooppsie, wrong patch. Same remarks.

Needs rerolling now that the final version of #1067120: Missing hook_taxonomy_term_view() or hook_entity_view() when viewing a term got in.

Removing the "sprint" tag since the sprint is over.

I think #1783964: Allow entity types to provide menu items might be an interesting premise for the render controller.

tagging
adding platform initiative because this is needed for #731724: Convert comment settings into a field to make them work with CMI and non-node entities

Here's a first re-roll.

Lot's to do, just want to see where we're at with the tests and I don't have time to continue right now.

Re-roll to fix the php errors.

Working on this

Ok, here we go.

Spent a lot of time cleaning up, documenting and refactoring this issue.

- Re-added entity_view() and entity_view_multiple(), my previous re-rolls lost them and then stuff obviously broke.
- Unified all view and view_multiple functions to use the same arguments and use the mentioned functions above.
- Removed all remaining build_content functions.
- Documented all render classes according to the guidelines.
- Renamed some functions and changed them to make more sense. The naming didn't make much sense previously, I assume due to refactorings. For example, there was a prepareBuild() method which was called after buildContent(). Changed that to afterBuild() and made &$build by reference.
- Also renamed prepareView() to buildFieldContent(), which is what the documentation says, integrated entity_prepare_view() into it and called it from the default buildContent() implementation. Not quite happy yet with the naming there and the logic.

Let's see what the testbot has to say about it.

Re-roll, listing patch conflicted with this.

Cannot do an in-depth review, unfortunately, here's what I found :

I'm not sure I get the need for buildFieldContent() separated from buildContent() - esp. given that in the current patch buildFieldContent() invokes hook_entity_prepare_view().
(FWIIW, I'm not really convinced by the naming and flow around buildContent() either, but I don't have actual proposals right now :-/. I wish we could get rid of that "build" terminilogy, that is a leftover from #658364: Does build/view/formatter terminology make sense? where we settled on 'view' against 'build' see #9 over there)

Also, the original entity_prepare_view() function had :
"By convention, entity_prepare_view() is called after field_attach_prepare_view() to allow entity level hooks to act on content loaded by field API."
The patch changes that order ?

+++ b/core/modules/comment/comment.moduleundefined
@@ -980,104 +981,8 @@ function comment_prepare_thread(&$comments) {
+  $comment->node = $node;

Not necessarily for this patch, but I think we should remove $node from the signature of comment_view(), and have the $node fetched from $comment->nid in CommentRenderController::buildContent() (there's already code for that in there, btw)

+++ b/core/lib/Drupal/Core/Entity/EntityRenderController.phpundefined
@@ -0,0 +1,163 @@
+      // Allow modules to modify the structured comment.

Wrong copy/paste - refers to 'comment' while we're in the base controller.

Thanks for the review!

Agreed, it probably makes sense to merge the build functions together, I'm also happy to rename to whatever is preferred, not exactly sure what yet. Also not sure if we still need to "don't execute this twice" logic anymore in there, because it now always goes through viewMultiple and the function is only called once for all entities. Not sure what should happen if you call entity_view() twice on the same entity, maybe with a different view mode?

I probably messed up the other while merging functions together. Will fix that.

Nice, two fails only and I already fixed these while waiting for the test result :p Tests failed because the old build order didn't match the new one. Not sure if we really want to keep it or fix the tests.

Reverted the order. Sounds like additional test coverage there wouldn't be a bad idea as there weren't

Re-roll, removed $node from the comment_view() parameters and cleaned up comment rendering a bit.

Code looks fine, but I am hoping that each entity does not need to override the default. Would be nice to not duplicate similar render code for each entity.

Hm. I'm not sure if we should try to consolidate a lot more. Could make the patch considerably bigger and harder to re-roll. The form controller for example went in with minimal consolidation too and lots of follow-up issues.

I'm quite sure that most core entities will need their own render controllers for a while. Maybe we can somehow unify/standardize link handling (that makes up quite a big part of the custom rendering), there are issues open for that but the logic for comments is quite complex, with threading and stuff. Also, we can possibly build ->content using properties, once all entities have them. But that will take a while.

+++ b/core/modules/user/lib/Drupal/user/UserRenderController.phpundefined
@@ -0,0 +1,30 @@
+/**
+ * Render controller for users.
+ */
+class UserRenderController extends EntityRenderController {
+
+  /**
+   * Overrides Drupal\Core\Entity\EntityRenderController::getBuildDefaults().
+   */
+  protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langcode) {
+    $return = parent::getBuildDefaults($entity, $view_mode, $langcode);
+
+    // @todo rename "theme_user_profile" to "theme_user", 'account' to 'user'.
+    $return['#theme'] = 'user_profile';
+    $return['#account'] = $return['#user'];
+
+    return $return;

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermRenderController.phpundefined
@@ -0,0 +1,55 @@
+  protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langcode) {
+    $return = parent::getBuildDefaults($entity, $view_mode, $langcode);
+
+    // TODO: rename "term" to "taxonomy_term" in theme_taxonomy_term().
+    $return['#term'] = $return["#{$this->entityType}"];
+    unset($return["#{$this->entityType}"]);
+
+    return $return;

This default build variables should be relatively easy to get rid of, but might mean quite a bit of code changes, to rename all references to templates and #account/#term. I'm not sure if we should do this in this issue.

To be taken care of in the next re-roll:

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermRenderController.phpundefined
@@ -0,0 +1,55 @@
+      $entity_view_mode = $entity->content['#view_mode'];
+      $settings = field_view_mode_settings($this->entityType, $bundle);
+	    $fields = field_extra_fields_get_display($this->entityType, $bundle, $entity_view_mode);
+      if (!empty($entity->description) && isset($fields['description']) && $fields['description']['visible']) {

oh, tab. Where does that come from?!

+++ b/core/modules/comment/lib/Drupal/comment/CommentRenderController.phpundefined
@@ -0,0 +1,99 @@
+
+    // Array is known not be empty, and all comments apply to the same node,
+    // so we can just fetch the node from the first comment.
+    $entity = reset($entities);
+    $node = node_load($entity->nid);
+    if (empty($node)) {
+      throw new \InvalidArgumentException(t('Invalid node for comment.'));

Left over that can be removed.

Setting to needs work wasn't the plan, but it needed a re-roll anyway. Removed some unecessary code while I was it and fixed the tab.

@linclark that could be an interesting idea, but better suited to a followup with its own discussion than to this specific issue, which has already been pending for too long, I think.

Yes, same thought, sounds interesting but follow-up material :)

And as you said in the comment, terminology with controllers is tricky, we need to think about that. Speaking of that, your post also confused me for a moment, because you where talking about Content Types, the HTTP header one, not node types ;) (My initial thought was, "WTF, per-bundle render controllers?" ;)

#53: entity-render-controller-1026616-53.patch queued for re-testing.

OK, all feedback was incorporated and tests pass. lets move this along.

This looks excellent, awesome!

I only have a few nitpicks, which can be happily adjusted post-commit:

+++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php
@@ -121,7 +121,9 @@ function testSearchingMultilingualFieldValues() {
       // See whether we get the same node as a result.
-      $this->assertEqual($search_result[0]['node']->nid, $node->nid, 'The search has resulted the correct node.');
+      $sts = $this->assertTrue(!empty($search_result[0]['node']->nid)
+        && $search_result[0]['node']->nid == $node->nid,
+        'The search has resulted the correct node.');

?

+++ b/core/modules/search/search.api.php
@@ -362,7 +362,7 @@ function hook_update_index() {
     // Render the node.
-    node_build_content($node, 'search_index');
+    $build = node_view($node, 'search_index');
     $node->rendered = drupal_render($node->content);

Unnecessary $build?

+++ b/core/modules/user/lib/Drupal/user/UserRenderController.php
@@ -0,0 +1,30 @@
+    // @todo rename "theme_user_profile" to "theme_user", 'account' to 'user'.
+    $return['#theme'] = 'user_profile';
+    $return['#account'] = $return['#user'];

Let's create a major follow-up task for this?

While this is currently classified as a feature request, and thus subject to thresholds, which we are currently over, I think like #1723892: Support for revisions for entity save and delete operations I consider this more refactoring than a new feature, so I'm re-classifying as a task.

+++ b/core/modules/comment/comment.moduleundefined
@@ -979,105 +978,8 @@ function comment_prepare_thread(&$comments) {
-function comment_build_content(Comment $comment, Node $node, $view_mode = 'full', $langcode = NULL) {
-  if (!isset($langcode)) {
-    $langcode = language(LANGUAGE_TYPE_CONTENT)->langcode;
-  }
-
-  // Remove previously built content, if exists.
-  $comment->content = array();
-
-  // Allow modules to change the view mode.
-  $context = array('langcode' => $langcode);
-  drupal_alter('entity_view_mode', $view_mode, $comment, $context);
-
-  // Build fields content.
-  field_attach_prepare_view('comment', array($comment->cid => $comment), $view_mode, $langcode);
-  entity_prepare_view('comment', array($comment->cid => $comment), $langcode);
-  $comment->content += field_attach_view('comment', $comment, $view_mode, $langcode);
-
-  $comment->content['links'] = array(
-    '#theme' => 'links__comment',
-    '#pre_render' => array('drupal_pre_render_links'),
-    '#attributes' => array('class' => array('links', 'inline')),
-  );
-  if (empty($comment->in_preview)) {
-    $comment->content['links']['comment'] = array(
-      '#theme' => 'links__comment__comment',
-      '#links' => comment_links($comment, $node),
-      '#attributes' => array('class' => array('links', 'inline')),
-    );
-  }
-
-  // Allow modules to make their own additions to the comment.
-  module_invoke_all('comment_view', $comment, $view_mode, $langcode);
-  module_invoke_all('entity_view', $comment, $view_mode, $langcode);
+function comment_view(Comment $comment, $view_mode = 'full', $langcode = NULL) {
+  return entity_view($comment, $view_mode, $langcode);

My goodness, that looks so much nicer.

+++ b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.phpundefined
@@ -121,7 +121,9 @@ function testSearchingMultilingualFieldValues() {
-      $this->assertEqual($search_result[0]['node']->nid, $node->nid, 'The search has resulted the correct node.');
+      $sts = $this->assertTrue(!empty($search_result[0]['node']->nid)
+        && $search_result[0]['node']->nid == $node->nid,
+        'The search has resulted the correct node.');

Like sun, I also have no idea what this is doing.

Unlike sun, I don't think renaming theme functions and arguments deserves a "major" status. This code works just fine without it, and it's not worth blocking other features on this kind of clean-up. But yes, let's create a follow-up for that, as well as the taxonomy_term one.

Committed and pushed to 8.x! This will need a change notice.

FYI, might be of interest to the participants of this thread : #1812720-3: Implement the new panels-ish controller [it's a good purple]

We're once again over the critical task threshold. Please try and find time to write up documentation for your API change ASAP so your fellow contributors' features aren't blocked. Thanks.

Here's a start on the API changes for the change notice:

+++ b/core/includes/entity.api.phpundefined
@@ -27,6 +27,8 @@
+ *   - render controller class: The name of the class that is used to render
+ *     the entities. Deafaults to Drupal\Core\Entity\EntityRenderController.

+++ b/core/includes/entity.incundefined
@@ -49,6 +49,7 @@ function entity_get_info($entity_type = NULL) {
+          'render controller class' => 'Drupal\Core\Entity\EntityRenderController',

Added to hook_entity_info().

+++ b/core/includes/entity.incundefined
@@ -305,47 +306,6 @@ function entity_get_controller($entity_type) {
-function entity_prepare_view($entity_type, $entities) {

+++ b/core/modules/comment/comment.moduleundefined
@@ -979,105 +978,8 @@ function comment_prepare_thread(&$comments) {
-function comment_build_content(Comment $comment, Node $node, $view_mode = 'full', $langcode = NULL) {

+++ b/core/modules/node/node.moduleundefined
@@ -1175,145 +1176,6 @@ function node_revision_delete($revision_id) {
-function node_build_content(Node $node, $view_mode = 'full', $langcode = NULL) {

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -574,138 +581,35 @@ function taxonomy_term_delete_multiple(array $tids) {
-function taxonomy_term_show(Term $term) {
...
-function taxonomy_term_build_content(Term $term, $view_mode = 'full', $langcode = NULL) {

+++ b/core/modules/user/user.moduleundefined
@@ -2062,61 +2063,25 @@ function user_view_page($account) {
-function user_build_content($account, $view_mode = 'full', $langcode = NULL) {

Removed API functions.

+++ b/core/includes/entity.incundefined
@@ -550,3 +510,57 @@ function entity_list_controller($entity_type) {
+function entity_render_controller($entity_type) {
...
+function entity_view(EntityInterface $entity, $view_mode, $langcode = NULL) {
...
+function entity_view_multiple(array $entities, $view_mode, $langcode = NULL) {

+++ b/core/lib/Drupal/Core/Entity/EntityRenderController.phpundefined
@@ -0,0 +1,145 @@
+class EntityRenderController implements EntityRenderControllerInterface {

+++ b/core/lib/Drupal/Core/Entity/EntityRenderControllerInterface.phpundefined
@@ -0,0 +1,74 @@
+interface EntityRenderControllerInterface {

+++ b/core/modules/comment/lib/Drupal/comment/CommentRenderController.phpundefined
@@ -0,0 +1,85 @@
+class CommentRenderController extends EntityRenderController {

+++ b/core/modules/node/lib/Drupal/node/NodeRenderController.phpundefined
@@ -0,0 +1,93 @@
+class NodeRenderController extends EntityRenderController {

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermRenderController.phpundefined
@@ -0,0 +1,54 @@
+class TermRenderController extends EntityRenderController {

+++ b/core/modules/user/lib/Drupal/user/UserRenderController.phpundefined
@@ -0,0 +1,30 @@
+class UserRenderController extends EntityRenderController {

Added functions, classes, and interfaces.

Might as well.

Change notice filed. Leaving open for the followup -- I'd suggest opening a new issue? It's not clear to me from the (lack of) summary or from the recent comments what specifically needs followup, so I'll leave it to someone else to file that issue and then mark this one fixed.

Is this still open because of waiting a follow-up for theme clean-up?

i think that's more about the "render controller per content type" suggested by linclark in http://drupal.org/node/1026616#comment-6567612

Yes, I think it just needs a follow-up for the clean-up and theme variables. The per content type stuff is currently being implemented using Serializers.

I'm not sure about what the cleanup is supposed to do exactly, could you open a follow-up explaining it Berdir?

Another follow-up we should do:
#1067126: Support rendering entities in page mode

Notifying the fine folks in this thread : #1852966: Rework entity display settings around EntityDisplay config entity - thanks to the generic render controller, we can unify how display settings for the various components (field, 'extra field', contrib stuff like field_group) are stored.

Opened #1857324: Use default theme name and entity key in $build for user entity and #1857336: Use entity variable in $build for taxonomy_term entity. marking as fixed.