My idea for this would be two diffs:

1. Diff one in libphutil, which adds a new exception class AphrontQueryObjectNotFoundException.php
2. Diff two in phabricator, which starts throwing this exception instead of the existing one, as well as adds code in index.php to generically catch it and return an Aphront404Response.

Sound good? I think this approach would let us do this pretty easily for other applications where this scenario might occur (aggressively loading up data in willProcessRequest).

I don't think we should throw a new exception type, we have a lot of code which returns array() from execute() or null from executeOne() to indicate "no query results". The load boils down to PhabricatorRepositoryQuery->withCallsigns(...), which definitely shouldn't start throwing random exceptions or it will break lots of stuff (and generally behave unconventionally).

This is messy because all the loading is implicit and happens really early on. One approach would be to make that stuff explicit instead, but then we'll need a lot of this in all the Diffusion controllers:

$drequest = $this->buildDiffusionRequest($request);
if (!$drequest) {
  return new Aphront404Response();
}

Another would be to make processRequest() final on DiffusionController, and move DiffusionRequest construction there instead of willProcessRequest(). Then it could call processDiffusionRequest($request, $drequest) or similar. All the subclasses would need to be adjusted:

$drequest = blah blah;
if (!$repository) {
  return new Aphront404Response();
}

return $this->processDiffusionRequest($request, $drequest);

Another would be to let willProcessRequest() return a Response. The previous call (willBeginExecution) already can, so this doesn't seem terrible.

Another would be to introduce a ResponseException which forces a particular response.

Overall, I think I favor making processRequest() final?

I'd also like to get rid of willProcessRequest() in the long run:

• In 98% of cases, it just reads out of $data. Instead, AphrontRequest should have getRouteData($key, $default = null) or similar and these reads should just happen on the first few lines of processRequest().
• In the other 2% of cases it's used to implement crazy nonsense like this, which probably is better expressed through more explicit structuring.

Some discussion in T1806, but specifically I imagine the signatures changing from this:

private $id;

public function willProcessRequest(array $data) {
  $this->id = idx($data, 'id');
}

public function processRequest() {
  $request = $this->getRequest();
  // ...
}

...to this:

public function processRequest(AphrontRequest $request) {
  $id = $request->getRouteData('id');
  // ...
}

Which is much simpler and aligns perfectly with usage 98% of the time.

I dig the bigger picture plan.

The only bit I'd contend with for intellectual fun and curiosity is that swapping untyped exception A with typed exception B should not really introduce more issues; any try catch code was catching just Exception since A is just exception and that will still catch the more granular B exception just the same.

In T5646#69864, @btrahan wrote:

I dig the bigger picture plan.

D10698 enables the new APIs discussed above, but only ports one controller over.