Details
Viewed some commits where the identity was mapped to a user and another that wasn't; saw the header render either a link to the user or the identity object.
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| src/applications/diffusion/controller/DiffusionCommitController.php | ||
|---|---|---|
| 721–724 | This turned out to be way easier than trying to render user handles inside the identity handle renderer, and less ugly than copy/pasting a long function that's used everywhere. | |
Could we go further and just turn this:
// Update after T12164. if ($commit->hasAuthorIdentity()) { $author_identity = $commit->getAuthorIdentity(); $author_phid = $author_identity->getIdentityHandle(); } else { $author_phid = $data->getCommitDetail('authorPHID'); }
...into this:
$author_phid = $commit->getAuthorDisplayPHID();
Tthat should give us even less stuff to handle/update around null later on, since we have ~3 of those blocks here and probably (?) more elsewhere.
| src/applications/repository/storage/PhabricatorRepositoryCommit.php | ||
|---|---|---|
| 449 | Minor, but $viewer can probably typehint as PhabricatorUser here and elsewhere. | |
| 458 | Verrrrry verrrry nitpicky, but this might be more clear: if (x) {
return;
}
if (y) {
return;
}
return;...than this: if (x) {
return;
} else {
if (y) {
return;
} else {
return;
}
}These methods also both implicitly return null at the bottom and maybe that would be clearer as an explicit return null; | |
| src/applications/repository/storage/PhabricatorRepositoryIdentity.php | ||
| 81 | It wasn't obvious to me that this returns a PHID rather than a Handle. Maybe call it getIdentityDisplayPHID() or similar to make it more clear that we aren't getting a Handle back? | |
The array typehint on $handles might eventually run into trouble because it might be a PhabricatorHandleList (an object which works like an array, but resolves lazily) sometimes. If you hit that, all you can do is remove the array typehint since there's no way to formally say array|PhabricatorHandleList.
(We could create some PhabricatorHandleList::newFromAlreadyResolvedArray(...) and then force everything to be PhabricatorHandleList, I guess.)
Looks quite clean otherwise, way better than the old code.
Yeah -- long ago they all got array typehints, and then a bunch of them got removed when PhabricatorHandleList / PhabricatorHandlePool landed, but the newer Handle stuff hasn't made its way into all the crevices yet so there are still a few array typehints hiding out. If you run into one, it's appropriate to just remove it.