Cphalcon: \Phalcon\Debug::onUncaughtException() accepts \Exception instead of PHP 7's \Throwable

[frickenate]

Expected and Actual Behavior

Describe what you are trying to achieve and what goes wrong.

Advanced tldr:
I understand that my use case is odd, but under PHP 7 set_exception_handler() handlers are required to accept \Throwable, not \Exception (see the PHP docs). \Phalcon\Debug::onUncaughtException() however, is type-hinted to accept \Exception, not \Throwable -- which includes core \Error class and its children. My case below is more convoluted, but in theory I should also have been able to extend \Phalcon\Debug and override onUncaughtException() to accept \Throwable, but this doesn't work as an extending method prototype must match the parent class's signature.

Now, my example:
I want to modify the output rendered by the \Phalcon\Debug class on exceptions/errors, in order to inject the developer toolbar I am using. \Phalcon\Debug::onUncaughtException() is simply echoing the rendered html, so instead of throwing the exception/error for the handler to catch, I am using ob_start(), calling onUncaughtException() directly with the \Exception or \Error object, and then injecting my own html into what I get back from ob_get_clean().

This _does work_ - almost, with a code-breaking gotcha. Calling onUncaughtException() directly with an \Exception object works as expected, however trying to pass an \Error object throws this error:

TypeError: Argument 1 passed to
Phalcon\Debug::onUncaughtException() must be an
instance of Exception, instance of Error given

This is due to the fact that onUncaughtException() is type-hinted to only accept \Exception, rather than PHP 7's \Throwable interface (see its prototype signature), which both \Exception and \Error base classes implement. This is especially odd because if you throw(new \Error()), onUncaughtException() is somehow catching it!

Provide minimal script to reproduce the issue

class App extends \Phalcon\Mvc\Application {
    public function run() {
        $debug = (new \Phalcon\Debug())->listenLowSeverity();

        try {
            // alternate these 2 lines to test an \Exception vs. an \Error

            // throw new \Exception('test Exception class');
            throw new \Error('test Error class');
        }
        // uncaught throwables - extending from both \Exception and \Error
        catch (\Throwable $e) {
            // if we throw, \Phalcon\Debug successfully catches both \Exception and \Error
            // throw $e;

            // instead of throwing to \Phalcon\Debug handler, we call its onUncaughtException()
            // method directly so we can modify its html before rendering. double ob_start()
            // as onUncaughtException() calls ob_end_clean() on first level of buffering.
            // thankfully it doesn't loop to close all buffers, so this workaround works.
            ob_start();
            ob_start();
            $debug->onUncaughtException($e);

            // inject the developer toolbar (static fake example)
            $this->response->setContent(preg_replace(
                '!(?=</body>\s*</html>\s*\z)!i',
                '<div style="position: fixed; bottom: 0; width: 100%; background: #f00; padding: 30px 0;">' .
                    '<strong>DEV TOOLBAR GOES HERE</strong>' .
                '</div>',
                ob_get_clean()
            ));

            $this->response->send();
        }
    }
}

(new App(new \Phalcon\Di\FactoryDefault()))
    ->run();

Details

• Phalcon version: 3.0.0
• PHP Version: 7.0.8-0ubuntu0.16.04.2
• Operating System: Ubuntu 16.04.1 LTS
• Installation type: Compiling from source
• Zephir version (if any): 0.9.3a-dev-e716dbe641
• Server: Nginx

[tembem]

I don't understand.

I read http://php.net/manual/en/class.throwable.php. PHP classes cannot implement the Throwable interface directly, and must instead extend Exception. Why not catch Exception instead?

[andrew-demb]

Phalcon 3.0 still supports PHP 5.5, where is no \Throwable interface.
So, you can't change type for input param in this method

[frickenate]

@tembem The point is that set_exception_handler() in PHP 5.x was only designed to handle the only existing "exception"-like base class: \Exception. In PHP 7, they split what we consider "exceptions" into two separate base classes - \Exception and \Error. Both of these classes implement \Throwable, and set_exception_handler() is expected to receive anything that implements \Throwable. Catching only \Exception is not supposed to be a choice; it's simply wrong and non-compliant in PHP 7.

@andrew-demb I was under the impression that there was a significant rewrite of Zephir to make PHP 7 support happen. It does look like the one repo handles PHP 5.x and 7 extensions in a side-by-side build environment. Is is not possible to macro out the argument accepted by a Zephir internal method? PHP 5.x should be \Exception, and PHP 7 should be \Throwable.

More details

I just managed to compile phalcon source files for my first time (took a while to Google for the zephir build setup). It's _technically_ enough to simply remove the type-hint (change public function onUncaughtException(<\Exception> exception) -> boolean to public function onUncaughtException(exception) -> boolean. A set_exception_handler() will never result in a bad object being passed in, but of course the top of the method should be hardened to verify that the passed exception argument is in fact an object, that is zephir_is_instance_of() of \Exception or \Throwable (no need to check for zephir_interface_exists() first).

I just spent 30 minutes trying to code a basic if {} block to do this myself so I could submit a pull request. Unfortunately, as a complete newcomer to Zephir (let alone Zend internals), I can't even get a basic line of code to pass the parsing of the file during build. Tried to mimic other calls to zephir_*() functions present in ext/kernel/object.c, but the basics of defining and passing a variable to a function (var, let, zval... oh my) eludes me.

The "good news" is that this is the only occurrence of set_exception_handler() usage in the entire codebase, so fixing this one location is all it will take.

[andresgutierrez]

Remove the type hint from the parameter is a good idea.

[loloajax]

Totally agree with @frickenate. As long as the type hint is there, we can't use \Phalcon\debug::onUncaughtException() correctly.

The problem is to cast \Throwable into \Exception which is not allowed in PHP. The only way is to declare a new \Exception object with all previous args :

// $exception is a Throwable object
$new = new \ErrorException($exception->getMessage(), $exception->getCode(), 1, $exception->getFile(), $exception->getLine());

and then call \Phalcon\Debug::onUncaughtException($new);

But, we completely lose the stacktrace of the error because it's not possible to redeclare \ErrorException::setTrace()

4.x release doesn't seem to take in account this issue, which is worrying...

[sergeyklay]

Lets fix this in 3.3.x branch guys

[someson]

3.3.0 is out. Bug not fixed, maaaaan!

[sergeyklay]

@someson Phalcon 3.x still supports PHP 5.x, where is no \Throwable interface. Do you have an idea how to fix this without breaking backward compatibility?

[RickyGit]

if(is_a($e, 'Error')) {
    $e = new \ErrorException($e->getMessage(), $e->getCode(), 1, $e->getFile(), $e->getLine());
}

?

[stale[bot]]

Thank you for contributing to this issue. As it has been 90 days since the last activity, we are automatically closing the issue. This is often because the request was already solved in some way and it just wasn't updated or it's no longer applicable. If that's not the case, please feel free to either reopen this issue or open a new one. We will be more than happy to look at it again! You can read more here: https://blog.phalconphp.com/post/github-closing-old-issues

[someson]

As a workaround (polyfill) maybe can be helpful for someone:

namespace Your\Namespace;

class Debug extends \Phalcon\Debug
{
    /** @var \Exception */
    private $_e;

    public function __construct(\Throwable $e)
    {
        try {
            if ($e instanceof \Error) {
                // losing the trace :(
                throw new \ErrorException(
                    $e->getMessage(), $e->getCode(), 1, $e->getFile(), $e->getLine()
                );
            }
            throw new \Exception($e->getMessage(), $e->getCode(), $e);
        } catch (\ErrorException $e) {
            $this->_e = $e;
        } catch (\Exception $e) {
            $this->_e = $e->getPrevious();
        }
    }

    public function onUncaughtException(\Exception $e = null): bool
    {
        return parent::onUncaughtException($this->_e);
    }
}

and by using instead of:

// $e can be only type of \Exception
return (new \Phalcon\Debug())->onUncaughtException($e);

insert this:

// $e can be a \Throwable|\Error
return (new \Your\Namespace\Debug($e))->listen($exceptions = true, $errors = true);

[adhocore]

since php 7.2 onwards support type widening, you can instead use this:

namespace Your;

class Debug extends \Phalcon\Debug
{
    public function onUncaughtException($e): bool
    {
        if ($e instanceof \Error) {
            $e = new \ErrorException(
                $e->getMessage(), $e->getCode(), \E_ERROR, $e->getFile(), $e->getLine()
            );
        }

        return parent::onUncaughtException($e);
    }
}