Ref T12164. Make it easier to work with identity objects by attaching them to commits and attaching users to identities.
Details
Loaded some commits with ->needIdentities(true) and checked the resulting objects.
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
src/applications/diffusion/query/DiffusionCommitQuery.php | ||
---|---|---|
407 | Prefer using IdentityQuery, mostly just so the rules end up consistent if we add view policy stuff later, and sometimes we start doing implicit attachment or weird filtering or whatever. (The guidance is roughly "use Query if it exists".) | |
src/applications/repository/query/PhabricatorRepositoryIdentityQuery.php | ||
134–135 | Prefer PeopleQuery, per above. | |
src/applications/repository/storage/PhabricatorRepositoryCommit.php | ||
198 | At least for now, allowing $author to = null may avoid some issue: for example, installs that haven't run rebuild-identities yet won't get an Author identity for older commits and will try to attach null, I think. | |
src/applications/repository/storage/PhabricatorRepositoryIdentity.php | ||
24–29 | Consider removing the boolean flag and just using hasEffectiveUser() at callsites -- this is a little more standard/readable, and getEffectiveUser(false) can be a little hard to guess the meaning of. |
src/applications/repository/storage/PhabricatorRepositoryCommit.php | ||
---|---|---|
198 | Yep, but I intend to land this after the migration. Does it still make sense to allow null under those circumstances? |
src/applications/repository/storage/PhabricatorRepositoryCommit.php | ||
---|---|---|
208 | This should probably be getCOmmitterIdentity |
src/applications/repository/storage/PhabricatorRepositoryCommit.php | ||
---|---|---|
208 | Good catch! |
src/applications/repository/storage/PhabricatorRepositoryCommit.php | ||
---|---|---|
208 | I assume you figured it out, but the capitalization of "o" wasn't intentional. |