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

Incorrect 'Command unexpectedly terminated' error message when proc_get_status is disabled via disable_functions #53

Closed
maxguru opened this issue Oct 19, 2020 · 4 comments

Comments

@maxguru
Copy link

maxguru commented Oct 19, 2020

Hi,

I get a PHP warning that proc_get_status() has been disabled for security reasons on this one system. Apparently, proc_open is allowed, but not proc_get_status. I looked at the code and it looks like the call to proc_get_status was added to solve issue #20.

The thing is, I believe if proc_get_status is disabled, you get the Command unexpectedly terminated without error message even when the command is still running. I suggest changing the code at src/Command.php:431 to check if proc_get_status() is usable and/or check the contents of $status before using it and doing something else if the contents of $status are invalid.

                    while ($isRunning) {
                        $status = @proc_get_status($process);
                        if(!is_array($status))
                        // something...

I am sure there is a better solution that can be used. Maybe seeing if pipes are still open or setting useExecto true in such cases. Maybe useExec can be automatically set based on some checks?

Any thoughts?

@mikehaertl
Copy link
Owner

See #50.

I don't want to add checks like this:

  • The error message already tells you what's wrong
  • By default this function is enabled in the PHP core and IMO it's overkill if libraries start to test for all kinds of functions that could be disabled somewhere.
  • Using useExec automatically has other drawbacks (reduced error reporting) and it would just hide what is the real problem

@maxguru
Copy link
Author

maxguru commented Oct 19, 2020

I respect your decision, however, I have a differing opinion.

At least some users of the library will need this functionality (such as myself) and they all will need to re-implement this functionality outside of the library code and keep it updated as the library code changes. I would say that is an impetus for adding it to the library. It doesn't have to be on by default. It might be a simple function that would check compatibility and set useExec when necessary. If you are interested, I can create a pull request.

Thanks for all the great work!

@mikehaertl
Copy link
Owner

I still disagree. We can't check for each and every thinkable PHP configuration and try to fix things. This is way out of the scope of this library and it will be hard to maintain this kind of checks over time (people will come up with all kinds of weird server configs and ask to fix things - when in fact it's their responsibility as a developer to ensure a well configured prod environment).

@maxguru
Copy link
Author

maxguru commented Oct 20, 2020

I don't know if I buy that argument. Sometimes the developer doesn't control the production environment and would have to programmatically ensure it. Also, the developer that uses the library doesn't always know all these little details without doing an in-depth library code review. It is far easier for the developer of the library itself to know what is required of the environment. But this is up to you, thanks for looking into it.

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

No branches or pull requests

2 participants