Ref T7123. Turns out that we might throw ConduitClientException now in proxied scenarios. For all but one callsite remove the try / catch bit and don't issue the call for SVN. For the remaining callsite, also don't issue the call for SVN but keep in the exception logic since its renders a pretty error message in the non-proxied case?
Details
Details
- Reviewers
epriestley - Maniphest Tasks
- T7123: Clean up `catch (ConduitException $ex)` in Diffusion
- Commits
- Restricted Diffusion Commit
rP81d2f2686c4e: Diffusion - clean up catching ConduitException
played around with diffusion and things looked okay.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- T7123
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 4523 Build 4537: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
Comment Actions
Nice.
| src/applications/diffusion/controller/DiffusionBrowseSearchController.php | ||
|---|---|---|
| 86 | This one could still potentially pop out as ConduitClientException, right? | |
| src/applications/diffusion/controller/DiffusionBrowseSearchController.php | ||
|---|---|---|
| 86 | Yes, in the proxied case. I wasn't sure how the ConduitClientException gets constructed in terms of the ConduitException its wrapping; maybe this can just getErrorDescription? In practice, this is just making a little search error that tells you if something is weird on your system, e.g. the vcs grep command failed for some reason. maybe it can just be omitted entirely? | |
Comment Actions
Omitting it entirely sounds reasonable to me. I think the way exceptions work should probably get cleaned up eventually, so just kind of kicking this one down the road a bit and accepting a reasonable-but-not-perfect behavior seems good.