Ref T12164. Updates another controller to use identities.
Details
Pretty ad-hoc, but loaded the main pages of several different repos with and without repo identities. I'm not totally convinced the author from this data structure is actually being used:
$return = array( 'commit' => $modified, 'date' => $date, 'author' => $author, 'details' => $details, );
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
I think the "Graph" tab may still use this, maybe check that?
We could also consider just moving DiffusionView::renderName() into renderAuthor()/renderCommitter(), perhaps -- should be safe since the path which uses Identities definitely returns markup.
I think the "Graph" tab may still use this, maybe check that?
Hmmm, I wonder why your screenshot has clickable "epriestley" links but my full name is showing up as text? Here's my version of the same page:
Oh -- my screenshot is on my local install, and I haven't made you an account there, so there's no Phabricator account to associate the commit with. I get the same view as you do here on secure.
We could also consider just moving DiffusionView::renderName() into renderAuthor()/renderCommitter(), perhaps -- should be safe since the path which uses Identities definitely returns markup.
How exactly would this work? In the case where renderXXX() ends up trying to return a string, just pass it through renderName() first? I guess having that nice tooltip would be strictly better than plain text. Is it possible that it might render on some page where the CSS isn't expecting it, though?
In the case where renderXXX() ends up trying to return a string, just pass it through renderName() first?
Yeah. Or possibly the renderName() logic can just move to Commit entirely, depending on how many callsites we're left with after converting things.
Is it possible that it might render on some page where the CSS isn't expecting it, though?
Since we'll sometimes return a complex piece of HTML now from renderAuthor() anyway (when we have an Identity and the Identity is bound to a user, we link to their profile) and generally did that before anyway (just with a lot more if (authorPHID) checks) I suspect we won't run into any problems. Definitely possible, but it's likely we'll catch it while converting to Identities if we do run afoul of some CSS.
Or possibly the renderName() logic can just move to Commit entirely, depending on how many callsites we're left with after converting things.
There are no other callsites aside from this controller, but I don't feel great about moving that kind of UI-specific logic into Commit. Here's what we'd be moving:
final public static function renderName($name) { $email = new PhutilEmailAddress($name); if ($email->getDisplayName() && $email->getDomainName()) { Javelin::initBehavior('phabricator-tooltips', array()); require_celerity_resource('aphront-tooltip-css'); return javelin_tag( 'span', array( 'sigil' => 'has-tooltip', 'meta' => array( 'tip' => $email->getAddress(), 'align' => 'S', 'size' => 'auto', ), ), $email->getDisplayName()); } return hsprintf('%s', $name); }
If you think we should move that into Commit, let me know and I'll put up a new diff for that.