[NFR][PHP 7]beforeThrowable, beforeError in dispatcher #12289

Open
Jurigag opened this Issue Oct 4, 2016 · 26 comments

Projects

None yet

3 participants

@Jurigag
Contributor
Jurigag commented Oct 4, 2016 edited

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.

@Jurigag Jurigag changed the title from [NFR][PHP 7]beforeThrowable,beforError in dispatcher to [NFR][PHP 7]beforeThrowable, beforError in dispatcher Oct 4, 2016
@Jurigag Jurigag changed the title from [NFR][PHP 7]beforeThrowable, beforError in dispatcher to [NFR][PHP 7]beforeThrowable, beforeError in dispatcher Oct 4, 2016
@virgofx
Contributor
virgofx commented Oct 4, 2016

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

@sergeyklay sergeyklay added this to the 4.0.0 milestone Oct 4, 2016
@Jurigag
Contributor
Jurigag commented Oct 4, 2016 edited

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
Contributor
Jurigag commented Oct 5, 2016

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

@sergeyklay
Collaborator

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

@virgofx
Contributor
virgofx commented Oct 5, 2016

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
Contributor
Jurigag commented Oct 5, 2016 edited

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
Contributor
virgofx commented Oct 5, 2016 edited

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
Contributor
Jurigag commented Oct 5, 2016

How it hurts performance ?

@virgofx
Contributor
virgofx commented Oct 5, 2016

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
Contributor
Jurigag commented Oct 5, 2016 edited

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

@virgofx
Contributor
virgofx commented Oct 5, 2016 edited

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
Contributor
Jurigag commented Oct 5, 2016 edited

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
Contributor
virgofx commented Oct 5, 2016

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

@Jurigag
Contributor
Jurigag commented Oct 5, 2016 edited

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
Contributor
virgofx commented Oct 5, 2016

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
Contributor
Jurigag commented Oct 5, 2016 edited

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

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

will throw exception in zephir for php 5

@sergeyklay
Collaborator
} catch \Throwable, e {

the same

@virgofx
Contributor
virgofx commented Oct 5, 2016

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

@virgofx
Contributor
virgofx commented Oct 5, 2016

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
Contributor
Jurigag commented Oct 5, 2016 edited
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
Collaborator

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

This is why this is 4.0.0 milestone

@Jurigag
Contributor
Jurigag commented Oct 5, 2016 edited

No, 4.0.0 milestone is this:

#12288

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

@sergeyklay
Collaborator
sergeyklay commented Oct 5, 2016 edited

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

@Jurigag
Contributor
Jurigag commented Oct 5, 2016 edited

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
Collaborator
sergeyklay commented Oct 5, 2016 edited

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

@sergeyklay sergeyklay added the RFC label Oct 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment