In D25505#14525, @20after4 wrote:

Is this needed simply because PhabricatorApplicationSearchController doesn't actually have a try { } block to catch this particular case? Maybe we could improve the exception handling instead of return null?

Not sure I can follow, there is a try block at https://we.phorge.it/source/phorge/browse/master/src/applications/search/controller/PhabricatorApplicationSearchController.php$246-348 which also catches PhabricatorTypeaheadInvalidTokenException. I don't know why PhabricatorTypeaheadDatasource.php "overwrites" the error handling for PhabricatorTypeaheadInvalidTokenException in PhabricatorApplicationSearchController though.

How about other functions that might require a logged-in user? "current Viewer's Projects" for example?

the simplest other-solution would be to extract the exception thrown to its own method, and override that method in the child-class:

if !(allow_partial) {
  $this->throwInvalidTokenForNonPartial($function);
}

...

protected function throwInvalidTokenForNonPartial($function) {
      throw new PhabricatorTypeaheadInvalidTokenException(
          pht(
            'This datasource ("%s") can not evaluate the function "%s(...)".',
            get_class($this),
            $function));
}

and then have PhabricatorPeopleUserFunctionDatasource override it with a new exception class, and catch that exception somewhere and convert to a nicer error message...

ok, that doesn't sound "simplest" now :)

Alternatively, maybe catch the existing exception and show a nice error message + the form?

In D25505#14533, @avivey wrote:

How about other functions that might require a logged-in user? "current Viewer's Projects" for example?

Same Unhandled Exception behavior as described in this ticket for git master and same "Query Errors / Invalid Function: viewer()" behavior with my branch applied.

In D25505#14536, @avivey wrote:

Alternatively, maybe catch the existing exception and show a nice error message + the form?

IIUC that's what the current patch is doing - see the screenshots in T15704#14922. But it does so by hardcoding a specific class...

This single-casing of a specific class is not ideal, but given the complexity of other proposals here I'd like to get the current four lines in. Any other opinions? :)

I'm currently stuck on this since I'm trying to refactor the functions to support an indications about "I work for everybody" and "I work for only logged-in users" to be able to distinguish the case "The token exists" VS "The token exists but it's not visible" VS "The token does not exist"

I think this is part of the maximum efforts minimum results team. Sorry if I cannot share further help but I think this would be a good refactor, if somebody starts such a stub.

Thanks again for this patch that highlighted the valley and the mountain in a very clear way.

In D25505#14536, @avivey wrote:

maybe catch the existing exception and show a nice error message + the form?

Something like this? (incomplete, but it starts providing a more specific exception):

D25621