Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Continue running the go program after exit() in PHP #59

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

thekid
Copy link
Contributor

@thekid thekid commented Oct 1, 2018

Currently, if a PHP script calls exit(), not only does the script terminate, but also the GO binary running it - see #31. This pull request changes this in a backwards-compatible way* such that calls to Exec() and Eval() will return an error which can be checked for.

Example

val, err := context.Eval(script);

if err != nil {
    if exit, ok := err.(*php.ExitError); ok {
        fmt.Printf("PHP exited with exit status %d\n", exit.Status)
    } else {
        fmt.Printf("Could not execute script %s: %v", script, err)
    }
}

*: ...instead of changing the return types, or introducing any global state in context, or adding new methods. The API is modeled a bit after os/exec

@@ -19,6 +19,7 @@ const char engine_ini_defaults[] = {
"expose_php = 0\n"
"default_mimetype =\n"
"html_errors = 0\n"
"error_reporting = E_ALL\n"
Copy link
Contributor Author

@thekid thekid Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm, this didn't have the desired effect of fixing the tests; unsure what is causing them to fail on Travis CI, then. However, these test failures have been around for a while looking at other pull requests.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there have been some flaky tests that need fixing. I'll get on it.

@deuill
Copy link
Owner

deuill commented Oct 7, 2018

Apologies for the delay in responding to these PRs, and thanks for doing the work. Looking now.

Copy link
Owner

@deuill deuill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a general level, I'm wondering if we should be storing more complex references to errors at the engine_context struct level, and have have introspected and returned in calls to Exec or Eval. Not bailing out when an error occurs is definitely desirable, but we may want to get more granular errors if we can?

@@ -48,9 +48,11 @@ void context_exec(engine_context *context, char *filename) {
script.opened_path = NULL;
script.free_filename = 0;

ret = php_execute_script(&script);
ret = zend_execute_scripts(ZEND_REQUIRE, NULL, 1, &script);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see php_execute_script does a bunch of other stuff but ends up calling zend_execute_scripts. Was this changed because php_execute_script will truncate EG(exit_status)?

_context_eval(op, &tmp, exit);

// Script called exit()
if (*exit != -1) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it may be possible to skip changing the signature of the version-specific functions (_context_eval and _context_exec) and just use EG(exit_status) here, since nothing else should be using the same context concurrently.

@@ -25,10 +25,9 @@ static void _context_eval(zend_op_array *op, zval *ret) {

zend_try {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be zend_first_try too (my bad)?

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