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
from

Conversation

@Thoronir42
Copy link

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 Thoronir42 force-pushed the Thoronir42:dev-feature/responding-handlers branch from c99c04e to 039a29c Aug 23, 2019

@Thoronir42

This comment has been minimized.

Copy link
Author

commented Aug 23, 2019

Force-push to amend commit with bad indentation style

@Thoronir42 Thoronir42 force-pushed the Thoronir42:dev-feature/responding-handlers branch from 039a29c to d8e1321 Aug 28, 2019

@dg

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

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

@mabar

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Apparently this is a thing no one needs. 🤔

@mabar

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

I understand.

@dg dg added this to the v3.1 milestone Sep 10, 2019

@dg dg force-pushed the nette:master branch 4 times, most recently from e17599b to 67f45fb Sep 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.