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

[BUG] Action forwarding is not working if Exception is handled in the Controller's initialize() phase #12931

Closed
temuri416 opened this Issue Jun 29, 2017 · 8 comments

Comments

Projects
None yet
5 participants
@temuri416
Contributor

temuri416 commented Jun 29, 2017

Hi,

I use this approach to handle exceptions thrown in Controller Actions. Nothing fancy here:

$eventsManager->attach('dispatch:beforeException', function($event, $dispatcher, $exception) {
    // Forward to Error controller:
    $dispatcher->forward([
        'controller' => 'error',
        'action' => 'oops',
        'params' => null,
    ]);
}

Everything works fine if an Exception is thrown inside Controller's Action, i.e. past this line:

https://github.com/phalcon/cphalcon/blob/master/phalcon/dispatcher.zep#L654

The problem is that if an Exception is thrown inside Controller's initialize(), then forwarding code above does not work (it does get executed).

Looking at the code I see something that looks like an oversight to me. This is the code that handles Exception in the named action (link):

try {
    // We update the latest value produced by the latest handler
    let this->_returnedValue = this->callActionMethod(handler, actionMethod, params);
} catch \Exception, e {
    if this->{"_handleException"}(e) === false {
        if this->_finished === false {
            continue;
        }
    } else {
        throw e;
    }
}

As you can see, an Exception is caught and _handleException is fired inside the dispatch loop, thus giving Dispatcher's forward() a chance to affect execution in subsequent iteration.

And this is how Controller's initialize is called (link):

if wasFresh === true {
    if method_exists(handler, "initialize") {
        handler->initialize();
    }
    ...
}

So, if an Exception is thrown in initialize, the error bubbles up and out of the dispatch loop.

So, shouldn't there be a try\catch around the initialize() call as well?

Thanks!

Details

  • Phalcon version: 3.2.x
  • PHP Version: (php -v) 7.1
  • Operating System: Unix
  • Installation type: Compiling from source
  • Zephir version (if any): Latest
@virgofx

This comment has been minimized.

Show comment
Hide comment
@virgofx

virgofx Jun 30, 2017

Member

I believe this is fixed with PR #12209 (not merged) .. just need to rebase it and resubmit it. It fixes a bunch of looping and performance issues as well.

Member

virgofx commented Jun 30, 2017

I believe this is fixed with PR #12209 (not merged) .. just need to rebase it and resubmit it. It fixes a bunch of looping and performance issues as well.

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Jul 5, 2017

Contributor

will you be doing that any time soon?

Contributor

temuri416 commented Jul 5, 2017

will you be doing that any time soon?

@virgofx

This comment has been minimized.

Show comment
Hide comment
@virgofx

virgofx Jul 5, 2017

Member

Yeah, it's on my todo list

Member

virgofx commented Jul 5, 2017

Yeah, it's on my todo list

@kowach

This comment has been minimized.

Show comment
Hide comment
@kowach

kowach Jul 14, 2017

I bumped on this bug too.

kowach commented Jul 14, 2017

I bumped on this bug too.

@NextSeason

This comment has been minimized.

Show comment
Hide comment
@NextSeason

NextSeason Aug 6, 2017

I hope you can fix this bug quickly, I can't continue doing my work because of this bug.

NextSeason commented Aug 6, 2017

I hope you can fix this bug quickly, I can't continue doing my work because of this bug.

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Sep 19, 2017

Contributor

@virgofx @sergeyklay

Any update on this ticket? Is there some other dependency?

Contributor

temuri416 commented Sep 19, 2017

@virgofx @sergeyklay

Any update on this ticket? Is there some other dependency?

virgofx added a commit to virgofx/cphalcon that referenced this issue Oct 7, 2017

Fixed Dispatcher::dispatch() to ensure proper flow for all event hand…
…ling, forwarding, exceptions, bubbling and performance improvements. Fixes phalcon#12931

virgofx added a commit to virgofx/cphalcon that referenced this issue Oct 7, 2017

Fixed Dispatcher::dispatch() to ensure proper flow for all event hand…
…ling, forwarding, exceptions, bubbling and performance improvements. Fixes phalcon#12931

virgofx added a commit to virgofx/cphalcon that referenced this issue Oct 7, 2017

Fixed Dispatcher::dispatch() to ensure proper flow for all event hand…
…ling, forwarding, exceptions, bubbling and performance improvements. Fixes phalcon#12931

virgofx added a commit to virgofx/cphalcon that referenced this issue Oct 7, 2017

Fixed Dispatcher::dispatch() to ensure proper flow for all event hand…
…ling, forwarding, exceptions, bubbling and performance improvements. Fixes phalcon#12931
@virgofx

This comment has been minimized.

Show comment
Hide comment
@virgofx

virgofx Oct 7, 2017

Member

@temuri416 Finally got around to this. Can you check:

PR - #13112

This issue -
https://github.com/virgofx/cphalcon/blob/80123d3628794ee937077c7a167c3c48730efcad/tests/unit/Mvc/Dispatcher/DispatcherInitalizeMethodTest.php#L180-L220
It should handle this specific use case.

In general, you can't use forward() directly in the initialize() method as this is essentially a constructor; however, you can forward as result of an exception caught via the dispatch:beforeException which is what this addresses.

Member

virgofx commented Oct 7, 2017

@temuri416 Finally got around to this. Can you check:

PR - #13112

This issue -
https://github.com/virgofx/cphalcon/blob/80123d3628794ee937077c7a167c3c48730efcad/tests/unit/Mvc/Dispatcher/DispatcherInitalizeMethodTest.php#L180-L220
It should handle this specific use case.

In general, you can't use forward() directly in the initialize() method as this is essentially a constructor; however, you can forward as result of an exception caught via the dispatch:beforeException which is what this addresses.

sergeyklay added a commit that referenced this issue Oct 9, 2017

Merge pull request #13112 from virgofx/feat/dispatcher
Fixed Dispatcher::dispatch() to ensure proper flow for all event handling, forwarding, exceptions, bubbling and performance improvements. Fixes #12931
@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Oct 12, 2017

Member

Fixed in the 3.2.x branch. Feel free to open a new issue if the problem appears again. Thank you for contributing.

Member

sergeyklay commented Oct 12, 2017

Fixed in the 3.2.x branch. Feel free to open a new issue if the problem appears again. Thank you for contributing.

@sergeyklay sergeyklay closed this Oct 12, 2017

chilimatic added a commit to chilimatic/cphalcon that referenced this issue Nov 2, 2017

Fixed Dispatcher::dispatch() to ensure proper flow for all event hand…
…ling, forwarding, exceptions, bubbling and performance improvements. Fixes phalcon#12931
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment