Skip to content

[Console] Suppress proc_open errors within Terminal::readFromProcess #58332

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 28, 2024
Merged

[Console] Suppress proc_open errors within Terminal::readFromProcess #58332

merged 1 commit into from
Sep 28, 2024

Conversation

fritzmg
Copy link
Contributor

@fritzmg fritzmg commented Sep 20, 2024 •
edited
Loading

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Issues
License MIT

When instantiating SymfonyStyle in a command it will try to determine the maximum width of the current console interface.

symfony/src/Symfony/Component/Console/Style/SymfonyStyle.php

Line 53 in 6687e4e

$width = (new Terminal())->getWidth() ?: self::MAX_LINE_LENGTH;

This will execute stty -a | grep columns down the line. Access to stty might be disallowed however, resulting in the following error:

ErrorException: Warning: proc_open(): Exec failed: Permission denied
#16 /vendor/symfony/console/Terminal.php(220): Symfony\Component\Console\Terminal::readFromProcess
#15 /vendor/symfony/console/Terminal.php(204): Symfony\Component\Console\Terminal::getSttyColumns
#14 /vendor/symfony/console/Terminal.php(170): Symfony\Component\Console\Terminal::initDimensionsUsingStty
#13 /vendor/symfony/console/Terminal.php(153): Symfony\Component\Console\Terminal::initDimensions
#12 /vendor/symfony/console/Terminal.php(94): Symfony\Component\Console\Terminal::getWidth
#11 /vendor/symfony/console/Style/SymfonyStyle.php(55): Symfony\Component\Console\Style\SymfonyStyle::__construct
#10 /vendor/symfony/messenger/Command/ConsumeMessagesCommand.php(136): Symfony\Component\Messenger\Command\ConsumeMessagesCommand::interact
#9 /vendor/symfony/console/Command/Command.php(311): Symfony\Component\Console\Command\Command::run

(Stack Trace actually from Symfony 6)

The phpDoc of Terminal::getSttyColumns states

Runs and parses stty -a if it's available, suppressing any error output.

The latter might refer to ['suppress_errors' => true] (though I am not sure) - which is a Windows only functionality. In any case, since Terminal::readFromProcess already checks for

if (!$process = proc_open(…)) {
    return null;
}

and

if (!\is_resource($process)) {
    return null;
}

upstream in Symfony 6/7, indicating that proc_open might fail - this error can additionally be suppressed using @. Besides, Process::start also uses @proc_open (added in 099481f "Prevent warning in proc_open()").

@fritzmg fritzmg requested a review from chalasr as a code owner September 20, 2024 10:49
@carsonbot carsonbot added this to the 7.2 milestone Sep 20, 2024
@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "7.2" but it seems your PR description refers to branch "5.4".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@fritzmg fritzmg changed the base branch from 7.2 to 5.4 September 20, 2024 10:50
@fritzmg fritzmg mentioned this pull request Sep 20, 2024
Closed
@fritzmg
Copy link
Contributor Author

fritzmg commented Sep 22, 2024

Instead of using proc_open directly, we could also use Process, which would likely solve the issue. wdyt?

@nicolas-grekas nicolas-grekas modified the milestones: 7.2, 5.4 Sep 28, 2024
@nicolas-grekas
Copy link
Member

Thank you @fritzmg.

@nicolas-grekas nicolas-grekas merged commit 2be812d into symfony:5.4 Sep 28, 2024
8 of 10 checks passed
@fritzmg
Copy link
Contributor Author

fritzmg commented Sep 28, 2024 •
edited
Loading

@nicolas-grekas should I do a separate PR for Symfony 6, as the code is different there?

@fritzmg fritzmg deleted the suppress-read-from-process-errors branch September 28, 2024 11:22
@xabbuh
Copy link
Member

xabbuh commented Sep 28, 2024

@fritzmg I think merging 5.4 up solved this. But please send a PR if you think that something's still missing.

@fritzmg
Copy link
Contributor Author

fritzmg commented Sep 28, 2024

Ah yes, all good 👍

This was referenced Oct 27, 2024
Merged
Merged
Merged
@blumilk-renovate blumilk-renovate bot mentioned this pull request Apr 4, 2025
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xabbuh xabbuh xabbuh approved these changes

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

Assignees
No one assigned
Labels
Bug Console Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
4 participants
@fritzmg @carsonbot @nicolas-grekas @xabbuh