Provide hasChildren() to replace isEmptyContent()

Summary:
Fixes T3698. Sometimes views need to render differently depending on whether they contain content or not. The existing approach for this is `isEmptyContent()`, which doesn't work well and is sort of hacky (it implies double-rendering content, which is not always free or side-effect free).

Instead, provide a test for an element without children. This test is powerful enough to catch the easy cases of `null`, etc., and just do the expected thing, but will not catch a View which is reduced upon rendering. Since this is rare and we have no actual need for it today, just accept that as a limitation.

Test Plan:
Viewed Timeline and Feed UI examples. Viewed Feed (feed), Pholio (timelineview), and Differential (old transactionview).

{F53915}

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T3698

Differential Revision: https://secure.phabricator.com/D6718

Author: epriestley

src/__phutil_library_map__.php

@@ -775,6 +775,7 @@
     'PhabricatorAllCapsTranslation' => 'infrastructure/internationalization/PhabricatorAllCapsTranslation.php',
     'PhabricatorAnchorView' => 'view/layout/PhabricatorAnchorView.php',
     'PhabricatorAphrontBarExample' => 'applications/uiexample/examples/PhabricatorAphrontBarExample.php',
+    'PhabricatorAphrontViewTestCase' => 'view/__tests__/PhabricatorAphrontViewTestCase.php',
     'PhabricatorApplication' => 'applications/base/PhabricatorApplication.php',
     'PhabricatorApplicationApplications' => 'applications/meta/application/PhabricatorApplicationApplications.php',
     'PhabricatorApplicationAudit' => 'applications/audit/application/PhabricatorApplicationAudit.php',
@@ -2804,6 +2805,7 @@
     'PhabricatorAllCapsTranslation' => 'PhabricatorTranslation',
     'PhabricatorAnchorView' => 'AphrontView',
     'PhabricatorAphrontBarExample' => 'PhabricatorUIExample',
+    'PhabricatorAphrontViewTestCase' => 'PhabricatorTestCase',
     'PhabricatorApplicationApplications' => 'PhabricatorApplication',
     'PhabricatorApplicationAudit' => 'PhabricatorApplication',
     'PhabricatorApplicationAuth' => 'PhabricatorApplication',

src/view/AphrontNullView.php

@@ -3,7 +3,7 @@
 final class AphrontNullView extends AphrontView {
 
   public function render() {
-    return phutil_implode_html('', $this->renderChildren());
+    return $this->renderChildren();
   }
 
 }

src/view/AphrontView.php

@@ -1,64 +1,143 @@
 <?php
 
+/**
+ * @task children   Managing Children
+ */
 abstract class AphrontView extends Phobject
   implements PhutilSafeHTMLProducerInterface {
 
   protected $user;
   protected $children = array();
 
+
+/* -(  Configuration  )------------------------------------------------------ */
+
+
+  /**
+   * @task config
+   */
   public function setUser(PhabricatorUser $user) {
     $this->user = $user;
     return $this;
   }
 
+
+  /**
+   * @task config
+   */
   protected function getUser() {
     return $this->user;
   }
 
+
+/* -(  Managing Children  )-------------------------------------------------- */
+
+
+  /**
+   * Test if this View accepts children.
+   *
+   * By default, views accept children, but subclases may override this method
+   * to prevent children from being appended. Doing so will cause
+   * @{method:appendChild} to throw exceptions instead of appending children.
+   *
+   * @return bool   True if the View should accept children.
+   * @task children
+   */
   protected function canAppendChild() {
     return true;
   }
 
+
+  /**
+   * Append a child to the list of children.
+   *
+   * This method will only work if the view supports children, which is
+   * determined by @{method:canAppendChild}.
+   *
+   * @param  wild   Something renderable.
+   * @return this
+   */
   final public function appendChild($child) {
     if (!$this->canAppendChild()) {
       $class = get_class($this);
       throw new Exception(
         pht("View '%s' does not support children.", $class));
     }
+
     $this->children[] = $child;
+
     return $this;
   }
 
+
+  /**
+   * Produce children for rendering.
+   *
+   * Historically, this method reduced children to a string representation,
+   * but it no longer does.
+   *
+   * @return wild Renderable children.
+   * @task
+   */
   final protected function renderChildren() {
     return $this->children;
   }
 
+
   /**
-   * @deprecated
+   * Test if an element has no children.
+   *
+   * @return bool True if this element has children.
+   * @task children
    */
-  final protected function renderSingleView($child) {
-    phutil_deprecated(
-      'AphrontView->renderSingleView()',
-      "This method no longer does anything; it can be removed and replaced ".
-      "with its arguments.");
-    return $child;
+  final public function hasChildren() {
+    if ($this->children) {
+      $this->children = $this->reduceChildren($this->children);
+    }
+    return (bool)$this->children;
   }
 
-  final protected function isEmptyContent($content) {
-    if (is_array($content)) {
-      foreach ($content as $element) {
-        if (!$this->isEmptyContent($element)) {
-          return false;
+
+  /**
+   * Reduce effectively-empty lists of children to be actually empty. This
+   * recursively removes `null`, `''`, and `array()` from the list of children
+   * so that @{method:hasChildren} can more effectively align with expectations.
+   *
+   * NOTE: Because View children are not rendered, a View which renders down
+   * to nothing will not be reduced by this method.
+   *
+   * @param   list<wild>  Renderable children.
+   * @return  list<wild>  Reduced list of children.
+   * @task children
+   */
+  private function reduceChildren(array $children) {
+    foreach ($children as $key => $child) {
+      if ($child === null) {
+        unset($children[$key]);
+      } else if ($child === '') {
+        unset($children[$key]);
+      } else if (is_array($child)) {
+        $child = $this->reduceChildren($child);
+        if ($child) {
+          $children[$key] = $child;
+        } else {
+          unset($children[$key]);
         }
       }
-      return true;
-    } else {
-      return !strlen((string)$content);
     }
+    return $children;
   }
 
+
+/* -(  Rendering  )---------------------------------------------------------- */
+
+
   abstract public function render();
 
+
+/* -(  PhutilSafeHTMLProducerInterface  )------------------------------------ */
+
+
   public function producePhutilSafeHTML() {
     return $this->render();
   }

src/view/__tests__/PhabricatorAphrontViewTestCase.php

@@ -0,0 +1,25 @@
+<?php
+
+final class PhabricatorAphrontViewTestCase extends PhabricatorTestCase {
+
+  public function testHasChildren() {
+    $view = new AphrontNullView();
+    $this->assertEqual(false, $view->hasChildren());
+
+    $values = array(
+      null,
+      '',
+      array(),
+      array(null, ''),
+    );
+
+    foreach ($values as $value) {
+      $view->appendChild($value);
+      $this->assertEqual(false, $view->hasChildren());
+    }
+
+    $view->appendChild("!");
+    $this->assertEqual(true, $view->hasChildren());
+  }
+
+}

src/view/layout/PhabricatorTimelineEventView.php

@@ -100,10 +100,8 @@ public function setColor($color) {
   }
 
   public function render() {
-    $content = $this->renderChildren();
-
     $title = $this->title;
-    if (($title === null) && $this->isEmptyContent($content)) {
+    if (($title === null) && !$this->hasChildren()) {
       $title = '';
     }
 
@@ -163,7 +161,7 @@ public function render() {
     $classes = array();
     $classes[] = 'phabricator-timeline-event-view';
     $classes[] = 'phabricator-timeline-border';
-    if (!$this->isEmptyContent($content)) {
+    if ($this->hasChildren()) {
       $classes[] = 'phabricator-timeline-major-event';
       $content = phutil_tag(
         'div',
@@ -182,7 +180,7 @@ public function render() {
               array(
                 'class' => 'phabricator-timeline-core-content',
               ),
-              $content),
+              $this->renderChildren()),
           )));
       $content = array($image, $wedge, $content);
     } else {

src/view/layout/PhabricatorTransactionView.php

@@ -136,14 +136,13 @@ private function renderTransactionStyle() {
   }
 
   private function renderTransactionContent() {
-    $content = $this->renderChildren();
-    if ($this->isEmptyContent($content)) {
+    if (!$this->hasChildren()) {
       return null;
     }
     return phutil_tag(
       'div',
       array('class' => 'phabricator-transaction-content'),
-      $content);
+      $this->renderChildren());
   }
 
 }

src/view/phui/PHUIFeedStoryView.php

@@ -124,7 +124,7 @@ public function render() {
 
     require_celerity_resource('phui-feed-story-css');
     Javelin::initBehavior('phabricator-hovercards');
-    $oneline = $this->isEmptyContent($this->renderChildren());
+    $oneline = !$this->hasChildren();
 
     $body = null;
     $foot = null;