WIP: Add handling of result from action* and handle*

[Thoronir42]

trafficstars

• new feature
• BC break? possibly yes
  • if someone relies on Component::tryCall's bool return type, they are screwed, they are ok if they only check truthy/falsy value
• doc PR: nette/docs#??? no additions

This PR adds possibility to presenters action* methods and components handle* methods to directly return response.

If they return scalar/array value, it is wrapped into typical JsonResponse (this response creation could and probably should be done via an additional service so the functionality is not coupled in the Presenter.

Right now, this PR is more of an API proposal to reduce coupling of Component handlers dependency on their presenter. More verbose description can be found on the forum.

As I said, I'm not entirely happy about storing the handle result in a property but it is about the best I could come up with while not changing public API of the system too much.

TODO:

• [ ] Agree on usage
• [ ] Add some kind of "ResponseFactory" which creates response based on given data, maybe something like https://github.com/Thoronir42/praxos/blob/develop/sources/backend/src/Api/Presentation/JsonResponseFactory.php (don't mind rest of the project, it's botched up team semestral project)
• [ ] Add some tests
• [ ] Update doc blocks of changed code

[Thoronir42]

Force-push to amend commit with bad indentation style

[dg]

Perhaps it would be better to discuss it in a forum where it will find more people.

[mabar]

Perhaps it would be better to discuss it in a forum where it will find more people.

It is already posted at forum, but there were no reactions for days so I suggested PR to don't lost it in forum history https://forum.nette.org/en/32437-response-returning-from-presenter

[dg]

Apparently this is a thing no one needs. 🤔

[mabar]

Not really needed, just makes code cleaner imho. Methods like $presenter->sendResponse() and $presenter->error() terminates method execution, but it's not statically analyzable

[Thoronir42]

I agree that no one needs this, my point is however, that this might make sending response less driven by exeptions. My main motivation is to offer a way to avoid this (example copied from forum)

try {
  ...
  $this->sendResponse(new FileResponse('bananaphone.midi'));
} catch (\Throwable $ex) {
  // AbortException is thrown within the method and might be accidentally caught
  //  here - needs to be rethrown
  echo 'something is wrong, sorry';
  exit;
}

by using this:

try {
  ...
  return new FileResponse('bananaphone.midi');
} catch (\Throwable $ex) {
  // AbortException is thrown after the method is exitted in the Presenters run
  //  method itself so we don't have to worry about accidentally catching AbortException
  //  if we just return the response 
  echo 'something is wrong, sorry';
  exit;
}

[dg]

I understand.

[mabar]

I'm taking back statement about static analysis, phpstan understands it https://github.com/phpstan/phpstan#custom-early-terminating-method-calls

[dakur]

It make sense to me as well. Why should be presenter aware of what is component sending to itself e.g. when making asynchronous request? I was a bit confused about no way to return a response directly from component right now (searching for send).

[patrickkusebauch]

:+1: For this feature. It would be way nicer IMHO in Controls' signals where you need to send a response to the Client. Better than calling $this-getPresenter()->sendResponse()

[dakur]

I'm taking back statement about static analysis, phpstan understands it https://github.com/phpstan/phpstan#custom-early-terminating-method-calls

Yeah, but it has its problem – you have to mirror each such call into phpstan-nette config which is error prone and neither convenient nor clean in my opinion..

[Thoronir42]

If i may direct this discussion, I’d rather keep the partially related tooling apart from it and focus on aspect of the changes implications.

The fact that phpstan posses options to amend it’s understandable inability to recognize code flow abortion via the always throwing AbortException methods is simply just a thing that is already there. Besides, even if the direction of Nette Application picks up this change, we aren’t getting rid of this any time soon, if at all - all projects would have to adopt the new approach first. If we were to change the usage of sendResponse(), it would take minimally few versions while being deprecated and that is out of scope of this proposition. Here I’m suggesting to pave way for the possibility to do so.

I’d rather focus on the possibility of using standard language construct - the return statement - which would imho also make it more intuitive for the framework users. We could forget about the need to ->sendResponse(), and sometimes about need to ->getPresenter() too (which imho breaks the encapsulation a bit as it takes away the control flow from parent components). We could just return the specific response or the response data.