Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 3 commits into from

Conversation

Thoronir42
Copy link

@Thoronir42 Thoronir42 commented Aug 22, 2019

  • 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:

@Thoronir42
Copy link
Author

Force-push to amend commit with bad indentation style

@dg
Copy link
Member

dg commented Sep 9, 2019

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

@mabar
Copy link
Contributor

mabar commented Sep 9, 2019

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
Copy link
Member

dg commented Sep 9, 2019

Apparently this is a thing no one needs. 🤔

@mabar
Copy link
Contributor

mabar commented Sep 9, 2019

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
Copy link
Author

Thoronir42 commented Sep 9, 2019

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
Copy link
Member

dg commented Sep 10, 2019

I understand.

@dg dg added this to the v3.1 milestone Sep 10, 2019
@dg dg force-pushed the master branch 4 times, most recently from e17599b to 67f45fb Compare September 18, 2019 18:48
@dg dg force-pushed the master branch 7 times, most recently from 7e42997 to f7df426 Compare October 21, 2019 19:23
@mabar
Copy link
Contributor

mabar commented Nov 3, 2019

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

@dg dg force-pushed the master branch 4 times, most recently from 0e26720 to e65cf05 Compare November 19, 2019 19:03
@dg dg force-pushed the master branch 3 times, most recently from 7fe78c1 to 8f1bb54 Compare December 11, 2019 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants