Entity view controller title rendering is expensive

+++ b/.htaccess
@@ -2,6 +2,11 @@
 
+php_value auto_prepend_file "/Users/catch/xhprof/header.php"
+php_value auto_append_file "/Users/catch/xhprof/footer.php"
+
+
+
 # Protect files and directories from prying eyes.
 <FilesMatch "\.(engine|inc|install|make|module|profile|po|sh|.*sql|theme|twig|tpl(\.php)?|xtmpl|yml)(~|\.sw[op]|\.bak|\.orig|\.save)?$|^(\..*|Entries.*|Repository|Root|Tag|Template)$|^#.*#$|\.php(~|\.sw[op]|\.bak|\.orig\.save)$">
   <IfModule mod_authz_core.c>
@@ -39,9 +44,6 @@ AddEncoding gzip svgz

@@ -39,9 +44,6 @@ AddEncoding gzip svgz
   php_value mbstring.http_input             pass
   php_value mbstring.http_output            pass
   php_flag mbstring.encoding_translation    off
-  # PHP 5.6 has deprecated $HTTP_RAW_POST_DATA and produces warnings if this is
-  # not set.
-  php_value always_populate_raw_post_data   -1
 </IfModule>

left-over? no idea why you had to make the php_value change, maybe an old file that you copied in? :)

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
   @@ -57,6 +57,21 @@ public static function create(ContainerInterface $container) {
   +      $page['#title'] = $this->renderer->render($page[$label_field]);
   

   I'm curious, does this works for view displays, which don't have their label field configured as to be shown?

2. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
   @@ -78,22 +93,9 @@ public function view(EntityInterface $_entity, $view_mode = 'full', $langcode =
   -    // If the entity's label is rendered using a field formatter, set the
   -    // rendered title field formatter as the page title instead of the default
   -    // plain text title. This allows attributes set on the field to propagate
   -    // correctly (e.g. RDFa, in-place editing).
   

   Let's ensure we don't loose the documentation here ... I think its not obvious why you can't just use $entity->label() and be done

If it's broke for some reason we could go back to the initial patch on here, but would be nice to keep the cold cache optimisation if we can.

So the node title is configured as be possible. What about checking for isset() and otherwise fallback to $entity->label() directly?

    $fields['title'] = BaseFieldDefinition::create('string')
      ->setLabel(t('Title'))
      ->setRequired(TRUE)
      ->setTranslatable(TRUE)
      ->setRevisionable(TRUE)
      ->setDefaultValue('')
      ->setSetting('max_length', 255)
      ->setDisplayOptions('view', array(
        'label' => 'hidden',
        'type' => 'string',
        'weight' => -5,
      ))

+++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
@@ -57,6 +57,24 @@ public static function create(ContainerInterface $container) {
+    $view_mode = $page['#view_mode'];

$view_mode is not used.

Trying to fix things.

I removed EntityViewControllerTest, because all what its doing is to setup a bunch of mocks, IMHO there is no logic tested in there.

Wow, amazing find!

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
   @@ -57,6 +57,31 @@ public static function create(ContainerInterface $container) {
   +   *   A render array.
   

   A *page* render array.

2. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
   @@ -78,22 +103,9 @@ public function view(EntityInterface $_entity, $view_mode = 'full', $langcode =
   +    $page['#pre_render'][] = array($this, 'buildTitle');
   

   Let's use the [] syntax?

3. +++ b/core/modules/node/src/Controller/NodePreviewController.php
   @@ -55,6 +54,23 @@ public function view(EntityInterface $node_preview, $view_mode_id = 'full', $lan
   +    $build['nodes'] = call_user_func($build['nodes']['#pre_render'][0], $build['nodes']);
   +    $build['nodes'] = call_user_func($build['nodes']['#pre_render'][1], $build['nodes']);
   

   Besides being very weird… (is there really no other way?) this will fail to execute any #pre_render callbacks beyond the first two.

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
   @@ -58,18 +58,25 @@ public static function create(ContainerInterface $container) {
   -      $page['#title'] = $this->renderer->render($page[$label_field]);
   +      if (isset($page[$label_field]) && $title = $this->renderer->render($page[$label_field])) {
   +        $page['#title'] = $title;
   +      }
   

   Won't this prevent us from having an empty title? What if $page[$label_field]['#markup'] was explicitly set to the empty string?

2. +++ b/core/modules/node/src/Controller/NodePreviewController.php
   @@ -55,6 +54,23 @@ public function view(EntityInterface $node_preview, $view_mode_id = 'full', $lan
   +    $build['nodes'] = call_user_func($build['nodes']['#pre_render'][0], $build['nodes']);
   +    $build['nodes'] = call_user_func($build['nodes']['#pre_render'][1], $build['nodes']);
   

   This does not look reliable at first sight, it could at least use an inline comment.

Edit: oops, missed Wim's review :)

Won't this prevent us from having an empty title? What if $page[$label_field]['#markup'] was explicitly set to the empty string?

Mh I thought this fixed something, nevermind let's try it different.

This does not look reliable at first sight, it could at least use an inline comment.

Used foreach and added some comment.

Oh nice fix!

Here is the RTBC

RTBC +1

(lol @ #18 failing badly :D)

Committed 4fb37aa and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation.

+++ /dev/null
@@ -1,75 +0,0 @@
-class EntityViewControllerTest extends UnitTestCase{

I agree this test does not look useful.

Opened a follow-up #2507967: \Drupal\Core\Entity\Controller\EntityViewController::buildTitle() assumes the $entity is in the render array