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

[virgofx]

This is a an update of previously closed PR #12209. It resolves all dispatch issues, adds additional notes in the code for future deprecations in 4.0. Various performance improvements in the resulting dispatch process as result of variable caching and dropping the global try/catch which previously hooked into the RVAL for every statement previously.

Fixes current issue: #12931

Related to (and ensures fixes for) previous issues:
#12154
#11819

cc @andresgutierrez
cc @temuri416
cc @sergeyklay
cc @niden

Unit Tests (69) -------------------------------------------------------------------------------
✔ DispatcherAfterDispatchLoopTest: After dispatch loop forward (0.03s)
✔ DispatcherAfterDispatchLoopTest: After dispatch loop return false (0.00s)
✔ DispatcherAfterDispatchLoopTest: After dispatch loop with before exception returning false (0.00s)
✔ DispatcherAfterDispatchLoopTest: After dispatch loop with before exception bubble (0.00s)
✔ DispatcherAfterDispatchLoopTest: After dispatch loop with before exception forward once (0.00s)
✔ DispatcherAfterDispatchTest: After dispatch forward once (0.00s)
✔ DispatcherAfterDispatchTest: After dispatch return false (0.00s)
✔ DispatcherAfterDispatchTest: After dispatch with before exception returning false (0.00s)
✔ DispatcherAfterDispatchTest: After dispatch with before exception bubble (0.00s)
✔ DispatcherAfterDispatchTest: After dispatch with before exception forward once (0.00s)
✔ DispatcherAfterExecuteRouteMethodTest: After execute route forward once (0.00s)
✔ DispatcherAfterExecuteRouteMethodTest: After execute route return false (0.00s)
✔ DispatcherAfterExecuteRouteMethodTest: After execute route with before exception returning false (0.00s)
✔ DispatcherAfterExecuteRouteMethodTest: After execute route with before exception bubble (0.00s)
✔ DispatcherAfterExecuteRouteMethodTest: After execute route with before exception forward once (0.00s)
✔ DispatcherAfterExecuteRouteTest: After execute route forward once (0.00s)
✔ DispatcherAfterExecuteRouteTest: After execute route return false (0.00s)
✔ DispatcherAfterExecuteRouteTest: After execute route with before exception returning false (0.00s)
✔ DispatcherAfterExecuteRouteTest: After execute route with before exception bubble (0.00s)
✔ DispatcherAfterExecuteRouteTest: After execute route with before exception forward once (0.00s)
✔ DispatcherAfterInitializeTest: After initialize forward once (0.00s)
✔ DispatcherAfterInitializeTest: After initialize return false (0.00s)
✔ DispatcherAfterInitializeTest: After initialize with before exception returning false (0.00s)
✔ DispatcherAfterInitializeTest: After initialize with before exception bubble (0.00s)
✔ DispatcherAfterInitializeTest: After initialize with before exception forward once (0.00s)
✔ DispatcherBeforeDispatchLoopTest: Before dispatch loop forward (0.00s)
✔ DispatcherBeforeDispatchLoopTest: Before dispatch loop return false (0.00s)
✔ DispatcherBeforeDispatchLoopTest: Before dispatch loop with before exception returning false (0.00s)
✔ DispatcherBeforeDispatchLoopTest: Before dispatch loop with before exception bubble (0.00s)
✔ DispatcherBeforeDispatchLoopTest: Before dispatch loop with before exception forward (0.00s)
✔ DispatcherBeforeDispatchTest: Before dispatch forward once (0.00s)
✔ DispatcherBeforeDispatchTest: Before dispatch return false (0.00s)
✔ DispatcherBeforeDispatchTest: Before dispatch with before exception returning false (0.00s)
✔ DispatcherBeforeDispatchTest: Before dispatch with before exception bubble (0.00s)
✔ DispatcherBeforeDispatchTest: Before dispatch with before exception forward once (0.00s)
✔ DispatcherBeforeExecuteRouteMethodTest: Before execute route forward once (0.00s)
✔ DispatcherBeforeExecuteRouteMethodTest: Before execute route return false (0.00s)
✔ DispatcherBeforeExecuteRouteMethodTest: Before execute route with before exception returning false (0.00s)
✔ DispatcherBeforeExecuteRouteMethodTest: Before execute route with before exception bubble (0.00s)
✔ DispatcherBeforeExecuteRouteMethodTest: Before execute route with before exception forward (0.00s)
✔ DispatcherBeforeExecuteRouteTest: Before execute route forward once (0.00s)
✔ DispatcherBeforeExecuteRouteTest: Before execute route return false (0.00s)
✔ DispatcherBeforeExecuteRouteTest: Before execute route with before exception returning false (0.00s)
✔ DispatcherBeforeExecuteRouteTest: Before execute route with before exception bubble (0.00s)
✔ DispatcherBeforeExecuteRouteTest: Before execute route with before exception forward once (0.00s)
✔ DispatcherInitalizeMethodTest: Initialize forward (0.00s)
✔ DispatcherInitalizeMethodTest: Initialize return false (0.00s)
✔ DispatcherInitalizeMethodTest: Initialize with before exception returning false (0.00s)
✔ DispatcherInitalizeMethodTest: Initialize with before exception bubble (0.00s)
✔ DispatcherInitalizeMethodTest: Initialize with before exception forward (0.00s)
✔ DispatcherTest: Default dispatch loop events (0.00s)
✔ DispatcherTest: Default dispatch loop events with no handlers (0.00s)
✔ DispatcherTest: Controller action local forward (0.00s)
✔ DispatcherTest: Controller action external forward (0.00s)
✔ DispatcherTest: Controller action return value string (0.00s)
✔ DispatcherTest: Controller action return value int (0.00s)
✔ DispatcherTest: Params and return value (0.00s)
✔ DispatcherTest: Cyclical routing (0.00s)
✔ DispatcherTest: Handler not found (0.05s)
✔ DispatcherTest: Handler invalid (0.00s)
✔ DispatcherTest: Handler action not found (0.00s)
✔ DispatcherTest: Last handler (0.00s)
✔ DispatcherTest: Last handler forward (0.00s)
✔ DispatcherTest: No namespaces (0.00s)
✔ DispatcherTest: Mixing namespace forward (0.00s)
✔ DispatcherTest: Default resolve (0.02s)
✔ DispatcherTest: Manual call action (0.00s)
✔ DispatcherTest: Last handler exception forward (0.00s)
✔ DispatcherTest: Exception in before exception (0.00s)
-------------------------------------------------------------------------------------------

Time: 2.23 seconds, Memory: 12.00MB

OK (69 tests, 173 assertions)

[sergeyklay]

Good job. Could you deal with failed test
https://github.com/phalcon/cphalcon/blob/master/unit-tests/DispatcherMvcEventsTest.php#L206-L209

There was 1 failure:

1. DispatcherMvcEventsTest::testEvents
   Failed asserting that two strings are equal.

--- Expected
+++ Actual
@@ @@
-'beforeDispatch-beforeExecuteRoute-afterExecuteRoute-afterDispatch'
+'beforeDispatch-beforeExecuteRoute-afterInitialize-afterExecuteRoute-afterDispatch'

[virgofx]

For clarity — the test expected value is marked in green and the new behavior is in red (Wording backwards).

For reference — The failed test was prevalent on the first checkin in the old unit-tests suite. Since everything is moving over into tests\unit, I subsequently removed the old test which is covered completely in entirety in the new suite and thus it passes with behavior as noted in [A] below.

---

The reason the (now removed) old unit test failed was because of a nuance in the old test. Specifically, it did not contain an initialize() method in the controller used in the old test case and thus it should never fire the dispatch:afterInitialize event. I can see this both ways. The change was made to interpret the following logic:

• An event should be thrown right before/after the corresponding action is about to happen. E.g. in the case of the view, if it's disabled the view events are not thrown.

My original thought is that the existing behavior is incorrect; however, I could be compelled to add a new test case to revert to the old behavior if you have some insight. E.g. If the use case was to somehow globally instantiate controllers with shared functionality via the dispatch:afterInitialize method for all controllers with or without an initialize() method.

In the case of the dispatcher, it depends how you read the afterInitialize event. Do you:

A) interpret initialize as being specific to initializing via the initialize() method ... or

• Current/new unit test suite interpretation

B) refer to it as being general such that the event should be fired in it's relative location even without a direct call into the initialize() - e.g. The controller has been loaded up and is "ready to go". Think of this event as being named differently — dispatch:afterLoadedFirstTime. For B/C we would have to keep the same name dispatch:afterInitialize; however, I would update the code and docs to convey the new meaning for this event.

• Old DispatcherMvcEventsTest.php case

---

Side note --- there is a separate note in the code. The whole [ initialize/afterInitialize ] block is in the wrong location. The beforeExecuteRoute block should never be executed before the initialize block. This was a bug in the original code/documentation that I left it in place currently to not break B/C; however, if you argue with the logic of [B] above ... you may want to also think about moving this block ahead of the beforeExecuteRoute which is slated to be done in the next major 4.0 release regardless --- if you now think of initialize in the [B] sense ... potentially consider making this change now as well (which again, would break B/C -- but is the correct behavior) Or we can leave as is ... and update in 4.0 -- In fact I can submit the 4.0 version to the 4.0 branch right now if you'd prefer.

---

The issue here is the conflicting naming with the initialize() method and afterInitialize event. If the event were named something different, I would have been 100% in favor of [B]. However, after thinking o the global use case I mentioned above I could see someone using this in that fashion? Perhaps we add it back in and then in 4.0 switch the order as noted above?

[sergeyklay]

Yes, the test expected value is marked in green and the new behavior is in red . I just copied phpunit output from the terminal. Well, could you just remove wrong test?

[virgofx]

@sergeyklay I updated original comment with more info and questions. It is removed as noted now (hence passing); however, do you have thoughts on [A] vs. [B] above? I'm open to reverting back to the [B] model as I could see there be a global use case for performing actions from an eventhook globally in controllers regardless of the presence of initialize()

[sergeyklay]

Actually I prefer [B] but in general your PR looks really great. I will keep this open for a day or two to allow other contributors a chance to speak up, and to take a fresh look at it later. But at the moment it seems good to me.

cc: @phalcon/core-team

[niden]

This looks great. I would also prefer [B].

Great work buddy, thank you so much for the contribution!

[virgofx]

Modified to include option [B] above. This means:

1. Patch is 100% backwards compatible (with proper bugfixes only)
2. The legacy unit-tests that originally failed .... now passes (even though it doesn't exist anymore). Replaced by the new suite and specifically one of the core dispatcher tests: https://github.com/virgofx/cphalcon/blob/da5f3e0736263ed8eba7505c11c644286f90aee3/tests/unit/Mvc/Dispatcher/DispatcherTest.php#L93-L104
3. A few documentation updates forthcoming to include some things that are not able to fix in this version. (For example returning false does not work when there exist more than one event handler/listener - Only the last return value is preserved. The only workaround is to stop the Event. I need to outline how to do this in the documentation for 3rd party applications/libraries that may publish plugins that hook into events and need to stop dispatch actions. The only sure fire way would be to return false AND stop the event). This could be potentially reworked for 4.0.
4. Add a 4.0 version with corresponding changes

[niden]

For 3.x we should keep things the way they are. If I need events to stop (subsequent listeners do not fire) we will need to do $event->stop() and return false. This has to be documented for 3.x

For 4.x this behavior needs to change. Simply return false should suffice to stop the execution of subsequent listeners to the same event.

The above will be OK behavior so long as we ensure that we highlight it in the documentation.

[niden]

This looks good to me @sergeyklay whenever you get a chance hit the magic button

[sergeyklay]

Thank you!

[virgofx]

@sergeyklay What's the 4.0 plan? Is 3.2.x getting merged up to 4.0 currently? Should I submit a new PR against the current 4.0 branch? or Should I wait until you've merged 3.2.x/master back down into 4.0?

[sergeyklay]

We're plan to release 3.2.3 in next days and start 3.3.0 branch. You can cherry-pick changes from 3.2.x to 4.0.x branch if you want or leave this job for me. I'll do this later (but can't say when exactly).

[virgofx]

Doesn't matter to me ... I can just help with the 4.0 changes related to this feature. Is the 4.0 branch working/building and is it PHP7.x only ? What would you prefer I do ?

[sergeyklay]

Yes, we'll drop support of the Zend Engine 2.x from Phalcon 4.x.

[temuri416]

@virgofx @sergeyklay

It looks like this patch needs a small review.

It broke an existing behaviour: after forwarding to an action within the same controller - its initialize() method does not get called again after forward.

However, when forwarding to a different controller, controller's initialize() does get invoked again.

<?php

use Phalcon\Mvc\Controller;

class IndexController extends Controller
{
    public function initialize()
    {
        $this->view->location = $this->di->get('dispatcher')->getHandlerClass();
        $this->view->action = $this->di->get('dispatcher')->getActionName();
    }

    public function indexAction()
    {
        /**
         * Before #13112 this forward will result in "Your location: IndexController/welcome".
         *
         * After #13112 this forward will result in "Your location: IndexController/index".
         *
         * @var IndexController
         */
        $this->di['dispatcher']->forward([
            'controller' => 'index',
            'action' => 'welcome',
        ]);
    }

    public function welcomeAction()
    {
    }
}

This line seems to be the key: https://github.com/phalcon/cphalcon/blob/v3.2.3/phalcon/dispatcher.zep#L520.

Current controller is no longer considered a "fresh" instance.

So, at the moment, a workaround for this problem is:

$eventsManager->attach('dispatch:beforeForward', function(Phalcon\Events\Event $event, Dispatcher $dispatcher) {
    $handlerClass = $dispatcher->getHandlerClass();
    $dispatcher->getDi()->remove($handlerClass);
});

Code to reproduce is attached.
issue-13112.zip

Thanks!

[sergeyklay]

@virgofx Could you please take a look

[virgofx]

@temuri416 That's the correct behavior. The initialize() is only called ONCE ever per controller. It's more of a static onConstruct(). It was designed to parallel how other areas of Phalcon code do initialization - e.g. Models

It's not great from a wording (which I updated in code) and perhaps the out-of-order issue is causing confusion? See note below (will be fixed in next version)

Also .. this is how things were in the past, the patch didn't explicitly modify that behavior (although there were other bugs that may have blocked the intended behavior and thus it may appear different).

Important:

1. Next version WILL move the initialize() logic BEFORE the beforeExecuteRoute() handlers.

As for the issue your experiencing, I believe the initialize() is correct in that it should only be executed once per Dispatcher/Di setup. It's a one-time initialization --- stuff like doing ACL for controllers is where this should happen. If you need something to happen EVERY time in some sort of cyclical cycle -- that should happen in the beforeExecuteRoute

@temuri416 Let me know if you have issues moving your code into beforeExecuteRoute. The code you listed above should be in beforeExecuteRoute and NOT initialize. The sample workaround WORKS; however, is silly and unnecessary. Just move logic into correct location which makes more sense.

@sergeyklay With regards to Fix #1 above (and noted in above comments and inside code) .. do you want fix submitted into 3.3 branch? Or waiting until 4.0. Also, Which is the branch that is going to drop all the mixed types and go strict types (e.g. function (array $forward)) which will prevent a ton of is object checks everywhere?

[temuri416]

ok, no big deal. I can move my logic to beforeExecuteRoute.

[sergeyklay]

@virgofx These changes already in 3.3.x branch. I'll deal with 4.0.0 a bit later