Optimize user_view('compact') in template_preprocess_node()

Impact will be even greater on long comment threads if site builder enables pictures on comments.

I don't immediately know how to resolve this. We could try to group all the user renders into one user_view_multiple() as yched proposes. One way tyo do that is to introduce a 'View Mode' reference Field which would point to User+Compact. That would get intelligently built during entity_view_multiple($nodes). It would issue a user_view_multiple($accounts) just like yched proposed. This new Field would not appear on entity Forms - it would just be a render thing. Hope this makes sense.

Don't know if we want to wait for that, but if once all entities have been converted to EntityNG, entity_view_multiple() could check for Entity reference items, so doing entity_view_multiple($nodes) would view_multiple() the node authors, the comments, the comment authors, ... auto-magically.
I'm not too adept yet at the TypedData/Field stuff, so that might be totally bogus.

Yeah, #2 might be a way to go though I'm not sure how the compact view mode gets passed down to that last view_multiple($accounts, 'compact').

Speaking of view modes, I'm now questioning my decision to use a 'compact' view mode for user picture on comments and nodes. The problem is that I want to add a tiny user picture to the Account item in the new responsive toolbar. In this case, we definately only want the user picture, and not a name, bio, etc. So perhaps we should add a sitewide variable where admin pics which imagefield represents the user picture. standard.install would set to user_picture. Then rendering a user picture would be n field_view_field('user', $account, 'user_picture', $display) instead of user_view(account, 'compact'). Perhaps thats not enough of an improvement to satisfy this issue. In any case, would folks support that? yched?

I haven't thought about all implications of the following, and it would again be something that would probably take a while to complete, but the above smells to me a bit like an 'image' entity and then an entity reference/image reference on the user. With image styles, images already have something like "view modes", so... Entity reference fields will have to specify the view mode of the referenced entity anyway, and currently for an image field, the image style is selected already.

I like NodeRenderController::buildContent($nodes) takes care of doing user_view_multiple($all_the_authors), and putting the result in each $nodes[$nid]->content['user_picture']. The one thing that holds me back that we need to verify that gravatar can just be a formatter on the user_picture field. If so, we have met our flexibility needs.

Is user_view_multiple($all_the_authors) really that much faster than a loop of individual renders? Is the savings just due to skipping duplicates or is there more? I agree that skipping duplicates is pretty important on its own.

One reason I went with a compact build module for nodes/comments is that folks can show more than just an image. They can cleanly add the author's name and job title and so on. Not sure how important that is.

One reason I went with a compact build module for nodes/comments is that folks can show more than just an image. They can cleanly add the author's name and job title and so on. Not sure how important that is.

That's super important from my perspective. The new 'compact' user view mode is essentially + finally the beginning of http://drupal.org/project/user_display in core.

The essential concept of user_display goes one step further and basically turns the entity type + bundle + view mode of the parent entity (e.g., node, comment) into a unique context identifier (e.g., 'comment.node_article.full') instead of a single 'compact' view mode. That, in turn, allows you to control how the author of a comment appears within that particular context, and likewise, for all other possible contexts (which is not limited to parent-entity contexts; the context could as well be 'block.user.whos-online').

@sun - how would you implement the rendering of a tiny user picture in the new toolbar similar to gmail? we want to add that instead of the generic face that we use today. The options are:

1. Add a new 'tiny picture' view mode. This seems best to me, even though it mostly duplicates the compact view mode. The only difference would be a tiny image style instead of thumbnail.
2. Add a new variable pointing to the user_picture field instance so that the system knows that it represents the user picture. The rendering would then be a custom field rendering that passes in a tiny image style.

I think the approach of user_display is a new variant

3) Allow modules to specify an infinite amount of unique context identifiers. Each context identifier may either be used directly, or could also be registered somewhere. Not every context has to be configured, there's always a fallback to a default view mode. The important aspect is that there are unlimited contexts, and site builders want to configure defaults (e.g., 'compact'), but also override the view for particular contexts.

For the concrete use-case of a tiny picture in the toolbar, that would sorta translate into a (hard-coded) toolbar.user context, which is called via entity_view('user', $account, 'compact:toolbar.user') (or similar), and if there's no custom configuration for that "view mode", then we'd fallback to theme_username().

The same would apply to comments; i.e., entity_view('user', $account, 'compact:comment.node_article.full')

All of that being said, this doesn't really sound relevant for this issue — sorry. :-/

OK, here is the approach that yched suggested. It contains a silly bug fix for user_view_multiple().

Hm. The changes make sense, but the user-specific method really does not belong into the generic EntityRenderController.

Looks like a clear horizontal extensibility problem to me, and I'm not sure how we can resolve it properly. For now, I guess the most sensible thing would be to move the method into NodeRenderController, and require all other render controllers that share the same need to re-implement it on their own.

Technically, I guess it would make most sense if the author/user was declared as an extra field (reference), and if its view mode was defined as a formatter setting.

In turn, rendering a node/comment would also render all referenced entities with the specified formatter settings (view mode), which would be a generic behavior that applies to all entities, inherently making it part of the generic entity render controller again.

In turn, the template variable would turn into just:

  {{ author }}

I don't have a problem with BuildUserPictures in the entityRenderController. Entities already have a uid property, and that often gets rendered as a user picture. It all hangs together for me.

Even the entity reference patch does not turn entity->uid into a reference Field. I don't think we can expect this patch to do that.

I'm not sure why we are getting test fails. The pictures are showing for me outside of testing system.

Hopefully licked the test fails

Not all entities have a uid. This should live in the Node and Comment controllers separately, although perhaps we could add an AccountAware controller they both inherit from?

Also I left a comment on http://drupal.org/node/1859352#comment-6814706 - I think we can simplify a lot of the dependency issues and conceptual weirdness by only relying on an account and removing explicit references to user pictures - the variable that's produced is a rendered account rather than just a picture already.

@catch - thats more or less what I do in this patch. The node and comment controllers are the ones who kick off the building of picture render array. Those controllers call into a helper method (buildUserPictures) in the EntityRenderController. You prefer that helper method to live in a new class? Am open to other approaches.

New patch inspired by IRC chat with catch. This one populates $entity->content['account'] instead of 'user_picture'. A profile2 or other module could choose to populate that if it wants to. The patch also gets rid of the toggle_ theme variables related to user picture. Disabling a user picture can be removing the Field instance or removing the picture Field using Manage Display. This simplifies the rendering of author in nodes and comments. In a followup, we could consider moving the 'name' template variable into the compact Display.

+++ b/core/lib/Drupal/Core/Entity/EntityRenderController.phpundefined
@@ -66,6 +66,23 @@ public function buildContent(array $entities = array(), $view_mode = 'full', $la
+   * Build the 'compact' rendering for each attached user account. Used in core
+   * Drupal by node and comment entity types.

Should start with "Builds", and the second part should be in a separate paragraph

+++ b/core/lib/Drupal/Core/Entity/EntityRenderController.phpundefined
@@ -66,6 +66,23 @@ public function buildContent(array $entities = array(), $view_mode = 'full', $la
+  function buildAccount(array $entities = array(), $view_mode = 'compact', $langcode = NULL) {

Missing public

+++ b/core/lib/Drupal/Core/Entity/EntityRenderController.phpundefined
@@ -66,6 +66,23 @@ public function buildContent(array $entities = array(), $view_mode = 'full', $la
+      $accounts[$entity->account->id()] = $entity->account;

This is in EntityRenderController, which is the default for all entities. Including Views :) We don't have an account property

Fixed first two points.

The last point is not a problem. buildAccount() method is only called by the node and comment render controllers. They do so because they want to render account info. views would not call it. buildAccount() method lives in entityRender class just because it is shared code. I could put it somewhere else if folks can think of a more appropriate place.

WTF - attaching again

OK, buildAccount now moved to both node and comment render classes.

1. Catch and i discussed this and we thought that getting rid of these toggles acknowledging the ramifications that you describe is the best of the available options. If an empty view mode takes a non-trivial amount of time to render, lets fix that.
2. Fixed.

We could keep some kind of configuration but make it on the entire account?

I was thinking we could potentially move the username rendering to this as opposed to it being its own variable, but that'd need to be a separate patch and it's still valid to not render the username on nodes.

About the performance part, instead of having custom toggle variables, can we expose this as a field on nodes/comments? The we can control on the display fields screen if we want to show it.

If I understood @yched's work on the entity display issue correctly (Unfortunately haven't reviewed it really yet), then it would mean that if the field is not displayed, then we don't have to build and render it at all, right? And if not that, then maybe Entity Field API could do this?

Fields that are 'hidden' don't get built at all on render, so if it's a field that'd work. Would EntityNG allow us to add formatter settings for this without making it a field as such?

#27: user_view_multiple.patch queued for re-testing.

Anyone available to refresh this and swtich from theme toggle variables to an Extra Field as per the end of #32?

Fix status

Just came upon this out of #1930934: Set realistic image upload limits for user picture field, where I realized that calling this 'user_picture' in a template doesn't make sense anymore.

I'll take on a refresh of #27 and the extra field piece, but won't making this an extra field render it below the author / date information? Is that what we want? Shouldn't the author / date info also be an extra field? That'd be my preference, but may be outside of this patch's scope.

Here's a re-roll of #27 with a couple of tiny fixes that should apply cleanly. Will add extra fields after further discussion RE: the author / date info.

Marking as needs review to trigger the tests.

I'd be OK with author/date being an extra field. Mostly I want this in for performance reasons.

This ought to fix all that.

#41: drupal_user_compact_1853378_41.patch queued for re-testing.

Thanks for getting to green. Looking forward to making this an Extra Field

Thinking about this a bit more, discussing it with moshe in IRC, and considering what yched says in #32, I don't think the extra field approach makes sense.

In Bartik's comments, for example, the compact user display is shown off to the side of the comment. We couldn't keep that look and have the user display be an extra field with drag-n-drop weight. I definitely don't think it makes sense for a user to be able to change the weight of the extra field and not have it move. Just about every theme I've ever done has created a disconnect between settings like that and what actually happens, but I don't think that's appropriate for core.

So what's the best solution? Should we put the theme setting toggles back in? How about a per-content-type checkbox a la 'display author and date information'? Or do we not care about the performance hit of rendering the empty display?

IMO, lets put the theme toggles back in and commit this thing.

This has the theme_toggles back in.

+++ b/core/modules/node/lib/Drupal/node/NodeRenderController.phpundefined
@@ -30,6 +30,12 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
+    // Populate $entity->content['account'], usually with a user picture if the
      // theme setting is enabled.

WIll we get a notice during rendering if content['account'] is not set?

+++ b/core/modules/system/system.admin.incundefined
@@ -424,7 +424,7 @@ function system_theme_settings($form, &$form_state, $key = '') {
-    $disabled['toggle_comment_user_verification'] = TRUE;
+    $disabled['toggle_comment_user_vertification'] = TRUE;

typo

Typo fixed...I'm not getting any warnings...twig magic?

OK, ready to fly.

I think this is a step forward. However, I also think:

1) It really feels icky that, even after introducing tons of entity/field/property abstraction, we still need to manually iterate over these properties one by one in order to load, process, and render referenced objects. I thought this was exactly what the new entity/field API is supposed to deliver... And if it's not capable of doing this in a performant way, then I seriously don't understand why we've introduced the massive abstraction of EntityNG?

2) It would be great to create a separate issue for the discussion around #9; i.e., "Configurable context to view mode mappings", so as to allow all modules to specify in which contexts they are rendering entities, and to allow the site builder to assign these contexts to view modes.

#49: drupal_user_compact_49.patch queued for re-testing.

Assigning to catch as he's been involved with this issue

I'm not convinced by adding back the theme toggles but I expect this is going to need more followups anyway (i.e. the username should probably move to this instead of a separate variable). If we're going to that though, can we at least rename them to something sane - it could be toggle_comment_compact_account or something.

+++ b/core/modules/comment/lib/Drupal/comment/CommentRenderController.phpundefined
@@ -33,10 +33,19 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
+    if (theme_get_setting('toggle_comment_user_picture')) {
+      $this->buildAccount($entities, 'compact', $langcode);

fix formatting

This could be done partially in 8.x but require BC somehow

+++ b/core/modules/comment/templates/comment.tpl.php
@@ -20,8 +20,6 @@
- * - $user_picture: The comment author's picture. Use render($user_picture) to
- *   print it.

@@ -80,7 +78,7 @@
-    <?php print render($user_picture); ?>
+    <?php print render($content['account']); ?>

+++ b/core/modules/node/templates/node.html.twig
@@ -9,7 +9,6 @@
- * - user_picture: The node author's picture from user-picture.html.twig.

@@ -88,7 +87,7 @@
-      {{ user_picture }}
+      {{ content.account }}

not sure it's possible to remove this from template cos templates are freezed til 9.x

Also let's avoid setting $entity->account. We have moved away from the pattern of setting dynamic variables (mostly, at least). Let's just pass $accounts as a separate variable. Also I see no reason for buildAccount() to be public.

attempt reroll

Rerolled against 8.3.x branch.

Thanks, but the reroll of the previous patch was already incorrect, this is 10x as big as it should be now. We need to start of with the patch in #49 again.

That said, I don't think this is still possible for 8.x, only if we keep backwards compatibility in the template/theming layer.

Added Re-roll, changed status to Needs review

@tameeshbd there is no point in re-rolling this, did you look through this patch? Please see Berdirs comment in #65, what he means is we need to go back to the ideas encompassed in #49 but re-write it to bring it up to day with todays D8.