From bd065eab326e3eb2d51e77005eb7b01def205cec Mon Sep 17 00:00:00 2001 From: Jonas Kattendick Date: Sat, 14 Feb 2026 20:49:08 +0100 Subject: [PATCH] fix!: handle errors properly --- src/Child.php | 48 ++++++++++++++++++++++++++++------------ src/ChildException.php | 22 ++++++++++++++++-- src/Command.php | 15 ++++++------- src/CommandException.php | 17 +++++--------- src/ExitStatus.php | 4 +++- src/StreamException.php | 36 ++++++++++++++++++++++++++++-- 6 files changed, 103 insertions(+), 39 deletions(-) diff --git a/src/Child.php b/src/Child.php index a871141..9f248be 100644 --- a/src/Child.php +++ b/src/Child.php @@ -4,6 +4,8 @@ declare(strict_types=1); namespace Nih\CommandBuilder; +use InvalidArgumentException; + /** * Representation of a running or exited child process. */ @@ -26,6 +28,11 @@ final class Child */ private ?ExitStatus $status = null; + /** + * The child's output. + */ + private ?Output $output = null; + /** * The handle for writing to the child’s standard input (stdin), if it has * been captured. @@ -45,13 +52,17 @@ final class Child public readonly ?ChildStderr $stderr; /** - * @param resource $process The child's process handle. - * @param array $pipes File pointers. + * @param resource $process The child's process resource handle. + * @param array $pipes + * The indexed array of file pointers that set by {@see \proc_open()}. + * + * @throws InvalidArgumentException + * When the given process is not a process resource. */ public function __construct(mixed $process, array $pipes) { if (get_resource_type($process) !== 'process') { - throw new ChildException('invalid resource type: not a process'); + throw new InvalidArgumentException('resource is not a process'); } $this->process = $process; @@ -89,8 +100,6 @@ final class Child * waiting. This helps avoid deadlock: it ensures that the child does not * block waiting for input from the parent, while the parent waits for the * child to exit. - * - * @throws ChildException If the resource was already closed */ public function wait(): ExitStatus { @@ -111,7 +120,7 @@ final class Child proc_close($this->process); return $this->status = new ExitStatus( - $status['exitcode'], + $status['exitcode'] < 0 ? null : $status['exitcode'], $status['signaled'] ? $status['termsig'] : null, $status['stopped'] ? $status['stopsig'] : null, ); @@ -120,6 +129,8 @@ final class Child /** * Simultaneously waits for the child to exit and collect all remaining * output on the stdout/stderr handles, returning an {@see Output} instance. + * This function will continue to have the same return value after it has + * been called at least once. * * The stdin handle to the child process, if any, will be closed before * waiting. This helps avoid deadlock: it ensures that the child does not @@ -131,12 +142,13 @@ final class Child * parent and child. Use the `stdout` and `stderr` functions of {@see * Command}, respectively. * - * @throws ChildException If the resource was already closed + * @throws ChildException + * @throws StreamException */ public function waitWithOutput(): Output { - if (!is_resource($this->process)) { - throw new ChildException('Resource was already closed'); + if ($this->output) { + return $this->output; } // Avoid possible deadlock before waiting. @@ -146,21 +158,29 @@ final class Child $stderr = $this->stderr?->getContents(); $status = $this->wait(); - return new Output($stdout, $stderr, $status); + return $this->output = new Output($stdout, $stderr, $status); } /** * Forces the child process to exit. * * This is equivalent to sending a SIGKILL. + * + * @throws ChildException */ - public function kill(): bool + public function kill(): void { - if (!is_resource($this->process)) { - return true; + if ($this->status) { + return; } - return proc_terminate($this->process, 9); + try { + set_error_handler(ChildException::handleError(...)); + $success = proc_terminate($this->process, 9); + assert($success); + } finally { + restore_error_handler(); + } } public function __destruct() diff --git a/src/ChildException.php b/src/ChildException.php index 5bc1bc9..bced9c6 100644 --- a/src/ChildException.php +++ b/src/ChildException.php @@ -4,5 +4,23 @@ declare(strict_types=1); namespace Nih\CommandBuilder; -class ChildException extends CommandException -{} +/** + * A PHP error occured during an operation on a process resource. + */ +final class ChildException extends CommandException +{ + public static function handleError( + int $severity, + string $message, + string $filename, + int $line, + ): never { + throw new self( + message: $message, + code: 0, + severity: $severity, + filename: $filename, + line: $line, + ); + } +} diff --git a/src/Command.php b/src/Command.php index 390164e..ff564c9 100644 --- a/src/Command.php +++ b/src/Command.php @@ -278,6 +278,8 @@ final class Command implements Stringable * Executes the command as a child process, returning a handle to it. * * By default, stdin, stdout and stderr are inherited from the parent. + * + * @throws ChildException */ public function spawn(): Child { @@ -299,6 +301,8 @@ final class Command implements Stringable * collecting its status. * * By default, stdin, stdout and stderr are inherited from the parent. + * + * @throws ChildException */ public function status(): ExitStatus { @@ -311,6 +315,8 @@ final class Command implements Stringable * * By default, stdout and stderr are captured (and used to provide the * resulting output). Stdin is not inherited from the parent. + * + * @throws ChildException */ public function output(): Output { @@ -396,13 +402,6 @@ final class Command implements Stringable } } - if (!is_executable($program)) { - throw new CommandException(sprintf( - 'Program "%s" is not executable', - $program, - )); - } - // Handle environment inheritance. $environment = $this->environment; if (is_array($environment) && $this->environmentInherit) { @@ -414,7 +413,7 @@ final class Command implements Stringable } try { - set_error_handler(CommandException::handleError(...)); + set_error_handler(ChildException::handleError(...)); $proc = proc_open( [$program, ...$this->args], $descriptorSpec, diff --git a/src/CommandException.php b/src/CommandException.php index 26ebeb6..3de4ad0 100644 --- a/src/CommandException.php +++ b/src/CommandException.php @@ -4,16 +4,9 @@ declare(strict_types=1); namespace Nih\CommandBuilder; -use RuntimeException; +use ErrorException; -class CommandException extends RuntimeException -{ - public static function handleError(int $errno, string $errstr, string $file, int $line): never - { - $exception = new self($errstr); - $exception->file = $file; - $exception->line = $line; - - throw $exception; - } -} +/** + * + */ +abstract class CommandException extends ErrorException {} diff --git a/src/ExitStatus.php b/src/ExitStatus.php index 4cf5b09..9cef6c4 100644 --- a/src/ExitStatus.php +++ b/src/ExitStatus.php @@ -27,7 +27,9 @@ final readonly class ExitStatus */ public function success(): bool { - return $this->code === 0; + return $this->code === 0 + && $this->signal === null + && $this->stoppedSignal === null; } /** diff --git a/src/StreamException.php b/src/StreamException.php index be4cd8a..293ea65 100644 --- a/src/StreamException.php +++ b/src/StreamException.php @@ -5,6 +5,38 @@ declare(strict_types=1); namespace Nih\CommandBuilder; /** - * An error prevented reading from or writing to a stream resource. + * A PHP error occured during an operation on a stream resource. */ -class StreamException extends CommandException {} +final class StreamException extends CommandException +{ + /** + * A note on handling IO errors by setting an error handler before each + * operation: Yes, this is slower, but not by much (1.5x). + * + * However, it is even slower if wrapped with another function, so it is better + * to do this inline every time before the function (e.g. fopen) is called. + * + * ```php + * try { + * set_error_handler(CommandException::handleError(...)); + * $handle = fopen('foo', 'r'); + * } finally { + * restore_error_handler(); + * } + * ``` + */ + public static function handleError( + int $severity, + string $message, + string $filename, + int $line, + ): never { + throw new self( + message: $message, + code: 0, + severity: $severity, + filename: $filename, + line: $line, + ); + } +}