Resolve link framing issue for public feed

Summary:
  - Use $this->linkTo($phid) to render all links.
  - Simplify code.

Test Plan: Public feed renders with 'target="_top"' links. Nonpublic feed
doesn't. Looked at a bunch of feed stories, none seem broken.

Reviewers: btrahan, aran, nh, jungejason, ide

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T453

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

src/applications/feed/builder/feed/PhabricatorFeedBuilder.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -19,11 +19,17 @@
 final class PhabricatorFeedBuilder {
 
   private $stories;
+  private $framed;
 
   public function __construct(array $stories) {
     $this->stories = $stories;
   }
 
+  public function setFramed($framed) {
+    $this->framed = $framed;
+    return $this;
+  }
+
   public function setUser(PhabricatorUser $user) {
     $this->user = $user;
     return $this;
@@ -57,6 +63,7 @@ final class PhabricatorFeedBuilder {
     foreach ($stories as $story) {
       $story->setHandles($handles);
       $story->setObjects($objects);
+      $story->setFramed($this->framed);
 
       $date = phabricator_date($story->getEpoch(), $user);
       if ($date == $today) {

src/applications/feed/controller/publicstream/PhabricatorFeedPublicStreamController.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -38,6 +38,7 @@ final class PhabricatorFeedPublicStreamController
 
     $builder = new PhabricatorFeedBuilder($stories);
     $builder
+      ->setFramed(true)
       ->setUser($request->getUser());
 
     $view = $builder->buildView();

src/applications/feed/story/base/PhabricatorFeedStory.php

@@ -22,6 +22,7 @@ abstract class PhabricatorFeedStory {
 
   private $handles;
   private $objects;
+  private $framed;
 
   final public function __construct(PhabricatorFeedStoryData $data) {
     $this->data = $data;
@@ -37,6 +38,11 @@ abstract class PhabricatorFeedStory {
     return array();
   }
 
+  final public function setFramed($framed) {
+    $this->framed = $framed;
+    return $this;
+  }
+
   final public function setHandles(array $handles) {
     $this->handles = $handles;
     return $this;
@@ -80,9 +86,33 @@ abstract class PhabricatorFeedStory {
   final protected function renderHandleList(array $phids) {
     $list = array();
     foreach ($phids as $phid) {
-      $list[] = '<strong>'.$this->getHandle($phid)->renderLink().'</strong>';
+      $list[] = $this->linkTo($phid);
     }
     return implode(', ', $list);
   }
 
+  final protected function linkTo($phid) {
+    $handle = $this->getHandle($phid);
+
+    // NOTE: We render our own link here to customize the styling and add
+    // the '_top' target for framed feeds.
+
+    return phutil_render_tag(
+      'a',
+      array(
+        'href'    => $handle->getURI(),
+        'target'  => $this->framed ? '_top' : null,
+      ),
+      phutil_escape_html($handle->getLinkName()));
+  }
+
+  final protected function renderSummary($text, $len = 128) {
+    if ($len) {
+      $text = phutil_utf8_shorten($text, $len);
+    }
+    $text = phutil_escape_html($text);
+    $text = str_replace("\n", '<br />', $text);
+    return $text;
+  }
+
 }

src/applications/feed/story/base/__init__.php

@@ -8,5 +8,8 @@
 
 phutil_require_module('phabricator', 'applications/phid/handle');
 
+phutil_require_module('phutil', 'markup');
+phutil_require_module('phutil', 'utils');
+
 
 phutil_require_source('PhabricatorFeedStory.php');

src/applications/feed/story/differential/PhabricatorFeedStoryDifferential.php

@@ -36,7 +36,6 @@ class PhabricatorFeedStoryDifferential extends PhabricatorFeedStory {
   public function renderView() {
     $data = $this->getStoryData();
 
-    $handles = $this->getHandles();
     $author_phid = $data->getAuthorPHID();
 
     $objects = $this->getObjects();
@@ -49,9 +48,9 @@ class PhabricatorFeedStoryDifferential extends PhabricatorFeedStory {
     $verb = DifferentialAction::getActionPastTenseVerb($action);
 
     $view->setTitle(
-      '<strong>'.$handles[$author_phid]->renderLink().'</strong>'.
+      $this->linkTo($author_phid).
-      ' '.$verb.' revision '.
+      " {$verb} revision ".
-      '<strong>'.$handles[$revision_phid]->renderLink().'</strong>.');
+      $this->linkTo($revision_phid).'.');
     $view->setEpoch($data->getEpoch());
 
     $action = $data->getValue('action');
@@ -66,14 +65,8 @@ class PhabricatorFeedStoryDifferential extends PhabricatorFeedStory {
     }
 
     if ($full_size) {
-      if (!empty($handles[$author_phid])) {
+      $view->setImage($this->getHandle($author_phid)->getImageURI());
-        $image_uri = $handles[$author_phid]->getImageURI();
+      $content = $this->renderSummary($data->getValue('feedback_content'));
-        $view->setImage($image_uri);
-      }
-
-      $content = phutil_escape_html($data->getValue('feedback_content'));
-      $content = str_replace("\n", '<br />', $content);
-
       $view->appendChild($content);
     } else {
       $view->setOneLineStory(true);

src/applications/feed/story/differential/__init__.php

@@ -10,7 +10,5 @@ phutil_require_module('phabricator', 'applications/differential/constants/action
 phutil_require_module('phabricator', 'applications/feed/story/base');
 phutil_require_module('phabricator', 'applications/feed/view/story');
 
-phutil_require_module('phutil', 'markup');
-
 
 phutil_require_source('PhabricatorFeedStoryDifferential.php');

src/applications/feed/story/maniphest/PhabricatorFeedStoryManiphest.php

@@ -42,7 +42,6 @@ class PhabricatorFeedStoryManiphest extends PhabricatorFeedStory {
     $task_phid = $data->getValue('taskPHID');
 
     $objects = $this->getObjects();
-    $handles = $this->getHandles();
     $action = $data->getValue('action');
 
     $view = new PhabricatorFeedStoryView();
@@ -54,7 +53,7 @@ class PhabricatorFeedStoryManiphest extends PhabricatorFeedStory {
         if ($owner_phid) {
           $extra =
             ' to '.
-            '<strong>'.$this->getHandle($owner_phid)->renderLink().'</strong>';
+            $this->linkTo($owner_phid);
         } else {
           $verb = 'placed';
           $extra = ' up for grabs';
@@ -63,9 +62,9 @@ class PhabricatorFeedStoryManiphest extends PhabricatorFeedStory {
     }
 
     $title =
-      '<strong>'.$this->getHandle($author_phid)->renderLink().'</strong>'.
+      $this->linkTo($author_phid).
       " {$verb} task ".
-      '<strong>'.$this->getHandle($task_phid)->renderLink().'</strong>';
+      $this->linkTo($task_phid);
     $title .= $extra;
     $title .= '.';
 
@@ -83,15 +82,8 @@ class PhabricatorFeedStoryManiphest extends PhabricatorFeedStory {
     $view->setEpoch($data->getEpoch());
 
     if ($full_size) {
-      if (!empty($handles[$author_phid])) {
+      $view->setImage($this->getHandle($author_phid)->getImageURI());
-        $image_uri = $handles[$author_phid]->getImageURI();
+      $content = $this->renderSummary($data->getValue('description'));
-        $view->setImage($image_uri);
-      }
-
-      $content = phutil_escape_html(
-        phutil_utf8_shorten($data->getValue('description'), 128));
-      $content = str_replace("\n", '<br />', $content);
-
       $view->appendChild($content);
     } else {
       $view->setOneLineStory(true);

src/applications/feed/story/maniphest/__init__.php

@@ -10,8 +10,5 @@ phutil_require_module('phabricator', 'applications/feed/story/base');
 phutil_require_module('phabricator', 'applications/feed/view/story');
 phutil_require_module('phabricator', 'applications/maniphest/constants/action');
 
-phutil_require_module('phutil', 'markup');
-phutil_require_module('phutil', 'utils');
-
 
 phutil_require_source('PhabricatorFeedStoryManiphest.php');

src/applications/feed/story/phriction/PhabricatorFeedStoryPhriction.php

@@ -34,7 +34,6 @@ class PhabricatorFeedStoryPhriction extends PhabricatorFeedStory {
   public function renderView() {
     $data = $this->getStoryData();
 
-    $handles = $this->getHandles();
     $author_phid = $data->getAuthorPHID();
     $document_phid = $data->getValue('phid');
 
@@ -46,9 +45,9 @@ class PhabricatorFeedStoryPhriction extends PhabricatorFeedStory {
     $verb = PhrictionActionConstants::getActionPastTenseVerb($action);
 
     $view->setTitle(
-      '<strong>'.$handles[$author_phid]->renderLink().'</strong>'.
+      $this->linkTo($author_phid).
-      ' '.$verb.' the document '.
+      " {$verb} the document ".
-      '<strong>'.$handles[$document_phid]->renderLink().'</strong>.');
+      $this->linkTo($document_phid).'.');
     $view->setEpoch($data->getEpoch());
 
     $action = $data->getValue('action');
@@ -62,14 +61,8 @@ class PhabricatorFeedStoryPhriction extends PhabricatorFeedStory {
     }
 
     if ($full_size) {
-      if (!empty($handles[$author_phid])) {
+      $view->setImage($this->getHandle($author_phid)->getImageURI());
-        $image_uri = $handles[$author_phid]->getImageURI();
+      $content = $this->renderSummary($data->getValue('content'));
-        $view->setImage($image_uri);
-      }
-
-      $content = phutil_escape_html($data->getValue('content'));
-      $content = str_replace("\n", '<br />', $content);
-
       $view->appendChild($content);
     } else {
       $view->setOneLineStory(true);

src/applications/feed/story/phriction/__init__.php

@@ -10,7 +10,5 @@ phutil_require_module('phabricator', 'applications/feed/story/base');
 phutil_require_module('phabricator', 'applications/feed/view/story');
 phutil_require_module('phabricator', 'applications/phriction/constants/action');
 
-phutil_require_module('phutil', 'markup');
-
 
 phutil_require_source('PhabricatorFeedStoryPhriction.php');

src/applications/feed/story/project/PhabricatorFeedStoryProject.php

@@ -39,23 +39,22 @@ class PhabricatorFeedStoryProject extends PhabricatorFeedStory {
     $type = $data->getValue('type');
     $old = $data->getValue('old');
     $new = $data->getValue('new');
-    $proj = $this->getHandle($data->getValue('projectPHID'));
+    $proj_phid = $data->getValue('projectPHID');
 
     $author_phid = $data->getAuthorPHID();
-    $author = $this->getHandle($author_phid);
 
     switch ($type) {
       case PhabricatorProjectTransactionType::TYPE_NAME:
         if (strlen($old)) {
           $action = 'renamed project '.
-            '<strong>'.$proj->renderLink().'</strong>'.
+            $this->linkTo($proj_phid).
             ' from '.
             '<strong>'.phutil_escape_html($old).'</strong>'.
             ' to '.
             '<strong>'.phutil_escape_html($new).'</strong>.';
         } else {
           $action = 'created project '.
-            '<strong>'.$proj->renderLink().'</strong>'.
+            $this->linkTo($proj_phid).
             ' (as '.
             '<strong>'.phutil_escape_html($new).'</strong>).';
         }
@@ -66,30 +65,30 @@ class PhabricatorFeedStoryProject extends PhabricatorFeedStory {
 
         if ((count($add) == 1) && (count($rem) == 0) &&
             (head($add) == $author_phid)) {
-          $action = 'joined project <strong>'.$proj->renderLink().'</strong>.';
+          $action = 'joined project '.$this->linkTo($proj_phid).'.';
         } else if ((count($add) == 0) && (count($rem) == 1) &&
                    (head($rem) == $author_phid)) {
-          $action = 'left project <strong>'.$proj->renderLink().'</strong>.';
+          $action = 'left project '.$this->linkTo($proj_phid).'.';
         } else if (empty($rem)) {
           $action = 'added members to project '.
-            '<strong>'.$proj->renderLink().'</strong>: '.
+            $this->linkTo($proj_phid).': '.
             $this->renderHandleList($add).'.';
         } else if (empty($add)) {
           $action = 'removed members from project '.
-            '<strong>'.$proj->renderLink().'</strong>: '.
+            $this->linkTo($proj_phid).': '.
             $this->renderHandleList($rem).'.';
         } else {
           $action = 'changed members of project '.
-            '<strong>'.$proj->renderLink().'</strong>, added: '.
+            $this->linkTo($proj_phid).', added: '.
             $this->renderHandleList($add).'; removed: '.
             $this->renderHandleList($rem).'.';
         }
         break;
       default:
-        $action = 'updated project <strong>'.$proj->renderLink().'</strong>.';
+        $action = 'updated project '.$this->linkTo($proj_phid).'.';
         break;
     }
-    $view->setTitle('<strong>'.$author->renderLink().'</strong> '.$action);
+    $view->setTitle($this->linkTo($author_phid).' '.$action);
     $view->setOneLineStory(true);
 
     return $view;

src/applications/feed/story/status/PhabricatorFeedStoryStatus.php

@@ -33,28 +33,17 @@ class PhabricatorFeedStoryStatus extends PhabricatorFeedStory {
   public function renderView() {
     $data = $this->getStoryData();
 
-    $handles = $this->getHandles();
     $author_phid = $data->getAuthorPHID();
 
-    $objects = $this->getObjects();
-
     $view = new PhabricatorFeedStoryView();
 
-    $view->setTitle(
+    $view->setTitle($this->linkTo($author_phid));
-      '<strong>'.$handles[$author_phid]->renderLink().'</strong>');
     $view->setEpoch($data->getEpoch());
+    $view->setImage($this->getHandle($author_phid)->getImageURI());
 
-    if (!empty($handles[$author_phid])) {
+    $content = $this->renderSummary($data->getValue('content'), $len = null);
-      $image_uri = $handles[$author_phid]->getImageURI();
-      $view->setImage($image_uri);
-    }
-
-    $content = phutil_escape_html($data->getValue('content'));
-    $content = str_replace("\n", '<br />', $content);
-
     $view->appendChild($content);
 
-
     return $view;
   }
 

src/applications/feed/story/status/__init__.php

@@ -9,7 +9,5 @@
 phutil_require_module('phabricator', 'applications/feed/story/base');
 phutil_require_module('phabricator', 'applications/feed/view/story');
 
-phutil_require_module('phutil', 'markup');
-
 
 phutil_require_source('PhabricatorFeedStoryStatus.php');

src/applications/phid/handle/PhabricatorObjectHandle.php

@@ -195,15 +195,7 @@ class PhabricatorObjectHandle {
 
 
   public function renderLink() {
-
+    $name = $this->getLinkName();
-    switch ($this->getType()) {
-      case PhabricatorPHIDConstants::PHID_TYPE_USER:
-        $name = $this->getName();
-        break;
-      default:
-        $name = $this->getFullName();
-    }
-
     $class = null;
 
     if ($this->status != PhabricatorObjectHandleStatus::STATUS_OPEN) {
@@ -223,4 +215,15 @@ class PhabricatorObjectHandle {
       phutil_escape_html($name));
   }
 
+  public function getLinkName() {
+    switch ($this->getType()) {
+      case PhabricatorPHIDConstants::PHID_TYPE_USER:
+        $name = $this->getName();
+        break;
+      default:
+        $name = $this->getFullName();
+    }
+    return $name;
+  }
+
 }