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

Allow build command to work without pcntl #479

Merged
merged 1 commit into from Aug 8, 2023

Conversation

kohenkatz
Copy link
Contributor

PR #365 added a way to intercept cancellation of the build command and do some cleanup. PR #473 updated this to use Symfony's built-in SignalableCommandInterface, but also broke the command on some platforms. That code assumes that the pcntl extension is present when it references the SIGINT constant. This prevents the command from being run in Docker containers with a minimal set of extensions installed and on Windows.

Following the example of spatie/laravel-backup#1311, this PR checks for the presence of the SIGINT constant before attempting to use it.

@owenvoke owenvoke merged commit 833f62a into laravel-zero:master Aug 8, 2023
6 checks passed
@owenvoke
Copy link
Member

owenvoke commented Aug 8, 2023

I had assumed that Symfony itself handled the PCNTL check, but apparently not (I'm surprised). This looks like a good fix. I guess we could have alternatively checked for extension_loaded('pcntl'), but 🤷🏻

Thanks!

@kohenkatz kohenkatz deleted the patch-dont-require-pcntl branch August 8, 2023 13:21
@kohenkatz
Copy link
Contributor Author

I had assumed that Symfony itself handled the PCNTL check, but apparently not (I'm surprised).

I took a look through the Symfony code, and I found this:

class SignableCommand extends BaseSignableCommand implements SignalableCommandInterface
{
    public function getSubscribedSignals(): array
    {
        return SignalRegistry::isSupported() ? [\SIGUSR1] : [];
    }

    // ... [SNIP]
}

So it looks like they explicitly intend for users to check for support.

This looks like a good fix. I guess we could have alternatively checked for extension_loaded('pcntl'), but 🤷🏻

The definition of Symfony\Component\Console\SignalRegistry\SignalRegistry::isSupported() that is used in the Symfony test case is:

public static function isSupported(): bool
{
    return \function_exists('pcntl_signal');
}

In the end, there are probably several dozen equally valid ways to check it.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants