Skip to content

Added third option to Application::$catchExceptions#63

Open
enumag wants to merge 1 commit intonette:masterfrom
enumag:exceptions
Open

Added third option to Application::$catchExceptions#63
enumag wants to merge 1 commit intonette:masterfrom
enumag:exceptions

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

@dg dg force-pushed the master branch 6 times, most recently from 6074ad2 to 81a1a34 Compare May 25, 2015 10:49
@dg dg force-pushed the master branch 6 times, most recently from 51b53ed to d36f845 Compare June 21, 2015 14:21
@dg dg force-pushed the master branch 3 times, most recently from 0c73a68 to d07d7b3 Compare July 1, 2015 15:46
@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.

@dg dg force-pushed the master branch 12 times, most recently from 8f2b6ff to 83f19ce Compare May 3, 2016 06:22
@dg dg force-pushed the master branch 4 times, most recently from b8d5735 to 9dbfd53 Compare May 9, 2016 15:38
@dg dg force-pushed the master branch 6 times, most recently from c0400a1 to 5b60e95 Compare May 19, 2016 00:29
@dg dg force-pushed the master branch 2 times, most recently from aa42c27 to 7b1ec30 Compare May 23, 2016 01:06
@f3l1x
Copy link
Member

f3l1x commented Mar 15, 2017

@enumag Nice. I'm looking forward to it.

@josefsabl
Copy link

@enumag
Copy link
Contributor Author

enumag commented Sep 21, 2018

@josefsabl Since my suggestions were refused I just kept using my own hacked Application class (contains some other hacks too) to do this. I'm not working with nette anymore so don't expect me to send any PRs regarding this. Though I'm still interested to see if some solution gets implemented in nette.

@josefsabl
Copy link

@josefsabl Since my suggestions were refused I just kept using my own hacked Application class (contains some other hacks too) to do this. I'm not working with nette anymore so don't expect me to send any PRs regarding this. Though I'm still interested to see if some solution gets implemented in nette.

What a shame. Thanks for the reply anyway.

@josefsabl
Copy link

I extended the \Nette\Application\Application class like this:

final class Application extends \Nette\Application\Application
{
    /**
     * @throws \Throwable
     */
    public function run(): void
    {
        try {
            parent::run();
        } catch (\Nette\Application\BadRequestException $e) {
            $this->processException($e);
        }
    }
}

and changed the application.application service to use this class instead like this in one of my extensions:

$this->getContainerBuilder()
    ->getDefinition('application.application');
    ->setFactory(\My\Application::class);

... any idea what might go wrong? :-)

Looks like it is working well after the quick test.

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.

6 participants