Fixes T5862. The Diffusion table uses id but all the other infrastructure uses phid so just do a quick load of the repositories to get the ids. Long term, we should re-key the table by phid I think.
Details
Details
- Reviewers
epriestley - Maniphest Tasks
- T5862: Allow audits to be searched for by repository
- Commits
- Restricted Diffusion Commit
rP644e950ea3f1: Audit - add ability to query by repositories
made a query with a repository and got a proper result set
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- T5862
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 2228 Build 2232: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
Comment Actions
One weird mess inline that I don't have a clean answer for.
| src/applications/audit/query/PhabricatorCommitSearchEngine.php | ||
|---|---|---|
| 70 | We should make sure $repositories is not empty, and automatically fail the query if it is. I don't think there's an easy way to do that right now. You could do something silly/hacky like pass array(-1) I guess, or maybe make some dummy Query subclass like PhabricatorNeverAnyResultsQuery, and return that? Or add some flag to Query like setMatchNoResults(true)? Or expose withRepositoryPHIDs() on the Query, and have it do this lookup internally, so it can throw PhabricatorEmptyQueryException immediately? Maybe that's cleanest? | |
Comment Actions
add withRepositoryPHIds; add comment to say to prefer withRepositoryIDs and why
re-tested and it still works
Comment Actions
One possible inline, looks solid otherwise.
| src/applications/diffusion/query/DiffusionCommitQuery.php | ||
|---|---|---|
| 70–77 | I think this miiight have to happen at query time for the exception to be dealt with properly? i.e., move the actual lookup to the buildWhereClause() part? That should fix the setViewer() thing too. | |