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

Override Symfony Process. #175

Closed
wants to merge 18 commits into from
Closed

Override Symfony Process. #175

wants to merge 18 commits into from

Conversation

mjauvin
Copy link
Member

@mjauvin mjauvin commented May 3, 2024

This is required in order to handle an exception when open_basedir restrictions are in effect, preventing a process to be created with TTY mode enabled.

@mjauvin mjauvin requested a review from LukeTowers May 4, 2024 02:41
@LukeTowers
Copy link
Member

LukeTowers commented May 7, 2024

Symfony looks like it already does this? symfony/symfony@a1398f6

@mjauvin
Copy link
Member Author

mjauvin commented May 7, 2024

Symfony looks like it already does this? symfony/symfony@a1398f6

This code:

    public function setTty(bool $tty): static
    {
        if ('\\' === \DIRECTORY_SEPARATOR && $tty) {
            throw new RuntimeException('TTY mode is not supported on Windows platform.');
        }

        if ($tty && !self::isTtySupported()) {
            throw new RuntimeException('TTY mode requires /dev/tty to be read/writable.');
        }

        $this->tty = $tty;

        return $this;
    }

Is not the same at all, if you look carefully.

@LukeTowers
Copy link
Member

@mjauvin what's the difference? It looks like isTtySupported() also checks the readable / writable status of /dev/tty

@mjauvin
Copy link
Member Author

mjauvin commented May 7, 2024

@mjauvin what's the difference? It looks like isTtySupported() also checks the readable / writable status of /dev/tty

It does, but it looks like PHP's open_basedire restrictions don't apply for this check, but they do for is_readable()

@LukeTowers
Copy link
Member

Interesting. Can we override isTtySupported() instead and also submit an issue to Symfony to report the problem?

@mjauvin
Copy link
Member Author

mjauvin commented May 7, 2024

Interesting. Can we override isTtySupported() instead and also submit an issue to Symfony to report the problem?

I tried with this overriden method but it doesn't get called somehow:

    public static function isTtySupported(): bool
    {   
        static $isTtySupported;

        if ('/' === \DIRECTORY_SEPARATOR) {
            try {
                $readable = is_readable('/dev/tty');
            } catch (\Exception $e) {
                throw new RuntimeException($e->getMessage());
            }
        }

        return $isTtySupported ??= ('/' === \DIRECTORY_SEPARATOR && stream_isatty(\STDOUT));
    }

@mjauvin
Copy link
Member Author

mjauvin commented May 7, 2024

Only if I replicate Symfony's setTty() method in my overriden class, then my isTtySupported method gets called.

Maybe because this is a static method ?

@mjauvin
Copy link
Member Author

mjauvin commented May 7, 2024

@LukeTowers The problem is Symfony's setTty() method uses "self::isTtySupported()instead ofstatic::isTtySupported()` which prevents my overriden class method to be called.

@mjauvin
Copy link
Member Author

mjauvin commented May 15, 2024

Symfony merged the PR, so we won't be needing this.

@LukeTowers LukeTowers deleted the override-symfony-process branch May 16, 2024 17:28
@mjauvin mjauvin removed this from the 1.2.7 milestone Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants