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

Added third option to Application::$catchExceptions #63

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

enumag
Copy link
Contributor

@enumag enumag commented Feb 15, 2015

I use a bunch of services to check whether the application is in state to run the request, if the use has privileges and so on. However these services are not supposed to handle the error correctly with a flashmessage and redirect, they only throw an exception (some specific descendant of BadRequestException).

I handle these exceptions in ErrorPresenter which works fine but I have some troubles with this in debug mode - I don't want to see these exceptions because they are kind of expected and I want to see if the ErrorPresenter handles them correctly. Therefore in debug mode I want to catch BadRequestException.

I consider this approach to be far better then the usual. When writing the application I don't repeat the flash messages and redirects everywhere (it get's even worse if I need to translate them - I need to inject the translator). Instead I just throw an exception and later add a line or two to ErrorPresenter. Presenters and controls are much cleaner because they focus on what they are supposed to do and don't have to deal with errors at all.

What do you think about this solution? I use it for about year or so and am really satisfied with it.

@enumag
Copy link
Contributor Author

enumag commented Feb 15, 2015

It gets even better when you use module-specific ErrorPresenters like I do. I'll try to send a PR with that later if this is merged.

@fprochazka
Copy link
Contributor

I'm not sure I understand. But I like the idea, that the BadRequestException won't show up in tracy but rather in ErrorPresenter even in debug mode.

@JanTvrdik
Copy link
Contributor

What about this solution to your problem:

  1. Enable catching all exceptions regardless of debugMode
  2. In ErrorPresenter check if debugMode is enabled or not and then either pass them directly to Tracy error handler or handle in ErrorPresenter as usual.

@JanTvrdik
Copy link
Contributor

By looking at the code I think that another solution may be possible – implement onError event listener in which you either enable or disable catchExceptions based on catched exception and debugMode.

@enumag
Copy link
Contributor Author

enumag commented Feb 15, 2015

@JanTvrdik You missed the purpose of this PR entirely. I'm not looking for ways to do what the PR does without changing Nette, I already know several ways to do that without your help. I want to know if you like the idea of putting error handling in ErrorPresenter. If so I'd like to make it the recommended way for Nette, hence this PR to make it convenient for other people than myself.

@JanTvrdik
Copy link
Contributor

Ok, regarding this PR. I hate the magic NULL value. But the idea of having a way to pass BadRequestException to error presenter in dev mode is good.

@enumag
Copy link
Contributor Author

enumag commented Feb 15, 2015

Good. :-) I'm not really happy with the NULL value either, I just couldn't think of a better solution - none of these seem much better to me:

  1. Pass string 'smart' instead.
  2. Use some constants like CATCH_ALL, CATCH_NONE, CATCH_SMART. Values could be bools/ints/strings whatever (probably bools for compatibility + a string or null for CATCH_SMART).
  3. Pass a type to catch \Exception would catch all, NULL would catch none, \Nette\Application\BadRequestException for the desired behaviour.

Any suggestions?

@EdaCZ
Copy link

EdaCZ commented Feb 16, 2015

It would be nice to have clean solution for this problem. Sometimes I deal with it.
I would appreciate this "feature" 👍

@enumag
Copy link
Contributor Author

enumag commented Feb 16, 2015

@JanTvrdik What do you think about this? enumag@bef9392

@enumag
Copy link
Contributor Author

enumag commented Jul 24, 2015

ping @JanTvrdik

@JanTvrdik
Copy link
Contributor

I need this today 😄 But I still don't like your proposal.

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.

None yet

6 participants