Skip to content

Conversation

@stesie
Copy link
Member

@stesie stesie commented Aug 23, 2015

This pull requests adds (optional) support to propagate PHP exceptions to JavaScript. This can be enabled by passing the FLAG_PROPAGATE_PHP_EXCEPTIONS flag to executeString method.

If set, thrown PHP exception objects are converted to JavaScript objects obeying normal conversion behaviour and thrown in JavaScript. This is all public method (and properties) can be used on the caught exception object from JavaScript side.

If JS doesn't catch the exception a V8JsException is thrown that points to the original PHP exception via (private) previous property, that can be got with getPrevious() method.

@tahpot, @rosmo looking forward to your review

this pull request fixes #144

@tahpot
Copy link

tahpot commented Aug 29, 2015

This is looking really good.

I did run into some confusion when intentionally calling a method with a missing argument. This raised a TypeError in Javascript which was not a built-in Exception object (PHP5.6) so my expectation of being able to call methods on a PHP Exception object (eg: getMessage()) did not work. Interestingly, doing this natively in PHP simply raises a warning and continues code execution.

I would also recommend documenting that err.name will give the type of PHP Exception thrown as I didn't realise that for a while.

Code snippet of raising a TypeError:

class MyObject {

    public function v8Exec($code, $data) {
        $v8 = new \V8Js("server");
        foreach ($data as $k => $v)
            $v8->$k = $v;

        return $v8->executeString($code, null, \V8Js::FLAG_PROPAGATE_PHP_EXCEPTIONS, 0, 0);
    }

    // test() expects one argument, but the JS code doesn't provide, causing a TypeError
    public function test($arg1) {
    }

}

$code = <<<EOT
    try {
        server.myObject.test()
    } catch (err) {
        print(err.getMessage());    // does not work
        //var_dump(err);
    }
EOT;

$obj = new MyObject();
$obj->v8Exec($code, ["myObject" => $obj]);

Executing the code produces a V8JsScriptException "TypeError: err.getMessage is not a function"

@stesie
Copy link
Member Author

stesie commented Sep 1, 2015

The TypeError behaviour is default behaviour of V8Js. It already is in place in master branch as long as I'm part of the project. And yes in that regard V8Js is stricter than normal PHP is, but I consider that a good thing in the "safe by default" sense

But I agree that the destinction between errors and exceptions can become confusing.
Contrary converting PHP exceptions to more Error-esque objects might be confusing as well, especially when it comes to custom PHP exceptions.

@tahpot
Copy link

tahpot commented Sep 2, 2015

V8Js being more strict with TypeError behaviour seems fine, might be good to document though.

On the whole, I think the handling as implemented in this request is a significant improvement and will be extremely useful as it currently stands.

I notice PHP 7 has TypeError as an Exception object which will resolve my specific example.

@stesie
Copy link
Member Author

stesie commented Sep 2, 2015

@tahpot well yes, but the TypeError object isn't equal to JavaScript TypeError object then ... or won't be until we add some glue, probably. The PHP objects tend to have dedicated getter methods named like getXXX where JS tends to have just the properties, maybe read-only

And you're right, there's much documentation to write.
Especially there's that whole official information stuff over at http://php.net/v8js ... very much out of date. Solely README has been updated for a while.

So I'll merge after

@tahpot
Copy link

tahpot commented Sep 7, 2015

FYI -- this pull request also fixes #122 which I recently encountered when using master.

stesie added a commit that referenced this pull request Sep 23, 2015
@stesie stesie merged commit 83f51e5 into phpv8:master Sep 23, 2015
@stesie stesie deleted the php-exception-behaviour branch December 31, 2015 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants