Cphalcon: [NFR][PHP 7]beforeThrowable, beforeError in dispatcher

[Jurigag]

In php 7 there was added Throwable interface. We need to in dispatcher and micro, add option to catch throwable(so errors too). Im thinking how to do this exactly, there is my idea:

• just add new event beforeThrowable and catch all Throwable, if there is some handler attached to this event then use it
• if there is no handler attached to this event then if it's exception we will just pass it to beforeException, so current code will keep working but throw errors only
• add beforeError event to catch only errors - so we don't need to change any code we have currently with beforeException and we can add code to catch Error only

So in result no code changes will be needed if we don't care about errors in php 7 or our application will not produce them. If we want add support for it we can either switch beforeException to beforeThrowable with current code or just add new attach for beforeError with new handler for it.

[virgofx]

Dont think this is necessary. Just update all exceptions to throwables when 4.0 is ready --- although this is way down the road.

[Jurigag]

But 4.0 will be like in few years and for now this means we don't have way to handle errors in framework, otherwise than set_error_handler in php.

[Jurigag]

So please change it to 3.1.0 maybe @sergeyklay because i want add this to 3.1.0

[sergeyklay]

@Jurigag Could you please explain a bit more. Provide pseudocode

[virgofx]

You can still handle errors manually Jurigag. Changing the framework will break backwards compatibility. This should be implemented in dispatcher as well as across the board for all pieces of code when Phalcon goes minimum of PHP7. I'm not sure what milestone this will be ... maybe a year out or so with v4 or v5.

Right now.. you can simply catch (Throwable $t) in your application code.

[Jurigag]

No, it won't break a framework.

$eventsManager->attach('dispatcher:beforeException', function(Event $event, Dispatcher $dispatcer, Exception $exception) {
 // handle exceptions only
});
$eventsManager->attach('dispatcher:beforeThrowable', function(Event $event, Dispatcher $dispatcer, Throwable $throwable) {
 // handle throwables only
});
$eventsManager->attach('dispatcher:beforError', function(Event $event, Dispatcher $dispatcer, Error $throwable) {
 // handle errors only
});

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

change to:

        try {
            let handler = this->_dispatch();
        } catch \Error, e {
            if this->{"_handleError"}(e) === false {
                return false;
            }

            throw e;
        } catch \Throwable, e {
            if this->{"_handleThrowable"}(e) === false { //if there is nothing attached to beforeThrowable and if it's exception we will redirect it to _handleException
                return false;
            }

            throw e;
        } catch \Exception, e {
            if this->{"_handleException"}(e) === false {
                return false;
            }

            throw e;
        }

Similar code(in php) is working without any breaking stuff both in php5 and php7. Just if class don't exist it's ignored. Not sure about zephir though - if it's the same then no problem, the problem will be if it can't compile. But will see, maybe some comment to detect php version if it will be problem ?

I would just create a pr and do tests to prove it would work on both php versions without any problem, but don't want to do it on 3.0.x and then deal with rebasing etc on 3.1.x.

And when we will support only php7 just remove beforeException and beforeError and just leave only beforeThrowable.

there you can check - http://sandbox.onlinephpfunctions.com/code/35ea592f8ed7f57da4bfa76a570f8393ac784bdf this code works both on php5 and php7 without any errors or issues.

[virgofx]

Juri, this is the wrong solution. Your solution hurts performance. Correct solution is to handle this everywhere in code without extra handle event checks, upgrading all exceptions to throwables. This can be done in subsequent PHP7 only milestone.

[Jurigag]

How it hurts performance ?

[virgofx]

These checks to handle instanceof should be done in application land ... not core land. I have benchmarks since I've already rewritten the dispatcher in my current branch because your latest updates to dispatcher have broken in on 3.x PHP5.6

[Jurigag]

http://sandbox.onlinephpfunctions.com/code/cd17232774263b56a486dadcde0445144a3bc547
Here you have benchmarks. Are we really gonna care about 0.0001 second in 10000 for loop ?

[virgofx]

It's also the wrong approach for solving this issue. It's a specific use-case in your situation, but it needs to be applied globally .. which will be handled later

[Jurigag]

Not it shouldn't. It's already handled in framework for exception so why not errors and throwables ? I don't quite understand.

How it's wrong approach ? Then why there is beforeException in first place ?

[virgofx]

So this issue can be closed and a new one to make Phalcon support PHP7 only can be created.

[Jurigag]

Because ? You wrote it ? As i wrote - it can be done without literally no performance degredation to support both php5 and php7. Only php7 will be after few years, so for few years no built-in option to handle errors in throwables in framework ? This is kind of bad.

[virgofx]

We're not going to mix in more if/else/instanceof checks to handle situations for specific versions in code Phalcon code. Not going to happen. Zephir -- we'll use macros. Not in Phalconland. We avoid this by minimum compatibility and upgrading the core application as necessary. When Phalcon drops php5 support, throwables will be supported then.

You can handle this manually now with no change to the application. So like I said, this can be closed.

[Jurigag]

But there will be no if/else/instanceof check for SPECIFIC VERSIONS.

[sergeyklay]

function(Event $event, Dispatcher $dispatcer, Throwable $throwable) {

will throw exception in zephir for php 5

[sergeyklay]

} catch \Throwable, e {

the same

[virgofx]

Well ... at a minimum it introduces ambiguity that should be handled globally since we handle exceptions everywhere.

[virgofx]

And the signature will change breaking BC so it should be a major release and this change should be applied across board for all exceptions like I said earlier.

[Jurigag]

function(Event $event, Dispatcher $dispatcer, Throwable $throwable) {

you will just not add this for php 5 anway because Throwable don't exist in php 5

} catch \Throwable, e {

Well, it's not my problem if zephir don't support this. Just in php you can do catch Throwable in php5, it will work without any problem

[sergeyklay]

Well, it's not my problem if zephir don't support this.

This is why this is 4.0.0 milestone

[Jurigag]

No, 4.0.0 milestone is this:

https://github.com/phalcon/cphalcon/issues/12288

Just only beforeThrowable. If we need change zephir to make it supporting both versions then we sure should do it surely.

[sergeyklay]

We cant implement beforeThrowable now. We cant use Throwable in Zephir for PHP 5

[Jurigag]

Yes we can - https://travis-ci.org/phalcon/cphalcon/builds/165241576 PHP 5.x is totally fine with this, php7 is seg faulting during tests, will check in home.

[sergeyklay]

Oh yes, as I can see - this problem was recently solved in Zephir project

[niden]

Closing in favor of #13855. Will revisit if the community votes for it, or in later versions.